fix(sec-core): compose transform_llm_output hooks so skill-ledger warning is not dropped - #2621
Conversation
There was a problem hiding this comment.
- transform_llm_output 组合链依赖注册顺序和单一包装约定,建议在代码中明确顺序与幂等性约束。
- 组合链目前静默忽略非字符串返回值,建议约定并记录非字符串作为错误,避免未来扩展行为被吞掉。
- 使用全局 _TRANSFORM_HOOKS 收集回调在并发/重入场景下可能交叉污染,建议在代码中声明仅支持串行初始化或改为局部容器。
🤖 Generated by Qoder • View workflow run
| _TRANSFORM_HOOKS.append((capability_id, callback)) | ||
|
|
||
|
|
||
| def _compose_transform_chain(callbacks: list[tuple[str, Any]]): |
There was a problem hiding this comment.
[P1] transform_llm_output 组合链缺少顺序与幂等性约束说明
当前 _compose_transform_chain(callbacks) 直接依赖 callbacks 列表顺序,且在 register_capabilities 末尾用 "transform-compose" 统一包装。建议这里用注释明确:1)能力注册顺序即链执行顺序,后注册能力只能追加告警;2)transform-compose 自身不应在能力列表中重复注册,避免链上多次包装。
🤖 Generated by Qoder • Fix in Qoder
| def _chain(response_text: str = "", **kwargs: Any) -> str: | ||
| current = response_text | ||
| for _capability_id, callback in callbacks: | ||
| result = callback(response_text=current, **kwargs) | ||
| if isinstance(result, str) and result: |
There was a problem hiding this comment.
[P2] transform_llm_output 组合链未显式处理非字符串返回值
_chain 中仅在 isinstance(result, str) 且非空时更新 current,其余类型直接忽略。若未来某 capability transform 返回结构化对象或 None 表示特殊语义,现有实现会静默吞掉这类约定。建议在此明确约定:transform_llm_output 必须返回字符串,非字符串视为错误并记录日志,避免后续扩展产生难以发现的行为偏差。
🤖 Generated by Qoder • Fix in Qoder
| ctx, capabilities: list[AgentSecCoreCapability], config: dict | ||
| ) -> None: | ||
| """Register all enabled capabilities with the Hermes plugin context.""" | ||
| global _TRANSFORM_HOOKS |
There was a problem hiding this comment.
[P2] transform_llm_output 组合链对 _TRANSFORM_HOOKS 全局列表缺少并发安全说明
register_capabilities 通过全局 _TRANSFORM_HOOKS 收集能力回调,并在末尾清空。若 Hermes 插件未来支持并发 register_capabilities 调用或热重载能力,这种共享全局列表可能产生交叉污染。建议在此处增加显式约束:注册流程仅在初始化阶段串行执行,不支持并发/重入;如未来需要并发,需改为局部容器。
🤖 Generated by Qoder • Fix in Qoder
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e93633b9e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| capability's warning instead of only the first one to fire. | ||
| """ | ||
|
|
||
| def _chain(response_text: str = "", **kwargs: Any) -> str: |
There was a problem hiding this comment.
Accept session_id in composed transform hook
When Hermes invokes transform_llm_output with its documented (response_text, session_id, **kwargs) signature, this composed _chain rejects the second positional argument. Because register_capabilities wraps it in safe_hook_wrapper, that TypeError is swallowed and the composed transform returns None, so prompt/PII/skill-ledger final-response warnings are all dropped for that turn; keep the same signature or accept positional args and forward session_id to the capability callbacks.
AGENTS.md reference: src/agent-sec-core/AGENTS.md:L397-L405
Useful? React with 👍 / 👎.
| result = callback(response_text=current, **kwargs) | ||
| if isinstance(result, str) and result: | ||
| current = result | ||
| return current |
There was a problem hiding this comment.
Return None when the chain makes no change
When none of the collected capabilities emits a warning/redaction, current is still the original non-empty response_text, so the composed hook now returns a non-empty string for every normal turn. Under the first-non-empty-wins behavior this code is explicitly handling, that makes agent-sec-core claim the transform even though it made no change, preventing any later Hermes transform_llm_output hook from running; track whether any callback changed the text and return None on no-op.
Useful? React with 👍 / 👎.
…ning is not dropped
e93633b to
d401295
Compare
问题
Fixes #2620
当
prompt-scan与skill-ledger在同一 turn 都触发 warning 时,Hermes 的transform_llm_output采用 "first non-empty string wins" 语义,先注册的prompt-scan会抢占,导致skill-ledger的status=tampered安全告警被静默丢弃 —— 用户看不到"即将使用被篡改 skill"的告警。修复
将 hermes-plugin 多个 capability 的
transform_llm_output回调组合成单一 chained hook,而非各自独立注册。每个 capability 的_on_transform_llm_output本就只做 prepend(把自己的 warning 前置到传入的response_text),因此 chain 后自然拼接所有告警:registry.py:新增_TRANSFORM_HOOKS收集列表 +_compose_transform_chain();register_capabilities末尾注册单一 composed hook。capabilities/base.py:register()对transform_llm_output走_collect_transform_hook()而非独立ctx.register_hook()。验证
git apply+bash scripts/rpm-build.sh agent-sec-core→ exit 0,产出agent-sec-hermes-hook-0.10.1-1.alnx4.x86_64.rpm(rpm2cpio 解包确认已含 fix)status=tampered与[prompt-scan] 检测到安全风险Co-Authored-By: Claude noreply@anthropic.com