feat(agent-server): load hooks.json for v1 hooks#2035
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable (but needs critical fix)
Verdict: ❌ Needs rework - Missing error handling could crash production
Key Insight: The lack of error handling around file loading violates "never break userspace" - a typo in hooks.json will crash the service instead of falling back gracefully.
The core logic is sound and solves a real problem (repo-level hooks in SaaS). However, there is a critical error handling gap and the code is more complex than it needs to be. See inline comments for details.
Coverage Report •
|
||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Logic is sound but has some verbosity issues
The core idea is solid: load repo hooks from .openhands/hooks.json and merge with request hooks. However, the implementation has repeated patterns that violate DRY and some overly broad error handling. See inline comments for details.
…ce duplication Address PR feedback about DRY violation - the "check if not None and is_empty(), then set to None" pattern was repeated twice.
enyst
added
behavior-initiative
xingyaoww
left a comment
xingyaoww
left a comment
There was a problem hiding this comment.
I think maybe we should've implement this from the App side, rather than keep these in the SDK?
OpenHands/OpenHands#12773
I 100% disagree. It's simpler to implement/resolve reading project hooks.json hooks in the SDK first and then the app (and ALL other consumers of the SDK) can read it automatically, not the other way around. I also need this PR approved for a client POC this week. So that's another motivation for the simpler, working approach proposed here. |
…hook sources
Addresses PR feedback to support both project-level and user-level hooks,
similar to how skills work with load_project_skills and load_user_skills.
Changes:
- Add load_project_hooks(working_dir) to load from {working_dir}/.openhands/hooks.json
- Add load_user_hooks() to load from ~/.openhands/hooks.json
- Update event_service.py to merge hooks from all sources (request > project > user)
- Add comprehensive tests for new functions
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
|
|
||
| project_hook_config = load_project_hooks(workspace.working_dir) | ||
| user_hook_config = load_user_hooks() | ||
|
|
There was a problem hiding this comment.
Maybe this load_project_hooks and load_user_hooks could be disabled by default?
Please correct me if I’m wrong, I think load_project_skills/user is disabled by default in the SDK, we have the mechanism here, but we don’t load all the things from user machine - the decision True comes from the caller application 🤔
There was a problem hiding this comment.
@OpenHands Clone yourself the CLI and the repo named OpenHands, from the same org, and investigate what I just said: who loads Skills (project, user) in the CLI, and on the V1 app_server?
Note that OpenHands repo is a bit chaotic, it has V0 legacy code. We only want to follow the execution path in openhands/app_server, V1, and find where we call the SDK for skills / with skills and what does app-server do.
There was a problem hiding this comment.
I'm on it! enyst can track my progress at all-hands.dev
There was a problem hiding this comment.
test reply
Edited to add: HUMAN: hah! I sneaked in the conversation and told the agent to be careful to REPLY, because these days idk why, it sometimes wiped previous comment, “replying” by editing the question 😅
… I mean, I guess it was convenient 😂
There was a problem hiding this comment.
TL;DR
- SDK defaults are safe:
AgentContext(load_user_skills=False, load_public_skills=False)by default. - CLI explicitly opts in to loading user + public skills (and always loads project skills).
- V1 app-server explicitly opts in to loading public + user + org + project + sandbox skills by delegating to agent-server (
POST /skills). - In V1, the app-server does not load skills from disk itself; it’s a thin orchestrator/proxy. The agent-server is the component that touches the filesystem (workspace) and home dir.
1) SDK invariants / defaults
In the SDK, skills loading is off by default.
AgentContext defaults
In software-agent-sdk/openhands-sdk/openhands/sdk/context/agent_context.py:
load_user_skills: bool = Falseload_public_skills: bool = False
When load_user_skills=True, AgentContext runs a model validator that calls load_user_skills() and merges them into skills (dedup by name).
Project skills are not auto-loaded by AgentContext
load_project_skills(work_dir) is a separate helper in sdk/context/skills/skill.py.
So project-skill loading is always a caller decision.
2) Who loads skills in the CLI?
In the CLI repo (OpenHands-CLI), the loading decision is in:
openhands_cli/stores/agent_store.py
Function _build_agent_context():
skills = load_project_skills(get_work_dir())
...
return AgentContext(
skills=skills,
system_message_suffix=system_suffix,
load_user_skills=True,
load_public_skills=True,
)This matches the idea that the SDK has the mechanism, but the application chooses to turn it on.
3) Who loads skills in V1 app-server (openhands/app_server)?
V1 app-server does not call sdk.context.load_project_skills() or sdk.context.load_user_skills() directly.
Instead, it delegates to the agent-server.
Entry point
In OpenHands/openhands/app_server/app_conversation/live_status_app_conversation_service.py, conversation creation calls:
_finalize_conversation_request(...)- which, if
remote_workspaceexists, calls_load_skills_and_update_agent(...)
load_and_merge_all_skills() in V1
In OpenHands/openhands/app_server/app_conversation/app_conversation_service_base.py:
all_skills = await load_skills_from_agent_server(
agent_server_url=agent_server_url,
session_api_key=sandbox.session_api_key,
project_dir=project_dir,
org_config=org_config,
sandbox_config=sandbox_config,
load_public=True,
load_user=True,
load_project=True,
load_org=True,
)So V1 app-server explicitly opts in to everything (public/user/project/org + sandbox skill).
What app-server does vs agent-server does
App-server responsibilities:
- figures out org-level repo path (
{org}/.openhandsvsopenhands-configetc.) and obtains a pre-authenticated git URL (so agent-server can clone without interactive auth) - passes sandbox exposed URLs
- computes
project_dirinside the workspace (e.g.working_dir/<repo_name>) - calls agent-server endpoint
Agent-server responsibilities:
- actually loads skills from sources:
- public:
load_public_skills() - user:
load_user_skills()(home dir inside the agent-server runtime) - project:
load_project_skills(project_dir)(workspace filesystem) - org: clones org repo URL and loads from it
- sandbox: generates a synthetic
work_hostsskill
- public:
This is implemented in software-agent-sdk/openhands-agent-server/openhands/agent_server/skills_service.py::load_all_skills() and exposed via skills_router (POST /skills).
4) Important implication (re: “don’t load things from user machine by default”)
- SDK keeps the invariant: user/public skills are disabled by default.
- CLI enables them because it’s running on the user’s machine and that’s expected.
- V1 app-server enables them because it wants SaaS parity with CLI, but note that
load_user_skills()on agent-server will read from the agent-server container’s$HOME/.openhands/....- In SaaS, that home directory is not the end-user’s machine, but the agent runtime’s home.
- So “user skills” in SaaS effectively means “skills baked/volume-mounted into the agent runtime”, unless you explicitly sync user skills into that home.
So, if the product/security policy is “don’t implicitly load user/home content in hosted mode”, the knob for that in V1 is the booleans passed from app-server to agent-server (load_user, load_public, etc.). Right now they’re hardcoded to True in V1.
5) Hooks (since it motivated this thread)
Analogous pattern exists for hooks, but currently (in this PR) it’s in agent-server:
- SDK provides
load_project_hooks(working_dir)andload_user_hooks()(both returnNoneif file missing) - In
openhands-agent-server/openhands/agent_server/event_service.pywe currently do:request_hook_config = _normalize_hook_config(self.stored.hook_config) project_hook_config = load_project_hooks(workspace.working_dir) user_hook_config = load_user_hooks() merged = HookConfig.merge([request, project, user])
So hooks follow the same “caller decides to enable” pattern — except here the agent-server is unconditionally loading project/user hooks today.
If we want hooks to match skills’ “disabled-by-default unless caller opts-in” invariant, the likely design is to add request flags (e.g. load_project_hooks, load_user_hooks) and plumb them from V1 app-server / CLI similarly to how skills are plumbed.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
I’m OpenHands-GPT-5.2 (an OpenHands dev assistant agent). Here’s a concrete alternative design that mirrors the existing V1 /skills pattern.
Yes — that’s a very coherent design, and it’s actually the closest analogue to how V1 does skills (app-server orchestrates, agent-server loads from workspace). It also avoids adding “filesystem side effects” into event_service.start().
If we spell it out, it’s essentially “move hook source selection out of conversation start” and into an explicit, fetch-style endpoint.
Option F — Add /hooks endpoint (skills-style), app-server orchestrates + merges
Proposed flow
- App-server decides policy:
- should we load project hooks? user hooks? org hooks? plugin hooks? (plugin hooks still handled later in
LocalConversation)
- should we load project hooks? user hooks? org hooks? plugin hooks? (plugin hooks still handled later in
- App-server calls agent-server:
POST /hookswith booleans, plusproject_dir/ whatever is needed to locate repo root in workspace
- Agent-server returns a serialized
HookConfig(ornull). - App-server merges:
- request hooks (if any, from UI/API)
- hooks from
/hooks - (optionally) app-server-specific hooks
- App-server sends
StartConversationRequest(hook_config=merged)to agent-server. - Agent-server
event_service.start()does no implicit loading, it just usesself.stored.hook_config+ later plugin hooks merge insideLocalConversation.
Why this matches existing V1 “skills” architecture
- App-server does not have the workspace filesystem, agent-server does.
- Therefore, any “load from workspace” action in V1 generally becomes:
- app-server calls agent-server endpoint that touches the workspace FS.
- This is exactly what happens for skills today (
POST /skills).
So /hooks fits the pattern and keeps event_service.start() deterministic.
Key design choices to make it clean
1) API shape of /hooks
I’d mirror /skills:
Request
{
"load_project": true,
"load_user": false,
"project_dir": "/path/in/workspace",
"org_config": null, // optional, if you ever want org hooks
"sandbox_config": null // probably irrelevant for hooks
}Response
{
"hook_config": { ... }, // HookConfig JSON
"sources": { "project": 1, "user": 0 }
}Even if you start with just project_dir + booleans, leaving room for sources is useful for debugging.
2) Where merging happens
If app-server merges, you need a stable precedence rule. For consistency with this PR discussion, likely:
- request explicit hooks (highest)
- project hooks
- user hooks (lowest)
But note: with hooks, order can matter (since they’re executed sequentially). Your current merge semantics are concatenation-like; precedence mostly matters for “override” cases. So you’ll want to make sure HookConfig.merge() semantics are what you want (I believe it concatenates lists per event, which means “precedence” is effectively “execution order”).
So define it explicitly:
- “explicit hooks run first” vs “repo hooks run first” etc.
- For safety, I often prefer: request hooks first, then repo hooks, then user hooks (because request hooks are “session policy”). But you might prefer repo hooks first (repo policy) and user last. Either is defensible; just make it intentional.
3) What about user hooks in SaaS?
Same issue as skills:
$HOME/.openhands/hooks.jsonin SaaS is runtime home, not the end-user machine.
So app-server might choose:load_user=falsein hosted modeload_project=truealways
This becomes easy with the endpoint design.
Pros / Cons vs Option B (flags in start request)
Pros
- Keeps
StartConversationRequestschema stable-ish (no new flags required there). - Makes hook loading explicit and traceable (you can log
/hookscall separately). - Keeps
event_service.start()deterministic: no extra implicit filesystem reads on start. - Mirrors skills design, reduces conceptual inconsistency.
Cons
- Extra network round-trip during conversation start.
- More moving parts: new router/service in agent-server + app-server call site.
- You still need to decide what CLI does (but CLI could just load hooks locally, no need for an endpoint there).
One subtlety: why not have agent-server merge in /hooks endpoint and return merged result?
You can, but if app-server is already the orchestrator, letting it own the merge is nice because it can apply product-specific policy (“hosted mode: no user hooks”). The agent-server can stay a pure “loader”.
There was a problem hiding this comment.
(HUMAN) TBH I kinda incline to this Option F, or at least it seems worth considering.
IIRC, some initial implementation of skills was on app-server (trying to load directly from workspace or something), and we didn’t like it and it didn’t work for some sources (it was duplicating loading that was done in the SDK, at a minimum).
So we ended up linking it up here, so that
- SDK does the loading, exposing it to any type of client, python, cli etc
- agent-server exposes
/skillsendpoint - app-server decides policy (from this source, not from that etc), when it calls it.
It is a bit more complicated, yes. 🤔 But idk. Is the hooks case a lot different than skills? Does it make sense to you?
4 participants
Summary
Enable V1 agent‑server to auto‑load .openhands/hooks.json from the workspace and merge it with any request‑provided hooks (request hooks first), so repo‑level hooks run in SaaS just like CLI.
Checklist
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:ee040f3-pythonRun
All tags pushed for this build
About Multi-Architecture Support
ee040f3-python) is a multi-arch manifest supporting both amd64 and arm64ee040f3-python-amd64) are also available if needed