DRAFT: For PR #2035: add /hooks endpoint

[enyst]

Hi! I’m OpenHands-GPT-5.2 (an OpenHands dev assistant agent).

This is a stacked PR targeting PR #2035 (base: support-openhands-hooks-json-in-agent-server) as a friendly alternative proposal.

Motivation

Instead of auto-loading .openhands/hooks.json inside event_service.start() (agent-server), this proposes a skills-style approach:

• expose an explicit POST /api/hooks endpoint on agent-server
• app-server can call this endpoint during conversation setup, merge the returned HookConfig into the agent payload, and send it via StartConversationRequest.hook_config
• app-server can enforce policy centrally (e.g. default load_user=false), and agent-server conversation start stays more deterministic

What’s included

• openhands-agent-server/openhands/agent_server/hooks_router.py: new /hooks endpoint
• openhands-agent-server/openhands/agent_server/hooks_service.py: loads {project_dir}/.openhands/hooks.json
• openhands-agent-server/openhands/agent_server/api.py: wires router under /api
• Unit test: tests/agent_server/test_hooks_router.py

cc @ak684 — thanks! This PR is meant to be a little incremental change rebased on yours so we can compare designs side-by-side.

Co-authored-by: openhands openhands@all-hands.dev

@enyst can click here to continue refining the PR

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-agent-server/openhands/agent_server
api.py	150	10	93%	70, 82, 245, 248, 252–254, 256, 262, 303
hooks_router.py	30	2	93%	26–27
hooks_service.py	21	3	85%	33–35
TOTAL	16817	7316	56%

[all-hands-bot]

Taste Rating: 🟡 Acceptable

The core design is clean and simple—router delegates to service, service delegates to SDK. Good separation of concerns. However, there are security and testing gaps that need addressing before this is production-ready.

Key Insight: This is a sensible architectural choice (explicit API vs implicit loading), but the implementation needs hardening around path validation and more comprehensive test coverage.

[all-hands-bot]

🟡 Acceptable - Core logic is sound and well-tested, but has unnecessary complexity and weak validation that should be addressed.

[all-hands-bot]

Taste Rating: 🟡 Acceptable - Core design is solid and pragmatic, but error handling and path validation need hardening before production use.

[CRITICAL ISSUES]

Path Validation Security: The validation is too permissive and doesn't protect against path traversal or unauthorized access.

Error Handling: Swallowing all exceptions loses critical debugging information when things go wrong.

Test Logic Inconsistency: Test expects rejection of relative paths, but code actually resolves them.

[IMPROVEMENT OPPORTUNITIES]

API Design: Returning None for both "not found" and "invalid path" conflates different failure modes.

Test Coverage: Missing tests for malformed JSON, permission errors, and other failure scenarios.

Unrelated Change: AGENTS.md documentation is useful but unrelated to the hooks feature.

---

VERDICT: ✅ Worth merging after fixes - The skills-style approach is cleaner than auto-loading. Address the path validation and error handling issues first.

KEY INSIGHT: Good separation of concerns (router/service) and explicit control over loading behavior, but security and observability need attention before this touches production.

[enyst]

Note on path validation / workspace roots

AI review raised concerns about project_dir potentially enabling reads outside the workspace.

Current agent-server patterns:

• file_router endpoints require absolute paths but do not enforce a single workspace root.
• git_router currently accepts a Path parameter and does not validate it at all.

What this PR does now:

• /api/hooks performs lightweight normalization + rejects relative paths (to avoid accidental resolve() turning relative input into an absolute path).
• We intentionally do not attempt to enforce project_dir under a specific workspace root yet, because agent-server doesn’t have a stable canonical root across runtimes and clients.

Follow-up:

• Tracked in Agent-server hooks endpoint: define path validation policy for project_dir #2089: decide on a consistent path policy (workspace root in config vs resolving from conversation/workspace context vs changing API to avoid raw filesystem paths).

[enyst]

Path handling patterns in agent-server (context for /hooks)

On the path-safety concern: today agent-server already has multiple endpoints that accept filesystem paths and do not consistently enforce a single workspace-root sandbox policy. The design assumption is that agent-server runs inside the agent-box and client apps (app-server/CLI) provide the right paths + auth, so these are effectively internal APIs within the sandbox boundary.

Examples in current codebase:

• file_router requires absolute paths but doesn’t enforce a workspace root: it checks Path(path).is_absolute() and then reads/writes that path.
• git_router accepts a Path parameter and passes it to SDK helpers with no validation.
• /skills accepts project_dir and passes it through to load_all_skills(..., project_dir=request.project_dir) with no validation.

What we’re doing for /hooks for now:

• Keep it lightweight and consistent with the above: reject obvious relative inputs (to avoid resolve() implicitly turning a relative into an absolute cwd-based path), normalize absolute paths, and otherwise rely on the agent-box boundary + client auth/policy.

Follow-up:

• Tracked in Agent-server hooks endpoint: define path validation policy for project_dir #2089 to define a consistent repo-wide policy (workspace root enforcement or API change to avoid raw filesystem paths) and then apply it across endpoints (/skills, /hooks, git/file, etc.).