[yu-xin-c]

Summary

• add runtime session tracking for skills that declare metadata.hermes.verification.before_final
• record verifier specs when skill_view loads the main SKILL.md
• run required verifiers in finalize_turn before the final response and block failed block_final responses

Draft for #44637. This MVP treats a successful main skill_view call as activation; explicit activation and streaming suppression are intentionally left for maintainer direction.

Tests

• scripts/run_tests.sh tests/agent/test_skill_verification.py -- -q
• scripts/run_tests.sh tests/agent/test_skill_commands.py tests/agent/test_external_skills.py -- -q
• .venv/bin/ruff check agent/skill_verification.py agent/turn_finalizer.py tools/skills_tool.py tests/agent/test_skill_verification.py

[maxgris]

Nice to see this on real code so fast — and the core wiring is in the right place: gating in finalize_turn right after the transform_llm_output block, with a block_final failure replacing final_response and setting failed=True / completed=False, is the correct chokepoint (every surface reads final_response from there) and it fails closed. Good foundation. A few things before this leaves draft, the first one load-bearing:

1. _run_check shell-executes a command string taken straight from skill frontmatter — that's arbitrary command execution triggered by loading a skill.
   completed = subprocess.run(
   spec.command, cwd=..., shell=True, capture_output=True, text=True,
   timeout=spec.timeout_seconds,
   )

spec.command is before_final.command lifted verbatim from SKILL.md frontmatter, with no allowlist, no sandbox, and no user confirmation. The timeout bounds how long it runs, not what it can do. And because the spec is recorded on a successful main skill_view (point 2), merely viewing a skill causes its declared command to be shell-run at the next turn boundary. A command:-carrying skill can arrive two ways neither of which involves a human vetting that
command: the agent's own skill-authoring loop (#25833), or hermes skills install from a third-party registry (GitHub/ClawHub/etc. per tools/skills_hub.py). I'd treat "which skills may carry an enforced command" as a trust boundary the runtime decides — e.g. only locally-authored / signed skills, or commands constrained to a vetted runner — not something any frontmatter can opt into. This is the piece I'd want resolved before merge.

2. Treating a successful skill_view as activation is the conflation the issue is specifically trying to avoid. The PR body is upfront that this is the MVP shortcut, so flagging rather than faulting — but it has a concrete cost. The spec is recorded whenever skill_view is called without a file_path (the main SKILL.md), so a skill that's loaded and then not used — the model peeks, decides it's irrelevant, moves on — still arms its verifier, and the next final response gets gated on (and runs) that skill's command. That's a false-trigger on benign load-then-discard, and it's also what makes point 1 fire without intent. xueyu-jia's three-state loaded → activated → verified proposal in [Feature]: Runtime-enforced verification gates for Skills #44637 is the fix; I sketched why an explicit activation step matters there and won't repeat it here.

3. The gate fails open on its own internal error. In finalize_turn the verification call is wrapped:
   except Exception as exc:
   logger.warning("skill before-final verification failed: %s", exc)

So, if the verification machinery itself throws, the unverified response proceeds. For a feature whose whole point is failing closed, an infrastructure error should arguably block (or be explicitly configurable), not silently pass — otherwise an exception becomes a verification bypass.

Minor: tests/gateway/test_telegram_topic_mode.py (+4) looks unrelated to this change (it's a get_random_tip determinism patch) and probably shouldn't ride in this diff.

Happy to help on the runner side once the activation + trust-boundary direction is settled — the command-execution surface is the part I'd most want a second pair of eyes on.

[yu-xin-c]

Pushed ef58c0017 to address the review points here:

• Split load from activation: skill_view now records an inert verifier candidate, and the new skill_activate tool is the only path that arms it for before-final verification. A load/peek that is not used will not run a verifier.
• Removed the shell execution surface: verifier commands now execute as argv with shell=False, and command verifiers are disabled by default unless runtime config opts in with skills.verification.allow_commands: true.
• Made verification infrastructure errors fail closed in turn_finalizer, with skill_verification_error surfaced on the result metadata.
• Rewrote the branch history to drop the unrelated test-stabilization commits from this PR.

Local checks run:

• .venv/bin/ruff check agent/skill_verification.py agent/turn_finalizer.py agent/display.py agent/tool_guardrails.py agent/context_compressor.py agent/system_prompt.py agent/prompt_builder.py agent/transports/hermes_tools_mcp_server.py model_tools.py tools/skills_tool.py tests/agent/test_skill_verification.py tests/agent/test_turn_finalizer_cleanup_guard.py tests/run_agent/test_run_agent.py
• scripts/run_tests.sh tests/agent/test_skill_verification.py tests/agent/test_turn_finalizer_cleanup_guard.py tests/tools/test_skills_tool.py tests/tools/test_skill_usage.py -- -q
• scripts/run_tests.sh tests/run_agent/test_run_agent.py -- -q -k 'skills_prompt_derives_available_toolsets_from_loaded_tools or valid_tool_names'

CI is rerunning on the new head now.

[yu-xin-c]

Rebased this onto current main in 116d59589; the only manual conflict was in agent/display.py, where I kept both the newer mixture_of_agents preview mapping and this PR's skill_activate preview mapping.\n\nLocal verification:\n- uv run --extra dev python -m pytest tests/agent/test_skill_verification.py tests/agent/test_turn_finalizer_cleanup_guard.py tests/tools/test_skills_tool.py tests/tools/test_skill_usage.py -q\n- uv run --extra dev python -m pytest tests/run_agent/test_run_agent.py -q -k 'skills_prompt_derives_available_toolsets_from_loaded_tools or valid_tool_names'\n- uv run --extra dev ruff check agent/skill_verification.py agent/turn_finalizer.py agent/display.py agent/tool_guardrails.py agent/context_compressor.py agent/system_prompt.py agent/prompt_builder.py agent/transports/hermes_tools_mcp_server.py model_tools.py tools/skills_tool.py tests/agent/test_skill_verification.py tests/agent/test_turn_finalizer_cleanup_guard.py tests/run_agent/test_run_agent.py\n- scripts/run_tests.sh tests/agent/test_skill_verification.py tests/agent/test_turn_finalizer_cleanup_guard.py tests/tools/test_skills_tool.py tests/tools/test_skill_usage.py -- -q\n- scripts/run_tests.sh tests/run_agent/test_run_agent.py -- -q -k 'skills_prompt_derives_available_toolsets_from_loaded_tools or valid_tool_names'\n\nGitHub CI is green on the new head now.

[teknium1]

Thanks for addressing the initial load/activation and fail-closed feedback. The PR still has blocking issues before it can provide an operable, safe gate.

Problems

• agent/skill_verification.py:373 calls cfg_get with a dotted string instead of a config dict and key path. cfg_get returns its default for non-dict input (hermes_cli/config.py:6611-6654), so skills.verification.allow_commands: true can never enable a verifier. The tests monkeypatch this function at tests/agent/test_skill_verification.py:58 and :130, so they miss the real path.
• agent/skill_verification.py:232 executes argv from SKILL.md frontmatter. shell=False removes shell parsing, but does not authorize arbitrary executables. This remains a trust-boundary issue for mutable external skills, which are fully integrated by design (website/docs/user-guide/features/skills.md:319-336).
• The new global verifier maps (agent/skill_verification.py:25-26) are never cleared by a production lifecycle caller. Existing shutdown/rotation seams are run_agent.py:3315-3365; the PR adds clear_session_verifications() but does not invoke it.

Suggested changes

• Fix the config lookup and add a temp-HERMES_HOME integration test without monkeypatching it.
• Define a trusted runner/provenance or approval boundary for command-bearing skills.
• Clear state at session close and rotation, with lifecycle coverage.

Automated hermes-sweeper review.

[teknium1]

cfg_get takes a config dict followed by individual path keys (cfg_get(cfg, "skills", "verification", "allow_commands", default=False)). Passing the dotted string as cfg makes it return None, so this gate is permanently false. Please use the loaded config and add an unmocked config.yaml integration test.

[teknium1]

shell=False prevents shell interpretation but does not make this frontmatter-controlled executable safe. Once the global switch is enabled, a mutable externally installed or agent-authored SKILL.md can select any executable and arguments. This needs a runtime trust/provenance, vetted-runner, or approval boundary before command execution is enabled.

[teknium1]

This cleanup helper is not wired into any production session close or rotation path, while the module retains per-session global maps above. Please invoke it from the existing AIAgent lifecycle seams and add closed-session and rotated-session coverage.