Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d437191f8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| allowed_skill_ids = list(dict.fromkeys([ | ||
| *(agent.available_skills or []), | ||
| *([workflow_skill_name] if workflow_skill_name else []), | ||
| ])) |
There was a problem hiding this comment.
When a bound skill is negated in the request, task profiling records it in task_profile.excluded_resources.skill_names, but this catalog still includes every agent.available_skills entry. The newly registered search_skills tool therefore remains able to expose and load a skill the user explicitly requested not to use, unlike the filtering already applied to excluded knowledge bases and workflows. Subtract the excluded skill IDs before constructing allowed_skills.
Useful? React with 👍 / 👎.
| if len(catalog) <= self._small_catalog_threshold: | ||
| hits = tuple(SkillRetrievalHit(item, 1.0, ('all',)) for item in catalog) | ||
| return self._result(hits, 'all', len(catalog), started) |
There was a problem hiding this comment.
For catalogs at or below the threshold, this branch returns every descriptor before limit is clamped or applied. Because the same retriever backs search_skills, a call such as search_skills(query, limit=1) against 20 skills returns and exposes all 20, contradicting the tool's documented maximum and defeating selective discovery. The full-catalog behavior may be appropriate for initial metadata injection, but tool searches still need to cap the exposed hits.
Useful? React with 👍 / 👎.
| except FutureTimeoutError as exc: | ||
| future.cancel() |
There was a problem hiding this comment.
If an embedding call has already started, Future.cancel() returns false and does not stop its thread. Four stalled backend calls can therefore occupy every worker in the class-level executor; all later retrievals queue behind them, wait the full configured timeout, and fall back to lexical retrieval even after new embedding requests could otherwise succeed. The timeout path needs cancellation at the embedding-client level or worker isolation/recovery that does not leave the shared pool occupied.
Useful? React with 👍 / 👎.
d437191 to
6376dbe
Compare
|
感谢这个 PR，整体方向和我们下一版本的 Skill Retrieval 规划比较一致。首轮只召回相关 Skill，并支持 Agent 在执行过程中通过 建议后续主要考虑以下几点：
|
This branch has not been deployed
2 participants
What
SkillRetriever：目录 ≤20 全量元数据；>20 走 lexical（BM25 风格，中英）+ dense（embed_main）两路 RRF 融合 TopK=8；embedding 缺失/报错/超时自动降级 lexicalsearch_skills工具：Loop 内二次发现，命中后动态 expose，下一轮 get_skill 可用Why
原实现用 query 与 skill 路径名字面打分截 Top5，语义相近但字面不近的表达会在模型判断前被过滤；首轮漏召回后也没有二次发现入口。
Tests
test_skill_retriever.py：50 descriptor，中英/复合/CV 歧义/正逆序/降级/缓存/线程池test_skill_runtime_integration.py：真实 ReactAgent + fake LLM，初始 expose→Loop get_skill、并发 session 隔离、Loop 内二次发现/纠错边界（诚实声明）