Skip to content

feat(agent-context): add load_project_skills flag with server-side injection#3346

Merged
VascoSch92 merged 9 commits into
mainfrom
feat/agent-context-project-skills
May 27, 2026
Merged

feat(agent-context): add load_project_skills flag with server-side injection#3346
VascoSch92 merged 9 commits into
mainfrom
feat/agent-context-project-skills

Conversation

@VascoSch92
Copy link
Copy Markdown
Member

@VascoSch92 VascoSch92 commented May 21, 2026

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 the extension_config-style field anticipated by agent-canvas#707.

AgentContext already auto-loads user and public skills via load_user_skills / load_public_skills. It could not auto-load project skills because they require the workspace path, which is unknown at AgentContext validation time.

Approach

  • AgentContext.load_project_skills (new acp_compatible flag): a request flag. Like the other auto-load flags it is opt-in (default False); unlike them it is resolved by the conversation, not by AgentContext.
  • LocalConversation._ensure_plugins_loaded: resolves project skills from self.workspace.working_dir and 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 standalone LocalConversation and for the agent-server, which runs conversations through LocalConversation. An earlier revision resolved this only in the agent-server, which left AgentContext(load_project_skills=True) silently ignored in standalone SDK usage — addressed by review feedback.

Notes

  • Scoped to project skills via load_available_skills(include_project=True). Org/sandbox sources are unaffected.
  • Worktree-safe: in agent-server the workspace handed to LocalConversation is already the worktree workspace, so resolution uses the correct dir (the concern raised in agent-canvas#707).
  • Gated behind a default-False flag, so no extra disk IO for anyone who doesn't opt in. Idempotent on resume (_plugins_loaded guard, name-keyed merge).
  • Project skills are injected as explicit (non-auto-loaded) entries, so the _serialize_skills trimming in agent_context.py does not drop them.

Follow-up in agent-canvas

After this lands, createAgentFromSettings() in agent-server-adapter.ts just needs load_project_skills: true. No project_dir plumbing 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

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.13-nodejs22-slim Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:9a6a4c1-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-9a6a4c1-python \
  ghcr.io/openhands/agent-server:9a6a4c1-python

All tags pushed for this build

ghcr.io/openhands/agent-server:9a6a4c1-golang-amd64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-golang-amd64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-golang-amd64
ghcr.io/openhands/agent-server:9a6a4c1-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:9a6a4c1-golang-arm64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-golang-arm64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-golang-arm64
ghcr.io/openhands/agent-server:9a6a4c1-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:9a6a4c1-java-amd64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-java-amd64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-java-amd64
ghcr.io/openhands/agent-server:9a6a4c1-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:9a6a4c1-java-arm64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-java-arm64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-java-arm64
ghcr.io/openhands/agent-server:9a6a4c1-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:9a6a4c1-python-amd64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-python-amd64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-python-amd64
ghcr.io/openhands/agent-server:9a6a4c1-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-amd64
ghcr.io/openhands/agent-server:9a6a4c1-python-arm64
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-python-arm64
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-python-arm64
ghcr.io/openhands/agent-server:9a6a4c1-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim-arm64
ghcr.io/openhands/agent-server:9a6a4c1-golang
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-golang
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-golang
ghcr.io/openhands/agent-server:9a6a4c1-golang_tag_1.21-bookworm
ghcr.io/openhands/agent-server:9a6a4c1-java
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-java
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-java
ghcr.io/openhands/agent-server:9a6a4c1-eclipse-temurin_tag_17-jdk
ghcr.io/openhands/agent-server:9a6a4c1-python
ghcr.io/openhands/agent-server:9a6a4c1ea574ae209b01fe75c3be8b0bab2c584d-python
ghcr.io/openhands/agent-server:feat-agent-context-project-skills-python
ghcr.io/openhands/agent-server:9a6a4c1-nikolaik_s_python-nodejs_tag_python3.13-nodejs22-slim

About Multi-Architecture Support

  • Each variant tag (e.g., 9a6a4c1-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 9a6a4c1-python-amd64) are also available if needed

…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.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

Python API breakage checks — ✅ PASSED

Result:PASSED

Action log

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

REST API breakage checks (OpenAPI) — ✅ PASSED

Result:PASSED

Action log

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/context
   agent_context.py151696%334, 351–352, 420, 443, 449
openhands-sdk/openhands/sdk/conversation/impl
   local_conversation.py5424491%309, 314, 458, 504, 573, 589, 654, 874–875, 878, 991, 1002–1005, 1012–1013, 1016, 1022–1023, 1026, 1032, 1047, 1050, 1054–1055, 1059–1061, 1068, 1154, 1159, 1269, 1271, 1275–1276, 1287–1288, 1313, 1508, 1512, 1582, 1589–1590
openhands-sdk/openhands/sdk/skills
   skill.py4632893%100–101, 267–268, 464, 585–588, 775–776, 848–849, 908–909, 1001–1002, 1079, 1107, 1130, 1137–1138, 1187–1188, 1194–1195, 1201–1202
TOTAL27853818770% 

@VascoSch92 VascoSch92 requested a review from all-hands-bot May 21, 2026 14:01
all-hands-bot

This comment was marked as outdated.

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread openhands-sdk/openhands/sdk/context/agent_context.py Outdated
Comment thread openhands-agent-server/openhands/agent_server/conversation_service.py Outdated
Comment thread openhands-agent-server/openhands/agent_server/conversation_service.py Outdated
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

This comment was marked as outdated.

@VascoSch92 VascoSch92 requested a review from all-hands-bot May 21, 2026 14:19
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

This comment was marked as outdated.

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 ⚠️ Snapshot observed via 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.py

Observed 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 e2fb5ba06890e5c2753dcc431b1e44da63bc487e

Step 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.py

Observed 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.
@VascoSch92
Copy link
Copy Markdown
Member Author

Thanks for the review — addressed in d67124f:

  • Doc example paths (agent_context.py): updated to .openhands/skills/, AGENTS.md to match what's actually loaded.
  • Error handling (now on the LocalConversation path): wrapped the project-skill load in a best-effort try/except that logs and continues, so a workspace read error can't block conversation start. Added test_load_project_skills_failure_does_not_block_conversation covering the failure path. Note load_available_skills already guards the project source internally (skill.py:1196), so this is belt-and-suspenders.
  • conversation_service.py comments (lines 98 / 591): these are stale — that _inject_project_skills helper was removed when resolution moved into LocalConversation (option 1, per earlier review feedback), so the LocalWorkspace/BaseWorkspace typing and call-site concerns no longer apply. The agent-server now picks this up automatically by running through LocalConversation.

@VascoSch92 VascoSch92 requested a review from all-hands-bot May 21, 2026 14:32
@VascoSch92 VascoSch92 marked this pull request as ready for review May 21, 2026 14:33
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Resolution in LocalConversation rather than AgentContext — The workspace path isn't known at AgentContext validation time, so lazy resolution is the only sound approach. Moving this out of the agent-server and into LocalConversation ensures the flag behaves consistently for standalone SDK users and server-hosted conversations alike.

  2. project_skills_loaded gate on the agent update — Only triggers model_copy when skills were actually merged, not just when the flag is set. No unnecessary agent state mutation when the workspace has no skills.

  3. Idempotency — The existing _plugins_loaded guard prevents re-injection on resume or subsequent send_message calls.

  4. 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 → PermissionError swallowed, 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 when load_project_skills=True. The default=False gate 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:

  1. Add a .agents/skills/custom-codereview-guide.md file to your branch with the /codereview trigger and the context the reviewer is missing.
  2. Re-request a review — the reviewer reads guidelines from the PR branch, so changes take effect immediately.
  3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread openhands-sdk/openhands/sdk/context/agent_context.py
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 ⚠️ 19 successful, 1 failing Review Thread Gate, 8 pending, 4 skipped when checked; I did not rerun tests
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.py

Relevant 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.py

Relevant 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.

@VascoSch92 VascoSch92 requested a review from enyst May 22, 2026 14:21
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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OpenHands investigate (clone yourself agent-canvas and openhands/openhands too) and respond directly in this thread please.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh! There was an unexpected error starting the job :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OpenHands investigate (clone yourself agent-canvas and openhands/openhands too) and respond directly in this thread please.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exact SDK agent-server code path I meant is here:

  • /api/skills route forwards the request flags and project_dir into load_all_skills(...):
    # Call the service
    result = load_all_skills(
    load_public=request.load_public,
    load_user=request.load_user,
    load_project=request.load_project,
    load_org=request.load_org,
    project_dir=request.project_dir,
    org_repo_url=org_repo_url,
    org_name=org_name,
    sandbox_exposed_urls=sandbox_urls,
  • load_all_skills(...) then loads project skills specifically with work_dir=project_dir and include_project=load_project:
    # 5. Load project skills (highest precedence)
    project_skills = load_available_skills(
    work_dir=project_dir if load_project else None,
    include_user=False,
    include_project=load_project,
    include_public=False,
    )
    sources["project"] = len(project_skills)
    skill_lists.append(list(project_skills.values()))
  • That calls the SDK helper branch here (load_available_skillsload_project_skills(work_dir)):
    if include_project and work_dir:
    try:
    for s in load_project_skills(work_dir):
    available[s.name] = s
    except Exception as e:
    logger.warning(f"Failed to load project skills: {e}")
  • 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):
    def load_project_skills(work_dir: str | Path) -> list[Skill]:
    """Load skills from project-specific directories.
    Searches for skills in {work_dir}/.agents/skills/, {work_dir}/.openhands/skills/,
    and {work_dir}/.openhands/microagents/ (legacy).
    If the working directory is inside a Git repository, this function also loads
    skills from the Git repo root, so running from a subdirectory still picks up
    repo-level guidance (e.g., AGENTS.md).
    Skills are merged in priority order, with the *working directory* taking
    precedence over the Git repo root when duplicates exist.
    Use .agents/skills for new skills. .openhands/skills is the legacy OpenHands
    location, and .openhands/microagents is deprecated.
    Example: If "my-skill" exists in both .agents/skills/ and .openhands/skills/,
    the version from .agents/skills/ is used.
    Also loads third-party skill files (AGENTS.md, .cursorrules, etc.) from the
    working directory and (if different) the git repo root.
    Args:
    work_dir: Path to the project/working directory.
    Returns:
    List of Skill objects loaded from project directories.
    Returns empty list if no skills found or loading fails.
    """
    if isinstance(work_dir, str):
    work_dir = Path(work_dir)
    all_skills = []
    seen_names: set[str] = set()
    git_root = _find_git_repo_root(work_dir)
    # Working dir takes precedence (more local rules override repo root rules)
    search_roots: list[Path] = [work_dir]
    if git_root is not None and git_root != work_dir:
    search_roots.append(git_root)
    # First, load third-party skill files (AGENTS.md, .cursorrules, etc.) from each
    # search root. This ensures they are loaded even if .openhands/skills doesn't
    # exist.
    for root in search_roots:
    third_party_files = find_third_party_files(
    root, Skill.PATH_TO_THIRD_PARTY_SKILL_NAME
    )
    for path in third_party_files:
    try:
    skill = Skill.load(path)
    if skill.name not in seen_names:
    all_skills.append(skill)
    seen_names.add(skill.name)
    logger.debug(f"Loaded third-party skill: {skill.name} from {path}")
    except (SkillError, OSError, yaml.YAMLError) as e:
    logger.warning(f"Failed to load third-party skill from {path}: {e}")
    # Load project-specific skills from .agents/skills, .openhands/skills,
    # and legacy microagents (priority order; first wins for duplicates)
    for root in search_roots:
    project_skills_dirs = [
    root / ".agents" / "skills",
    root / ".openhands" / "skills",
    root / ".openhands" / "microagents", # Legacy support
    ]
    _load_and_merge_from_dirs(
    project_skills_dirs, seen_names, all_skills, "project skills"
    )
    logger.debug(
    f"Loaded {len(all_skills)} project skills: {[s.name for s in all_skills]}"
    )
    return all_skills

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VascoSch92 WDYT, it seems maybe we could avoid to create a redundant loading function?

Copy link
Copy Markdown
Member Author

@VascoSch92 VascoSch92 May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @enyst

Two pieces are involved here:

  • the loader (load_available_skillsload_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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

@enyst
Copy link
Copy Markdown
Member

enyst commented May 22, 2026

@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.

@openhands-ai
Copy link
Copy Markdown

openhands-ai Bot commented May 22, 2026

Uh oh! There was an unexpected error starting the job :(

@enyst
Copy link
Copy Markdown
Member

enyst commented May 22, 2026

@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.

@openhands-ai
Copy link
Copy Markdown

openhands-ai Bot commented May 22, 2026

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

Copy link
Copy Markdown
Member

enyst commented May 22, 2026

@enyst I traced the skill-loading paths. I do not see a load_workspace_skills function/method in this repo; the workspace/project concepts are covered by load_project_skills(...) and by the remote-workspace helper RemoteWorkspace.load_skills_from_agent_server(...).

Summary of the execution paths I found:

Core SDK loaders

  • Skill.load(...) (openhands-sdk/openhands/sdk/skills/skill.py) is the leaf parser. It loads either AgentSkills-style SKILL.md directories or legacy OpenHands .md skills, including frontmatter, AgentSkills .mcp.json, and resource dirs.
  • load_skills_from_dir(...) discovers skills/<name>/SKILL.md and regular .md files under one skills dir, calls Skill.load, and categorizes them as repo / knowledge / agentskills. It intentionally does not load AGENTS.md / .cursorrules directly.
  • load_user_skills() loads ~/.agents/skills, ~/.openhands/skills, and legacy ~/.openhands/microagents; then it appends enabled installed skills via load_installed_skills() with lower precedence.
  • load_project_skills(work_dir) loads from the working dir and, if different, the git repo root. It first loads third-party instruction files like AGENTS.md, .cursorrules, agent.md, claude.md, gemini.md, then .agents/skills, .openhands/skills, and legacy .openhands/microagents. Working dir wins over repo root; .agents/skills wins over legacy dirs for duplicate names.
  • load_public_skills() clones/updates the public OpenHands/extensions cache, filters by marketplace when configured, and loads those skills.
  • load_available_skills(...) is the shared SDK merger for public/user/project, with precedence public < user < project.

Runtime / injection paths

  1. Explicit context: callers can pass AgentContext(skills=[...]). Those skills are already loaded objects; no disk discovery happens except validation.
  2. AgentContext auto-load: AgentContext._load_auto_skills() runs during model validation when load_user_skills and/or load_public_skills is true. It calls load_available_skills(work_dir=None, include_project=False, ...); explicit skills with the same name win over auto-loaded user/public skills.
  3. This PR’s project-skill path: LocalConversation._ensure_plugins_loaded() runs lazily before first send_message() / run().
    • It already loads plugins through Plugin.load(...) → plugin _load_skills(...) from plugin_dir/skills, then Plugin.add_skills_to(...) merges plugin skills into the agent context.
    • With this PR, if agent_context.load_project_skills is true, it then calls load_available_skills(work_dir=self.workspace.working_dir, include_project=True, include_user=False, include_public=False) and writes the merged context back to self.agent and conversation state. Project skills override same-named existing/context/plugin skills here.
    • This is also the agent-server conversation path, because server conversations run through LocalConversation; it is not only a standalone SDK path.
  4. Agent-server /api/skills catalog path: skills_router.get_skills()skills_service.load_all_skills(...). This can load sandbox-generated work_hosts, SDK public/user, org skills (load_org_skills_from_url, cloned temp repo), and project skills, then merges with precedence sandbox < public < user < org < project. This endpoint is a catalog/settings/UI path unless a client explicitly turns the returned skills into an AgentContext.
  5. Remote workspace helper: RemoteWorkspace.load_skills_from_agent_server(...) calls /api/skills once or once per project dir, converts API skill dicts back into SDK Skill objects, and returns both the list and an AgentContext(skills=..., load_public_skills=False).
  6. File-based subagents: agent_definition_to_factory(..., work_dir=...) resolves skill names listed in an AgentDefinition by calling load_available_skills(work_dir, include_user=True, include_project=True, include_public=False) when building the subagent factory.

After skills are loaded

  • AgentContext.get_system_message_suffix() partitions the already-loaded skills into always-active repo context vs <available_skills>.
  • AgentContext.get_user_message_suffix() injects trigger-matched skill content into user messages.
  • InvokeSkillTool does not discover/load skills from disk; it renders an already-loaded skill by name from the current conversation context.

So the naming split is: load_project_skills(...) is the underlying disk loader, while the new AgentContext.load_project_skills field is a request flag that only becomes effective once LocalConversation has a workspace path.

This comment was generated by an AI agent (OpenHands) on behalf of the user.

@openhands-ai
Copy link
Copy Markdown

openhands-ai Bot commented May 22, 2026

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.
@VascoSch92 VascoSch92 requested a review from all-hands-bot May 26, 2026 09:03
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread openhands-sdk/openhands/sdk/skills/skill.py Outdated
Comment thread openhands-sdk/openhands/sdk/context/agent_context.py Outdated
Comment thread openhands-sdk/openhands/sdk/context/agent_context.py
@VascoSch92 VascoSch92 requested a review from all-hands-bot May 26, 2026 09:11
…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.
@VascoSch92 VascoSch92 requested a review from enyst May 26, 2026 09:17
Copy link
Copy Markdown
Collaborator

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py line 102: .openhands/skills/ in the field description is the legacy path; .agents/skills/ is the current preferred location (see unresolved comment #3282006522).
  • agent_context.py line 100: the field description does not state that project skills loaded via this flag replace same-named explicit skills — the inverse of _load_auto_skills precedence where explicit skills win (see unresolved comment #3302617241).
  • agent_context.py line 177: the refactoring silently dropped the logger.debug('Skipping auto-loaded skill ...') call that helped diagnose name-conflict suppression (see unresolved comment #3302617246).
  • skill.py lines 1217-1220: the merge_skills_by_name docstring 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.

Comment thread openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py
@VascoSch92 VascoSch92 merged commit e3ad3e9 into main May 27, 2026
40 of 41 checks passed
@VascoSch92 VascoSch92 deleted the feat/agent-context-project-skills branch May 27, 2026 13:04
@simonrosenberg simonrosenberg mentioned this pull request May 27, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants