diff --git a/.pr/prd-198/01-org-marketplace-legacy-default.png b/.pr/prd-198/01-org-marketplace-legacy-default.png new file mode 100644 index 000000000..4602fd35b Binary files /dev/null and b/.pr/prd-198/01-org-marketplace-legacy-default.png differ diff --git a/.pr/prd-198/03-org-marketplace-prefer-organization-saved-reopened.png b/.pr/prd-198/03-org-marketplace-prefer-organization-saved-reopened.png new file mode 100644 index 000000000..8cf90c0cc Binary files /dev/null and b/.pr/prd-198/03-org-marketplace-prefer-organization-saved-reopened.png differ diff --git a/.pr/prd-198/README.md b/.pr/prd-198/README.md new file mode 100644 index 000000000..e94825d90 --- /dev/null +++ b/.pr/prd-198/README.md @@ -0,0 +1,26 @@ +# PRD-198 review evidence + +Temporary review artifacts. The `.pr/` directory is removed when the PR is approved. + +## Screenshots + +Both are 1280x860 captures of this branch's frontend in headless Chromium, driven by Playwright. All data is synthetic (`Example Org (synthetic)`, `github:example-org/approved-skills`). The repo's MSW mocks stood in for the backend, plus a small in-memory store for organization marketplaces and settings readback. + +- `01-org-marketplace-legacy-default.png`: an organization marketplace with no saved preference. The new selector shows **Use existing behavior**, which saves `null` and keeps today's merge order. +- `03-org-marketplace-prefer-organization-saved-reopened.png`: the same marketplace after choosing **Use this marketplace's skill**, saving, refetching settings and reopening the editor. The saved `"marketplace"` value reappears. + +Assertions checked during the same run: +- An unrelated edit on a legacy registration sends `public_skill_precedence: null`. +- Choosing the marketplace option sends `"marketplace"`, and the settings readback and the reopened editor both return `"marketplace"`. + +## Matched-version probe + +`test_prd198_matched.py` runs Enterprise and the patched SDK/Agent Server in one process, with no network. It checks the effective `github` skill content under each preference with public skills on and off, and checks that the plugin catalog reports `disabled` when the catalog setting is off. 7 cases passed. + +To reproduce: +1. Install the companion SDK change (`openhands-sdk` and `openhands-agent-server`, built from the SDK PR branch) into this repo's environment in place of the pinned 1.60.1. +2. Copy `tests/public_extension_fixtures.py` from the SDK PR branch into this repo's `tests/` (the probe imports `tests.public_extension_fixtures`). +3. Run `uv run --no-sync pytest .pr/prd-198/test_prd198_matched.py` so uv keeps the patched packages installed for the probe. +4. Delete the copied fixture and restore the pinned versions with `uv sync --frozen`. + +Not covered here: live deployment, warm-claimed or resumed sandboxes, and automation-triggered conversations. diff --git a/.pr/prd-198/test_prd198_matched.py b/.pr/prd-198/test_prd198_matched.py new file mode 100644 index 000000000..f871f94be --- /dev/null +++ b/.pr/prd-198/test_prd198_matched.py @@ -0,0 +1,163 @@ +"""Enterprise -> patched agent-server/SDK, same process, no network.""" + +import httpx +import pytest +from fastapi.testclient import TestClient + +from openhands.agent_server.api import create_app +from openhands.agent_server.config import Config +from openhands.app_server.app_conversation import skill_loader +from openhands.app_server.app_conversation.live_status_app_conversation_service import ( + _to_sdk_marketplace_registrations, +) +from openhands.app_server.sandbox.sandbox_spec_service import get_agent_server_env +from openhands.app_server.settings.settings_models import ( + MarketplaceRegistration, + MarketplaceScope, +) +from openhands.sdk import LLM, Agent, AgentContext +from openhands.sdk.conversation.impl.local_conversation import LocalConversation +from openhands.sdk.skills.skill import _invalidate_public_skills_cache +from tests.public_extension_fixtures import ( + ORG_GITHUB, + PLUGIN_GITHUB, + PUBLIC_GITHUB, + make_plugin_marketplace, + make_public_repo, + make_standalone_marketplace, +) + + +@pytest.fixture +def env(tmp_path, monkeypatch): + monkeypatch.setenv('EXTENSIONS_PATH', str(make_public_repo(tmp_path / 'pub'))) + for k in ('OH_PUBLIC_SKILLS_ENABLED', 'OH_PUBLIC_PLUGIN_CATALOG_ENABLED'): + monkeypatch.delenv(k, raising=False) + _invalidate_public_skills_cache() + yield + _invalidate_public_skills_cache() + + +def _org(tmp_path, factory, precedence): + # Production sources are remote; local fixtures bypass the source validator. + return [ + MarketplaceRegistration.model_construct( + name='acme', + source=str(factory(tmp_path / 'org')), + ref=None, + repo_path=None, + auto_load=True, + public_skill_precedence=precedence, + scope=MarketplaceScope.ORG, + ) + ] + + +def _apply_sandbox_env(monkeypatch): + """What a cold, warm-claimed or resumed sandbox receives from Enterprise.""" + for k, v in get_agent_server_env().items(): + if k.startswith('OH_PUBLIC_'): + monkeypatch.setenv(k, v) + + +async def _discover(monkeypatch, registrations, load_public): + client = TestClient(create_app(Config(session_api_keys=[]))) + + class _Async: + def __init__(self, *a, **k): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *a): + return False + + async def post(self, url, json, headers=None, timeout=None): + r = client.post('/api/skills', json=json) + return httpx.Response( + r.status_code, json=r.json(), request=httpx.Request('POST', url) + ) + + monkeypatch.setattr(skill_loader.httpx, 'AsyncClient', _Async) + skills = await skill_loader.load_skills_from_agent_server( + agent_server_url='http://sandbox', + session_api_key=None, + project_dir='/nonexistent', + load_public=load_public, + load_user=False, + load_project=False, + load_org=False, + registered_marketplaces=registrations, + load_registered_marketplaces=True, + ) + return skills + + +def _by_name(skills): + return {s.name: s.content.strip() for s in skills} + + +def _runtime(tmp_path, registrations, skills): + agent = Agent( + llm=LLM(model='test/m', api_key='k'), + tools=[], + agent_context=AgentContext( + skills=skills, + registered_marketplaces=_to_sdk_marketplace_registrations(registrations), + ), + ) + ws = tmp_path / 'ws' + ws.mkdir(exist_ok=True) + conv = LocalConversation(agent=agent, workspace=ws, visualizer=None) + conv._ensure_plugins_loaded() + ctx = conv.agent.agent_context + conv.close() + return _by_name(ctx.skills) + + +@pytest.mark.usefixtures('env') +@pytest.mark.parametrize( + ('public_on', 'factory', 'precedence', 'expected'), + [ + (True, make_standalone_marketplace, None, PUBLIC_GITHUB), + (True, make_standalone_marketplace, 'marketplace', ORG_GITHUB), + (True, make_plugin_marketplace, 'public', PUBLIC_GITHUB), + (True, make_plugin_marketplace, 'marketplace', PLUGIN_GITHUB), + (False, make_standalone_marketplace, None, ORG_GITHUB), + (False, make_plugin_marketplace, 'public', PLUGIN_GITHUB), + ], +) +async def test_discovery_then_runtime( + tmp_path, monkeypatch, public_on, factory, precedence, expected +): + monkeypatch.setenv('OH_PUBLIC_SKILLS_ENABLED', str(public_on).lower()) + _apply_sandbox_env(monkeypatch) + regs = _org(tmp_path, factory, precedence) + from openhands.app_server.utils.public_extension_policy import public_skills_enabled + + returned = await _discover(monkeypatch, regs, public_skills_enabled()) + discovered = _by_name(returned) + assert discovered['github'] == expected + github = next(s for s in returned if s.name == 'github') + assert github.source # provenance survives the launch handoff + assert ('public-only' in discovered) is public_on + + # Enterprise hands the returned Skill objects (with source) to the + # conversation as explicit skills; the runtime then activates marketplaces. + effective = _runtime(tmp_path, regs, returned) + assert effective['github'] == expected + assert ('public-only' in effective) is public_on + if not public_on: + assert PUBLIC_GITHUB not in ''.join(effective.values()) + + +@pytest.mark.usefixtures('env') +def test_catalog_off_on_sandbox(monkeypatch): + monkeypatch.setenv('OH_PUBLIC_PLUGIN_CATALOG_ENABLED', '0') + _apply_sandbox_env(monkeypatch) + client = TestClient(create_app(Config(session_api_keys=[]))) + assert client.get('/api/plugins/marketplace').json() == { + 'plugins': [], + 'disabled': True, + } diff --git a/frontend/__tests__/components/features/settings/marketplace-modal.test.tsx b/frontend/__tests__/components/features/settings/marketplace-modal.test.tsx index e58d03769..14508a84c 100644 --- a/frontend/__tests__/components/features/settings/marketplace-modal.test.tsx +++ b/frontend/__tests__/components/features/settings/marketplace-modal.test.tsx @@ -546,13 +546,17 @@ describe("MarketplaceModal", () => { }); it("renders scope selector when scope selection is allowed (add mode)", () => { - renderWithProviders(); + renderWithProviders( + , + ); expect(screen.getByTestId("marketplace-scope-select")).toBeInTheDocument(); }); it("includes the chosen scope in onSave when scope selection is allowed", async () => { - renderWithProviders(); + renderWithProviders( + , + ); const user = userEvent.setup(); await user.type(screen.getByPlaceholderText("e.g., my-skills"), "team"); @@ -610,4 +614,73 @@ describe("MarketplaceModal", () => { screen.getByText("SETTINGS$MARKETPLACE_SOURCE_READONLY"), ).toBeInTheDocument(); }); + + describe("public skill precedence (org marketplaces)", () => { + const orgMarketplace = { + name: "acme", + source: "github:acme/skills", + scope: "org" as const, + auto_load: true, + }; + + const renderEdit = (precedence?: "public" | "marketplace" | null) => + renderWithProviders( + , + ); + + it("keeps an unset preference as legacy (null) on an unrelated edit", async () => { + renderEdit(undefined); + const user = userEvent.setup(); + + const select = screen.getByLabelText( + "SETTINGS$MARKETPLACE_PUBLIC_PRECEDENCE_LABEL", + ); + expect(select).toHaveValue("legacy"); + + await user.click(screen.getByTestId("marketplace-save-button")); + await waitFor(() => expect(onSave).toHaveBeenCalled()); + expect(onSave.mock.calls[0][0].public_skill_precedence).toBeNull(); + }); + + it("saves an explicit choice and can return to legacy", async () => { + renderEdit("public"); + const user = userEvent.setup(); + const select = screen.getByLabelText( + "SETTINGS$MARKETPLACE_PUBLIC_PRECEDENCE_LABEL", + ); + expect(select).toHaveValue("public"); + + await user.selectOptions(select, "marketplace"); + await user.click(screen.getByTestId("marketplace-save-button")); + await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1)); + expect(onSave.mock.calls[0][0].public_skill_precedence).toBe( + "marketplace", + ); + + await user.selectOptions(select, "legacy"); + await user.click(screen.getByTestId("marketplace-save-button")); + await waitFor(() => expect(onSave).toHaveBeenCalledTimes(2)); + expect(onSave.mock.calls[1][0].public_skill_precedence).toBeNull(); + }); + + it("is not offered for personal marketplaces", () => { + renderWithProviders( + , + ); + expect( + screen.queryByTestId("marketplace-precedence-select"), + ).not.toBeInTheDocument(); + }); + }); }); diff --git a/frontend/__tests__/routes/skills-settings.test.tsx b/frontend/__tests__/routes/skills-settings.test.tsx index 7f03308ed..747760e94 100644 --- a/frontend/__tests__/routes/skills-settings.test.tsx +++ b/frontend/__tests__/routes/skills-settings.test.tsx @@ -15,7 +15,7 @@ import SkillsService from "#/api/skills-service"; import OptionService from "#/api/option-service/option-service.api"; import * as orgStore from "#/stores/selected-organization-store"; import { organizationService } from "#/api/organization-service/organization-service.api"; -import { MOCK_PERSONAL_ORG, MOCK_TEAM_ORG_ACME } from "#/mocks/org-handlers"; +import { MOCK_TEAM_ORG_ACME } from "#/mocks/org-handlers"; import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers"; import type { WebClientConfig } from "#/api/option-service/option.types"; @@ -469,3 +469,62 @@ describe("skill descriptions", () => { expect(screen.queryByText("Code review helpers")).not.toBeInTheDocument(); }); }); + +describe("org marketplace public skill precedence", () => { + it("passes the saved preference into the edit modal", async () => { + vi.spyOn(OptionService, "getConfig").mockResolvedValue({ + ...mockConfig, + app_mode: "saas", + }); + orgStore.useSelectedOrganizationStore.setState({ + organizationId: MOCK_TEAM_ORG_ACME.id, + }); + vi.spyOn(organizationService, "getMe").mockResolvedValue({ + org_id: MOCK_TEAM_ORG_ACME.id, + user_id: "user-1", + email: "test@example.com", + role: "admin", + llm_api_key: "", + max_iterations: 100, + llm_model: "gpt-4", + llm_base_url: "", + status: "active", + permissions: ["edit_org_settings"], + }); + vi.spyOn(organizationService, "getOrganizations").mockResolvedValue({ + items: [MOCK_TEAM_ORG_ACME], + currentOrgId: MOCK_TEAM_ORG_ACME.id, + }); + vi.spyOn(SettingsService, "getSettings").mockResolvedValue({ + ...MOCK_DEFAULT_USER_SETTINGS, + registered_marketplaces: [], + inherited_marketplaces: [ + { + name: "example-org-skills", + source: "github:example-org/approved-skills", + scope: "org", + public_skill_precedence: "marketplace", + }, + ], + }); + vi.spyOn(SkillsService, "getSkills").mockResolvedValue([]); + vi.spyOn(SkillsService, "getMarketplaceSkills").mockResolvedValue({ + skills: [], + plugins: [], + marketplace_skills: {}, + errors: [], + }); + + renderSkillsSettings(); + const user = userEvent.setup(); + const editButton = await screen.findByRole("button", { + name: "BUTTON$EDIT example-org-skills", + }); + await waitFor(() => expect(editButton).toBeEnabled()); + await user.click(editButton); + + expect( + await screen.findByTestId("marketplace-precedence-select"), + ).toHaveValue("marketplace"); + }); +}); diff --git a/frontend/src/components/features/settings/marketplace-modal.tsx b/frontend/src/components/features/settings/marketplace-modal.tsx index 93ff66200..6f30a118c 100644 --- a/frontend/src/components/features/settings/marketplace-modal.tsx +++ b/frontend/src/components/features/settings/marketplace-modal.tsx @@ -57,6 +57,12 @@ export function deriveMarketplaceName(source: string): string { .replace(/-+$/g, ""); } +type PrecedenceChoice = "legacy" | "public" | "marketplace"; + +const toPrecedenceChoice = ( + value: "public" | "marketplace" | null | undefined, +): PrecedenceChoice => value ?? "legacy"; + interface MarketplaceModalProps { isOpen: boolean; mode: "add" | "edit"; @@ -73,6 +79,7 @@ interface MarketplaceModalProps { ref?: string; repo_path?: string; auto_load?: boolean; + public_skill_precedence?: "public" | "marketplace" | null; scope?: "org" | "personal"; }) => void; isSaving?: boolean; @@ -94,6 +101,9 @@ export function MarketplaceModal({ const [repoPath, setRepoPath] = React.useState(marketplace?.repo_path || ""); const [autoLoad, setAutoLoad] = React.useState(!!marketplace?.auto_load); const [scope, setScope] = React.useState<"org" | "personal">("personal"); + const [precedence, setPrecedence] = React.useState( + toPrecedenceChoice(marketplace?.public_skill_precedence), + ); // Tracks whether the user has hand-edited Name, so auto-derivation from the // repository stops clobbering an intentional name. const [nameEdited, setNameEdited] = React.useState(false); @@ -122,6 +132,7 @@ export function MarketplaceModal({ setAutoLoad(!!marketplace?.auto_load); // Default new marketplaces to personal; admins can opt into org. setScope(marketplace?.scope === "org" ? "org" : "personal"); + setPrecedence(toPrecedenceChoice(marketplace?.public_skill_precedence)); // An existing (edit) name counts as user-owned; a fresh add starts clean. setNameEdited(Boolean(marketplace?.name)); setNameError(null); @@ -133,6 +144,9 @@ export function MarketplaceModal({ // Scope selection only applies when adding and the caller allows it. const showScopeSelector = mode === "add" && allowScopeSelection; + const isOrgScoped = showScopeSelector + ? scope === "org" + : marketplace?.scope === "org"; const handleSave = () => { // Validate name on click @@ -172,6 +186,12 @@ export function MarketplaceModal({ ref: ref.trim() || undefined, repo_path: trimmedRepoPath || undefined, auto_load: autoLoad, + // "legacy" saves null so an unrelated edit keeps the existing winner. + public_skill_precedence: isOrgScoped + ? precedence === "legacy" + ? null + : precedence + : undefined, scope: showScopeSelector ? scope : undefined, }; if (pluginWithoutMarketplace) { @@ -396,6 +416,36 @@ export function MarketplaceModal({ )} + {isOrgScoped && ( +
+ + +
+ )} + {/* Ref field (optional) */}