fix: native tools#266
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR adds structured hosted MCP tool-error handling, diagnostic tool-registry APIs, removes default-toolset resolver behavior, adds hosted MCP logging, enriches hosted tool metadata, and updates prompt/catalog text to reference canonical agh__* tool IDs. ChangesHosted MCP Tool Error Responses
Registry Diagnostics and Policy Resolver Simplification
Hosted MCP Disabled/Unavailable Logging
Hosted Tool Metadata and Descriptions
Canonical Tool ID Guidance
Daemon Integration Tests
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant HostedMCPHandler
participant respondHostedMCPError
participant core
participant Client
HostedMCPHandler->>respondHostedMCPError: err
respondHostedMCPError->>respondHostedMCPError: isHostedMCPToolError(err)
alt tool error
respondHostedMCPError->>core: RespondToolError(c, err, false)
else other error
respondHostedMCPError->>core: RespondError(c, hostedMCPStatus(err), hostedMCPJSONError(err), false)
end
core-->>Client: structured JSON error
sequenceDiagram
participant Caller
participant RuntimeRegistry
participant PolicyEvaluator
Caller->>RuntimeRegistry: DiagnosticSearch(ctx, scope, query)
RuntimeRegistry->>RuntimeRegistry: DiagnosticProjection(ctx, scope)
RuntimeRegistry->>PolicyEvaluator: evaluator for indexed IDs
PolicyEvaluator-->>RuntimeRegistry: effective decisions per tool
RuntimeRegistry-->>Caller: filtered ToolView list
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
internal/api/udsapi/hosted_mcp.go (2)
204-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable
isHostedMCPToolErrorcheck.respondHostedMCPErroralready handles tool errors before callinghostedMCPJSONError, so this branch can never run.🤖 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 `@internal/api/udsapi/hosted_mcp.go` around lines 204 - 234, Remove the unreachable hosted MCP tool error branch from hostedMCPJSONError, since respondHostedMCPError already routes tool errors before this helper is called. Update hostedMCPJSONError to only handle the remaining status and mcppkg.ErrHosted* mappings, keeping the existing error messages intact for the valid paths.
259-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the canonical tool-error classifier
internal/api/core/errors.goalready owns the tool sentinel mapping, so this localisHostedMCPToolErrorlist duplicates the same cases and can drift. Extract a sharedcore.IsToolErrorhelper (or reuse the existing mapping) instead of keeping a second enum here.🤖 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 `@internal/api/udsapi/hosted_mcp.go` around lines 259 - 275, The hosted MCP tool-error check duplicates the canonical tool sentinel mapping, so update isHostedMCPToolError to use the shared classifier from internal/api/core/errors.go instead of maintaining its own errors.As/errors.Is list. Add or reuse a core.IsToolError helper that centralizes the ToolError, ValidationError, and sentinel cases, then call that helper from isHostedMCPToolError so the mapping stays in one place.internal/testutil/acpmock/fixture.go (1)
33-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStripping logic traced against test fixtures — looks correct.
Verified the new
stripLeadingSkillsCatalogInstructions/stripLeadingNetworkResponseRegisterhelpers against all cases infixture_test.go(session-prefixed prompts, network-register-prefixed prompts, malformed wrapper). Boundary detection relies on the fixedcurrentSkillsCatalogOpeningLineprefix and the first"\n\n"after it, which matches howcatalog.gojoins instruction lines — logic is sound given current text formatting.One note: the legacy fallback branch in
stripLeadingSkillsCatalogBlock(cutting oncurrentSkillsCatalogFinalLinewhenstripLeadingSkillsCatalogInstructionsreturnsfalse) appears to be effectively unreachable now, since every current test fixture's skills-catalog block starts withcurrentSkillsCatalogOpeningLineand thus takes the primary path. Not blocking, but worth a follow-up cleanup if confirmed dead.Also applies to: 467-468, 491-528
🤖 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 `@internal/testutil/acpmock/fixture.go` around lines 33 - 40, The legacy fallback path in stripLeadingSkillsCatalogBlock appears dead because stripLeadingSkillsCatalogInstructions already handles every current skills-catalog fixture shape before that branch can run. Clean up the function by removing the unreachable currentSkillsCatalogFinalLine-based fallback (or folding its behavior into the primary path if needed), and keep stripLeadingSkillsCatalogInstructions as the single source of truth for stripping the leading catalog block.internal/daemon/native_tools.go (1)
1147-1157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "diagnostic registry unavailable" error construction.
toolSearchandtoolInfobuild the identicalToolErrorwhen the type assertion fails. Extract a small helper to avoid drift between the two copies.♻️ Proposed refactor
+func diagnosticRegistryUnavailableError(toolID toolspkg.ToolID) error { + return toolspkg.NewToolError( + toolspkg.ErrorCodeUnavailable, + toolID, + "tool diagnostic registry is unavailable", + toolspkg.ErrToolUnavailable, + toolspkg.ReasonDependencyMissing, + ) +} + func (n *daemonNativeTools) toolSearch( ctx context.Context, scope toolspkg.Scope, req toolspkg.CallRequest, ) (toolspkg.ToolResult, error) { var input toolSearchInput if err := decodeNativeInput(req, &input); err != nil { return toolspkg.ToolResult{}, err } registry, ok := n.registry().(nativeToolDiagnosticRegistry) if !ok { - return toolspkg.ToolResult{}, toolspkg.NewToolError( - toolspkg.ErrorCodeUnavailable, - req.ToolID, - "tool diagnostic registry is unavailable", - toolspkg.ErrToolUnavailable, - toolspkg.ReasonDependencyMissing, - ) + return toolspkg.ToolResult{}, diagnosticRegistryUnavailableError(req.ToolID) }Apply the same replacement in
toolInfo.Also applies to: 1177-1187
🤖 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 `@internal/daemon/native_tools.go` around lines 1147 - 1157, The `toolSearch` and `toolInfo` paths duplicate the same “diagnostic registry is unavailable” `ToolError` construction after the `nativeToolDiagnosticRegistry` type assertion fails. Extract a small helper near these methods to centralize that fallback error creation, then replace both inline blocks in `toolSearch` and `toolInfo` with the helper so the message, error code, and reason stay in sync.
🤖 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 `@internal/daemon/daemon_mock_agents_integration_test.go`:
- Around line 249-251: The test in
TestDaemonE2EHostedMCPProjectsAndCallsNonBootstrapNativeTool is missing the
required t.Run("Should ...") wrapper. Move the existing test body into a subtest
named with the “Should...” convention, keeping the current setup calls like
acpmock.RequireDriver and t.Parallel in the appropriate place so the test still
runs correctly under the repository’s Go test pattern.
In `@internal/daemon/hosted_mcp_test.go`:
- Around line 1-2: The new unit test file is missing the required
non-integration build constraint. Add the `//go:build !integration` tag at the
top of the file before the `package daemon` declaration so `hosted_mcp_test.go`
is excluded from integration-only builds and matches the test file convention.
In `@internal/daemon/native_tools.go`:
- Around line 1096-1099: Add a compile-time assertion for
nativeToolDiagnosticRegistry so interface drift is caught at build time instead
of silently using the runtime unavailable path. In native_tools.go, add an
assertion near the nativeToolDiagnosticRegistry definition that
toolspkg.RuntimeRegistry satisfies nativeToolDiagnosticRegistry, so any changes
to DiagnosticSearch or DiagnosticGet signatures fail compilation immediately.
In `@internal/mcp/hosted_proxy_test.go`:
- Around line 49-86: `TestApplyHostedToolsAddsAnthropicMetadata` should follow
the file’s test style by moving its assertions into a `t.Run("Should ...", ...)`
subtest while keeping `t.Parallel()` on the test/subtest as appropriate. Update
the body around `applyHostedTools`, `mcpServer.ListTools`, and the
`requireHostedToolMetaString` checks so the scenario is wrapped in a named
`t.Run` rather than asserted directly in the top-level test function.
---
Nitpick comments:
In `@internal/api/udsapi/hosted_mcp.go`:
- Around line 204-234: Remove the unreachable hosted MCP tool error branch from
hostedMCPJSONError, since respondHostedMCPError already routes tool errors
before this helper is called. Update hostedMCPJSONError to only handle the
remaining status and mcppkg.ErrHosted* mappings, keeping the existing error
messages intact for the valid paths.
- Around line 259-275: The hosted MCP tool-error check duplicates the canonical
tool sentinel mapping, so update isHostedMCPToolError to use the shared
classifier from internal/api/core/errors.go instead of maintaining its own
errors.As/errors.Is list. Add or reuse a core.IsToolError helper that
centralizes the ToolError, ValidationError, and sentinel cases, then call that
helper from isHostedMCPToolError so the mapping stays in one place.
In `@internal/daemon/native_tools.go`:
- Around line 1147-1157: The `toolSearch` and `toolInfo` paths duplicate the
same “diagnostic registry is unavailable” `ToolError` construction after the
`nativeToolDiagnosticRegistry` type assertion fails. Extract a small helper near
these methods to centralize that fallback error creation, then replace both
inline blocks in `toolSearch` and `toolInfo` with the helper so the message,
error code, and reason stay in sync.
In `@internal/testutil/acpmock/fixture.go`:
- Around line 33-40: The legacy fallback path in stripLeadingSkillsCatalogBlock
appears dead because stripLeadingSkillsCatalogInstructions already handles every
current skills-catalog fixture shape before that branch can run. Clean up the
function by removing the unreachable currentSkillsCatalogFinalLine-based
fallback (or folding its behavior into the primary path if needed), and keep
stripLeadingSkillsCatalogInstructions as the single source of truth for
stripping the leading catalog block.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f08a951-9709-434e-9b54-813f18764760
⛔ Files ignored due to path filters (13)
internal/testutil/acpmock/testdata/hosted_native_tools_fixture.jsonis excluded by!**/*.jsonpackages/site/content/blog/posts/introducing-agh-the-first-agent-network-protocol.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/agents/definitions.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/configuration/agent-md.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/configuration/config-toml.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/skills/bundled.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/skills/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/tools/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/tools/toolsets.mdxis excluded by!**/*.mdxskills-lock.jsonis excluded by!**/*.jsonskills/agh/references/agent-definitions.mdis excluded by!**/*.mdskills/agh/references/native-tools.mdis excluded by!**/*.mdskills/agh/references/tools-and-skills.mdis excluded by!**/*.md
📒 Files selected for processing (26)
internal/api/core/tools.gointernal/api/udsapi/hosted_mcp.gointernal/api/udsapi/hosted_mcp_test.gointernal/daemon/daemon_mock_agents_integration_test.gointernal/daemon/daemon_network_collaboration_integration_test.gointernal/daemon/harness_context_integration_test.gointernal/daemon/hosted_mcp.gointernal/daemon/hosted_mcp_test.gointernal/daemon/native_tools.gointernal/daemon/native_tools_test.gointernal/daemon/prompt_skills_test.gointernal/daemon/runtime_prompt.gointernal/daemon/tool_policy_resolver.gointernal/mcp/hosted_proxy.gointernal/mcp/hosted_proxy_test.gointernal/session/manager_start.gointernal/session/manager_test.gointernal/skills/catalog.gointernal/skills/catalog_test.gointernal/testutil/acpmock/fixture.gointernal/testutil/acpmock/fixture_test.gointernal/tools/builtin/catalog.gointernal/tools/policy_resolver.gointernal/tools/registry.gointernal/tools/registry_test.gopackages/site/lib/__tests__/runtime-tools-canonical-docs.test.ts
💤 Files with no reviewable changes (1)
- internal/daemon/tool_policy_resolver.go
This was referenced Jul 3, 2026
Closed
Closed
Closed
pedronauck
added a commit
that referenced
this pull request
Jul 4, 2026
Curated release highlights for v0.0.9 covering task blocking/recovery and needs-attention triage (#269), network delivery policies, subscriptions and thread-to-task promotion (#263), and clearer native tool errors and availability diagnostics (#266). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 4, 2026
Closed
Merged
pedronauck
added a commit
that referenced
this pull request
Jul 4, 2026
## Release v0.0.9 This PR prepares the release of version v0.0.9. ### Changelog ## 0.0.9 - 2026-07-04 ### ♻️ Refactoring - Tasks orchestration (#269) ### 🐛 Bug Fixes - Network optimizations (#263) - Native tools (#266) - Align release-gated network integration tests with #263 behavior ### 📚 Documentation - Update skills - New skill - Add feature stories - Add v0.0.9 release notes ### 📦 Build System - Update go deps - Gitignore ### Release Notes #### Features ##### Network delivery policies, subscriptions, and thread-to-task promotion AGH network channels gain durable delivery control. Each channel now carries a fan-out policy — deliver to all members, route to a designated coordinator peer, or match by capability (the default) — managed with `agh network channels create` and `agh network channels update`. Peers and tasks can subscribe to channels: inspect subscriptions with `agh network subscriptions` and manage per-task subscriptions with `agh task subscribe <task-id>`. Executable thread messages can be promoted into durable workspace tasks straight from the CLI with `agh network promote`, or by an agent through the `agh__task_promote_from_thread` native tool, and designated sibling task runs can be fanned out as part of a workflow. Network and task views now surface task links, peer and coordination cost, and delivered size and token metrics, while mentions are normalized (trimmed, empties removed) and size and token limits are enforced as non-negative. ##### Task blocking, recovery, and needs-attention triage Tasks are now first-class to block, triage, and recover. A task can be explicitly blocked and unblocked with `agh task block <id>` and `agh task unblock <id>`, and every block is kept as durable history you can inspect with `agh task blocks <id>`. A new `needs_attention` status surfaces tasks that stalled and need a human or coordinator to step in — task details now carry the blocked reason and the wake creator so it is clear why a task is waiting and who nudged it. Clear the state with `agh task recover <id>`, recover a specific stalled run with `agh task run recover <run-id>`, or let an agent do it through the `agh__task_recover` native tool and the HTTP/UDS API. Task completion now returns the IDs of any tasks it created, and stronger validation rejects invalid created-task references before they are persisted. Claim tokens are redacted across task outputs and hook payloads, and the new `needs_attention_after` setting controls how long a task may wait before it is flagged for attention. #### Fixes ##### Clearer native tool errors and availability diagnostics Native and hosted tool calls now fail legibly. Hosted MCP tool calls return richer, structured JSON error details — the tool ID, an error code, and any denial reasons — instead of opaque failures, and advertised tools carry improved metadata and descriptions. When runtime diagnostic data cannot be retrieved, native tool lookups report a clear "unavailable" diagnostic rather than a silent miss. Hosted and session MCP availability handling is safer overall, with clearer informational and warning logs and graceful fallbacks when a feature is disabled. Documentation and the runtime envelope now consistently direct agents to the canonical `agh__*` tool IDs and the harness-returned tool references. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated a bundled web assets dependency to a newer version. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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 by CodeRabbit
New Features
Bug Fixes
Documentation
agh__*IDs and harness-returned tool references.