fix(acp): tool exposure + spec compliance + vendor default args (consolidated)#1078
Closed
codebit0 wants to merge 4 commits into
Closed
fix(acp): tool exposure + spec compliance + vendor default args (consolidated)#1078codebit0 wants to merge 4 commits into
codebit0 wants to merge 4 commits into
Conversation
- ACP MCP 브리지 노출: Gemini가 skill_search 등 goclaw 빌트인 툴 사용 가능 - NewBridgeServer가 BuiltinToolStore에서 활성화된 툴 목록을 동적으로 로딩 - bridgeAlwaysExcluded(spawn, create_forum_topic, heartbeat) 제외 처리 - Gemini McpServer 헤더 포맷 수정: 스펙(object) 대신 Gemini CLI 0.36.x 실제 구현에 맞춰 []McpServerKV 배열 형식으로 변경 - ACP 퍼-세션 cwd 격리: cli-workspaces와 동일하게 세션별 독립 워크스페이스 생성 - enrichGeminiACPArgs: skills-store 등 5개 스킬 소스 디렉토리를 --include-directories로 추가 - buildACPMcpServersFunc: context 헤더 + HMAC 인증 + UI 등록 MCP 서버 통합 - is_system 시맨틱 정리: UpsertSystemSkill이 is_system을 강제하지 않음 - INSERT: is_system=false (운영자가 명시적으로 설정) - UPDATE: is_system 컬럼 유지 (시더 재실행이 운영자 설정 덮어쓰지 않음) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Python dependency check used a 5s context deadline. Importing a typical skill bundle (numpy + vectorbt + pandas + psycopg2 + dotenv) takes ~4.4s on idle and easily crosses 5s under load. On context deadline, the previous code marked every package as missing and re-triggered pip install for the whole set, which produced more load and made the next check time out again — an install loop visible in journalctl as the same packages being installed every ~30 seconds. - Raise depCheckTimeout from 5s to 30s. - On context.DeadlineExceeded, log a warning and return nil (assume present) instead of treating every import as missing. - On other exec errors, surface the captured stderr so future failures are diagnosable instead of opaque.
ACP-mode agents (Atlas, Cortana, etc.) reach goclaw tools only through the /mcp/bridge MCP server, which exposes the subset of tools listed in builtin_tools with enabled=true. Several tools registered in the Go tool registry were never seeded into builtin_tools, so ACP agents could not call them and reported errors like 'datetime tool is not available'. Native-provider agents read tools.Registry directly and were unaffected, which masked the gap. - Seed datetime, delegate, mcp_tool_search, list_group_members, vault_read, vault_search in builtinToolSeedData. The seed reconcile step (DELETE WHERE name != ALL) means manual INSERTs were wiped on every restart; the seed function is the single source of truth. - Always construct an mcp.Manager (empty when no servers configured) and register mcp_tool_search. This lets the BM25 discovery tool be available to LLMs immediately; it returns 'no tools found' until external MCP servers are added, then becomes useful without a redeploy.
…or default args
End-to-end Gemini CLI 0.40.1 ACP fixes, verified live with `datetime` tool
returning correct time text through the full ACP→MCP bridge path.
Changes:
1. ACP spec method name aliases (tool_bridge.go)
The ACP spec uses snake_case (`fs/read_text_file`,
`terminal/wait_for_exit`, `session/request_permission`); only Claude CLI
ever emitted camelCase. Each switch case now accepts both, so newer
Gemini builds no longer fall through to "unknown method" — the JSON-RPC
error that Gemini's catch path stringifies as `[object Object]` in
tool_call_update content because the rejection isn't a JS Error.
2. Kind-based session permission (tool_bridge.go, types.go)
Replace the legacy `RequestPermissionRequest`/`Response` (flat outcome)
with new `SessionRequestPermissionRequest`/`Response` that match the
spec nested-outcome shape: `{outcome: {outcome: "selected", optionId}}`
or `{outcome: {outcome: "cancelled"}}`. handleSessionPermission selects
by `PermissionOption.Kind` (allow_once / allow_always / reject_once /
reject_always) since OptionID is agent-defined and unstable across
versions. Approve-all prefers allow_once over allow_always.
3. Provider-layer vendor-arg + include-directories injection
(acp_provider.go, gateway_providers.go)
Removes the cmd-level `enrichGeminiACPArgs` in favor of two cleaner
primitives in the providers package, mirroring the architecture in
merge/2026-04-29:
- `WithIncludeDirectories(dirs []string)` option: callers pass
candidate skill directories; NewACPProvider stat-filters and emits
`--include-directories <dir>` pairs only for gemini.
- `applyVendorDefaultArgs(binary, args)` helper: appends per-binary
CLI defaults that goclaw's deployment requires regardless of user
state. For gemini, injects `--skip-trust` so MCP discovery runs
even when the per-session cwd inherits DO_NOT_TRUST from
`~/.gemini/trustedFolders.json` (ACP sessions always run inside a
goclaw-managed sandbox, so the user-facing trust gate is moot).
The cmd layer now only enumerates candidate skill dirs via
`acpSkillCandidateDirs(workspace)` and passes them as an option;
binary gating and vendor flag emission live in the provider.
4. ACP ClientInfo.Name reset to empty
Restored to "" (no ClientInfo identity broadcast); aligns with the
pre-debug state.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Superseded — folding the same content (consolidated ACP work) directly into #1072 instead of opening a parallel PR. |
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
Consolidated branch covering ACP-mode agent fixes verified end-to-end against Gemini CLI 0.40.1. Combines the tool-exposure work from #1072 with the spec-compliant ACP standardization, in the same provider-layer architecture as #1069.
Relationship to existing PRs
fix(skills): prevent infinite pip install loop,fix(tools): seed missing builtin tools and wire mcp_tool_search,fix(mcp): expose builtin_tools-seeded tools via dynamic bridge lookup). If this PR lands, feat(acp): full Gemini CLI integration — tool exposure + spec compliance + watchdog + GEMINI.md #1072 can close as superseded.WithIncludeDirectoriesoption +applyVendorDefaultArgshelper); ko i18n and other ops items in feat: ACP standardization + ko i18n + ops hardening (full branch) #1069 are not duplicated and would still need to land separately.Reviewers may prefer to merge #1069 first and rebase this on top, or pick one branch to consolidate everything.
Major themes
1. ACP agent tool exposure (from #1072)
internal/skills/dep_checker.go— raisedepCheckTimeout5s→30s; onDeadlineExceededlog a warning and assume packages present (the previous behaviour falsely flagged everything missing under load and triggered a reinstall loop every ~30s).cmd/gateway_builtin_tools.go— seed six tools that ACP-mode agents could not see (datetime,delegate,mcp_tool_search,list_group_members,vault_read,vault_search). Native-provider agents read the registry directly and were unaffected, masking the gap.internal/mcp/bridge_server.go— replace the hardcodedBridgeToolNameswhitelist withBuiltinToolStore.ListEnabled()lookup. The UI builtin-tools toggle is now the single source of truth for ACP exposure;bridgeAlwaysExcludedkeeps internal-only tools (spawn,create_forum_topic,heartbeat) off the bridge.cmd/gateway_setup.go— always construct anmcp.Managersomcp_tool_searchis available even when no external MCP servers are configured (returns "no tools found" until servers are added; no redeploy on add).2. ACP spec compliance for Gemini CLI 0.40.1
fs/read_text_file,fs/write_text_file,terminal/wait_for_exit,session/request_permission. Each tool-bridge switch case now accepts both snake_case and the legacy camelCase. Without this, requests fall through to "unknown method", which Gemini's catch path stringifies as[object Object]intool_call_update.contentbecause the rejection isn't a JSErrorinstance.SessionRequestPermissionRequest/Responsetypes match the spec's nested-outcome shape ({outcome: {outcome: "selected", optionId}}/{outcome: {outcome: "cancelled"}}).handleSessionPermissionselects byPermissionOption.Kind(allow_once/allow_always/reject_once/reject_always) rather than agent-defined OptionID strings, since Kind is stable across Gemini versions while OptionID values are not. Approve-all prefersallow_onceso operator policy still gates each tool call.3. Provider-layer vendor and include-directories injection
Removes the cmd-level
enrichGeminiACPArgsin favour of two cleaner primitives in the providers package:WithIncludeDirectories(dirs []string) ACPOption— callers pass candidate skill directories;NewACPProviderstat-filters and emits--include-directories <dir>pairs only for the gemini binary.applyVendorDefaultArgs(binary, args)— appends per-binary CLI defaults that goclaw's deployment requires unconditionally. Currently injects--skip-trustfor gemini so MCP discovery runs even when the per-session cwd inheritsDO_NOT_TRUSTfrom~/.gemini/trustedFolders.json. ACP sessions always run inside a goclaw-managed sandbox, so the user-facing trust gate is moot. New vendor rules go here rather than scattering binary-name checks across call sites.cmd/gateway_providers.gonow only enumerates candidate skill directories viaacpSkillCandidateDirs(workspace)and passes them as theWithIncludeDirectoriesoption; binary gating and vendor flag emission live in the provider.Test plan
go build ./...(PG)go build -tags sqliteonly ./...(Desktop / SQLite)go vet ./...go test ./internal/providers/acp/...go test ./internal/skills/...session/new→session/prompt→session/request_permission(Kind-based, approved withallow_once) → MCPtools/callfordatetime→ tool result text returned to LLM → final user-facing message contains the correct timestamp. No[object Object]content intool_call_update.mcp.bridge: tools registered count=37 skipped_excluded=1— the seeded tools appear intools/listresults.