feat(registry): add team publishing and team-private visibility - #1640

web/src/components/registry/agent-edit-form.tsx (1)

64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move AgentDetail to the shared type module.

web/src/pages/registry/agents/detail.tsx defines the same API shape. Keep one exported AgentDetail type in @/lib/types and import it in both files. This prevents team and visibility fields from drifting between the detail page and edit form.

As per coding guidelines, “Centralize frontend types in src/lib/types.ts; do not define inline API response types.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/components/registry/agent-edit-form.tsx` around lines 64 - 65, Move
the exported AgentDetail type from the registry detail page into the shared
`@/lib/types` module, then import and reuse it in both detail.tsx and the agent
edit form. Remove the duplicate local API type definitions while preserving the
existing team_id and visibility fields.

Source: Coding guidelines

web/src/pages/registry/agents/detail.tsx (1)

168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the agent API response type to the shared type module.

AgentDetail now duplicates team_id, visibility, and is_private from RegistryItem. Export an agent detail type from web/src/lib/types/registry.ts and import it here. This prevents response-contract drift between registry screens.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/pages/registry/agents/detail.tsx` around lines 168 - 170, Move the
AgentDetail API response type from the detail page into the shared registry
types module, exporting it alongside RegistryItem with the existing team_id,
visibility, and is_private fields. Update the detail page to import and use the
shared agent detail type, removing the duplicated local definition.

Source: Coding guidelines

tests/test_dashboard_leaderboard.py (1)

156-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Pair each download query with its table by name, not by position.

zip(_LISTING_TABLES, _download_queries(db), strict=True) assumes component_leaderboard emits the five download queries in exactly the _LISTING_TABLES order. If the route reorders the component types, the assertion fails with a confusing message about the wrong table. Select the statement whose SQL contains the table name instead.

♻️ Proposed pairing by table name

-    for table, stmt in zip(_LISTING_TABLES, _download_queries(db), strict=True):
-        sql = _sql(stmt)
+    queries = [_sql(stmt) for stmt in _download_queries(db)]
+    assert len(queries) == len(_LISTING_TABLES)
+    for table in _LISTING_TABLES:
+        sql = next(q for q in queries if table in q)
         assert "agents.is_private = false" in sql

Also applies to: 170-174

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_dashboard_leaderboard.py` around lines 156 - 160, Update the
download-query assertions around _LISTING_TABLES and _download_queries(db) to
pair each table with the statement whose SQL contains that table name, rather
than relying on positional zip order. Apply the same table-name lookup to the
related assertions around the second download-query block, while preserving the
existing privacy checks.

tests/test_draft_workflow.py (1)

362-367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the redundant db rebinding.

_sequenced_db(...) builds db, then app, db, _ = _app_with(user=user, db=db) rebinds the same object. The second assignment reads as if _app_with produced a new mock. Drop the rebinding to keep one owner of the sequenced db. The same pattern repeats at lines 391, 431, 448, 466 and 487.

♻️ Proposed change

-        app, db, _ = _app_with(user=user, db=db)
+        app, _, _ = _app_with(user=user, db=db)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_draft_workflow.py` around lines 362 - 367, Remove the redundant db
rebinding in the affected tests: keep the _sequenced_db result assigned to db,
then unpack only app and the unused return value from _app_with while passing db
as its argument. Apply this consistently to the repeated cases identified in the
comment, preserving the existing mock object.

tests/test_registry_types.py (2)

751-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Parametrize the route cases instead of looping inside each test.

The four tests iterate over _list_endpoint_cases() in a for loop. When one route fails, pytest stops at that route and the failure message does not name it, and the remaining routes are not checked. Move the cases into @pytest.mark.parametrize so each route reports as a separate test id.

♻️ Proposed parametrization

+_LIST_CASES = [pytest.param(*case, id=case[1]) for case in _list_endpoint_cases()]
+
+
 `@pytest.mark.asyncio`
-async def test_component_lists_hide_team_private_listings_from_anonymous_callers():
-    for list_items, table, filters, _expected in _list_endpoint_cases():
-        sql = _inline_sql(await _run_list(list_items, filters, current_user=None))
-        assert f"{table}.is_private = false" in sql
-        assert f"{table}.is_private = true" not in sql
-        assert "team_memberships" not in sql
+@pytest.mark.parametrize(("list_items", "table", "filters", "_expected"), _LIST_CASES)
+async def test_component_lists_hide_team_private_listings_from_anonymous_callers(
+    list_items, table, filters, _expected
+):
+    sql = _inline_sql(await _run_list(list_items, filters, current_user=None))
+    assert f"{table}.is_private = false" in sql
+    assert f"{table}.is_private = true" not in sql
+    assert "team_memberships" not in sql

_list_endpoint_cases() imports route modules at call time, so move the imports to module scope or wrap the case list in a fixture if collection-time import order matters.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_registry_types.py` around lines 751 - 791, Parametrize all four
tests with the route cases from _list_endpoint_cases() so each route executes as
an independent pytest case with a descriptive id. Remove the internal for loops
and accept the unpacked case values as test arguments. Because parametrization
is evaluated during collection, move _list_endpoint_cases() route imports to
module scope or provide the cases through a fixture if collection-time import
order requires it.

---

741-748: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Two helpers named _membership_predicate assert different SQL. Both build the correlated EXISTS on team_memberships as a raw SQL substring, but the bodies diverge: the copy in tests/test_registry_types.py includes the is_private = true AND team_id IS NOT NULL prefix, and the copy in tests/test_dashboard_leaderboard.py includes only the bare EXISTS. A reader who trusts the shared name assumes both assert the same guarantee, and the dashboard copy accepts SQL that gates membership without checking is_private.

• tests/test_registry_types.py#L741-L748: move this helper into a shared test module and keep the full predicate, including the is_private and team_id prefix.
• tests/test_dashboard_leaderboard.py#L44-L50: delete the local copy and import the shared helper, or rename it to state that it asserts only the EXISTS fragment.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_registry_types.py` around lines 741 - 748, Move the full
`_membership_predicate` helper from tests/test_registry_types.py:741-748 into a
shared test module, preserving the is_private and team_id conditions, and update
callers there to use it. Remove the duplicate helper from
tests/test_dashboard_leaderboard.py:44-50 and import the shared implementation;
do not retain a same-named helper that only asserts the EXISTS fragment.

tests/test_component_update_visibility.py (1)

73-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

One listing mock is copied into two test files. Both files build the same MagicMock listing with the same ~80 type-specific attributes and the same explicit is_private / team_id / visibility triple. When a model gains a field, one copy is updated and the other keeps returning a truthy MagicMock attribute, which is the exact failure mode both files added comments to prevent.

• tests/test_component_update_visibility.py#L73-L152: replace the local _listing_mock with an import of the shared builder.
• tests/test_listing_detail_access.py#L62-L81: move this _listing_mock into a shared test helper module or a conftest.py fixture that both files use, keeping the is_private, team_id, and visibility parameters.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_component_update_visibility.py` around lines 73 - 152, The
duplicated listing mocks must use one shared builder. In
tests/test_listing_detail_access.py lines 62-81, move the local _listing_mock
into a shared test helper or conftest.py while preserving the is_private,
team_id, and visibility parameters; in tests/test_component_update_visibility.py
lines 73-152, remove the local _listing_mock and import/use that shared builder.
Keep both tests’ existing call behavior unchanged.

tests/test_team_publishing.py (1)

73-115: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Cover non-member rejection in resolve_publish_target. The implementation already raises HTTP 403 for a non-member with a supplied team_id; add a regression test for this branch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_team_publishing.py` around lines 73 - 115, Extend
TestResolvePublishTargetAutoApprove with an async regression test that supplies
a team_id for a user who has no team membership, invokes resolve_publish_target,
and asserts the call raises HTTP 403. Reuse the existing _mock_db, _user, and
team_id setup helpers and preserve the current matrix tests.

tests/test_insights_access.py (1)

396-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The static guard only inspects async handlers.

The loop skips ast.FunctionDef, so a synchronous route handler added later passes this check without any gate. Include both node types to close that gap.

♻️ Proposed change

     unguarded = []
     for node in ast.walk(tree):
-        if not isinstance(node, ast.AsyncFunctionDef) or node.name.startswith("_"):
+        if not isinstance(node, ast.AsyncFunctionDef | ast.FunctionDef) or node.name.startswith("_"):
             continue

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_insights_access.py` around lines 396 - 410, Update the AST
traversal in the authorization-coverage check around the unguarded handler
collection to inspect both ast.AsyncFunctionDef and ast.FunctionDef nodes.
Preserve the existing private-name and exempt-handler filtering, call detection,
and assertion behavior for all route handlers.

observal-server/tests/test_registry_namespace.py (1)

337-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Strengthen the owner_org_id pin.

The entity is constructed with owner_org_id=None at Line 337, so the assertion at Line 366 also passes if transfer_ownership never reads or writes the field. To pin the "no organization inheritance on transfer" rule, start from a non-null value and assert the transfer does not replace it with target.org_id.

♻️ Proposed test change

-        owner_org_id=None,
+        owner_org_id=original_org_id,

-    assert entity.owner_org_id is None
+    assert entity.owner_org_id == original_org_id
+    assert entity.owner_org_id != target.org_id

Define original_org_id = uuid.uuid4() before building _TransferEntity.

Also applies to: 366-366

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/tests/test_registry_namespace.py` at line 337, Strengthen the
ownership-transfer test by defining a non-null original_org_id with uuid.uuid4()
before constructing _TransferEntity, passing it as owner_org_id, and retaining
the assertion that transfer preserves this value rather than replacing it with
target.org_id.

tests/test_team_visibility_migrations.py (1)

70-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Relax the exact-set assertion on migration operations.

op_calls == {...} fails whenever the migration stops using one of the four allowed operations, even though that change is safe. The property under test is "no operation outside the allowlist". Assert containment instead.

♻️ Proposed change

-    assert op_calls == {f"op.{name}" for name in ALLOWED_SCHEMA_OPS}
+    assert op_calls <= {f"op.{name}" for name in ALLOWED_SCHEMA_OPS}
     assert not any(call.startswith("sa.update") for call in calls)
     assert "op.execute" not in op_calls

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_team_visibility_migrations.py` around lines 70 - 74, Update the
migration operation assertion in the test around _called_op_methods and
COMPONENT_SOURCE_MIGRATION so it verifies op_calls is contained within the
operations generated from ALLOWED_SCHEMA_OPS, rather than requiring an exact set
match. Preserve the existing assertions rejecting sa.update calls and
op.execute.

tests/test_insights_agent_lookup.py (1)

14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Duplicate _result SQLAlchemy result mock in two insights test modules. Both modules define the same helper because no shared test helper exists for the insights suite.

• tests/test_insights_agent_lookup.py#L14-L19: remove the local _result definition and import it from a shared conftest.py helper.
• tests/test_insights_access.py#L32-L37: move this definition into the shared conftest.py helper so both modules use one implementation.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_insights_agent_lookup.py` around lines 14 - 19, Centralize the
duplicated _result SQLAlchemy mock helper in the shared tests/conftest.py,
moving the implementation from tests/test_insights_access.py lines 32-37. Remove
the local definition from tests/test_insights_agent_lookup.py lines 14-19 and
import/use the shared _result helper there; update both modules as needed so
they resolve the same implementation.

tests/test_sec009_component_source_ownership.py (1)

65-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore the previous dependency overrides instead of clearing them.

_api installs overrides on the shared main.app and then calls app.dependency_overrides.clear(). clear() also removes overrides that other test modules or session-scoped fixtures installed on the same application object. Snapshot the mapping and restore it, so this helper only undoes its own changes.

♻️ Proposed change

-        app.dependency_overrides[get_db] = _db
-        if user is not None:
-            app.dependency_overrides[get_current_user] = lambda: user
-        try:
-            async with _make_client() as client:
-                yield client, sessions
-        finally:
-            app.dependency_overrides.clear()
+        previous = dict(app.dependency_overrides)
+        app.dependency_overrides[get_db] = _db
+        if user is not None:
+            app.dependency_overrides[get_current_user] = lambda: user
+        try:
+            async with _make_client() as client:
+                yield client, sessions
+        finally:
+            app.dependency_overrides.clear()
+            app.dependency_overrides.update(previous)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_sec009_component_source_ownership.py` around lines 65 - 72, Update
the _api helper’s dependency-override cleanup: snapshot app.dependency_overrides
before installing get_db and get_current_user overrides, then restore that
snapshot in finally instead of calling clear(). Preserve all pre-existing
overrides while removing only the changes made by this helper.

observal-server/api/routes/agent_versions.py (2)

109-114: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Consider forwarding prefer_user_id for consistent name resolution.

Other routes call the base loader with prefer_user_id=current_user.id. This wrapper omits it. For a name-based agent_id, the version routes can therefore resolve to a different agent than the CRUD routes resolve for the same caller and the same name. Visibility is still enforced, so this is a consistency gap rather than an access issue.

♻️ Proposed change

 async def _load_agent(db: AsyncSession, agent_id: str, current_user: User | None = None) -> Agent | None:
     """Thin wrapper that delegates to the route-level _load_agent."""
     optic.trace("agent_id={}", agent_id)
     from api.routes.agent import _load_agent as _base_load
 
-    return await _base_load(db, agent_id, current_user=current_user)
+    return await _base_load(
+        db,
+        agent_id,
+        prefer_user_id=current_user.id if current_user else None,
+        current_user=current_user,
+    )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/agent_versions.py` around lines 109 - 114, Update
the wrapper _load_agent to pass prefer_user_id=current_user.id to the delegated
_base_load when current_user is available, while preserving None handling for
anonymous callers and the existing visibility behavior.

---

45-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Delete the unused audit coroutine and its test patches.

No production code calls api.routes.agent_versions.audit; the test patches target an unused attribute. Remove both the dead function and those patches.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/agent_versions.py` around lines 45 - 49, Remove
the unused audit coroutine from api.routes.agent_versions and delete the
corresponding test patches that target agent_versions.audit. Ensure no
references to this obsolete attribute remain.

Source: Coding guidelines

observal-server/api/routes/agent/install.py (1)

120-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the repeated scope-and-visibility wrapper.

The same apply_publish_scope(apply_visibility_filter(...)) wrapper is repeated for five listing models. A small local helper removes the duplication and keeps the two filters in a fixed order at every call site.

♻️ Proposed helper

def _scoped(stmt, model, current_user, target_team_id):
    return apply_publish_scope(
        apply_visibility_filter(stmt, model, current_user), model, target_team_id
    )

-        mcp_stmt = apply_publish_scope(
-            apply_visibility_filter(
-                select(McpListing).where(McpListing.id.in_(mcp_comp_ids)), McpListing, current_user
-            ),
-            McpListing,
-            component_target_team_id,
-        )
+        mcp_stmt = _scoped(
+            select(McpListing).where(McpListing.id.in_(mcp_comp_ids)),
+            McpListing,
+            current_user,
+            component_target_team_id,
+        )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/agent/install.py` around lines 120 - 217,
Introduce a local _scoped helper near the preload logic that applies
apply_visibility_filter before apply_publish_scope with the supplied statement,
model, current_user, and target team. Replace each repeated nested wrapper for
the MCP, skill, hook, prompt, and sandbox listing queries with _scoped,
preserving their existing filters, options, and execution behavior.

observal-server/api/routes/dashboard.py (1)

334-339: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Consider a caller-scoped cache key instead of no caching.

Removing the shared cache is correct, because a single cache entry would serve team-private rows to non-members. The cost is that component_leaderboard now runs five grouped join queries plus up to five backfill queries on every request, with no caching. A cache key that includes the caller's visibility scope (for example anonymous, or the caller's team id set) restores caching without cross-tenant leakage.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/dashboard.py` around lines 334 - 339, Update
component_leaderboard to use caller-scoped caching rather than disabling caching
entirely. Build the cache key from the requested window and limit plus the
caller’s visibility scope, distinguishing anonymous users and the caller’s team
ID set, so private leaderboard rows cannot be shared across tenants. Preserve
the existing query and backfill behavior on cache misses.

observal-server/api/routes/mcp.py (2)

404-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

_reject_visibility_edits is duplicated across five route modules. The function body and the docstring are identical in all five files. Only the item type in the error detail differs. A future change to the visibility policy must be applied five times, and any missed copy weakens the guard on that route. Extract one shared helper that takes the item type, and import it in each route module.

Suggested shared signature in observal-server/api/routes/_component_archive.py or a new _component_visibility.py:

def reject_visibility_edits(listing, req, item_type: str) -> None:
    """Refuse teamspace or visibility changes sent to a draft update route."""
    if req.team_id is not None and req.team_id != listing.team_id:
        raise HTTPException(
            status_code=400,
            detail="team_id cannot be changed here. A listing stays in the teamspace it was created under.",
        )
    if req.visibility is not None and req.visibility != listing.visibility:
        raise HTTPException(
            status_code=400,
            detail=(
                "visibility cannot be changed here. "
                f"Use PATCH /api/v1/registry/{item_type}/{listing.id}/visibility."
            ),
        )

• observal-server/api/routes/mcp.py#L404-L422: delete the local helper and call reject_visibility_edits(listing, req, "mcp") at line 440.
• observal-server/api/routes/prompt.py#L269-L287: delete the local helper and call reject_visibility_edits(listing, req, "prompt") at line 305.
• observal-server/api/routes/hook.py#L310-L328: delete the local helper and call reject_visibility_edits(listing, req, "hook") at line 346.
• observal-server/api/routes/sandbox.py#L249-L267: delete the local helper and call reject_visibility_edits(listing, req, "sandbox") at line 285.
• observal-server/api/routes/skill.py#L412-L430: delete the local helper and call reject_visibility_edits(listing, req, "skill") at line 448.

Delete each local copy in the same change. As per coding guidelines, "Use a hard rewrite policy: do not add deprecation wrappers; update callers in the same change and delete dead code immediately."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/mcp.py` around lines 404 - 422, Extract the
duplicated _reject_visibility_edits logic into one shared
reject_visibility_edits(listing, req, item_type: str) helper, preserving both
validation behaviors and using item_type in the visibility endpoint detail. In
observal-server/api/routes/mcp.py:404-422, prompt.py:269-287, hook.py:310-328,
sandbox.py:249-267, and skill.py:412-430, delete each local helper and update
their draft routes to call the shared helper with "mcp", "prompt", "hook",
"sandbox", and "skill" respectively; do not add wrappers or retain dead copies.

Source: Coding guidelines

---

308-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify the fallback guard.

Line 311 already proves listing is falsy before the reassignment. Line 313 then re-tests not listing inside a combined condition. observal-server/api/routes/hook.py (lines 215-223) and observal-server/api/routes/skill.py (lines 300-308) use a clearer two-step form for the same logic. Align this route with that form.

♻️ Proposed refactor

     if not listing:
         listing = await resolve_visible_listing(McpListing, listing_id, db, current_user)
-        if not listing or (
+        if not listing:
+            raise HTTPException(status_code=404, detail="Listing not found or not approved")
+        if (
             listing.status != ListingStatus.archived
             and get_effective_component_permission(listing, current_user) != "owner"
         ):
             raise HTTPException(status_code=404, detail="Listing not found or not approved")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/mcp.py` around lines 308 - 317, In the fallback
logic around resolve_visible_listing, remove the redundant not listing check
after reassignment, since the outer if already establishes the first lookup was
falsy. Align the guard with the two-step pattern used in the hook and skill
routes: re-resolve the listing, then separately raise 404 when the fallback
result is absent or fails the archived/owner permission condition.

observal-server/api/routes/skill.py (1)

227-233: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

Reconsider including skill_md_content in the keyword search.

skill_md_content holds the full SKILL.md document. Line 240 also runs a COUNT(*) over the same predicate. Every search request therefore scans full document text twice, and no index covers this column. Consider a dedicated full-text index or a materialized search column for the large fields.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/skill.py` around lines 227 - 233, Update the skill
search query to avoid applying the keyword predicate directly to
SkillVersion.skill_md_content, while preserving searches across the smaller
indexed fields. If full-document search is required, route it through an
appropriate dedicated full-text index or materialized search column and ensure
the matching COUNT(*) predicate uses the same optimized search path.

observal-server/api/deps.py (1)

468-477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the duplicated local or_ imports.

The module already imports or_ at module scope; Line 379 uses it. The local from sqlalchemy import or_ statements inside apply_publish_scope and apply_visibility_filter add no value.

♻️ Proposed cleanup

 def apply_publish_scope(stmt, model, target_team_id):
     """Limit components to what an agent published for one target can contain."""
-    from sqlalchemy import or_
-
     if not hasattr(model, "is_private") or not hasattr(model, "team_id"):

     Public listings are visible to everyone. Team-private listings require a
     membership row for the requesting user, while legacy private user listings
     remain visible only to their creator.
     """
-    from sqlalchemy import or_
-
     from models.team import TeamMembership

Also applies to: 508-508

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/deps.py` around lines 468 - 477, Remove the redundant
local sqlalchemy.or_ imports from apply_publish_scope and
apply_visibility_filter, and rely on the existing module-level or_ import while
preserving both functions’ query behavior.

observal-server/services/agent_resolver.py (1)

15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider moving the scope helpers out of the api layer.

services/agent_resolver.py now imports from api.deps at module scope. That inverts the usual dependency direction and creates a cycle risk, because api/routes/agent/helpers.py imports both api.deps and this service. apply_visibility_filter and apply_publish_scope are pure query builders over models. A neutral module such as services/visibility.py would let both layers import them safely.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/services/agent_resolver.py` at line 15, Move
apply_publish_scope and apply_visibility_filter from api.deps into a neutral
services visibility module, then update agent_resolver and
api/routes/agent/helpers.py to import them from that module. Remove the api.deps
dependency from services/agent_resolver.py while preserving both helpers’
query-building behavior and signatures.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

web/src/pages/registry/teamspace-detail.tsx (2)

627-636: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Leaving a teamspace runs without confirmation.

The Leave button calls leaveTeam.mutate directly and then navigates away. A team owner can also lose access this way. Confirm the action first, as the Delete button does.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/pages/registry/teamspace-detail.tsx` around lines 627 - 636, Update
the Leave button handler in the teamspace detail component to show the same
confirmation flow used by the Delete button before invoking leaveTeam.mutate.
Only proceed with leaving and navigating to /teamspaces after confirmation,
including for team owners.

---

344-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Member removal runs without confirmation.

A single click on the trash button removes the member. The action is destructive and has no undo. The same page already confirms teamspace deletion with AlertDialog at lines 698-716. Add the same confirmation step before removeMember.mutate.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/pages/registry/teamspace-detail.tsx` around lines 344 - 356, Update
the member removal button’s onClick flow in the canManageMembers block to show
an AlertDialog confirmation before invoking removeMember.mutate(member.id).
Reuse the page’s existing teamspace deletion AlertDialog pattern and only
perform the mutation after the user confirms.

web/src/pages/registry/agents/detail.tsx (1)

721-746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared visibility-change flow. Both detail pages carry byte-identical copies of readReturnedToReview and applyVisibility, plus the same confirmation dialog copy. The shared root cause is a copied flow rather than one extracted module. A future change to the review-queue messaging or the response contract must be applied twice, and the copies will diverge.

• web/src/pages/registry/agents/detail.tsx#L721-L746: replace the local applyVisibility and the readReturnedToReview helper at lines 122-129 with the shared implementation, passing the display name and type: "agents".
• web/src/pages/registry/components/detail.tsx#L170-L195: replace the local applyVisibility and the readReturnedToReview helper at lines 63-70 with the same shared implementation, passing the display name and the current type.

A shared hook, for example useVisibilityChange({ type, id, displayName }) returning { applyVisibility, isPending }, keeps the dialog markup in each page while centralizing the response parsing and the toast text.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/pages/registry/agents/detail.tsx` around lines 721 - 746, Extract the
duplicated readReturnedToReview and applyVisibility flow into a shared
useVisibilityChange implementation returning applyVisibility and isPending;
preserve the existing response parsing and toast behavior. Update
web/src/pages/registry/agents/detail.tsx lines 721-746 to use the shared
implementation with type "agents" and the display name, and update
web/src/pages/registry/components/detail.tsx lines 170-195 with the shared
implementation using the current type and display name; keep the confirmation
dialog markup in each page.

web/src/routes/_authed/teamspaces.$handle.tsx (1)

15-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

COMPONENT_TYPES is duplicated and can drift.

web/src/pages/registry/teamspace-detail.tsx declares its own COMPONENT_TYPES list at lines 70-76 for the component tabs. If a new registry type is added to that list only, validateSearch drops the type search parameter and the selected tab is not preserved in the URL. The failure is silent. Export one list and derive the validation set from it.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/routes/_authed/teamspaces`.$handle.tsx around lines 15 - 22, Remove
the duplicated COMPONENT_TYPES declaration from the route and reuse the exported
component-type list from teamspace-detail.tsx in validateSearch. Ensure the
validation set is derived from that shared list so newly supported registry
types preserve the type search parameter consistently.

web/src/pages/registry/components/detail.tsx (1)

137-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the redundant visibility cast.

RegistryItem declares visibility and is_private in web/src/lib/types/registry.ts. The fallback already produces a string accepted by PickerSelect.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/pages/registry/components/detail.tsx` around lines 137 - 138, Remove
the unnecessary string cast from the currentVisibility assignment in the detail
component. Use item?.visibility directly with the existing nullish fallback
based on item?.is_private, preserving the string value passed to PickerSelect.

Source: Coding guidelines

web/src/routes/_authed/teamspaces.tsx (1)

14-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a file-based index route for /teamspaces.

Make teamspaces.tsx render only <Outlet />, and move TeamspacesPage to teamspaces.index.tsx. This uses TanStack Router’s route matching instead of manual location.pathname matching. A configured basepath is stripped from useLocation().pathname.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/routes/_authed/teamspaces.tsx` around lines 14 - 23, Update
TeamspacesRoute in teamspaces.tsx to render only Outlet, remove the manual
useLocation/pathname matching, and move the TeamspacesPage rendering into a new
teamspaces.index.tsx file-based index route so /teamspaces is handled by
TanStack Router.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/test_component_update_visibility.py (1)

43-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Wire db.execute explicitly in _mock_db.

db is a bare AsyncMock, so await db.execute(...) returns a MagicMock, and result.scalar_one_or_none() returns a truthy sentinel. If the update route runs any query, the success-path tests in TestUnchangedUpdatesSucceed pass on that sentinel instead of on real behavior. The sibling helpers document this hazard and wire the result explicitly: _db_with_membership in tests/test_draft_workflow.py and _mock_db in tests/test_listing_detail_access.py.

Set db.execute to an AsyncMock with an explicit result, or to a side_effect sequence that raises on an unexpected query.

♻️ Proposed helper change

 def _mock_db():
     db = AsyncMock()
     db.add = MagicMock()
     db.commit = AsyncMock()
     db.flush = AsyncMock()
     db.refresh = AsyncMock()
     db.delete = AsyncMock()
+    # A bare AsyncMock hands back a truthy MagicMock for every query result.
+    # Fail loudly instead: these routes are expected to run no query here.
+    db.execute = AsyncMock(side_effect=AssertionError("unexpected query in the update route"))
     return db

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_component_update_visibility.py` around lines 43 - 50, Update the
_mock_db helper to explicitly configure db.execute with a realistic result or a
side-effect sequence that raises on unexpected queries, ensuring update-route
tests validate actual query behavior instead of truthy AsyncMock sentinels.

tests/test_registry_visibility.py (1)

561-571: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Assert the review metadata on the never-approved path too.

_seed_team_private_component sets reviewed_by and reviewed_at on every seeded version, including the draft, pending, and rejected cases. test_team_owner_making_a_listing_public_sends_it_back_to_review asserts that both fields are cleared, but this test does not. A route that clears review metadata only on the approved branch, or that leaves stale team-level approval metadata on a never-approved listing, still passes here.

💚 Proposed assertion

         assert result["returned_to_review"] is False
         assert result["status"] == status.value
+
+        async with sessions() as session:
+            listing = await session.get(McpListing, seed.listing_id)
+            version = await session.get(McpVersion, listing.latest_version_id)
+            assert version.status == status

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_registry_visibility.py` around lines 561 - 571, Extend
test_never_approved_listing_is_not_moved_when_it_becomes_public to assert that
the listing’s review metadata is cleared after _patch_visibility: verify
reviewed_by and reviewed_at are both absent or null, while preserving the
existing returned_to_review and status assertions for draft, pending, and
rejected cases.

tests/test_team_publishing.py (1)

34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Set org_id explicitly on the mock user.

_user builds a MagicMock(spec=User) without org_id. The spec allows the attribute, so any read returns a truthy MagicMock. If resolve_publish_target or a submit route branches on org scoping, the tests take the org branch by accident. The sibling helpers in this PR set the field explicitly: tests/test_component_update_visibility.py line 39, tests/test_listing_detail_access.py line 37, and tests/test_preview_config.py line 30.

♻️ Proposed helper change

 def _user(role=UserRole.user, **kw):
     u = MagicMock(spec=User)
     u.id = kw.get("id", uuid.uuid4())
     u.role = role
     u.username = kw.get("username", "testuser")
     u.email = kw.get("email", "test@example.com")
+    # A bare spec'd attribute is truthy, which would put every caller in an org.
+    u.org_id = kw.get("org_id")
     return u

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_team_publishing.py` around lines 34 - 40, Update the _user test
helper to explicitly assign org_id on the User mock, using a kwarg override with
an appropriate default consistent with the sibling helpers, so org-scoping logic
does not observe an implicit truthy MagicMock.

tests/test_agent_composition.py (1)

1218-1232: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Document or align visibility-change status codes.

Agent routes return 422; component draft routes return 400 for this rejection. Record the intended API distinction or use one status code across both route families.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_agent_composition.py` around lines 1218 - 1232, Align the
visibility-change rejection status code between the agent route tested by
test_visibility_change_is_rejected and the corresponding component draft routes,
choosing either the existing 422/400 convention consistently or documenting the
intentional distinction in the relevant API contract.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/test_team_review.py (1)

596-664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deletion-guard tests match the current implementation but don't cover the soft-delete gap.

TestTeamDeletionGuard correctly validates the six-model count loop and the 409/204 branching against the current _team_owned_listing_counts and delete_team implementation. These tests mock db.scalar generically, so they cannot surface the missing deleted_at filter on the Agent count flagged in observal-server/api/routes/teams.py (Lines 205-232): a soft-deleted-only agent would still count as "owned" and block deletion in production, but this mock-based test can't distinguish that from a real filtered query. Once that fix lands, add a test that seeds Agent.deleted_at as set and asserts the query still returns 0 (or asserts the actual WHERE clause via a real query against the in-memory DB used earlier in this file, rather than a plain AsyncMock).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_team_review.py` around lines 596 - 664, Add coverage in
TestTeamDeletionGuard for a team containing only a soft-deleted Agent: seed
Agent.deleted_at as set and verify _team_owned_listing_counts or the delete_team
flow treats the count as zero, preferably using the file’s in-memory database or
by inspecting the generated query’s WHERE clause rather than generic db.scalar
mocking. Keep the existing deletion-guard tests unchanged.

tests/e2e/teamspace-review.spec.ts (2)

140-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The confirmation dialog's "confirm" path is untested.

This test only exercises the Cancel branch: it opens the dialog, verifies its copy, then cancels and asserts is_private stays true. There is no assertion for the accept path, where confirming should flip is_private to false and route the listing into public review. Since the dialog exists specifically to warn about that state transition, add a companion test (or extend this one) that clicks the confirm button and asserts the resulting is_private/review state.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/teamspace-review.spec.ts` around lines 140 - 160, Extend the
teamspace visibility test around the alertdialog to exercise the confirm path,
using the dialog’s confirm/accept button rather than the existing cancel
control. After confirmation, fetch the seeded listing and assert is_private
becomes false and the listing enters the expected public-review state, while
preserving the existing dialog-copy and cancellation coverage.

---

113-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Seeded skill listing is never cleaned up.

The "visibility confirmation" test creates a new skill via POST /api/v1/skills/submit on every run (e2e-vis-${Date.now()}) but never deletes it afterward. Repeated CI runs against the same environment will accumulate orphaned team-private skill listings tied to TEAM_HANDLE. Add an afterEach/finally cleanup call (or a delete API call) once the assertions complete.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/e2e/teamspace-review.spec.ts` around lines 113 - 160, The visibility
confirmation test leaves the skill created by the POST request in the
environment. Track seeded.id and add cleanup after the test, using the
authenticated owner token to delete the seeded listing via the appropriate
skills delete API, ensuring cleanup runs even when assertions fail.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

observal-server/api/routes/component_versions.py (1)

98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated version-visibility filter into a helper.

_list_versions, _get_version, and _review_version each build version_filters with the same two lines: append the listing filter, then append version_model.status == ListingStatus.approved unless may_view_unapproved is true. Extract this into a shared helper to avoid drift if the unapproved-visibility rule changes.

♻️ Proposed helper extraction

def _version_visibility_filters(listing, version_model, current_user, *extra):
    filters = list(extra)
    if not may_view_unapproved(get_effective_component_permission(listing, current_user), current_user):
        filters.append(version_model.status == ListingStatus.approved)
    return filters

Then in each function:

-    version_filters = [version_model.listing_id == listing.id]
-    if not may_view_unapproved(get_effective_component_permission(listing, current_user), current_user):
-        version_filters.append(version_model.status == ListingStatus.approved)
+    version_filters = _version_visibility_filters(listing, version_model, current_user, version_model.listing_id == listing.id)

Also applies to: 138-140, 282-284

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@observal-server/api/routes/component_versions.py` around lines 98 - 100,
Extract the shared version-visibility construction into a helper such as
_version_visibility_filters, accepting the listing, version model, current user,
and optional extra filters; ensure it includes listing.id matching and
conditionally appends approved status based on may_view_unapproved. Update
_list_versions, _get_version, and _review_version to use this helper while
preserving any function-specific filters.