feat(agent-context): add load_project_skills flag with server-side injection#3346
Conversation
…jection AgentContext could auto-load user and public skills, but not project skills from the conversation workspace: project skills require the workspace path, which is not known at AgentContext validation time. Add a `load_project_skills` request flag on AgentContext and resolve it server-side at conversation start (where the workspace path is known), merging project skills into agent_context.skills with project precedence. This replaces the client-side project-skill injection in agent-canvas#707.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
There was a problem hiding this comment.
Clean, well-scoped addition. The design choice of deferring project skill resolution to the server (rather than AgentContext itself) is clearly motivated, and the merge semantics (project skills win on name collision) are correct. Tests cover the key cases thoroughly.
Two issues worth addressing before merging, plus one suggestion.
This review was generated by an AI agent (OpenHands) on behalf of the user via OpenHands Automation.
Move project-skill resolution from the agent-server into LocalConversation._ensure_plugins_loaded, where the workspace path is also known. This makes AgentContext(load_project_skills=True) behave consistently for all SDK users (standalone LocalConversation as well as agent-server, which runs through LocalConversation) instead of being silently ignored outside agent-server. The agent-server _inject_project_skills helper is removed as redundant. Tests move to the SDK conversation suite.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Taste Rating: Good taste — the implementation is small and the current diff resolves the earlier server-only design concern.
I didn’t find new blocking code issues. The remaining actionable item is the existing AgentContext.load_project_skills description thread: when updating the example paths, also change “the agent-server resolves it” to the conversation/LocalConversation resolving it before first use, so the ACP/schema text matches this revision.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. This changes agent prompt/context behavior by injecting project skills, so per repo policy I’m leaving a COMMENT rather than approving until a human maintainer decides after lightweight eval validation.
This review was created by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26231837794
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Project skill auto-loading was functionally verified in standalone SDK conversation flows: before the PR the flag was unavailable/no-op, and at e2fb5ba it loads workspace AGENTS.md into the live agent context and system prompt only when enabled.
Does this PR achieve its stated goal?
Yes. The PR set out to add an opt-in AgentContext.load_project_skills flag so project skills from the conversation workspace reach conversations, while keeping default behavior unchanged and allowing project skills to override same-named context skills. Running a real LocalConversation with a temporary workspace confirmed all three behaviors: enabled loads the agents skill and prompt sentinel, default-off does not load it, and project agents content replaces an existing agents skill while preserving unrelated skills.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no tests/linters were run locally. |
| CI Status | gh pr checks: core tests/pre-commit/API checks were green, unresolved-review-threads was failing, and several Agent Server/OpenHands review jobs were still in progress. |
| Functional Verification | ✅ Real SDK conversation execution verified before/after behavior and field serialization. |
Functional Verification
Test 1: Project skills reach conversations only when opted in
Step 1 — Reproduce / establish baseline without the fix:
Ran:
git checkout --detach origin/main
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_project_skills_quiet.pyObserved output summary:
{
"default_off": {"field_available": false, "flag_value": "<missing>", "skill_names": [], "dynamic_contains_sentinel": false},
"enabled": {"field_available": false, "flag_value": "<missing>", "skill_names": [], "dynamic_contains_sentinel": false},
"merge_precedence": {"field_available": false, "flag_value": "<missing>", "skill_names": ["agents", "keep"], "old_agents_content_present": true, "keep_preserved": true}
}This confirms the baseline problem: load_project_skills was not available, passing it did not load the workspace AGENTS.md, and an existing same-named agents skill stayed as OLD_CONTENT.
Step 2 — Apply the PR's changes:
Checked out the PR commit:
git checkout --detach e2fb5ba06890e5c2753dcc431b1e44da63bc487eStep 3 — Re-run with the fix in place:
Ran the same SDK conversation script:
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_project_skills_quiet.pyObserved output summary:
{
"default_off": {"field_available": true, "flag_value": false, "skill_names": [], "dynamic_contains_sentinel": false},
"enabled": {"field_available": true, "flag_value": true, "skill_names": ["agents"], "agents_contains_sentinel": true, "dynamic_contains_sentinel": true},
"merge_precedence": {"field_available": true, "flag_value": true, "skill_names": ["agents", "keep"], "agents_contains_sentinel": true, "old_agents_content_present": false, "keep_preserved": true}
}This confirms the fix works: the enabled flag loads project skill content into both the live agent_context.skills and rendered dynamic prompt, default behavior remains off, and project agents overrides old agents while preserving the unrelated keep skill.
Test 2: New field is exposed for settings-style round trip
Ran:
OPENHANDS_SUPPRESS_BANNER=1 uv run python -c "from openhands.sdk.context import AgentContext; f=AgentContext.model_fields.get('load_project_skills'); print({'field_available': f is not None, 'default': f.default, 'json_schema_extra': f.json_schema_extra, 'dump_true': AgentContext(load_project_skills=True).model_dump().get('load_project_skills'), 'dump_default': AgentContext().model_dump().get('load_project_skills')})"Observed output:
{"default": false, "dump_default": false, "dump_true": true, "field_available": true, "json_schema_extra": {"acp_compatible": true}}This confirms the field is opt-in by default, serializes through model_dump(), and carries the expected ACP-compatible metadata.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
… load - Correct the load_project_skills doc examples to match what is actually loaded (.openhands/skills/, AGENTS.md). - Guard project-skill loading so a workspace read error cannot block conversation start, with a test covering the failure path.
|
Thanks for the review — addressed in d67124f:
|
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Taste Rating: Good taste — this is a small, well-scoped change, and I did not find new code-level issues in the current diff.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. This changes agent prompt/context behavior by injecting project skills, so per repo policy I’m leaving a COMMENT rather than approving until a human maintainer decides after lightweight eval validation.
This review was created by an AI agent (OpenHands) on behalf of the user.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/software-agent-sdk/actions/runs/26232624458
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review — feat(agent-context): add load_project_skills flag with server-side injection
🟢 Good taste — The current revision is clean, well-scoped, and addresses all concerns raised in previous review rounds.
What the PR does
AgentContext gains a new opt-in load_project_skills: bool = False flag. When enabled, LocalConversation._ensure_plugins_loaded resolves project skills from the conversation workspace and merges them into the live agent context (project skills win on name collision). The flag is gated off by default, so existing users are entirely unaffected.
Design decisions — confirmed correct
-
Resolution in
LocalConversationrather thanAgentContext— The workspace path isn't known atAgentContextvalidation time, so lazy resolution is the only sound approach. Moving this out of the agent-server and intoLocalConversationensures the flag behaves consistently for standalone SDK users and server-hosted conversations alike. -
project_skills_loadedgate on the agent update — Only triggersmodel_copywhen skills were actually merged, not just when the flag is set. No unnecessary agent state mutation when the workspace has no skills. -
Idempotency — The existing
_plugins_loadedguard prevents re-injection on resume or subsequentsend_messagecalls. -
Merge semantics —
{s.name: s for s in merged_context.skills}then.update(project_skills)gives project skills precedence while preserving all other pre-existing skills. This is the documented and tested behavior.
Outstanding thread — now addressed
The one unresolved thread (🟡 Suggestion on local_conversation.py, now outdated) asked for best-effort error handling around load_available_skills. The current diff implements exactly that:
try:
project_skills = load_available_skills(...)
except Exception:
logger.warning("Failed to load project skills; continuing without them", exc_info=True)
project_skills = {}The except Exception scope is intentionally broad here — a filesystem permission error, a malformed skill file, or any other transient failure must not block conversation startup. The warning is logged with exc_info=True so the root cause remains diagnosable.
Tests
Four test cases in test_repo_root_project_skills.py cover the critical paths:
- Flag on → skills injected into live agent context and system prompt
- Flag off (default) → no injection
- Merge semantics → project skills override same-named context skills; others preserved
- Load failure →
PermissionErrorswallowed, pre-existing skills untouched, conversation starts normally
The mock in the fourth test is appropriate — it targets the error-recovery code path specifically, not the loading logic itself, and the assertion checks real state (skills list on the live agent context).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM. This changes agent prompt/context behavior by injecting additional skills whenload_project_skills=True. Thedefault=Falsegate means no behavioral change for any existing user. The only risk surface is the new code path itself — which is guarded by best-effort error handling and covered by the new test suite.
VERDICT: ✅ Worth merging. The design is sound, all previous concerns are resolved, and the implementation is minimal and focused.
KEY INSIGHT: Deferring workspace-path-dependent resolution to LocalConversation (rather than AgentContext) is the correct architectural boundary — it keeps AgentContext a pure configuration object while ensuring the flag works uniformly across all conversation backends.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch with the/codereviewtrigger and the context the reviewer is missing.- Re-request a review — the reviewer reads guidelines from the PR branch, so changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
all-hands-bot
left a comment
There was a problem hiding this comment.
Clean, well-motivated implementation. Deferring project skill resolution to LocalConversation._ensure_plugins_loaded (rather than AgentContext or the agent-server) is the right call — it means load_project_skills=True works identically for standalone SDK users and server-hosted conversations, with the correct worktree path in both cases.
One documentation inaccuracy found below. Also noting that the existing unresolved thread on local_conversation.py (error-handling concern raised against an earlier revision) is already addressed by the try/except block in the current diff — that thread can safely be marked resolved.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Standalone SDK project-skill loading works as described: the PR turns an ignored request flag into opt-in project context injection without changing the default-off behavior.
Does this PR achieve its stated goal?
Yes. I reproduced the pre-PR behavior on origin/main: requesting load_project_skills=True did not add the workspace AGENTS.md skill to the live agent context or system prompt. On PR head d67124f, the same LocalConversation flow loaded the project agents skill, rendered the sentinel into dynamic context, preserved unrelated existing skills, and overrode a same-named existing agents skill.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run python created the project env, installed packages, and imported SDK classes successfully |
| CI Status | |
| Functional Verification | ✅ Before/after standalone SDK conversation exercised successfully |
Functional Verification
Test 1: load_project_skills=True injects workspace project guidance
Step 1 — Reproduce / establish baseline without the fix:
Ran on origin/main (604f3111):
git switch --detach origin/main
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_project_skills.pyRelevant output:
{
"flag_on": {
"live_context_has_load_project_skills": false,
"project_sentinel_in_dynamic_context": false,
"project_sentinel_in_live_agents_skill": false,
"skill_names": []
}
}This confirms the old behavior: passing the request flag did not result in project skills being present in the live context or rendered prompt.
Step 2 — Apply the PR's changes:
Checked out PR head d67124f21e3926f30af5d77a3a01660118f368b8.
Step 3 — Re-run with the fix in place:
Ran the same script:
git switch --detach d67124f21e3926f30af5d77a3a01660118f368b8
OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_project_skills.pyRelevant output:
{
"flag_on": {
"live_context_has_load_project_skills": true,
"project_sentinel_in_dynamic_context": true,
"project_sentinel_in_live_agents_skill": true,
"skill_names": ["agents"]
}
}This shows the requested flag is now part of the live context and the workspace AGENTS.md content reaches the actual system prompt used by the conversation.
Test 2: default-off behavior is preserved
PR output for the same script with no flag set:
{
"flag_off": {
"project_sentinel_in_dynamic_context": false,
"project_sentinel_in_live_agents_skill": false,
"skill_names": []
}
}This confirms users who do not opt in still avoid project-skill injection.
Test 3: project skills take precedence while preserving other skills
Baseline on origin/main with an existing agents skill retained the old content and did not load the project sentinel:
{
"merge_precedence": {
"keep_existing_skill_present": true,
"old_agents_content_present": true,
"project_sentinel_in_dynamic_context": false,
"project_sentinel_in_live_agents_skill": false,
"skill_names": ["agents", "keep"]
}
}PR output for the same scenario:
{
"merge_precedence": {
"keep_existing_skill_present": true,
"old_agents_content_present": false,
"project_sentinel_in_dynamic_context": true,
"project_sentinel_in_live_agents_skill": true,
"skill_names": ["agents", "keep"]
}
}This confirms the PR's stated merge semantics: unrelated context skills remain, while project skills override same-named context skills.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
| logger.info(f"Loaded {len(self._plugin_specs)} plugin(s) via Conversation") | ||
|
|
||
| # Resolve project skills from the workspace. AgentContext can't do this | ||
| # itself (the workspace path is unknown at validation time), so it is done |
There was a problem hiding this comment.
I’m a bit surprised about this… is there a reason why this worked on app-server and not in agent-canvas? I thought app-server uses /skills and loads these too? Or did we change how loading skills works?
There was a problem hiding this comment.
@OpenHands investigate (clone yourself agent-canvas and openhands/openhands too) and respond directly in this thread please.
There was a problem hiding this comment.
Uh oh! There was an unexpected error starting the job :(
There was a problem hiding this comment.
@OpenHands investigate (clone yourself agent-canvas and openhands/openhands too) and respond directly in this thread please.
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.
OpenHands encountered an error: Request timeout after 30 seconds to https://lzjukjubtpfwagon.prod-runtime.all-hands.dev/api/conversations/321822f7-47bf-4548-9bc0-fc102f24ac04/ask_agent
See the conversation for more information.
There was a problem hiding this comment.
The exact SDK agent-server code path I meant is here:
/api/skillsroute forwards the request flags andproject_dirintoload_all_skills(...):load_all_skills(...)then loads project skills specifically withwork_dir=project_dirandinclude_project=load_project:- That calls the SDK helper branch here (
load_available_skills→load_project_skills(work_dir)):software-agent-sdk/openhands-sdk/openhands/sdk/skills/skill.py
Lines 1196 to 1201 in 63fc397
- And
load_project_skills(...)is the function that scans the actual project locations (.agents/skills, legacy.openhands/skills,.openhands/microagents, plus AGENTS.md/.cursorrules-style files):software-agent-sdk/openhands-sdk/openhands/sdk/skills/skill.py
Lines 851 to 926 in 63fc397
So when app-server called the SDK agent-server /api/skills endpoint with load_project=True + project_dir, those lines are what loaded the project skills. The missing part in agent-canvas was not this loader; it was taking that loaded catalog and putting it onto the conversation's agent_context.skills (or now, with this PR, setting load_project_skills: true so LocalConversation does it).
This reply was generated by an AI agent (OpenHands) on behalf of the user.
There was a problem hiding this comment.
@VascoSch92 WDYT, it seems maybe we could avoid to create a redundant loading function?
There was a problem hiding this comment.
Hey @enyst
Two pieces are involved here:
- the loader (
load_available_skills→load_project_skills), and - the merge step that folds loaded skills into
agent_context.skills.
The loader is already the single shared entry point: both the AgentContext auto-load path and this PR go through load_available_skills, so that part isn't duplicated. What was duplicated is the small "merge by name" step, hand-rolled in two spots: AgentContext._load_auto_skills (user/public) and LocalConversation._ensure_plugins_loaded (project).
I pulled that into a shared merge_skills_by_name(primary, secondary) in openhands.sdk.skills and routed both through it:
_load_auto_skills:merge_skills_by_name(self.skills, auto_skills.values()), explicit skills stay authoritative._ensure_plugins_loaded:merge_skills_by_name(project_skills.values(), merged_context.skills), project skills stay authoritative.
I kept the orchestration split on purpose, though: user/public resolve eagerly at AgentContext validation, but project skills can't because work_dir isn't known then, and the workspace LocalConversation actually runs in is the resolved worktree path (the agent-canvas#707 concern). That's also why I avoided adding a work_dir/project_dir field to AgentContext: it'd be set before the real workspace is known. So the two sites resolve different sources at the only times each can, now sharing one merge primitive instead of two copies. Precedence stays explicit: public < user < explicit < project.
Happy to reshape if you'd rather it land differently.
|
@OpenHands Look for all functions or methods loading skills. I think maybe we have one … load_workspace_skills maybe, or load_project_skills? Tell me a summary of all loading skills execution paths. Respond directly on github. |
|
Uh oh! There was an unexpected error starting the job :( |
|
@OpenHands Look for all functions or methods loading skills. I think maybe we have one … load_workspace_skills maybe, or load_project_skills? Tell me a summary of all loading skills execution paths. Respond directly on github. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
@enyst I traced the skill-loading paths. I do not see a Summary of the execution paths I found: Core SDK loaders
Runtime / injection paths
After skills are loaded
So the naming split is: This comment was generated by an AI agent (OpenHands) on behalf of the user. |
|
OpenHands encountered an error: Request timeout after 30 seconds to https://gifpmmgdipgfbruh.prod-runtime.all-hands.dev/api/conversations/57cf71f7-5ac5-45d2-bdd6-ecdcfb32a9bf/ask_agent See the conversation for more information. |
…ject injection AgentContext._load_auto_skills (user/public) and LocalConversation._ensure_plugins_loaded (project) each hand-rolled the same by-name merge. Factor it into skills.merge_skills_by_name, where the higher-precedence source is passed as 'primary'. Loading stays split by design: user/public resolve eagerly at AgentContext validation, project skills resolve at conversation start where the (worktree) workspace is known. Only the merge step is shared. Precedence is unchanged: explicit skills stay authoritative over auto-loaded ones, and project skills stay authoritative over same-named context skills.
all-hands-bot
left a comment
There was a problem hiding this comment.
The design is sound and the implementation is clean. Resolving load_project_skills inside LocalConversation._ensure_plugins_loaded rather than in AgentContext itself is the correct call — it ensures the flag works consistently for standalone SDK users, not just agent-server users. Test coverage is solid (flag-on, flag-off, merge precedence, and failure resilience). Three focused observations below.
This review was generated by an AI agent (OpenHands) on behalf of the user via OpenHands Automation.
…tore skip log, decouple docstring - AgentContext.load_project_skills: note that resolved project skills override same-named explicit skills (opposite of user/public, which yield to explicit). - _load_auto_skills: restore the debug log naming auto-skills skipped due to an explicit name conflict, dropped in the merge_skills_by_name refactor. - merge_skills_by_name: drop the docstring paragraph enumerating private call sites; the contract paragraph stands on its own.
all-hands-bot
left a comment
There was a problem hiding this comment.
The design is well-reasoned: deferring project-skill resolution to LocalConversation._ensure_plugins_loaded (where workspace.working_dir is known) keeps the flag consistent for both standalone SDK and agent-server usage, and the _plugins_loaded guard naturally makes it idempotent on conversation resume. The refactoring that extracted merge_skills_by_name removes the hand-rolled duplicate merge loop and makes the precedence chain explicit (public < user < explicit < project). Test coverage is thorough — the four targeted scenarios (flag on, flag off, merge precedence, failure resilience) cover the key observable behaviours.
A few items from the previous review round still apply to the current diff and are worth resolving before merge:
agent_context.pyline 102:.openhands/skills/in the field description is the legacy path;.agents/skills/is the current preferred location (see unresolved comment #3282006522).agent_context.pyline 100: the field description does not state that project skills loaded via this flag replace same-named explicit skills — the inverse of_load_auto_skillsprecedence where explicit skills win (see unresolved comment #3302617241).agent_context.pyline 177: the refactoring silently dropped thelogger.debug('Skipping auto-loaded skill ...')call that helped diagnose name-conflict suppression (see unresolved comment #3302617246).skill.pylines 1217-1220: themerge_skills_by_namedocstring couples itself to private call-site names that are implementation details (see unresolved comment #3302617237).
This review was generated by an AI agent (OpenHands) on behalf of the user via OpenHands Automation.
Summary
Adds auto-loading of project skills (from the conversation workspace) into an agent, fixing agent-canvas#574 — repo skills in
.agents/skills/show in settings but never reach conversations — and shipping theextension_config-style field anticipated by agent-canvas#707.AgentContextalready auto-loads user and public skills viaload_user_skills/load_public_skills. It could not auto-load project skills because they require the workspace path, which is unknown atAgentContextvalidation time.Approach
AgentContext.load_project_skills(newacp_compatibleflag): a request flag. Like the other auto-load flags it is opt-in (defaultFalse); unlike them it is resolved by the conversation, not byAgentContext.LocalConversation._ensure_plugins_loaded: resolves project skills fromself.workspace.working_dirand merges them into the agent context (project skills take precedence over same-named existing skills). This reuses the existing workspace-aware skill/MCP merge + write-back point.Resolving in
LocalConversation(rather than in the agent-server) keeps the flag consistent for all SDK users: it works for standaloneLocalConversationand for the agent-server, which runs conversations throughLocalConversation. An earlier revision resolved this only in the agent-server, which leftAgentContext(load_project_skills=True)silently ignored in standalone SDK usage — addressed by review feedback.Notes
load_available_skills(include_project=True). Org/sandbox sources are unaffected.LocalConversationis already the worktree workspace, so resolution uses the correct dir (the concern raised in agent-canvas#707).Falseflag, so no extra disk IO for anyone who doesn't opt in. Idempotent on resume (_plugins_loadedguard, name-keyed merge)._serialize_skillstrimming inagent_context.pydoes not drop them.Follow-up in agent-canvas
After this lands,
createAgentFromSettings()inagent-server-adapter.tsjust needsload_project_skills: true. Noproject_dirplumbing is required — the server uses the conversation workspace.Test plan
tests/sdk/conversation/test_repo_root_project_skills.py: flag injects skills end-to-end in standalone SDK (system prompt + live agent context); flag off injects nothing; project skills override same-named context skills while others are preserved.pre-commit(ruff, pyright, pycodestyle, import rules) passing on changed files.Draft: opening for design feedback.
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.13-nodejs22-slimgolang: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:9a6a4c1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9a6a4c1-python) is a multi-arch manifest supporting both amd64 and arm649a6a4c1-python-amd64) are also available if needed