feat(hooks): implement prompt-based evaluation

[onatozmenn]

HUMAN:
I reviewed the implementation and test results. The PR is ready for review.

---

AGENT:

Why

HookType.PROMPT is already public, but it currently falls open without evaluating anything. This adds the intended single-completion policy check between command hooks and tool-using agent hooks.

Summary

• Runs prompt hooks with the conversation's current LLM.
• Keeps the policy separate from the untrusted HookEvent payload and parses the shared allow/deny result.
• Tracks timeout, telemetry, cost, and prompt-hook:<name> metrics.
• Fails open with success=False when the model is unavailable or returns an invalid result.
• Adds tests, a runnable example, and matching documentation in docs(hooks): document prompt-based evaluation docs#639.

Issue Number

Closes #2755

How to Test

• Full hook suite: 143 passed on the current branch.
• Changed-file pre-commit checks pass.
• The example was run against a real GitHub Models endpoint: the test command was allowed and the destructive command was denied. The output is in .pr/prompt-hooks-live-run.md.
• The docs example check passes against the open docs branch. It will remain red in GitHub until docs(hooks): document prompt-based evaluation docs#639 is merged or the workflow can check that fork branch.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

GitHub Copilot assisted with this PR.

This first version uses the active conversation LLM and the HookEvent payload only. Per-hook profile overrides and extra conversation or workspace context are left for follow-up; HookType.AGENT remains the option when tools or workspace investigation are needed.

[onatozmenn]

Maintainers: this changes LLM-backed hook decision behavior. Please add the integration-test label before the PR is marked ready, per the repository testing guidance.

[enyst]

Thank you for the PR!

Could you perhaps add some logs from the real-world example run, to a temporary (but committed) .pr/ directory? Generally, we have a convention that .pr/ directory is good sometimes, for artefacts that show or prove the PR's behavior, maybe with a .md file too to document results.

[enyst]

@OpenHands Do an initial /codereview of this PR. Post your feedback directly with gh api and event as you think appropriate.

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[smolpaws]

Initial review complete. I did not find a code-level correctness blocker in the prompt-hook implementation: policy/event separation, explicit fail-open signaling, current-LLM lookup, API routing, isolated metrics, lifecycle behavior, and REST compatibility are all covered coherently.

Local verification on the exact PR head (b4b21c12):

• 185 passed, 66 skipped: hooks + hook REST router + REST breakage checker + example discovery
• 6 passed: OpenAPI discriminator tests
• changed-file pre-commit: Ruff, pycodestyle, Pyright, dependency rules all passed

I’m leaving COMMENT rather than APPROVE because this changes LLM-backed agent policy behavior and still lacks the required evaluation evidence:

• integration-test label is absent. I attempted to add it, but the current authenticated account lacks label permissions.
• The requested live-provider .pr/ run artifact is absent, and this environment has no LLM_API_KEY, so I could not produce honest real-world logs.
• check-examples fails only because 55_prompt_hooks/main.py is not yet present on docs main; OpenHands/docs#639 contains the correct synchronized example block but is still open.

Residual follow-up, not introduced by this PR: hook callbacks are synchronous, so command/agent/prompt hooks can block an arun() event loop while evaluating. Fixing that requires an async hook pipeline rather than a prompt-hook-only patch.

Profile selection is addressed in the inline thread.

This review was created by an AI agent (OpenHands) on behalf of Engel Nyst.

[VascoSch92]

Just as a note: we will need also a PR in the docs to document the Hook

[onatozmenn]

Added the live run output in .pr/prompt-hooks-live-run.md. I ran the example against GitHub Models: the test command was allowed and the destructive command was denied.

The docs PR is already open at OpenHands/docs#639.

[onatozmenn]

@enyst @VascoSch92 both requests are done: the live run artifact is in .pr/prompt-hooks-live-run.md and the docs PR is open at OpenHands/docs#639.

Two things left that I can't do from my side:

• the integration-test label still needs to be added, since this changes LLM-backed hook decisions
• check-examples fails only because 55_prompt_hooks/main.py isn't on docs main yet, so it should clear once Remove deprecated to_llm_dict() and from_litellm_message() in favor of new responses API #639 lands

Happy to rework anything if you'd rather take a different shape here.

[github-actions]

📁 PR Artifacts Notice

This PR contains a .pr/ directory with temporary PR-specific documents. Because this is a fork PR, the directory will be automatically removed from main immediately after merge.

[all-hands-bot]

🟢 Good taste — the implementation keeps the trusted policy separate from untrusted event data, reuses the model-appropriate completion path, isolates and merges hook metrics, and preserves detectable fail-open behavior. I found no material code-level correctness, security, maintainability, or test-coverage issues.

Local verification at 7ca476be7fcca9adf9ca3d300a2d721869e9ce74: 116 passed, 67 skipped across the changed hook configuration/executor/manager tests and example-discovery tests. The PR also includes a real-provider run artifact.

This changes LLM-backed policy decisions that can allow or deny agent actions, but the PR still has no integration-test label and I found no completed eval-monitor run plus human confirmation. Per repository policy, I’m leaving COMMENT and deferring the merge decision to a human maintainer after lightweight integration/eval validation.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  The implementation is focused and well tested, but model-dependent allow/deny behavior can affect agent execution and benchmark outcomes. Invalid/provider-failure paths deliberately fail open, so production policy enforcement also depends on model reliability and observability.

VERDICT:
✅ Worth merging after human eval review: No material code findings; obtain the repository-required lightweight integration/eval signal first.

KEY INSIGHT:
The architectural boundary is clean: prompt hooks are single-completion policy evaluators using the active conversation LLM, while tool-using investigation remains an agent-hook concern.

This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.

[enyst]

👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).

🟢 Good taste

This is a focused implementation: the policy and untrusted event are separated, the completion path supports both model APIs, hook usage is accounted for, malformed outputs fail consistently with the existing hook policy, and the behavior is covered across configuration, dispatch, parsing, metrics, and examples. I did not find a concrete correctness regression in the changed code.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🔴 HIGH
  This adds LLM-driven policy decisions to the hook execution path, so it touches core orchestration and a security-adjacent boundary where prompt injection and fail-open behavior require explicit human judgment. The implementation is coherent, but this mechanism must not be treated as a hard security boundary.

Recommendation: Do not auto-merge. Request review from a human SDK/security maintainer to validate the prompt-injection and fail-open tradeoff.

VERDICT:
✅ Worth merging: Core logic is sound; merge after the human risk review above.

KEY INSIGHT:
Keeping the trusted policy in the system message and serializing the event as explicitly untrusted user data is the right shape for a probabilistic hook evaluator.

[enyst]

Thank you!

[VascoSch92]

@onatozmenn Could you also create the docs for this feature? Thanks

Let me know if you need more details about that

[onatozmenn]

@VascoSch92 docs PR is already open: OpenHands/docs#639. It documents the prompt mode in sdk/guides/hooks.mdx and adds the 56_prompt_hooks example, matching the one in this PR.

It has zero checks running though, looks like the fork workflows need a maintainer to approve them, so it may not show up as active on your end. A review there would unblock both, since check-examples here stays red until the example lands on docs main.

If you meant docs inside this repo instead of the docs site, say where and I'll add it.

[all-hands-bot]

🤖 OpenHands is reviewing this PR.

Head commit: 483a967344951a17a379d196b203d2ce63f21c9e
View the conversation: https://oss-agent-canvas.ngrok.dev/conversations/1461df0c-d1fe-4a3b-bf19-32bf5d29129e

This comment was posted by an AI agent (OpenHands).

[all-hands-bot]

Review: feat(hooks): implement prompt-based evaluation

I reviewed the full diff, surrounding code, and the existing review thread. I found no material correctness, security, or test-coverage issues that would block this PR.

What was verified

• Policy/event separation: The trusted policy (hook.prompt) is placed in the system message; the serialized HookEvent is placed in the user message and explicitly labeled as untrusted data. This is the correct mitigation pattern for an LLM-based policy check.
• Fail-open semantics: Missing LLM, provider failures, empty responses, unparseable JSON, and unknown decisions all fall open with success=False and decision=ALLOW, making execution failures distinguishable from deliberate allow verdicts. This is consistent with the existing agent-hook contract and is documented in the README.
• LLM isolation: model_copy(update={usage_id, timeout, stream=False}) + reset_metrics() correctly isolates hook spend, timeout, and the prompt-hook:<name> usage bucket from the parent LLM. The parent's stream setting is preserved (verified by test_timeout_usage_id_and_metrics_are_isolated).
• API routing: Delegates to make_llm_completion, which selects completion vs responses based on uses_responses_api(). With empty tools, add_security_risk_prediction=True is a harmless no-op.
• Metrics merging: The extracted _merge_usage_metrics helper correctly handles both agent-hook (conversation stats) and prompt-hook (per-LLM metrics) paths, including cumulative merging for repeated same-named hooks (test_repeated_prompt_hooks_merge_metrics).
• Config validation: PROMPT hooks correctly reject command and async_, and display_command generalizes to prompt-hook:<name> while preserving the REST command field contract.
• Tests: 143 hook tests pass. Coverage is thorough across dispatch, parsing (allow/deny/invalid), API routing, fall-open paths, metric isolation/merging, execute_all stop-on-deny, and live LLM-getter resolution. Manager-level tests cover both PreToolUse deny and Stop deny.

Non-blocking observation

Prompt hooks run a synchronous blocking make_llm_completion on the event-loop thread during async conversations (via the sync on_event -> HookEventProcessor.on_event -> run_pre_tool_use path). This is the same pattern already used by agent hooks (which spin up a full LocalConversation synchronously), so it is not a regression. If blocking the event loop during hook evaluation becomes a concern, both hook types would need an async dispatch path -- but that is a broader architectural change outside the scope of this PR.

Risk assessment

Low risk. The change is additive (fills in a previously stubbed HookType.PROMPT), preserves existing REST contracts, fails safely, and is well-tested. The prompt-injection surface is inherent to any LLM-based policy hook and is mitigated by the system/user separation; the PR description correctly scopes per-hook profile overrides and additional context to follow-up.

This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.

[all-hands-bot]

⚠️ OpenHands gave up on this review for commit 483a96734495 after 2h (last state: active).

This comment was posted by an AI agent (OpenHands).