Repository navigation
Conversation
…y-discover-default # Conflicts: # frontend/src/__tests__/features/resource-library/MyResources.test.tsx # frontend/src/__tests__/features/settings/components/TeamList.mode-filter.test.tsx # frontend/src/features/resource-library/ResourceLibraryPage.tsx # frontend/src/features/resource-library/components/MyResources.tsx # frontend/src/features/resource-library/types.ts # frontend/src/features/settings/components/SkillListWithScope.tsx # frontend/src/features/settings/components/TeamList.tsx # frontend/src/features/settings/components/TeamListWithScope.tsx
…y-discover-default
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a full Resource Library: backend schemas/services and APIs, discovery KB sync, frontend Discover/Mine UI with assistant and publish flows, tests, i18n, and documentation. ChangesResource Library Feature
Estimated code review effort: Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…y-discover-default # Conflicts: # backend/init_data/02-public-resources.yaml # frontend/src/__tests__/features/resource-library/MyResources.test.tsx # frontend/src/features/resource-library/components/MyResources.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/features/resource-library/components/DiscoverResources.tsx (1)
57-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden listing ingestion against unsupported resource types.
response.itemsis runtime data; assigning it directly can propagate unexpectedresource_typevalues and break downstream card rendering. Keep a runtime allowlist beforesetListings.Suggested fix
const response = await resourceLibraryApi.listListings({ resourceType, keyword: keyword || undefined, page: 1, limit: RESOURCE_LIBRARY_PAGE_SIZE, }) - setListings(response.items) + const safeItems = response.items.filter( + item => item.resource_type === 'agent' || item.resource_type === 'skill' + ) + setListings(safeItems)🤖 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 `@frontend/src/features/resource-library/components/DiscoverResources.tsx` around lines 57 - 64, The response.items from resourceLibraryApi.listListings can contain unsupported resource_type values; before calling setListings in DiscoverResources (the block that awaits response and calls setListings(response.items)), filter/validate each item against a runtime allowlist of permitted resource types (e.g., the same types your card renderers support) and only setListings with the filtered array (optionally log or drop unexpected entries and preserve other fields like resourceType/resource_type). Ensure you perform this check where response.items is handled so downstream rendering code consuming setListings receives only supported resource types.
🧹 Nitpick comments (5)
backend/app/schemas/resource_library.py (1)
17-125: ⚡ Quick winAdd class docstrings for the public schema models.
These models define external API contracts; brief class docstrings will make usage/intent clearer for maintainers and API consumers.
As per coding guidelines
**/*.py: Use descriptive names and docstrings for public functions and classes in Python code.🤖 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 `@backend/app/schemas/resource_library.py` around lines 17 - 125, The public Pydantic schema classes lack class docstrings; add brief descriptive docstrings to each public model (e.g., ResourceLibraryListingCreate, ResourceLibraryVersionCreate, ResourceLibraryInstallCreate, ResourceLibraryVersionResponse, ResourceLibraryListingResponse, ResourceLibraryListResponse, ResourceLibraryKnowledgeBaseRef, ResourceLibraryTeamRef, ResourceLibraryDiscoveryConfigResponse, ResourceLibraryInstallResponse, ResourceLibraryInstallListResponse) explaining their purpose and intended use in the API contract (one-line summary of the model and what it represents/contains). Place the docstring immediately below the class declaration as a triple-quoted string, keeping them concise (1–2 sentences) and mentioning notable fields when helpful to clarify intent, following the project's guideline to document public classes.backend/app/api/endpoints/adapter/resource_library.py (2)
118-118: ⚡ Quick winRemove unused variable assignment.
The assignment
_ = current_userserves no purpose. If authentication is required but the user object isn't used, rely on the dependency injection alone without the unused assignment.♻️ Proposed fix
): - _ = current_user return ResourceLibraryDiscoveryConfigResponse.model_validate(Alternatively, if you want to explicitly document that authentication is required but the user isn't used, add a comment instead:
): # Authentication required but user object not used in this endpoint return ResourceLibraryDiscoveryConfigResponse.model_validate(🤖 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 `@backend/app/api/endpoints/adapter/resource_library.py` at line 118, Remove the needless assignment `_ = current_user` in the endpoint; simply rely on the dependency injection to enforce authentication and either delete that line or replace it with a short explanatory comment (e.g., "Authentication required but user object not used in this endpoint") near the function where `current_user` is injected so the code around `ResourceLibraryDiscoveryConfigResponse.model_validate` remains clear.
113-321: 💤 Low valueConsider adding OpenTelemetry tracing to endpoints.
As per coding guidelines, use
@trace_syncdecorators for entire functions. While endpoint tracing is typically handled at the framework level, explicit tracing can improve observability for debugging and performance monitoring of the resource library API surface.This is optional since FastAPI middleware often handles request tracing, but explicit decorators enable adding span attributes for business-level observability (e.g., resource_type, listing_id).
As per coding guidelines: Use OpenTelemetry tracing decorators:
@trace_asyncor@trace_syncfor entire functions.🤖 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 `@backend/app/api/endpoints/adapter/resource_library.py` around lines 113 - 321, The endpoints lack explicit OpenTelemetry tracing; add the `@trace_sync` decorator (imported from your tracing utilities) to each route function (e.g., get_resource_library_discovery_config, list_resource_library_listings, create_resource_library_listing, get_resource_library_listing, create_resource_library_listing_version, archive_resource_library_listing, install_resource_library_listing, list_my_resource_library_installs, list_my_resource_library_published, upgrade_resource_library_install, remove_resource_library_install) so entire handler executions are traced; optionally, inside those handlers add span attributes for key business values (resource_type, listing_id, install_id, page/limit) using the current span API after decorating. Ensure the decorator is applied above the `@router`.<method> line or immediately above the function definition according to project convention.backend/app/services/resource_library/service.py (2)
343-354: ⚡ Quick winClarify placeholder sentinel values in ResourceMember creation.
The code sets
share_link_id=0,reviewed_by_user_id=0, andcopied_resource_id=0as placeholder values when creating a member record for resource library installation.Consider adding a comment explaining these are sentinel values for "not applicable" or "N/A" in the resource library context, or extract them as named constants to clarify intent:
# Sentinel values for resource library installs (not applicable) RESOURCE_LIBRARY_NOT_APPLICABLE_LINK_ID = 0 RESOURCE_LIBRARY_NOT_APPLICABLE_REVIEWER_ID = 0 RESOURCE_LIBRARY_NOT_APPLICABLE_COPIED_ID = 0🤖 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 `@backend/app/services/resource_library/service.py` around lines 343 - 354, The ResourceMember constructor uses numeric zero placeholders for share_link_id, reviewed_by_user_id, and copied_resource_id which are ambiguous; replace those magic numbers with descriptive sentinel constants (e.g., RESOURCE_LIBRARY_NOT_APPLICABLE_LINK_ID, RESOURCE_LIBRARY_NOT_APPLICABLE_REVIEWER_ID, RESOURCE_LIBRARY_NOT_APPLICABLE_COPIED_ID) defined near the top of the module or alongside ResourceMember-related constants, and/or add an inline comment where ResourceMember is created to state these are "not applicable" sentinel values for resource library installs so future readers see intent (update any other usages of those fields to use the new constants as well).
155-176: ⚖️ Poor tradeoffConsider query-level filtering for better performance.
The listing methods fetch all records and filter in-memory. For large datasets, this approach loads unnecessary data into memory and performs linear scans.
Consider adding SQLAlchemy filter clauses for
resource_typeand potentially indexing/full-text search forkeywordandtagsfields if the resource library grows beyond a few hundred listings. For now, in-memory filtering is acceptable for an MVP feature.Also applies to: 178-197, 199-232
🤖 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 `@backend/app/services/resource_library/service.py` around lines 155 - 176, list_listings currently loads all listings via _query_listings and filters in Python which is inefficient; change it to push filters into the DB by applying SQLAlchemy filter clauses on the query returned by _query_listings (filter by RESOURCE_LIBRARY_STATUS_PUBLISHED, resource_type, and existence of _source_for_listing-equivalent via join/filter) and implement optional DB-side filters for keyword and tags (e.g., ilike/full-text or contains) instead of performing matches in _matches_listing_filters; update related methods that call _query_listings (and other spots flagged: lines 178-197, 199-232) to use the new query-based filtering and return paginated results with skip/limit and total count from the DB.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/app/schemas/resource_library.py`:
- Around line 36-39: The schema ResourceLibraryInstallCreate currently permits
non-positive version_id and empty target_namespace; tighten validation by making
version_id a constrained positive integer (e.g., require >0) and require
target_namespace to be a non-empty string (e.g., min_length=1 and strip
whitespace or add a validator that rejects blank strings). Update the
ResourceLibraryInstallCreate field definitions or add pydantic validators for
version_id and target_namespace so invalid payloads are rejected at schema
validation rather than later in install logic.
In `@backend/app/services/resource_library/discovery.py`:
- Around line 96-99: The lookup in find_listing_document currently only filters
by resource_library_listing_id and must be restricted to the active discovery
knowledge base: change find_listing_document to accept (or derive) the active
discovery_knowledge_base_id and include it in the query filter (e.g., filter by
both resource_library_listing_id and discovery_knowledge_base_id), update all
call sites (the snippet that calls find_listing_document near the top of the
file and the other calls around lines 209-227) to pass or use the active
discovery KB id (from the service state or _source_config/listing), and ensure
the DB index/unique constraint is used/created for the composite
(resource_library_listing_id, discovery_knowledge_base_id) so documents in old
KBs are not matched.
- Around line 66-76: The find_discovery_config_kind function currently queries
Kind only by kind/name/namespace and must also include user_id; change the
function signature (find_discovery_config_kind) to accept a user_id parameter
and add a filter clause Kind.user_id == user_id to the db.query filter so the
lookup uses namespace, name and user_id; update any call sites to pass the
current user's id when invoking find_discovery_config_kind.
- Around line 44-340: Add OpenTelemetry tracing decorators to the service entry
points and key lookup methods: annotate
ResourceLibraryDiscoveryService.get_page_config, .sync_listing,
.remove_listing_document, .find_discovery_knowledge_base, and
.find_listing_document with the appropriate decorator (`@trace_sync` for
synchronous methods or `@trace_async` if any become async) so these flows are
instrumented; inside sync_listing add span events / attributes (using
add_span_event and set_span_attribute) for major branching points (e.g., when
sync is disabled, when listing is not published, when knowledge base is missing,
missing owner, and when returning statuses "updated"/"created"/"deleted"); in
get_page_config add a span attribute for the configured knowledge_base id/name
(use _knowledge_base_payload or _configured_knowledge_base_ref values) and in
find_discovery_knowledge_base add span events when selecting configured ref vs
scanning namespaces and when returning a KB id; reference the methods by name
above to locate locations to add the decorators and span calls.
- Line 73: The boolean comparisons using SQLAlchemy columns need to use
is_(True) or the bare column instead of == True; update occurrences of
Kind.is_active == True (in the discovery predicates used by functions/queries
referencing Kind) and Namespace.is_active == True to either
Kind.is_active.is_(True) / Namespace.is_active.is_(True) or simply
Kind.is_active / Namespace.is_active so the SQLAlchemy predicate semantics
remain the same and Ruff E712 is satisfied.
In `@backend/app/services/resource_library/service.py`:
- Line 247: The filter clauses use SQLAlchemy boolean comparisons like
Kind.is_active == True; update each occurrence (e.g., the Kind.is_active filter
in ResourceLibraryService methods around the queries at the mentioned locations)
to use the boolean column directly (Kind.is_active) instead of comparing to
True; apply the same change for any similar boolean column comparisons found in
the same file (the other occurrences the reviewer noted) so filters read
idiomatically like filter(Kind.is_active) rather than filter(Kind.is_active ==
True).
- Around line 386-400: The upgrade_install method currently ignores version_id
and returns the current install; implement real upgrade behavior in
upgrade_install: use _get_user_member to validate ownership, use
_listing_for_source_member to fetch the listing, validate the requested
version_id exists for that listing (or return HTTP 404/400), perform the DB
update to set the install's version to version_id (or call the existing helper
that persists installs if one exists), handle any migration/compatibility checks
and commit the session, then return the updated ResourceLibraryInstallRecord
(reuse _install_record or the persistence method that constructs records). If
upgrades are not supported yet, instead return a clear HTTPException (e.g., 400
or 501) stating upgrades aren’t implemented rather than silently ignoring
version_id.
- Around line 84-153: Add OpenTelemetry tracing and docstrings to each public
method in this service (e.g., create_listing and create_version): import
trace_sync from app.core.observability and decorate the methods with
`@trace_sync`, then add a concise docstring for each public function describing
parameters (db, user_id, payload/listing_id), return type (Kind or
ResourceLibraryVersionRecord), and side effects (commits, refreshes, calling
_sync_discovery_listing, possible HTTPException). Ensure you only modify public
methods (those without leading underscore) and keep existing logic, updating
docstrings to mention raised exceptions and any state changes to listing.json.
In `@backend/tests/api/endpoints/test_resource_library.py`:
- Around line 131-139: The lookup for copied_team using the Kind model omits the
namespace and can therefore return a same-name Kind from a different namespace;
update the query that builds copied_team to include Kind.namespace ==
source_team.namespace (alongside Kind.name == source_team.name and Kind.user_id
== consumer.id and Kind.is_active == True) so the Kind is uniquely identified by
namespace, name, and user_id.
In `@backend/tests/services/resource_library/test_resource_library_discovery.py`:
- Around line 183-186: The test directly indexes created_documents[0] which can
raise IndexError and hide a missing sync call; before accessing
created_documents[0] (in test_resource_library_discovery assertions), assert the
collection length or the sync call count (e.g., assert len(created_documents) >=
1 or assert_called_once on the document-sync mock) so the test fails with a
clear message when no documents were created; update the assertions around
created_documents and any mocks used for syncing (references: created_documents,
the document-sync mock call assertions) to check presence/count first, then
assert knowledge_base_id, user.id and content.
In `@backend/tests/services/resource_library/test_resource_library_service.py`:
- Around line 140-148: The test's copied_team lookup uses Kind.kind,
Kind.user_id, Kind.name and is_active but omits Kind.namespace, which can match
a different namespace; update the query that defines copied_team to include
Kind.namespace == source_team.namespace (keep existing filters: Kind.kind ==
"Team", Kind.user_id == consumer.id, Kind.name == source_team.name, and
Kind.is_active == True) so the lookup uses the full Kind identity (namespace,
name, user_id).
In
`@frontend/src/__tests__/features/resource-library/ResourceLibraryPage.test.tsx`:
- Around line 138-147: The test asserts window.location.search immediately after
clicking the 'resource-library-mine-tab', which can be updated asynchronously;
wrap the query-param assertion in a waitFor to wait for the URL state
propagation (keep the existing assertions for aria-pressed and DOM updates) so
the check for 'tab=mine' reliably waits until the location update completes when
testing ResourceLibraryPage and the 'resource-library-mine-tab' interaction.
In `@frontend/src/features/resource-library/components/DiscoverResources.tsx`:
- Around line 269-277: The assistant prompt Button in DiscoverResources (the
Button rendering the prompt and calling openAssistant(prompt)) uses class "h-9"
and lacks a data-testid; update the button classes to meet mobile target size by
replacing/augmenting the class with "h-11 min-w-[44px]" and add a descriptive
data-testid attribute (for example data-testid={`assistant-prompt-${prompt}` or
use the prompt index) so tests can target each chip; ensure the onClick remains
openAssistant(prompt) and keep the key as prompt.
In `@frontend/src/features/resource-library/components/PublishResourceDialog.tsx`:
- Around line 79-97: When opening the PublishResourceDialog with no
sourceResource the effect only resets selectedType; update the useEffect branch
that runs when open && !sourceResource to also reset all form state to their
defaults: call setSourceId(''), setName(''), setDisplayName(''),
setDescription(''), setTags([] or formatTags([]) depending on existing
conventions), setVersion('1.0.0' or the intended default), and
setSelectedType(defaultPublishType(resourceType)); this ensures stale source
values (sourceId, name, displayName, description, tags, version) are cleared
when switching to manual mode.
In `@frontend/src/features/settings/components/SkillListWithScope.tsx`:
- Around line 791-804: The publish icon Button for each skill uses h-8 w-8 which
is below the mobile touch target requirement; update the Button JSX (the element
rendering the UploadCloud icon where canPublishSkill(skill) is checked and
handlePublishSkill is invoked) to use the recommended sizing classes (e.g.
replace "h-8 w-8" with "h-11 min-w-[44px]" and keep the icon size via
UploadCloud className "w-4 h-4") so the interactive element meets the 44×44px
minimum on mobile while preserving visual icon sizing and existing aria/testid
props.
In `@frontend/src/features/settings/components/TeamList.tsx`:
- Around line 858-870: The publish button in TeamList.tsx uses mobile classes
"h-7 w-7" which are below the 44×44px touch target requirement; update the
Button rendering (the element that calls handlePublishTeam and is shown when
shouldShowPublish(team)) to use "h-11 min-w-[44px]" for mobile while preserving
the existing desktop sizing (keep sm:h-8 sm:w-8), and keep the title,
aria-label, data-testid, and UploadCloud icon unchanged so behavior and
accessibility remain intact; change only the className on that Button.
---
Outside diff comments:
In `@frontend/src/features/resource-library/components/DiscoverResources.tsx`:
- Around line 57-64: The response.items from resourceLibraryApi.listListings can
contain unsupported resource_type values; before calling setListings in
DiscoverResources (the block that awaits response and calls
setListings(response.items)), filter/validate each item against a runtime
allowlist of permitted resource types (e.g., the same types your card renderers
support) and only setListings with the filtered array (optionally log or drop
unexpected entries and preserve other fields like resourceType/resource_type).
Ensure you perform this check where response.items is handled so downstream
rendering code consuming setListings receives only supported resource types.
---
Nitpick comments:
In `@backend/app/api/endpoints/adapter/resource_library.py`:
- Line 118: Remove the needless assignment `_ = current_user` in the endpoint;
simply rely on the dependency injection to enforce authentication and either
delete that line or replace it with a short explanatory comment (e.g.,
"Authentication required but user object not used in this endpoint") near the
function where `current_user` is injected so the code around
`ResourceLibraryDiscoveryConfigResponse.model_validate` remains clear.
- Around line 113-321: The endpoints lack explicit OpenTelemetry tracing; add
the `@trace_sync` decorator (imported from your tracing utilities) to each route
function (e.g., get_resource_library_discovery_config,
list_resource_library_listings, create_resource_library_listing,
get_resource_library_listing, create_resource_library_listing_version,
archive_resource_library_listing, install_resource_library_listing,
list_my_resource_library_installs, list_my_resource_library_published,
upgrade_resource_library_install, remove_resource_library_install) so entire
handler executions are traced; optionally, inside those handlers add span
attributes for key business values (resource_type, listing_id, install_id,
page/limit) using the current span API after decorating. Ensure the decorator is
applied above the `@router`.<method> line or immediately above the function
definition according to project convention.
In `@backend/app/schemas/resource_library.py`:
- Around line 17-125: The public Pydantic schema classes lack class docstrings;
add brief descriptive docstrings to each public model (e.g.,
ResourceLibraryListingCreate, ResourceLibraryVersionCreate,
ResourceLibraryInstallCreate, ResourceLibraryVersionResponse,
ResourceLibraryListingResponse, ResourceLibraryListResponse,
ResourceLibraryKnowledgeBaseRef, ResourceLibraryTeamRef,
ResourceLibraryDiscoveryConfigResponse, ResourceLibraryInstallResponse,
ResourceLibraryInstallListResponse) explaining their purpose and intended use in
the API contract (one-line summary of the model and what it
represents/contains). Place the docstring immediately below the class
declaration as a triple-quoted string, keeping them concise (1–2 sentences) and
mentioning notable fields when helpful to clarify intent, following the
project's guideline to document public classes.
In `@backend/app/services/resource_library/service.py`:
- Around line 343-354: The ResourceMember constructor uses numeric zero
placeholders for share_link_id, reviewed_by_user_id, and copied_resource_id
which are ambiguous; replace those magic numbers with descriptive sentinel
constants (e.g., RESOURCE_LIBRARY_NOT_APPLICABLE_LINK_ID,
RESOURCE_LIBRARY_NOT_APPLICABLE_REVIEWER_ID,
RESOURCE_LIBRARY_NOT_APPLICABLE_COPIED_ID) defined near the top of the module or
alongside ResourceMember-related constants, and/or add an inline comment where
ResourceMember is created to state these are "not applicable" sentinel values
for resource library installs so future readers see intent (update any other
usages of those fields to use the new constants as well).
- Around line 155-176: list_listings currently loads all listings via
_query_listings and filters in Python which is inefficient; change it to push
filters into the DB by applying SQLAlchemy filter clauses on the query returned
by _query_listings (filter by RESOURCE_LIBRARY_STATUS_PUBLISHED, resource_type,
and existence of _source_for_listing-equivalent via join/filter) and implement
optional DB-side filters for keyword and tags (e.g., ilike/full-text or
contains) instead of performing matches in _matches_listing_filters; update
related methods that call _query_listings (and other spots flagged: lines
178-197, 199-232) to use the new query-based filtering and return paginated
results with skip/limit and total count from the DB.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc93329e-0544-4198-a7b7-b9641c765096
📒 Files selected for processing (51)
backend/app/api/api.pybackend/app/api/endpoints/adapter/resource_library.pybackend/app/core/config.pybackend/app/core/yaml_init.pybackend/app/mcp_server/tools/knowledge.pybackend/app/schemas/resource_library.pybackend/app/services/resource_library/__init__.pybackend/app/services/resource_library/discovery.pybackend/app/services/resource_library/service.pybackend/init_data/02-public-resources.yamlbackend/tests/api/endpoints/test_resource_library.pybackend/tests/models/test_resource_library_models.pybackend/tests/schemas/test_resource_library_schema.pybackend/tests/services/resource_library/test_resource_library_discovery.pybackend/tests/services/resource_library/test_resource_library_service.pydocs/en/developer-guide/architecture.mddocs/en/reference/yaml-specification.mddocs/en/user-guide/README.mddocs/en/user-guide/resource-library/README.mddocs/en/user-guide/resource-library/_category_.jsondocs/superpowers/plans/2026-05-28-resource-library-discover-default.mddocs/superpowers/specs/2026-05-28-resource-library-discover-default-design.mddocs/zh/developer-guide/architecture.mddocs/zh/reference/yaml-specification.mddocs/zh/user-guide/README.mddocs/zh/user-guide/resource-library/README.mddocs/zh/user-guide/resource-library/_category_.jsonfrontend/src/__tests__/apis/resourceLibrary.test.tsfrontend/src/__tests__/features/resource-library/DiscoverResources.test.tsxfrontend/src/__tests__/features/resource-library/MyResources.test.tsxfrontend/src/__tests__/features/resource-library/PublishResourceDialog.test.tsxfrontend/src/__tests__/features/resource-library/ResourceLibraryPage.test.tsxfrontend/src/__tests__/features/resource-library/ResourceLibraryRoute.test.tsxfrontend/src/__tests__/features/settings/components/SkillListWithScope.publish.test.tsxfrontend/src/__tests__/features/settings/components/TeamList.mode-filter.test.tsxfrontend/src/__tests__/i18n/resource-library-namespace.test.tsfrontend/src/apis/resourceLibrary.tsfrontend/src/features/resource-library/ResourceLibraryPage.tsxfrontend/src/features/resource-library/components/DiscoverAssistantDrawer.tsxfrontend/src/features/resource-library/components/DiscoverResources.tsxfrontend/src/features/resource-library/components/MyResources.tsxfrontend/src/features/resource-library/components/PublishResourceDialog.tsxfrontend/src/features/resource-library/components/ResourceDetailDrawer.tsxfrontend/src/features/resource-library/components/ResourceListingCard.tsxfrontend/src/features/resource-library/types.tsfrontend/src/features/settings/components/SkillListWithScope.tsxfrontend/src/features/settings/components/TeamList.tsxfrontend/src/features/settings/components/TeamListWithScope.tsxfrontend/src/i18n/locales/en/resource-library.jsonfrontend/src/i18n/locales/zh-CN/resource-library.jsonfrontend/src/i18n/setup.ts
Comment on lines
+36
to
+39
| class ResourceLibraryInstallCreate(BaseModel): | ||
| version_id: Optional[int] = None | ||
| target_namespace: str = Field(default="default", max_length=100) | ||
| install_options: Dict[str, Any] = Field(default_factory=dict) |
Contributor
There was a problem hiding this comment.
Tighten install payload validation at the schema boundary.
Line 37 allows non-positive version_id values, and Line 38 allows empty namespace values. Invalid payloads should be rejected here instead of failing deeper in install logic.
Suggested patch
class ResourceLibraryInstallCreate(BaseModel):
- version_id: Optional[int] = None
- target_namespace: str = Field(default="default", max_length=100)
+ version_id: Optional[int] = Field(default=None, ge=1)
+ target_namespace: str = Field(default="default", min_length=1, max_length=100)
install_options: Dict[str, Any] = Field(default_factory=dict)🤖 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 `@backend/app/schemas/resource_library.py` around lines 36 - 39, The schema
ResourceLibraryInstallCreate currently permits non-positive version_id and empty
target_namespace; tighten validation by making version_id a constrained positive
integer (e.g., require >0) and require target_namespace to be a non-empty string
(e.g., min_length=1 and strip whitespace or add a validator that rejects blank
strings). Update the ResourceLibraryInstallCreate field definitions or add
pydantic validators for version_id and target_namespace so invalid payloads are
rejected at schema validation rather than later in install logic.
Comment on lines
+44
to
+340
| def get_page_config(self, db: Session) -> dict[str, Any]: | ||
| config = self.find_discovery_config_kind(db) | ||
| spec = self._kind_spec(config) | ||
| knowledge_base = self.find_discovery_knowledge_base(db) | ||
| assistant_team_ref = spec.get("assistantTeamRef") | ||
| if not isinstance(assistant_team_ref, dict): | ||
| assistant_team_ref = DEFAULT_DISCOVERY_ASSISTANT_TEAM | ||
|
|
||
| return { | ||
| "knowledge_base_ref": ( | ||
| self._knowledge_base_payload(knowledge_base) | ||
| if knowledge_base | ||
| else self._configured_knowledge_base_ref(spec) | ||
| ), | ||
| "assistant_team_ref": { | ||
| "name": assistant_team_ref.get("name") | ||
| or DEFAULT_DISCOVERY_ASSISTANT_TEAM["name"], | ||
| "namespace": assistant_team_ref.get("namespace") | ||
| or DEFAULT_DISCOVERY_ASSISTANT_TEAM["namespace"], | ||
| }, | ||
| } | ||
|
|
||
| def find_discovery_config_kind(self, db: Session) -> Kind | None: | ||
| return ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.kind == DISCOVERY_CONFIG_KIND, | ||
| Kind.name == DISCOVERY_CONFIG_NAME, | ||
| Kind.namespace == DISCOVERY_CONFIG_NAMESPACE, | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
|
|
||
| def sync_listing(self, db: Session, listing: Kind) -> dict[str, Any]: | ||
| """Synchronize a published listing into the organization discovery KB.""" | ||
| if not settings.RESOURCE_LIBRARY_DISCOVERY_SYNC_ENABLED: | ||
| return {"status": "disabled"} | ||
| if resource_library_service.get_listing_status(listing) != ( | ||
| RESOURCE_LIBRARY_STATUS_PUBLISHED | ||
| ): | ||
| return self.remove_listing_document(db, listing) | ||
|
|
||
| knowledge_base = self.find_discovery_knowledge_base(db) | ||
| if knowledge_base is None: | ||
| return {"status": "missing_knowledge_base"} | ||
|
|
||
| owner = db.query(User).filter(User.id == knowledge_base.user_id).first() | ||
| if owner is None: | ||
| return {"status": "missing_knowledge_base_owner"} | ||
|
|
||
| content = self.build_listing_document_content(db, listing) | ||
| document = self.find_listing_document(db, listing_id=listing.id) | ||
| document_name = self._document_name(listing) | ||
| source_config = self._source_config(listing) | ||
|
|
||
| if document and document.attachment_id: | ||
| knowledge_orchestrator.update_document_content( | ||
| db=db, | ||
| user=owner, | ||
| document_id=document.id, | ||
| content=content, | ||
| trigger_reindex=True, | ||
| ) | ||
| document.name = document_name | ||
| document.source_config = source_config | ||
| flag_modified(document, "source_config") | ||
| db.commit() | ||
| return {"status": "updated", "document_id": document.id} | ||
|
|
||
| if document: | ||
| KnowledgeService.delete_document( | ||
| db=db, | ||
| document_id=document.id, | ||
| user_id=owner.id, | ||
| ) | ||
|
|
||
| created = knowledge_orchestrator.create_document_with_content( | ||
| db=db, | ||
| user=owner, | ||
| knowledge_base_id=knowledge_base.id, | ||
| name=document_name, | ||
| source_type="text", | ||
| content=content, | ||
| file_extension="md", | ||
| trigger_indexing=True, | ||
| trigger_summary=False, | ||
| ) | ||
| created_doc = ( | ||
| db.query(KnowledgeDocument) | ||
| .filter(KnowledgeDocument.id == created.id) | ||
| .first() | ||
| ) | ||
| if created_doc: | ||
| created_doc.source_config = source_config | ||
| flag_modified(created_doc, "source_config") | ||
| db.commit() | ||
| return {"status": "created", "document_id": created.id} | ||
|
|
||
| def remove_listing_document(self, db: Session, listing: Kind) -> dict[str, Any]: | ||
| knowledge_base = self.find_discovery_knowledge_base(db) | ||
| if knowledge_base is None: | ||
| return {"status": "missing_knowledge_base"} | ||
| owner = db.query(User).filter(User.id == knowledge_base.user_id).first() | ||
| document = self.find_listing_document(db, listing_id=listing.id) | ||
| if owner is None or document is None: | ||
| return {"status": "not_found"} | ||
|
|
||
| KnowledgeService.delete_document( | ||
| db=db, | ||
| document_id=document.id, | ||
| user_id=owner.id, | ||
| ) | ||
| return {"status": "deleted", "document_id": document.id} | ||
|
|
||
| def find_discovery_knowledge_base(self, db: Session) -> Kind | None: | ||
| config = self.find_discovery_config_kind(db) | ||
| spec = self._kind_spec(config) | ||
| configured_ref = self._configured_knowledge_base_ref(spec) | ||
| configured_id = configured_ref.get("id") | ||
| if configured_id: | ||
| try: | ||
| configured_kb_id = int(configured_id) | ||
| except (TypeError, ValueError): | ||
| configured_kb_id = 0 | ||
| knowledge_base = ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.id == configured_kb_id, | ||
| Kind.kind == "KnowledgeBase", | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) | ||
| if knowledge_base and self._is_organization_knowledge_base( | ||
| db, knowledge_base | ||
| ): | ||
| return knowledge_base | ||
|
|
||
| namespace_names = self._organization_namespace_names(db) | ||
| if not namespace_names: | ||
| return None | ||
|
|
||
| query = db.query(Kind).filter( | ||
| Kind.kind == "KnowledgeBase", | ||
| Kind.is_active == True, | ||
| Kind.namespace.in_(namespace_names), | ||
| ) | ||
| configured_namespace = str( | ||
| configured_ref.get("namespace") | ||
| or settings.RESOURCE_LIBRARY_DISCOVERY_KB_NAMESPACE | ||
| or "" | ||
| ).strip() | ||
| if configured_namespace: | ||
| query = query.filter(Kind.namespace == configured_namespace) | ||
|
|
||
| configured_name = str( | ||
| configured_ref.get("name") or settings.RESOURCE_LIBRARY_DISCOVERY_KB_NAME | ||
| ).strip() | ||
| for knowledge_base in query.order_by(Kind.updated_at.desc(), Kind.id.desc()): | ||
| spec = self._kind_spec(knowledge_base) | ||
| if spec.get("name") == configured_name: | ||
| return knowledge_base | ||
| return None | ||
|
|
||
| def find_listing_document( | ||
| self, | ||
| db: Session, | ||
| *, | ||
| listing_id: int, | ||
| ) -> KnowledgeDocument | None: | ||
| return ( | ||
| db.query(KnowledgeDocument) | ||
| .filter( | ||
| KnowledgeDocument.source_config["source"].as_string() | ||
| == DISCOVERY_SOURCE, | ||
| KnowledgeDocument.source_config[ | ||
| "resource_library_listing_id" | ||
| ].as_integer() | ||
| == listing_id, | ||
| ) | ||
| .order_by(KnowledgeDocument.updated_at.desc(), KnowledgeDocument.id.desc()) | ||
| .first() | ||
| ) | ||
|
|
||
| def build_listing_document_content(self, db: Session, listing: Kind) -> str: | ||
| spec = self._kind_spec(listing) | ||
| metadata = self._kind_metadata(listing) | ||
| source = resource_library_service._source_for_listing(db, listing) | ||
| source_metadata = self._kind_metadata(source) if source else {} | ||
| source_spec = self._kind_spec(source) if source else {} | ||
| source_description = ( | ||
| source_metadata.get("description") | ||
| or source_spec.get("description") | ||
| or source_spec.get("systemPrompt") | ||
| or "" | ||
| ) | ||
|
|
||
| tags = spec.get("tags") or [] | ||
| tag_text = ", ".join(str(tag) for tag in tags) if tags else "none" | ||
| resource_type = str(spec.get("resourceType") or "") | ||
|
|
||
| return "\n".join( | ||
| [ | ||
| f"# {spec.get('displayName') or listing.name}", | ||
| "", | ||
| "## Resource Library Listing", | ||
| f"- Listing ID: {listing.id}", | ||
| f"- Resource type: {resource_type}", | ||
| f"- Resource name: {spec.get('name') or listing.name}", | ||
| f"- Version: {spec.get('version') or '1.0.0'}", | ||
| f"- Tags: {tag_text}", | ||
| f"- Publisher user ID: {listing.user_id}", | ||
| f"- Updated at: {listing.updated_at}", | ||
| "", | ||
| "## Listing Description", | ||
| str(spec.get("description") or metadata.get("description") or ""), | ||
| "", | ||
| "## Source Resource", | ||
| f"- Kind: {source.kind if source else spec.get('sourceKind')}", | ||
| f"- ID: {source.id if source else spec.get('sourceKindId')}", | ||
| f"- Namespace: {source.namespace if source else spec.get('sourceNamespace')}", | ||
| f"- Name: {source.name if source else ''}", | ||
| f"- Description: {source_description}", | ||
| ] | ||
| ) | ||
|
|
||
| def _organization_namespace_names(self, db: Session) -> list[str]: | ||
| rows = ( | ||
| db.query(Namespace.name) | ||
| .filter( | ||
| Namespace.level == GroupLevel.organization.value, | ||
| Namespace.is_active == True, | ||
| ) | ||
| .all() | ||
| ) | ||
| return [row.name for row in rows] | ||
|
|
||
| def _knowledge_base_payload(self, knowledge_base: Kind) -> dict[str, Any]: | ||
| spec = self._kind_spec(knowledge_base) | ||
| return { | ||
| "id": knowledge_base.id, | ||
| "name": spec.get("name") or knowledge_base.name, | ||
| "namespace": knowledge_base.namespace, | ||
| } | ||
|
|
||
| def _configured_knowledge_base_ref(self, spec: dict[str, Any]) -> dict[str, Any]: | ||
| configured_ref = spec.get("knowledgeBaseRef") | ||
| if not isinstance(configured_ref, dict): | ||
| configured_ref = {} | ||
| return { | ||
| "id": configured_ref.get("id"), | ||
| "name": configured_ref.get("name") | ||
| or settings.RESOURCE_LIBRARY_DISCOVERY_KB_NAME, | ||
| "namespace": configured_ref.get("namespace") | ||
| or settings.RESOURCE_LIBRARY_DISCOVERY_KB_NAMESPACE | ||
| or None, | ||
| } | ||
|
|
||
| def _is_organization_knowledge_base( | ||
| self, db: Session, knowledge_base: Kind | ||
| ) -> bool: | ||
| return knowledge_base.namespace in self._organization_namespace_names(db) | ||
|
|
||
| def _source_config(self, listing: Kind) -> dict[str, Any]: | ||
| spec = self._kind_spec(listing) | ||
| return { | ||
| "source": DISCOVERY_SOURCE, | ||
| "resource_library_listing_id": listing.id, | ||
| "resource_type": spec.get("resourceType"), | ||
| "source_kind": spec.get("sourceKind"), | ||
| "source_kind_id": spec.get("sourceKindId"), | ||
| } | ||
|
|
||
| def _document_name(self, listing: Kind) -> str: | ||
| display_name = self._kind_spec(listing).get("displayName") or listing.name | ||
| cleaned = re.sub(r"\s+", " ", str(display_name)).strip() | ||
| return f"{cleaned[:180]} - 资源库说明" | ||
|
|
||
| def _listing_description(self, listing: Kind) -> str: | ||
| spec = self._kind_spec(listing) | ||
| return str(spec.get("description") or "") | ||
|
|
||
| def _kind_spec(self, kind: Kind | None) -> dict[str, Any]: | ||
| if not kind or not isinstance(kind.json, dict): | ||
| return {} | ||
| spec = kind.json.get("spec") | ||
| return spec if isinstance(spec, dict) else {} | ||
|
|
||
| def _kind_metadata(self, kind: Kind | None) -> dict[str, Any]: | ||
| if not kind or not isinstance(kind.json, dict): | ||
| return {} | ||
| metadata = kind.json.get("metadata") | ||
| return metadata if isinstance(metadata, dict) else {} | ||
|
|
||
|
|
||
| resource_library_discovery_service = ResourceLibraryDiscoveryService() |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Instrument public discovery service methods with OpenTelemetry tracing decorators.
The new service entry points are untraced, so sync/config flows currently bypass the repository’s tracing baseline.
As per coding guidelines **/*.py: Use OpenTelemetry tracing decorators: @trace_async or @trace_sync for entire functions, and add_span_event() or set_span_attribute() for adding details to spans.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 73-73: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
[error] 174-174: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
[error] 189-189: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
[error] 276-276: Avoid equality comparisons to True; use Namespace.is_active: for truth checks
Replace with Namespace.is_active
(E712)
🤖 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 `@backend/app/services/resource_library/discovery.py` around lines 44 - 340,
Add OpenTelemetry tracing decorators to the service entry points and key lookup
methods: annotate ResourceLibraryDiscoveryService.get_page_config,
.sync_listing, .remove_listing_document, .find_discovery_knowledge_base, and
.find_listing_document with the appropriate decorator (`@trace_sync` for
synchronous methods or `@trace_async` if any become async) so these flows are
instrumented; inside sync_listing add span events / attributes (using
add_span_event and set_span_attribute) for major branching points (e.g., when
sync is disabled, when listing is not published, when knowledge base is missing,
missing owner, and when returning statuses "updated"/"created"/"deleted"); in
get_page_config add a span attribute for the configured knowledge_base id/name
(use _knowledge_base_payload or _configured_knowledge_base_ref values) and in
find_discovery_knowledge_base add span events when selecting configured ref vs
scanning namespaces and when returning a KB id; reference the methods by name
above to locate locations to add the decorators and span calls.
Comment on lines
+66
to
+76
| def find_discovery_config_kind(self, db: Session) -> Kind | None: | ||
| return ( | ||
| db.query(Kind) | ||
| .filter( | ||
| Kind.kind == DISCOVERY_CONFIG_KIND, | ||
| Kind.name == DISCOVERY_CONFIG_NAME, | ||
| Kind.namespace == DISCOVERY_CONFIG_NAMESPACE, | ||
| Kind.is_active == True, | ||
| ) | ||
| .first() | ||
| ) |
Contributor
There was a problem hiding this comment.
Include user_id in discovery-config Kind lookup.
This query currently resolves by kind/name/namespace only. If duplicate records exist across users, the wrong config can be selected.
Suggested patch
def find_discovery_config_kind(self, db: Session) -> Kind | None:
return (
db.query(Kind)
.filter(
Kind.kind == DISCOVERY_CONFIG_KIND,
Kind.name == DISCOVERY_CONFIG_NAME,
Kind.namespace == DISCOVERY_CONFIG_NAMESPACE,
+ Kind.user_id == 0,
Kind.is_active == True,
)
.first()
)As per coding guidelines backend/**/*.py: A Kind resource is uniquely identified by THREE fields: namespace, name, and user_id - always use all three fields to locate a Kind.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def find_discovery_config_kind(self, db: Session) -> Kind | None: | |
| return ( | |
| db.query(Kind) | |
| .filter( | |
| Kind.kind == DISCOVERY_CONFIG_KIND, | |
| Kind.name == DISCOVERY_CONFIG_NAME, | |
| Kind.namespace == DISCOVERY_CONFIG_NAMESPACE, | |
| Kind.is_active == True, | |
| ) | |
| .first() | |
| ) | |
| def find_discovery_config_kind(self, db: Session) -> Kind | None: | |
| return ( | |
| db.query(Kind) | |
| .filter( | |
| Kind.kind == DISCOVERY_CONFIG_KIND, | |
| Kind.name == DISCOVERY_CONFIG_NAME, | |
| Kind.namespace == DISCOVERY_CONFIG_NAMESPACE, | |
| Kind.user_id == 0, | |
| Kind.is_active == True, | |
| ) | |
| .first() | |
| ) |
🧰 Tools
🪛 Ruff (0.15.15)
[error] 73-73: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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 `@backend/app/services/resource_library/discovery.py` around lines 66 - 76, The
find_discovery_config_kind function currently queries Kind only by
kind/name/namespace and must also include user_id; change the function signature
(find_discovery_config_kind) to accept a user_id parameter and add a filter
clause Kind.user_id == user_id to the db.query filter so the lookup uses
namespace, name and user_id; update any call sites to pass the current user's id
when invoking find_discovery_config_kind.
| Kind.kind == DISCOVERY_CONFIG_KIND, | ||
| Kind.name == DISCOVERY_CONFIG_NAME, | ||
| Kind.namespace == DISCOVERY_CONFIG_NAMESPACE, | ||
| Kind.is_active == True, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm remaining E712 pattern in this file.
rg -n '==\s*True' backend/app/services/resource_library/discovery.pyRepository: wecode-ai/Wegent
Length of output: 240
🏁 Script executed:
#!/bin/bash
sed -n '55,90p' backend/app/services/resource_library/discovery.py
echo '---'
sed -n '160,205p' backend/app/services/resource_library/discovery.py
echo '---'
sed -n '250,295p' backend/app/services/resource_library/discovery.pyRepository: wecode-ai/Wegent
Length of output: 5187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for start in 60 168 182 266; do
end=$((start+20))
echo "===== $start-$end ====="
sed -n "${start},${end}p" backend/app/services/resource_library/discovery.py
doneRepository: wecode-ai/Wegent
Length of output: 3098
Replace == True boolean comparisons to satisfy Ruff E712 (SQLAlchemy predicates).
In backend/app/services/resource_library/discovery.py, replace Kind.is_active == True (lines 73, 174, 189) and Namespace.is_active == True (line 276) with *.is_(True) (or the bare boolean column) to pass lint while preserving SQLAlchemy semantics.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 73-73: Avoid equality comparisons to True; use Kind.is_active: for truth checks
Replace with Kind.is_active
(E712)
🤖 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 `@backend/app/services/resource_library/discovery.py` at line 73, The boolean
comparisons using SQLAlchemy columns need to use is_(True) or the bare column
instead of == True; update occurrences of Kind.is_active == True (in the
discovery predicates used by functions/queries referencing Kind) and
Namespace.is_active == True to either Kind.is_active.is_(True) /
Namespace.is_active.is_(True) or simply Kind.is_active / Namespace.is_active so
the SQLAlchemy predicate semantics remain the same and Ruff E712 is satisfied.
Comment on lines
+96
to
+99
| document = self.find_listing_document(db, listing_id=listing.id) | ||
| document_name = self._document_name(listing) | ||
| source_config = self._source_config(listing) | ||
|
|
Contributor
There was a problem hiding this comment.
Scope listing-document lookup to the active discovery knowledge base.
find_listing_document matches only by resource_library_listing_id, so if the configured discovery KB changes, sync can update/delete a document in an old KB and leave the active KB unsynced.
Suggested patch
- document = self.find_listing_document(db, listing_id=listing.id)
+ document = self.find_listing_document(
+ db,
+ listing_id=listing.id,
+ knowledge_base_id=knowledge_base.id,
+ )
def find_listing_document(
self,
db: Session,
*,
listing_id: int,
+ knowledge_base_id: int | None = None,
) -> KnowledgeDocument | None:
- return (
- db.query(KnowledgeDocument)
- .filter(
+ query = db.query(KnowledgeDocument).filter(
KnowledgeDocument.source_config["source"].as_string()
== DISCOVERY_SOURCE,
KnowledgeDocument.source_config[
"resource_library_listing_id"
].as_integer()
== listing_id,
- )
- .order_by(KnowledgeDocument.updated_at.desc(), KnowledgeDocument.id.desc())
- .first()
- )
+ )
+ if knowledge_base_id is not None:
+ query = query.filter(KnowledgeDocument.knowledge_base_id == knowledge_base_id)
+ return query.order_by(
+ KnowledgeDocument.updated_at.desc(),
+ KnowledgeDocument.id.desc(),
+ ).first()Also applies to: 209-227
🤖 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 `@backend/app/services/resource_library/discovery.py` around lines 96 - 99, The
lookup in find_listing_document currently only filters by
resource_library_listing_id and must be restricted to the active discovery
knowledge base: change find_listing_document to accept (or derive) the active
discovery_knowledge_base_id and include it in the query filter (e.g., filter by
both resource_library_listing_id and discovery_knowledge_base_id), update all
call sites (the snippet that calls find_listing_document near the top of the
file and the other calls around lines 209-227) to pass or use the active
discovery KB id (from the service state or _source_config/listing), and ensure
the DB index/unique constraint is used/created for the composite
(resource_library_listing_id, discovery_knowledge_base_id) so documents in old
KBs are not matched.
Comment on lines
+138
to
+147
| it('switches to my resources and updates the tab query parameter', async () => { | ||
| render(<ResourceLibraryPage />) | ||
|
|
||
| fireEvent.click(screen.getByTestId('resource-library-mine-tab')) | ||
|
|
||
| expect(screen.getByTestId('resource-library-mine-tab')).toHaveAttribute('aria-pressed', 'true') | ||
| expect(screen.getByTestId('my-resource-management')).toBeInTheDocument() | ||
| expect(screen.queryByTestId('discover-resources')).not.toBeInTheDocument() | ||
| expect(window.location.search).toContain('tab=mine') | ||
| }) |
Contributor
There was a problem hiding this comment.
Wait for URL state propagation before asserting tab=mine.
window.location.search can update after the click cycle, so asserting it immediately risks flakiness. Wrap the query-param assertion in waitFor.
Suggested test hardening
it('switches to my resources and updates the tab query parameter', async () => {
render(<ResourceLibraryPage />)
fireEvent.click(screen.getByTestId('resource-library-mine-tab'))
expect(screen.getByTestId('resource-library-mine-tab')).toHaveAttribute('aria-pressed', 'true')
expect(screen.getByTestId('my-resource-management')).toBeInTheDocument()
expect(screen.queryByTestId('discover-resources')).not.toBeInTheDocument()
- expect(window.location.search).toContain('tab=mine')
+ await waitFor(() => {
+ expect(window.location.search).toContain('tab=mine')
+ })
})🤖 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
`@frontend/src/__tests__/features/resource-library/ResourceLibraryPage.test.tsx`
around lines 138 - 147, The test asserts window.location.search immediately
after clicking the 'resource-library-mine-tab', which can be updated
asynchronously; wrap the query-param assertion in a waitFor to wait for the URL
state propagation (keep the existing assertions for aria-pressed and DOM
updates) so the check for 'tab=mine' reliably waits until the location update
completes when testing ResourceLibraryPage and the 'resource-library-mine-tab'
interaction.
Comment on lines
+269
to
+277
| <Button | ||
| key={prompt} | ||
| type="button" | ||
| variant="outline" | ||
| className="h-9 px-3" | ||
| onClick={() => openAssistant(prompt)} | ||
| > | ||
| {prompt} | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Update assistant prompt chips to meet mobile target size and testability requirements.
These new interactive buttons are h-9 and have no data-testid. Please align them with the mobile minimum target and add descriptive test IDs.
Suggested fix
- ].map(prompt => (
+ ].map((prompt, index) => (
<Button
key={prompt}
type="button"
variant="outline"
- className="h-9 px-3"
+ className="h-11 min-w-[44px] px-3"
onClick={() => openAssistant(prompt)}
+ data-testid={`discover-assistant-prompt-${index}-button`}
>
{prompt}
</Button>
))}As per coding guidelines, “All interactive elements on mobile must be at least 44px × 44px - use h-11 min-w-[44px] for buttons” and “add data-testid to new interactive components using descriptive names”.
🤖 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 `@frontend/src/features/resource-library/components/DiscoverResources.tsx`
around lines 269 - 277, The assistant prompt Button in DiscoverResources (the
Button rendering the prompt and calling openAssistant(prompt)) uses class "h-9"
and lacks a data-testid; update the button classes to meet mobile target size by
replacing/augmenting the class with "h-11 min-w-[44px]" and add a descriptive
data-testid attribute (for example data-testid={`assistant-prompt-${prompt}` or
use the prompt index) so tests can target each chip; ensure the onClick remains
openAssistant(prompt) and keep the key as prompt.
Comment on lines
79
to
97
| useEffect(() => { | ||
| if (open) { | ||
| setSelectedType(defaultPublishType(resourceType)) | ||
| if (!open) { | ||
| return | ||
| } | ||
|
|
||
| if (sourceResource) { | ||
| setSelectedType(sourceResource.resourceType) | ||
| setSourceId(String(sourceResource.sourceId)) | ||
| setName(sourceResource.name) | ||
| setDisplayName(sourceResource.displayName || sourceResource.name) | ||
| setDescription(sourceResource.description || '') | ||
| setTags(formatTags(sourceResource.tags)) | ||
| setVersion('1.0.0') | ||
| return | ||
| } | ||
| }, [open, resourceType]) | ||
|
|
||
| setSelectedType(defaultPublishType(resourceType)) | ||
| }, [open, resourceType, sourceResource]) | ||
|
|
Contributor
There was a problem hiding this comment.
Reset all form fields when opening without sourceResource.
After a sourced open, reopening in manual mode currently resets only selectedType, so stale values can be submitted for sourceId, name, and metadata.
💡 Suggested fix
useEffect(() => {
if (!open) {
return
}
if (sourceResource) {
setSelectedType(sourceResource.resourceType)
setSourceId(String(sourceResource.sourceId))
setName(sourceResource.name)
setDisplayName(sourceResource.displayName || sourceResource.name)
setDescription(sourceResource.description || '')
setTags(formatTags(sourceResource.tags))
setVersion('1.0.0')
return
}
setSelectedType(defaultPublishType(resourceType))
+ setSourceId('')
+ setName('')
+ setDisplayName('')
+ setDescription('')
+ setTags('')
+ setVersion('1.0.0')
}, [open, resourceType, sourceResource])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| if (open) { | |
| setSelectedType(defaultPublishType(resourceType)) | |
| if (!open) { | |
| return | |
| } | |
| if (sourceResource) { | |
| setSelectedType(sourceResource.resourceType) | |
| setSourceId(String(sourceResource.sourceId)) | |
| setName(sourceResource.name) | |
| setDisplayName(sourceResource.displayName || sourceResource.name) | |
| setDescription(sourceResource.description || '') | |
| setTags(formatTags(sourceResource.tags)) | |
| setVersion('1.0.0') | |
| return | |
| } | |
| }, [open, resourceType]) | |
| setSelectedType(defaultPublishType(resourceType)) | |
| }, [open, resourceType, sourceResource]) | |
| useEffect(() => { | |
| if (!open) { | |
| return | |
| } | |
| if (sourceResource) { | |
| setSelectedType(sourceResource.resourceType) | |
| setSourceId(String(sourceResource.sourceId)) | |
| setName(sourceResource.name) | |
| setDisplayName(sourceResource.displayName || sourceResource.name) | |
| setDescription(sourceResource.description || '') | |
| setTags(formatTags(sourceResource.tags)) | |
| setVersion('1.0.0') | |
| return | |
| } | |
| setSelectedType(defaultPublishType(resourceType)) | |
| setSourceId('') | |
| setName('') | |
| setDisplayName('') | |
| setDescription('') | |
| setTags('') | |
| setVersion('1.0.0') | |
| }, [open, resourceType, sourceResource]) |
🤖 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 `@frontend/src/features/resource-library/components/PublishResourceDialog.tsx`
around lines 79 - 97, When opening the PublishResourceDialog with no
sourceResource the effect only resets selectedType; update the useEffect branch
that runs when open && !sourceResource to also reset all form state to their
defaults: call setSourceId(''), setName(''), setDisplayName(''),
setDescription(''), setTags([] or formatTags([]) depending on existing
conventions), setVersion('1.0.0' or the intended default), and
setSelectedType(defaultPublishType(resourceType)); this ensures stale source
values (sourceId, name, displayName, description, tags, version) are cleared
when switching to manual mode.
Comment on lines
+791
to
+804
| {canPublishSkill(skill) && ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => handlePublishSkill(skill)} | ||
| className="h-8 w-8 text-text-secondary hover:text-text-primary" | ||
| title={t('resource-library:actions.publish_to_library')} | ||
| aria-label={`${t('resource-library:actions.publish_to_library')} ${ | ||
| skill.displayName || skill.name | ||
| }`} | ||
| data-testid={`publish-skill-${skill.id}-button`} | ||
| > | ||
| <UploadCloud className="w-4 h-4" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Increase the publish icon button touch target on mobile.
The new publish action is h-8 w-8, which is below the required mobile touch size.
💡 Suggested fix
{canPublishSkill(skill) && (
<Button
variant="ghost"
size="icon"
onClick={() => handlePublishSkill(skill)}
- className="h-8 w-8 text-text-secondary hover:text-text-primary"
+ className="h-11 w-11 text-text-secondary hover:text-text-primary sm:h-8 sm:w-8"
title={t('resource-library:actions.publish_to_library')}
aria-label={`${t('resource-library:actions.publish_to_library')} ${
skill.displayName || skill.name
}`}
data-testid={`publish-skill-${skill.id}-button`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {canPublishSkill(skill) && ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => handlePublishSkill(skill)} | |
| className="h-8 w-8 text-text-secondary hover:text-text-primary" | |
| title={t('resource-library:actions.publish_to_library')} | |
| aria-label={`${t('resource-library:actions.publish_to_library')} ${ | |
| skill.displayName || skill.name | |
| }`} | |
| data-testid={`publish-skill-${skill.id}-button`} | |
| > | |
| <UploadCloud className="w-4 h-4" /> | |
| </Button> | |
| {canPublishSkill(skill) && ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => handlePublishSkill(skill)} | |
| className="h-11 w-11 text-text-secondary hover:text-text-primary sm:h-8 sm:w-8" | |
| title={t('resource-library:actions.publish_to_library')} | |
| aria-label={`${t('resource-library:actions.publish_to_library')} ${ | |
| skill.displayName || skill.name | |
| }`} | |
| data-testid={`publish-skill-${skill.id}-button`} | |
| > | |
| <UploadCloud className="w-4 h-4" /> | |
| </Button> |
🤖 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 `@frontend/src/features/settings/components/SkillListWithScope.tsx` around
lines 791 - 804, The publish icon Button for each skill uses h-8 w-8 which is
below the mobile touch target requirement; update the Button JSX (the element
rendering the UploadCloud icon where canPublishSkill(skill) is checked and
handlePublishSkill is invoked) to use the recommended sizing classes (e.g.
replace "h-8 w-8" with "h-11 min-w-[44px]" and keep the icon size via
UploadCloud className "w-4 h-4") so the interactive element meets the 44×44px
minimum on mobile while preserving visual icon sizing and existing aria/testid
props.
Comment on lines
+858
to
+870
| {shouldShowPublish(team) && ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => handlePublishTeam(team)} | ||
| title={t('resource-library:actions.publish_to_library')} | ||
| aria-label={`${t('resource-library:actions.publish_to_library')} ${getTeamDisplayName(team)}`} | ||
| className="h-7 w-7 sm:h-8 sm:w-8" | ||
| data-testid={`publish-team-${team.id}-button`} | ||
| > | ||
| <UploadCloud className="w-3.5 h-3.5 sm:w-4 sm:h-4" /> | ||
| </Button> | ||
| )} |
Contributor
There was a problem hiding this comment.
Publish button is too small for mobile touch targets.
The new team publish action renders at h-7 w-7 on mobile, which is below the required minimum.
💡 Suggested fix
{shouldShowPublish(team) && (
<Button
variant="ghost"
size="icon"
onClick={() => handlePublishTeam(team)}
title={t('resource-library:actions.publish_to_library')}
aria-label={`${t('resource-library:actions.publish_to_library')} ${getTeamDisplayName(team)}`}
- className="h-7 w-7 sm:h-8 sm:w-8"
+ className="h-11 w-11 sm:h-8 sm:w-8"
data-testid={`publish-team-${team.id}-button`}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {shouldShowPublish(team) && ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => handlePublishTeam(team)} | |
| title={t('resource-library:actions.publish_to_library')} | |
| aria-label={`${t('resource-library:actions.publish_to_library')} ${getTeamDisplayName(team)}`} | |
| className="h-7 w-7 sm:h-8 sm:w-8" | |
| data-testid={`publish-team-${team.id}-button`} | |
| > | |
| <UploadCloud className="w-3.5 h-3.5 sm:w-4 sm:h-4" /> | |
| </Button> | |
| )} | |
| {shouldShowPublish(team) && ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => handlePublishTeam(team)} | |
| title={t('resource-library:actions.publish_to_library')} | |
| aria-label={`${t('resource-library:actions.publish_to_library')} ${getTeamDisplayName(team)}`} | |
| className="h-11 w-11 sm:h-8 sm:w-8" | |
| data-testid={`publish-team-${team.id}-button`} | |
| > | |
| <UploadCloud className="w-3.5 h-3.5 sm:w-4 sm:h-4" /> | |
| </Button> | |
| )} |
🤖 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 `@frontend/src/features/settings/components/TeamList.tsx` around lines 858 -
870, The publish button in TeamList.tsx uses mobile classes "h-7 w-7" which are
below the 44×44px touch target requirement; update the Button rendering (the
element that calls handlePublishTeam and is shown when shouldShowPublish(team))
to use "h-11 min-w-[44px]" for mobile while preserving the existing desktop
sizing (keep sm:h-8 sm:w-8), and keep the title, aria-label, data-testid, and
UploadCloud icon unchanged so behavior and accessibility remain intact; change
only the className on that Button.
…y-discover-default # Conflicts: # frontend/src/__tests__/features/resource-library/MyResources.test.tsx # frontend/src/features/resource-library/components/MyResources.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
executor/agents/codex/event_mapper.py (2)
22-438: 🏗️ Heavy liftConsider adding OpenTelemetry tracing instrumentation.
The coding guidelines require using
@trace_asyncdecorators for async functions andadd_span_event()/set_span_attribute()for adding details. This event mapper is a critical integration boundary where tracing would provide significant observability for debugging production issues—tracking event flow, timing, and failure modes.As per coding guidelines, OpenTelemetry tracing should be used for instrumentation.
If the tracing infrastructure is already set up in this codebase, consider adding:
@trace_asyncdecorator on thehandlemethod- Span attributes for event method and type
- Span events for key state transitions (tool start/done, message completion, turn completion)
🤖 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 `@executor/agents/codex/event_mapper.py` around lines 22 - 438, Add OpenTelemetry tracing by decorating the async event entrypoint and emitting spans/events/attributes at key transitions: apply the `@trace_async` decorator to CodeXEventMapper.handle, set span attributes for the incoming event method/type (use local variable method and payload/type from _notification_item), and call add_span_event() or set_span_attribute() inside _handle_function_call (tool start details), _handle_function_call_output and _handle_command_execution_done (tool done/output/status), _handle_completed_message (message/phase/final_text), _append_commentary_delta/_complete_commentary_block (commentary block id and content/status), and _handle_turn_completed (turn status/error/usage) so each significant state change is recorded; follow existing tracing helpers/idioms in the repo when adding calls.Source: Coding guidelines
37-79: ⚡ Quick winAdd docstring to the public
handlemethod.This public method serves as the main entry point for translating Codex SDK events but lacks documentation. A docstring would clarify the expected event structure, method dispatch logic, and return value semantics for future maintainers.
As per coding guidelines, public functions and classes should have descriptive docstrings.
📝 Suggested docstring
async def handle(self, event: Any) -> Optional[TaskStatus]: + """Translate a Codex SDK event to Wegent Responses API events. + + Args: + event: Codex SDK event with method and payload attributes + + Returns: + TaskStatus if the turn completed, None otherwise + """ method = getattr(event, "method", "")🤖 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 `@executor/agents/codex/event_mapper.py` around lines 37 - 79, The public async method handle in executor/agents/codex/event_mapper.py lacks a docstring; add a concise docstring to the handle method describing the expected shape of the event parameter (it should have .method and .payload attributes), the dispatch branches it performs (e.g., "item/agentMessage/delta", "item/started", "item/completed", "thread/tokenUsage/updated", "turn/completed"), the side-effects performed (mutating final_text, _pending_agent_deltas, calling emitter.text_delta and helper methods like _append_commentary_delta, _handle_started_item, _handle_completed_item, _handle_usage, _handle_turn_completed), and the return semantics (awaitable returning Optional[TaskStatus] or None to indicate no status). Ensure the docstring is placed immediately below the async def handle signature and written for maintainers (short description, Args: event, Returns: Optional[TaskStatus], and any exceptions or notable behaviors).Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@executor/agents/codex/event_mapper.py`:
- Around line 22-438: Add OpenTelemetry tracing by decorating the async event
entrypoint and emitting spans/events/attributes at key transitions: apply the
`@trace_async` decorator to CodeXEventMapper.handle, set span attributes for the
incoming event method/type (use local variable method and payload/type from
_notification_item), and call add_span_event() or set_span_attribute() inside
_handle_function_call (tool start details), _handle_function_call_output and
_handle_command_execution_done (tool done/output/status),
_handle_completed_message (message/phase/final_text),
_append_commentary_delta/_complete_commentary_block (commentary block id and
content/status), and _handle_turn_completed (turn status/error/usage) so each
significant state change is recorded; follow existing tracing helpers/idioms in
the repo when adding calls.
- Around line 37-79: The public async method handle in
executor/agents/codex/event_mapper.py lacks a docstring; add a concise docstring
to the handle method describing the expected shape of the event parameter (it
should have .method and .payload attributes), the dispatch branches it performs
(e.g., "item/agentMessage/delta", "item/started", "item/completed",
"thread/tokenUsage/updated", "turn/completed"), the side-effects performed
(mutating final_text, _pending_agent_deltas, calling emitter.text_delta and
helper methods like _append_commentary_delta, _handle_started_item,
_handle_completed_item, _handle_usage, _handle_turn_completed), and the return
semantics (awaitable returning Optional[TaskStatus] or None to indicate no
status). Ensure the docstring is placed immediately below the async def handle
signature and written for maintainers (short description, Args: event, Returns:
Optional[TaskStatus], and any exceptions or notable behaviors).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18b040dd-6b23-4960-8900-d6ccba548609
📒 Files selected for processing (8)
executor/agents/codex/event_mapper.pyfrontend/src/__tests__/features/resource-library/MyResources.test.tsxfrontend/src/features/resource-library/components/MyResources.tsxfrontend/src/features/settings/components/SkillListWithScope.tsxfrontend/src/features/settings/components/TeamList.tsxfrontend/src/features/settings/components/TeamListWithScope.tsxfrontend/src/i18n/locales/en/resource-library.jsonfrontend/src/i18n/locales/zh-CN/resource-library.json
✅ Files skipped from review due to trivial changes (1)
- frontend/src/i18n/locales/zh-CN/resource-library.json
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/features/settings/components/TeamListWithScope.tsx
- frontend/src/tests/features/resource-library/MyResources.test.tsx
- frontend/src/features/settings/components/TeamList.tsx
- frontend/src/i18n/locales/en/resource-library.json
- frontend/src/features/settings/components/SkillListWithScope.tsx
- frontend/src/features/resource-library/components/MyResources.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
../backend/.venv/bin/python -m pytest tests/services/resource_library/test_resource_library_discovery.py tests/services/resource_library/test_resource_library_service.pynpm test -- src/__tests__/features/resource-library --runInBandnpm run lint -- --file src/features/resource-library/components/DiscoverResources.tsx --file src/features/resource-library/components/DiscoverAssistantDrawer.tsx --file src/features/resource-library/components/MyResources.tsx --file src/features/resource-library/ResourceLibraryPage.tsx --file src/apis/resourceLibrary.tsuv run black --check app/services/resource_library/discovery.py../backend/.venv/bin/python -m isort --check-only app/services/resource_library/discovery.py app/services/resource_library/service.py app/api/endpoints/adapter/resource_library.py app/schemas/resource_library.py tests/services/resource_library/test_resource_library_discovery.py tests/services/resource_library/test_resource_library_service.pygit diff --checkSummary by CodeRabbit
New Features
API / Backend
Documentation
Tests
Chores