fix: surface registry and entry-point tools in tools list#2476
Conversation
ToolResolver.list_available() previously enumerated only local tools.py, TOOL_MAPPINGS, and praisonai-tools, while the resolution chain also reached the wrapper ToolRegistry and the core SDK tool registry (entry-point plugins and runtime-registered tools). This made some resolvable tools invisible to `praisonai tools list`/`validate`/`info`. Discovery now matches resolution by also querying the wrapper registry and the core registry (triggering idempotent entry-point discovery), with source attribution in the CLI output. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
MervinPraison
added
pipeline/blocked:ci
Greptile SummaryThis PR makes tool discovery include registered tools.
Confidence Score: 4/5This is close, but the custom-source discovery path should be fixed before merging.
src/praisonai/praisonai/tool_resolver.py Important Files Changed
Reviews (3): Last reviewed commit: "fix: align tool discovery source attribu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
✅ Action performedReview finished.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesRegistered Tool Discovery
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/praisonai/praisonai/tool_resolver.py`:
- Around line 572-601: The discovery order in list_available() does not match
the resolution precedence used by default_sources/resolve(), so
wrapper-registered tools can be mislabeled as built-in or external tools. Update
the tool discovery flow in tool_resolver.py so the wrapper ToolRegistry lookup
runs before the praisonaiagents/praisonai-tools mapping blocks, ensuring names
added from self._registry and get_registry() win first and source attribution
matches runtime resolution. Keep the existing duplicate-check behavior, but make
the wrapper registry the earlier source in the same precedence chain as
resolve().
🪄 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: cf51b018-6a5f-4f57-a17b-ff93a34e783c
📒 Files selected for processing (3)
src/praisonai/praisonai/cli/commands/tools.pysrc/praisonai/praisonai/tool_resolver.pysrc/praisonai/tests/unit/test_tool_registry.py
|
|
||
| # 4. Add tools registered in the wrapper ToolRegistry (register_function API) | ||
| if self._registry is not None: | ||
| try: | ||
| names = self._registry.list_functions() | ||
| except Exception: | ||
| names = [] | ||
| for name in names: | ||
| if name not in available: | ||
| available[name] = "Registered tool (wrapper registry)" | ||
|
|
||
| # 5. Add tools from the core SDK tool registry (entry-point plugins and | ||
| # runtime-registered tools). These resolve at run time via the resolution | ||
| # chain, so discovery must surface them too. Mirror the resolution path by | ||
| # triggering entry-point discovery (idempotent) before listing. | ||
| try: | ||
| from praisonaiagents.tools.registry import get_registry | ||
| reg = get_registry() | ||
| try: | ||
| reg.discover_plugins() | ||
| except Exception: | ||
| pass | ||
| for name in reg.list_tools(): | ||
| if name not in available: | ||
| available[name] = "Registered/entry-point tool" | ||
| except ImportError: | ||
| pass | ||
| except Exception as e: | ||
| logger.debug(f"Error listing tools from core registry: {e}") | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Discovery ordering diverges from resolution precedence, so source attribution can be wrong.
The resolution chain (default_sources) orders wrapper-registry before praisonaiagents/praisonai-tools, but list_available() adds the wrapper registry (step 4) after the built-in (step 2) and external (step 3) sources. For a name present in both the wrapper registry and TOOL_MAPPINGS, step 2 wins the if name not in available race, so the tool is labeled "Built-in tool from praisonaiagents" — yet resolve() returns the wrapper-registry callable. tools info will then report the wrong source, contradicting the PR goal that discovery/info agree with runtime resolution.
Consider moving the wrapper-registry discovery block ahead of the praisonaiagents/praisonai-tools blocks to mirror resolution precedence.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 577-577: Do not catch blind exception: Exception
(BLE001)
[error] 592-593: try-except-pass detected, consider logging the exception
(S110)
[warning] 592-592: Do not catch blind exception: Exception
(BLE001)
[warning] 599-599: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@src/praisonai/praisonai/tool_resolver.py` around lines 572 - 601, The
discovery order in list_available() does not match the resolution precedence
used by default_sources/resolve(), so wrapper-registered tools can be mislabeled
as built-in or external tools. Update the tool discovery flow in
tool_resolver.py so the wrapper ToolRegistry lookup runs before the
praisonaiagents/praisonai-tools mapping blocks, ensuring names added from
self._registry and get_registry() win first and source attribution matches
runtime resolution. Keep the existing duplicate-check behavior, but make the
wrapper registry the earlier source in the same precedence chain as resolve().
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2476 ✅ Approved (with fixes pushed)
Phase 1 — Architecture review
Phase 2 — Valid issues found & fixedI confirmed and fixed three valid issues raised by reviewers:
Files modified:
Skipped: Ruff Phase 3 — TestsVerdict: ✅ Approve — all valid reviewer issues resolved, backward compatible, no SDK bloat. (I cannot formally approve via GitHub for security reasons, but I recommend merge.)Job run · branch |
- Reorder list_available() discovery to match resolve()/default_sources precedence (wrapper registry before built-in/external), so a name present in multiple sources is attributed to the source resolve() actually returns. - Add ToolResolver.list_available_sources() returning canonical source ids; CLI now uses it instead of fragile substring matching on descriptions, so a local tool whose docstring contains "Registered" is no longer mis-bucketed. - Do not advertise default-chain tools (built-in/external/core registry) when the resolver is built with a custom sources= chain it cannot resolve. - Add regression tests for precedence attribution, docstring isolation, and custom-source isolation. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2476 ✅ Approve (no new fixes needed)
Phase 1 — Architecture review
Phase 2 — Reviewer issues (all already resolved in commit
|
| if self._registry is not None: | ||
| try: | ||
| names = self._registry.list_functions() | ||
| except Exception: | ||
| names = [] | ||
| for name in names: | ||
| if name not in available: | ||
| available[name] = ("Registered tool (wrapper registry)", "registered") |
There was a problem hiding this comment.
When a resolver is created with a custom source chain, this block can still list wrapper-registry tools that the same resolver cannot resolve. For example, ToolResolver(sources=[], registry=registry) will include names from registry.list_functions() here, but resolve() only walks self._sources, so it returns None for those same names. Local tools loaded above this guard have the same problem when custom sources exclude the local source. This keeps list_available() from matching runtime resolution for custom-source resolvers.
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2474
Summary
praisonai tools list(and the discovery shared withtools validate/tools info) previously enumerated only three of the resolver's sources — localtools.py,praisonaiagents.tools.TOOL_MAPPINGS, and the curatedpraisonai-toolspackage. Tools registered in the wrapperToolRegistryor the core SDK registry (entry-point plugins / runtime-registered tools) resolved at run time but were invisible to discovery. Discovery now matches resolution.Changes
tool_resolver.py—ToolResolver.list_available()now also enumerates:ToolRegistry(list_functions()), labelledRegistered tool (wrapper registry)get_registry().list_tools()), triggering idempotentdiscover_plugins()to mirror the resolution path, labelledRegistered/entry-point toolcli/commands/tools.py—tools listgains aregisteredsource category/filter and count;tools inforeports the registry source.list_available()and resolve at run time.Test plan
tests/unit/test_tool_registry.py(13 passed, including 2 new regression tests)list_available()with correct source attributionGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes