fix(slack): gate require_mention on the event's own text, not injected thread context - #1503
Open
alex-coolfy wants to merge 253 commits into
Open
Conversation
…-stdio-runtime-validation fix: harden MCP stdio validation
makeCronJobHandler builds the agent RunRequest with no provider/model, so cron jobs always run on the agent default provider. High-frequency scheduled jobs (e.g. triage) can't be routed to a cheaper model the way heartbeats already can via agent_heartbeats.provider_id/model. Add provider_id/model to cron_jobs (Postgres migration 000074; SQLite schema + migration v42→43, both mirroring agent_heartbeats) and resolve them into RunRequest.ProviderOverride/ModelOverride in the handler, reusing the existing per-run override plumbing that heartbeats use. NULL/unset → agent default, so existing jobs are unaffected. Settable today via UpdateJob patch (CronJobPatch.ProviderID/Model) or directly in the DB; tool/RPC/dashboard exposure left as a follow-up. Verified: go build ./... and go build -tags sqlite ./... (exit 0), go vet, gofmt, and cron unit tests on both pg and sqlite stores.
…elbuilder#1232) The sessions.reset RPC only reset the native session store, leaving the Claude CLI-backed history (.jsonl + CLAUDE.md) in place. The /reset chat command already clears it via providers.ResetCLISession, so RPC callers got an incomplete reset for claude-cli agents — stale history was resumed on the next turn. Mirror the chat path by calling ResetCLISession from handleReset. It is a no-op when the CLI provider is unused. The call is wired through an overridable package var so the behavior can be regression-tested.
) Clear targetAgentID in handleMessage so the gateway consumer's resolveAgentRoute matches via cfg.Bindings instead of always using the channel instance's agent_id. Without this patch, all Discord channels route to the channel instance's default agent, ignoring bindings in config.json. Add slog.Info call for routing diagnostics. Co-authored-by: Duy /zuey/ <duy@wearetopgroup.com> Co-authored-by: Said <msaidf@users.noreply.github.com>
Use the existing relative-time guard when deciding whether to show channel diagnostics, so Go zero-value timestamps do not make healthy channels look broken. Add regression coverage for zero-value first_failed_at and active failure states. Co-authored-by: evgyur <evgyur@gmail.com>
…tion (nextlevelbuilder#1189) The legacy /open-apis/bot/v3/info endpoint returns the bot object at the top level of the response, not inside "data" like newer Lark APIs. The probe always failed to extract open_id, leaving botOpenID empty, which made every group mention count as a bot mention — the bot replied to messages mentioning other users even with requireMention enabled. - Read top-level "bot" first, fall back to data.bot defensively - Retry the startup probe with backoff to survive transient failures - Fix test fixtures to match the real API response shape Fixes nextlevelbuilder#895
…nextlevelbuilder#1145) Both PG and SQLite skill stores returned SKILL.md content without the {baseDir} substitution that skills.Loader.LoadSkill performs for file-based access. Agents reading SKILL.md via this RPC saw the literal placeholder, breaking script-invocation lines that expect absolute paths. Mirror the loader's substitution in both store implementations so behavior is consistent regardless of access path. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The BridgeToolNames allowlist omitted "delegate", so claude-cli / Claude-Code-backed agents never received the tool even when their orchestration mode resolved to "delegate" with active agent_links. The native agent loop injects delegate per orchestration mode, but bridge sessions only see this curated allowlist, making inter-agent delegation impossible from bridge-backed agents. delegate is safe to expose unconditionally: it self-gates via CanDelegate/agent_links and resolves its source agent from the X-Agent-ID header context, exactly like team_tasks (already bridged).
nextlevelbuilder#1235) require_user_credentials MCP servers loaded and connected, but their tool defs were never sent to the model, so the agent could never call the per-user mcp_<prefix>__* tools (it only ever saw the shared MCP server). The per-user tool objects are intentionally kept out of the shared registry to prevent cross-user credential leaks, so FilterTools could not emit them. Surface them through a request-scoped overlay registry passed to the policy engine: per-user tools are now evaluated AND emitted under the same allow/deny rules as registry tools, then resolved per-actor at execution time via executeToolForActor. The overlay is discarded after filtering, so the shared registry and other users stay unaffected. - add tools.NewUserToolOverlay (request-scoped, discarded post-filter) - buildFilteredTools: wrap registry in overlay when per-user tools present; no-policy path appends their defs directly - fix stale getUserMCPTools docstring (claimed shared-registry registration) - tests: overlay policy allow/deny + per-user surfacing / strip / dedup Co-authored-by: DangTinh311 <dangtinh31193@gmail.com>
…uilder#1248) Registering a remote MCP server (sse/streamable-http) whose hostname resolves to a private IP is rejected at config-validation time by the SSRF guard, with no production escape hatch -- the only bypass is a test-only loopback flag. Self-hosted MCP servers on a private network are therefore unregisterable, even though the runtime MCP client connects to them fine (Test Connection succeeds and lists tools; only create/update input validation blocks). Add an opt-in, operator-configured allowlist (GOCLAW_MCP_ALLOWED_HOSTS, empty by default) of trusted hostnames exempt from the private/loopback IP block during MCP server URL validation only: - security.ValidateAllowingHosts(url, allowedHosts): like Validate but skips the private/loopback block for allowlisted hostnames. The cloud-metadata/link-local (169.254/fe80), multicast and unspecified ranges are never exempted, even for allowlisted hosts. - mcp.SetAllowedHosts wires the operator allowlist into ValidateURL / ValidateServerConfig; default empty => no behavior change. - web_fetch / webhook / redirect SSRF paths are unchanged (those stay agent-influenced and fully guarded). Matching is case-insensitive on the pre-resolution hostname.
…cutions tenant isolation (migrations 082-083) (nextlevelbuilder#1246) Co-authored-by: Bruno Clermont <bruno.clermont@gmail.com>
…ingress (nextlevelbuilder#1128) When a member calls team_tasks(action="complete"), goclaw fans the result back to the Lead's session via the team-task announce queue. The Lead resumes a turn whose initial user message is the synthesized "[System Message] Team member ... completed task. Result: ..." string. Inside that resumed turn, the Lead is a regular agent — it can call any tool, including write_file and cron mutations. Those tools route through CheckFileWriterPermission / CheckCronPermission, which in group-scope sessions deny when the resumed RunRequest carries no SenderID: permission denied: system context cannot write files in group chats. If this is a legitimate user action, ensure the acting sender is preserved through the tool chain. team_tool_dispatch.go already stamps MetaOriginSenderID and MetaOriginRole into the dispatch metadata. consumer_handlers.go already reads inMeta on the completion-side teammate message. The gap was in between: - announceRouting (cmd/gateway_announce_queue.go) had no field for OriginSenderID / OriginRole - The RunRequest it built had no SenderID / Role set - loop_context.injectContext skips WithSenderID when req.SenderID is empty — so the Lead's resumed ctx had no sender attribution, and every group-scope permission check then tripped the deny path. Subagent path (subagentAnnounceRouting in gateway_subagent_announce_queue.go) already had these fields wired since nextlevelbuilder#915. This brings the team-task path to parity. No security relaxation: empty upstream → still empty downstream (still denies, as intended for genuine system-initiated turns); only legitimately-attributed dispatches now flow through. - cmd/gateway_announce_queue.go: add OriginSenderID + OriginRole to announceRouting; pass them into the RunRequest. - cmd/gateway_consumer_handlers.go: read MetaOriginSenderID + MetaOriginRole from inMeta when building the routing struct. - cmd/gateway_announce_routing_test.go: 2 unit tests guarding both "real human propagates through" and "empty stays empty" cases so this regression can't re-land silently. Tests: existing cmd/ + internal/tools/ suites pass; new tests pass.
nextlevelbuilder#1062) The CLI setup wizard and 'providers add' command were using 'openai-compat' (hyphen), but the API and database expect 'openai_compat' (underscore), causing provider creation to fail. Fixes nextlevelbuilder#1046
Refresh MiniMax to MiniMax-M3, update Z.AI defaults to glm-5.2, and add focused provider catalog/runtime coverage.
…nextlevelbuilder#1111) setupToolRegistry creates the rate limiter from cfg.Tools.RateLimitPerHour during early bootstrap (gateway.go line 137). System_configs DB overlay runs ~50 lines later via cfg.ApplySystemConfigs (line 191), so any DB override of tools.rate_limit_per_hour was silently lost - the limiter object was already initialised from the JSON5 default. Symptom in production: editing tools.rate_limit_per_hour via system_configs table or the config HTTP API had no effect on running gateways. Operators had to inject a config.json file to change the value, defeating the DB-as-source-of-truth pattern that other tunables rely on. Re-apply the limiter after ApplySystemConfigs runs. Safe ordering: server has not started yet, no in-flight tool calls, and SetRateLimiter is a plain field assignment with no shutdown cost on the discarded limiter. nil case (rate_limit_per_hour <= 0) also handled so DB writes can disable the limiter without restart. Tests: new TestRegistry_SetRateLimiter_ReplacesPriorLimiter covers both the replace-with-higher-limit path and the nil-disables path. All existing rate limiter tests still pass. Note: the same ordering pattern likely affects other config consumed inside setupToolRegistry (tools.scrub_credentials, MCP server wiring). Out of scope for this hotfix - they need a deeper restructure.
…extlevelbuilder#1192) * fix(agent): check nil td.Function to prevent panic on native tools Fix nil pointer dereference panic when agent processes native tools (such as image_generation) which do not contain function schema wrappers. Key changes: - loop_pipeline_callbacks.go & think_stage.go: add checks for td.Function != nil before accessing td.Function.Name. - loop_tool_filter.go & loop_history_toolnames.go: add safety checks when filtering and extracting tool names. - anthropic_request.go & codex_build.go: verify t.Function != nil when formatting tools for provider API requests. * test: add regression tests for native tools nil function panic Add targeted regression tests to verify that: - ThinkStage builds AllowedTools without panicking when mixed with native tools - Loop's buildFilteredTools filters native tools correctly without panicking across all filters - Anthropic request builder skips native tools safely - Codex request builder handles native tools with nil Function fields gracefully
…extlevelbuilder#1251) * feat(tools): implement image reference processing and native provider support - Support OpenAI image edits via both Multipart form-data and JSON payloads - Automatically append reference image descriptions to prompt under [Reference Image Roles] - Support downloading image URLs for Gemini native image generation - Deduplicate reference images to optimize API request size - Add unit tests for Codex, DashScope, MiniMax, BytePlus and local/remote path resolution * fix(tools): SSRF-guard reference-image URL downloads in create_image downloadImageBytes fetched caller-supplied ref_images[].url with a plain http.Client and unbounded io.ReadAll — no SSRF validation, redirect policy, or size cap, letting the gateway dial loopback/private/metadata hosts or read arbitrarily large responses. - Validate the URL via security.Validate and pin the resolved IP, then download through security.NewSafeClient (pinned dial, no redirects). - Cap the response with a bounded read (refImageMaxBytes, 20 MB). - Reject non-HTTP(S) reference URLs up front (file://, data:, gopher://) so provider-forwarded URLs stay HTTP(S)-only; document the trust boundary between gateway-side fetch and provider-forwarded URLs. - Add regression tests: blocked loopback/private/metadata, unfollowed redirect, oversized response, and non-http(s) scheme rejection.
nextlevelbuilder#1211) * feat(webhooks): add webhook management UI with delivery history & test Webhooks admin page on the web dashboard for managing inbound HTTP webhooks (llm + message kinds), backed by new admin endpoints. No schema changes (reuses migrations 000059-000061). Backend (internal/http): - GET /v1/webhooks/{id}/calls - paginated delivery history (trimmed DTO; status/limit/offset filters; ownership + tenant scoped) - GET /v1/webhooks/{id}/calls/{callId} - full single-call detail (request payload, full response, callback URL, idempotency key, timestamps) - POST /v1/webhooks/{id}/test - server-side test invocation using the admin session (no secret); dispatched by kind via RunTest() on the llm/message handlers; message tester nil-guarded (403 on Lite) Injects WebhookCallStore + SetTesters() in cmd/gateway_http_wiring.go, adds i18n key webhook.message_test_requires_standard (en/vi/zh), and unit tests for list/detail (filter, tenant isolation) and test (success/error/edition gate). Frontend (ui/web): - /webhooks admin-only page: list with revoked filter + badge, create/edit form (edition-gated message kind, Lite localhost_only lock), show-once secret dialog (create + rotate), test dialog, and delivery-history dialog with server-side pagination plus a click-through full call-detail dialog. - use-webhooks hooks, query keys, types, routing, sidebar entry (Cable icon to distinguish from the existing event "Hooks" page), webhooks i18n namespace (en/vi/zh). * fix(webhooks): validate admin message tests --------- Co-authored-by: Goon <duy@wearetopgroup.com>
* feat(mcp): MCP OAuth 2.1 client — full implementation with tests
Implements a complete MCP OAuth 2.1 authorization flow for tool servers that
require user-delegated access, covering all layers from DB to UI.
- discovery.go: RFC 9728 protected-resource → RFC 8414 AS metadata → OIDC
fallback chain with 5-min in-memory cache and InvalidateCache()
- dcr.go: RFC 7591 Dynamic Client Registration with response size guard
- flow.go: PKCE (S256) authorization code flow — StartFlow(), ExchangeCode(),
ClientCredentials(), auto-cleanup of expired flows; carries AS issuer through
PendingFlow for status display
- refresher.go: OAuthTokenProvider with in-memory token cache, automatic refresh
on expiry, per-user vs global slot isolation, InvalidateCache/InvalidateServer
- migrations/000074 + SQLite schema: mcp_oauth_tokens with AES-256-GCM encrypted
access/refresh tokens, partial unique index for global vs per-user rows,
ON DELETE CASCADE from mcp_servers
- store.MCPOAuthTokenStore: Upsert, Get/GetUser, Delete/DeleteUser, and
DeleteServerOAuthTokens (purge all rows for a server)
- PostgreSQL + SQLite implementations
- POST /v1/mcp/oauth/start — discovery + optional DCR + PKCE redirect URL;
client_credentials completes server-side (no redirect) and returns completed=true
- GET /v1/mcp/oauth/callback — exchange code, persist token, publish WS event;
payload built via json.Marshal (no reflected XSS via error_description)
- GET /v1/mcp/oauth/status/{id}, DELETE /v1/mcp/oauth/token/{id} — admin-gated
- POST /v1/mcp/oauth/discover/{id} — on-demand discovery probe
- All outbound calls go through the SSRF-safe client with pinned IPs
- pkg/protocol/mcp_events.go: EventMCPOAuthComplete routed only to the initiating
user (admins in-tenant included); fail-closed across tenants
- getUserMCPTools() injects Authorization: Bearer from OAuthTokenProvider; on a
401 for OAuth servers it purges the cached token so the next turn re-resolves
- handleUpdateServer purges all OAuth tokens (global + per-user), drops the
refresher cache, and evicts the pool when a server's URL or OAuth config
(client_id / endpoints / grant_type / scope / auth_type) changes — so the
status UI and agent never use a token minted for the old resource/AS
- MCPOAuthDialog (WS-driven), unified user-credentials dialog, OAuth settings
fields; handles the no-redirect client_credentials completion
- internal/mcp/oauth/*_test.go: discovery cache, PKCE, DCR, refresher
- internal/http/mcp_oauth_test.go + mcp_update_oauth_purge_test.go: routes, auth
gating, WS event, purge-on-URL/OAuth-config-change
- tests/integration: store + encryption + tenant isolation, E2E start→callback,
DeleteServerOAuthTokens
- internal/gateway/event_filter_test.go, internal/agent/loop_mcp_user_test.go
* fix(mcp): return 400 on OAuth callback with code but missing state
The callback handler rendered a 200 HTML page whenever code or state was
absent. An auth code WITH a missing state is a malformed / CSRF-risk
callback (state is the CSRF token), so reject that case with HTTP 400.
A bare hit with neither code nor state (user opening the URL directly),
provider errors, and exchange failures keep their 200 HTML popup page.
Adds a status code parameter to writeCallbackHTML. Fixes the
TestOAuthCallbackMissingState integration regression while keeping
TestHandleCallbackMissingCodeAndState (no params -> 200) green.
* fix(mcp): scope-based OAuth auth + honor manual OAuth endpoints
Addresses the two MCP/OAuth security-review findings.
Finding 1 — authorization. mcp_oauth_tokens is tenant-scoped, but
start/status/revoke were gated only by requireAuth(RoleAdmin), an RBAC
role check, not tenant membership, so a RoleAdmin caller could act on a
tenant they don't administer. A blanket requireTenantAdmin would have
broken per-user self-service, which the UI exposes (the per-user
MCPUserCredentialsDialog shows an "Authorize" button to regular users for
their own credentials). Instead mirror the existing per-user MCP
credentials model (resolveTargetUserID in mcp_user_credentials.go):
- start/status/revoke accept any authenticated user; each handler calls
authorizeOAuthScope.
- a caller may manage their OWN per-user token (self-service); the
global/server token (user_id="") and other users' tokens require
tenant-admin (owner bypass), so a RoleAdmin that is not a tenant admin
is rejected.
- discover stays admin-only (it only previews AS metadata for a server).
Add a TenantStore dependency. Tests cover self-service, on-behalf-of-
another (403), and global-by-non-tenant-admin (403).
Finding 2 — honor manual OAuth config end-to-end. The UI sent use_dcr /
auth_endpoint / token_endpoint and the update path fingerprinted them for
purge, but handleStart always discovered + DCR'd and ignored them. Now:
- use_dcr=false (a *bool, so legacy/absent stays discover+DCR) skips
discovery/registration and uses the operator endpoints, SSRF-validated.
- token_endpoint is always required; auth_endpoint only for auth-code
grants — client_credentials needs no authorization URL, matching the UI
which hides that field for that grant.
- the refresher already refreshes against the stored token_endpoint and
the callback persists it, so manual-mode tokens refresh correctly.
- oauthFingerprint includes use_dcr (nil normalized to true) so toggling
DCR mode purges stale tokens.
- the web form only serializes manual endpoints when use_dcr is off.
Audited all MCP dialogs (form, global OAuth, per-user credentials, grants,
tools): OAuth dialogs handle completed/auth_url identically and read
config from stored server settings; runtime connect uses the stored token
via the refresher (no re-discovery).
Tests: manual auth-code + client_credentials endpoints, missing/SSRF
endpoints, and the full self/global/on-behalf authorization matrix.
* fix: recover whatsapp qr after deleted device * fix: whatsapp connect * fix whatsapp instance device scoping --------- Co-authored-by: Duy /zuey/ <duy@wearetopgroup.com>
…echo, and hardening (nextlevelbuilder#1236) * refactor(bitrix24): rename "Path B" framing to maintainer-specified naming [B24:2794] Per maintainer hard rule nextlevelbuilder#10 (no generic "Path A/B" framing) from PR nextlevelbuilder#1061 review. The Bitrix24 MCP auto-onboard flow is Bitrix-specific glue ("Bitrix24 OAuth -> existing mcp_user_credentials bridge"), NOT a generic MCP architecture pattern. Naming convention applied consistently: - First mention per file: full "Bitrix24 OAuth -> existing mcp_user_credentials bridge" (matches maintainer comment verbatim). - Subsequent mentions in same file: shortened "mcp_user_credentials bridge". - Test/log context referencing literal endpoint /api/auto-onboard: keep "auto-onboard" reference (it's the actual API endpoint name). Changes are documentation-only: - Rename in code comments + test descriptions + plan docs. - Clarify framing in mcp_client.go + provisioner.go doc comments to emphasize Bitrix-specific glue (not generic MCP infra). - Reuse existing mcp_user_credentials table + MCPServerStore methods (no schema / store / abstraction change). Files: - cmd/gateway.go (factory registration doc) - internal/channels/bitrix24/{channel,factory,mcp_client,provisioner}.go - internal/channels/bitrix24/{mcp_client,provisioner}_test.go - plan/goclaw-mcp-integration.md (21 occurrences) Verified: go build + MCP-related tests pass (TestProvision*, TestInitMCPProvisioner*, TestMCPClient*). Phase 1 of Path C execution per plans/reports/decision-log-260519-1555-bitrix24-pr-fork-decision.md. * fix: confine outbound media paths to agent workspace [B24:2794] Tool MEDIA:<path> output reached channel file-upload sinks (Bitrix imbot.v2.File.upload, Telegram sendDocument, etc.) verbatim via parseMediaResult, with no workspace-boundary check. A malicious or buggy tool emitting MEDIA:/etc/passwd could exfiltrate arbitrary files to chat. Extract the EvalSymlinks+Rel containment from extractMediaFromContent into a shared confineToWorkspace helper and apply it at the parseMediaResult sink in processToolResult. Fixing at the source/egress boundary protects every channel at once rather than per-channel. Paths that escape the workspace are dropped and logged (security.media_path_rejected). Add TestConfineToWorkspace (boundary unit) and TestParseMediaResultConfinedToWorkspace (sink regression for H2). * feat(bitrix24): support inbound + outbound media via imbot.v2 File API [B24:2794] Bitrix24 channel was text-only; attachments were parsed but dropped. - Inbound: download chat files via imbot.v2.File.download (one-time URL), forward to the agent with MIME preserved (internal/channels/bitrix24/download.go). - Outbound: upload agent media to the chat via imbot.v2.File.upload (internal/channels/bitrix24/send_media.go). - Add BaseChannel.HandleMessageMedia to preserve MIME/filename through the bus. - Per-channel media_max_mb cap (default 20) applies to both directions. Tests: 92 pass (internal/channels/bitrix24 + internal/channels), go vet clean (PG + sqliteonly). * refactor(bitrix24): migrate messaging/bot-list/unregister to imbot v2 API [B24:2794] Move outbound REST calls to the imbot v2 family (keeps register on v1): - imbot.message.add -> imbot.v2.Chat.Message.send (fields.message shape, live-verified) - imbot.bot.list (+ legacy imbot.list fallback) -> imbot.v2.Bot.list; add botListRows to normalize the v2 {bots:[...]} envelope, legacy array, and id-keyed map forms - imbot.unregister -> imbot.v2.Bot.unregister Bot registration stays on v1 imbot.register: v2 imbot.v2.Bot.register changes the event-delivery model (per-event handler URLs -> eventMode), which would require rewriting the inbound event parser. No user-facing behavior change. Tests: bitrix24 package green; go vet ./... clean. * feat(bitrix24): route whisper via v1 SKIP_CONNECTOR + add v2 replyId [B24:2794] Bot was leaking HiddenMessage (whisper) replies to the external Zalo connector because every outbound call went through imbot.v2.Chat.Message.send, which has no equivalent of the v1 SKIP_CONNECTOR flag. Branch the outbound path on inbound visibility: whisper → imbot.message.add + SKIP_CONNECTOR=Y (v1, send_v1.go) public → imbot.v2.Chat.Message.send + fields.replyId (v2, send_v2.go) Pipeline: events.go parse data[PARAMS][PARAMS][COMPONENT_ID]=HiddenMessage into EventParams.IsHiddenMessage (form + JSON variants) handle.go set bitrix_visibility on InboundMessage.Metadata consumer forward visibility + message_id into OutboundMessage send.go resolveSendOptions + sendChunk dispatcher + shared callWithRateLimitRetry helper metadata_keys.go single source of truth for the keys + values Defaults preserve pre-refactor behaviour: callers that don't populate bitrix_visibility still go through v2 public, and replyId is omitted unless a numeric bitrix_message_id arrives in metadata. Tests: TestParseEvent_FormURLEncoded_IsHiddenMessage (3 cases) TestParseEvent_JSON_IsHiddenMessage (3 cases) TestResolveSendOptions (8 cases) TestSend_BranchesOnVisibility (4 cases) * feat(bitrix24): openline sender-tag echo on replies [B24:2794] Openline sender-tag echo (this change): - Capture the connector sender tag ("[name #id]:" or "[name] #id:") from inbound openline group messages, strip it from the body the agent sees, and re-prepend the canonical "[name] #id:" form to the reply so the Open Channel connector routes the answer back to the right external user. - New sender_prefix.go helper (+ test) accepts both inbound layouts and emits one canonical form; scoped to messages carrying the tag, so plain chats are unaffected. - metadata_keys.go: MetaKeySenderPrefix; handle.go capture/strip/stash; gateway_consumer_normal.go forwards the key; send.go prepends it on the first chunk before chunking. Bundled bitrix24 channel-core work already on this branch: - handle.go: @mention is the sole trigger for both staff and connector customers; unmentioned traffic is dropped (was: drop all connector msgs). - isGroupMessageType: treat SONET_GROUP "B" as a group. - handle_test.go, mcp_client_test.go: cover the above. * feat(bitrix24): accept colon-less openline sender tag, echo [name] #id [B24:2794] The Open Channel connector dropped the trailing colon from its sender tag: inbound now arrives as "[Name] #id <msg>" (was "[Name] #id: <msg>"). The id-bearing patterns required the colon, so the tag fell through to the name-only branch and the reply echoed "[Name]" — dropping the #id the connector needs to route the answer back. - sender_prefix.go: make the trailing ":" optional on both id layouts ([name #id] / [name] #id, with or without colon) and echo the canonical "[name] #id" (no colon) to match the connector's current format. Bare "[name]" (no id) still echoes "[name]" for Open Channel only. - handle.go: gate the bare name-only layout to Open Channel (isOpenChannel) so ordinary group chats starting with "[x] ..." are left untouched. - sender_prefix_test.go: cover colon/no-colon x id-inside/id-outside, the name-only openline case, and the non-openline no-op. * fix: security and robustness fixes from the bitrix24 channel review [B24:2794] - download.go: block redirect-based SSRF on inbound media. CheckRedirect re-validates each hop (http(s) only, reject private/loopback/link-local hosts, cap hops); the initial portal-domain pin is no longer bypassable via a 3xx to an internal service. Public-host redirects still allowed. - handle.go: extract/echo the openline sender tag only for Open Channel sessions (was: any group chat), removing bogus prefixes in CRM group chats and narrowing the forged-tag misroute surface. - loop_tools.go + loop_media.go: confine result.Media to the agent / team / tenant-allowed roots (new confineToAnyRoot) before a channel uploads it, so a prompt-injected out-of-workspace path (e.g. /etc/passwd) cannot exfiltrate, while legitimate cross-workspace media (team files, delegatee output) still flows. - send_media.go: bounded outbound read via io.LimitReader replaces the os.Stat + os.ReadFile pair, closing the TOCTOU size-cap bypass; cap a single message's outbound attachments at 10 (mirrors inbound). - register.go: paginate imbot.v2.Bot.list (limit/offset + hasNextPage, capped at 40 pages) so verify/lookup see bots past the first 50. - mcp_client.go: redact access_token / refresh_token / client_secret from an echoed MCP error body before it is logged or returned (+ test). * fix(security): validate resolved dial IP on Bitrix media redirects [B24:2794] The inbound media download redirect guard only string-checked the redirect hostname (isPrivateOrLoopback on req.URL.Hostname()), so a redirect to a public hostname that resolves to 127.0.0.1 / 169.254.169.254 / an RFC1918 address — or a DNS-rebinding swap between check and dial — still passed the guard and the client would connect. Reported in PR review. Add security.NewRedirectFollowingSafeClient: it follows redirects but validates the RESOLVED destination IP of every hop at dial time via net.Dialer.Control, reusing the existing blocked-CIDR list. The IP it checks is the IP actually dialed, so both redirect-to-internal and DNS rebinding are refused, while legitimate public CDN redirects still succeed. download.go now uses it instead of the hostname-string guard. Tests: deterministic dial-control table (loopback / link-local / private / multicast / unspecified / public, v4 + v6), malformed/non-IP addr, test bypass, loopback-dial-blocked client wiring, and redirect cap + scheme checks. * feat(bitrix24): per-participant Zalo openline identity from 3-token sender tag [B24:2794] Parse the connector's "[Name] #uid #msgId" sender tag so each external customer in a shared Open Channel group gets its own contact + USER.md instead of collapsing onto the connector proxy id. Identity minting is gated on IS_CONNECTOR=Y to reject operator forged tags. Echo back the msgId only ("#msgId") on replies; keep the legacy single-number and name-only layouts unchanged. Zero DB migration. - sender_prefix.go: parseOpenlineSenderTag() classifies 3-token / legacy / name-only - handle.go: synthetic senderID "openlines:{instance}:{chat}:{uid}" + participant_user_id metadata, gated on FromIsConnector - gateway_consumer_normal.go: deriveGroupUserID() routes participant -> per-person scope, group fallback otherwise - send.go: buildAddressMention numeric-id guard so synthetic ids don't emit invalid [USER=...] BBCode - MetaKeyMessageID kept as Bitrix MESSAGE_ID (drives v2 fields.replyId); connector msgId surfaced only via echo prefix --------- Co-authored-by: DangTinh311 <dangtinh31193@gmail.com> Co-authored-by: Chinh Dang <chinhdang@192.168.68.104>
…xtlevelbuilder#1107) Local pkg-helper installs an arm64 mcp-server binary on macOS hosts; the existing allowlist (deno, bun, uvx, ...) didn't cover it so MCP server config validation rejected legitimate entries.
…lder#1259) Co-authored-by: nguyenha935 <208228297+nguyenha935@users.noreply.github.com>
…extlevelbuilder#1099) Allow overriding the cron job execution timeout via env var, matching the existing GOCLAW_* env var pattern used throughout the codebase. Falls back to the existing default (10m) when unset or invalid, since CronConfig.JobTimeoutDuration() already handles parse errors with a slog.Warn and the DefaultJobTimeout constant. Co-authored-by: will.nguyen <will.nguyen@gemcommerce.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…extlevelbuilder#1260) stripGarbledToolXML removed only the tags of a model-emitted <function_calls> block, leaking the inner <parameter> argument values into the user-facing reply, and the dropped call was completely silent. Detect a complete tool-call block, remove it whole, and log the attempted tool name(s) at WARN so the no-op is diagnosable. Partial artifacts still fall through to the existing tag-level strip. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…extlevelbuilder#1261) Follow-up to nextlevelbuilder#1260. A model (notably the claude-cli proxy under a degraded session) sometimes emits a tool call as a bare <invoke name="...">...</invoke> block with no <function_calls> wrapper. fullToolCallBlockPattern only matched the wrapped form, so the bare block slipped through to the tag-only strip, leaking the inner <parameter> command text into the reply while the tool never ran. Match and remove complete bare <invoke> blocks too (after the wrapped form), add "<invoke name=" as a detection indicator, and log the dropped tool name(s). Partial/unterminated artifacts still fall through to the existing tag strip. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…extlevelbuilder#1262) The per-run session reset was gated on `!job.Stateless`, inverting the flag: stateless jobs (the token-saving default) skipped the reset and accumulated unbounded history, while stateful jobs were wiped every run. Reset for `job.Stateless` instead, and clear BOTH session layers — the goclaw session store AND the Claude CLI on-disk .jsonl. claude-cli resumes its own session by a deterministic per-key UUID, so without clearing the .jsonl a "stateless" run still replayed the entire accumulated history (and silently grew it run after run).
…der#1263) The agent config UI (tools-profile-section) and i18n already expose a per-agent "Rate Limit (per hour)" field saved into tools_config, but the backend ignored it — the tool rate limiter only ever used the global tools.rate_limit_per_hour. Wire the field through: ToolPolicySpec gains RateLimitPerHour; the agent loop threads it into the tool-execution context; the registry passes it to the limiter, which uses it in place of the global max when > 0 (0 inherits the global).
…velbuilder#1267) Replace the hardcoded 30s webhook agent-run deadline with a configurable timeout (default 600s, cap 3600s) for both the async worker and the sync/test HTTP handler. Legitimate multi-step runs were dying at 30s mid-tool-call; the same request over WebSocket completed fine. - internal/webhooks/timeout.go: ResolveTimeoutSec helper (<=0 → 600s, cap 3600s) - WorkerConfig.AsyncAgentTimeout (async worker) + WebhookLLMHandler.syncTimeout (sync + admin test), wired from config - config keys gateway.webhook_{async,sync}_timeout_sec, also settable via GOCLAW_WEBHOOK_{ASYNC,SYNC}_TIMEOUT_SEC env (env overrides config) - tests: timeout bounds + config file/env override
consistent language
…artifact-media-dedup fix(collaboration): protect artifact and media delivery lifecycle
…nds (nextlevelbuilder#1480) Added error handling for input reading in the agent chat client and for database row reading in the doctor commands, ensuring that errors are reported clearly to the user.
…xy (nextlevelbuilder#1465) SSRF protection resolves a hostname and judges the resulting IP. That model assumes DNS resolution describes where the traffic actually goes, which stops being true behind a TUN/fake-IP proxy: every query is answered with a synthetic address out of a reserved range, and the proxy then routes that address to the real public host. The IP is a handle, not a destination. In that environment web_fetch rejects ordinary public sites — observed with news.sina.cn resolving to 198.18.0.236 — and no configuration can fix it, because the block list is compiled in. The agent then burns iterations retrying URLs that can never succeed. GOCLAW_SSRF_ALLOWED_CIDRS lets an operator name the ranges their proxy hands out. Empty by default, so nothing changes for deployments that do not set it, and the accepted and refused entries are both logged at startup — this widens what LLM- and admin-supplied URLs can reach, so it should be visible. Ranges an SSRF actually targets can never be allowlisted: link-local (including cloud metadata at 169.254.169.254), multicast and unspecified are refused at parse time, in either direction, so neither an exact entry nor a wider range that swallows one gets through. Applied inside isBlocked rather than only in validate() because NewSafeClient re-checks the pinned IP at dial time through the same function — relaxing just the pre-flight check would pass validation and then fail to connect. internal/tools carries its own private-range list for web_fetch and web_search, separate from this package and not identical to it. It has to consult the same setting, or relaxing one gate leaves the other rejecting the very traffic the operator permitted. Unifying the two lists is left alone here; it is a wider change than this one. Co-authored-by: Conner Mo <connermo@ConnerdeMacBook-Pro.local>
Add Lightpanda as alternative to Chrome for the browser automation tool
…dispatch-sharding perf(channels): shard outbound dispatch per conversation
…x/add-atlascloud-provider-goclaw Add Atlas Cloud provider defaults
…evelbuilder#1461) The workstations page was written against snake_case field names the gateway never served. Go's json decoder drops unknown members silently, so create requests arrived with an empty WorkstationKey and were rejected with "workstationKey is required", while list rows read undefined and rendered a blank key, "backend.undefined" and "Invalid Date". Align the TypeScript contract on the camelCase shape that both the WS handler and store model already use: - Rename Workstation, CreateWorkstationParams and every field read on the workstations page to workstationKey/backendType/createdAt/updatedAt. - Nest update params under `updates`; a flattened body decoded to an empty map and was rejected with "no updates provided". - Replace the Identity File input, which mapped to a field SSHMetadata does not accept, with a private key or password credential path. - Add the image field required by DockerMetadata and map the container name onto host and the daemon endpoint onto socketPath, so Docker workstations can be created at all. - Extract the payload builder so the wire contract is unit-testable, and move validation strings into the en/vi/zh catalogs. Covered by tests asserting the create payload shape and list rendering against a verbatim gateway response body. Co-authored-by: Eddy Lockwood <doakythanh@gmail.com>
Co-authored-by: cyphercodes <cyphercodes@users.noreply.github.com>
Gemini drops tool_call_id and pairs functionCall/functionResponse by function name, so its OpenAI-compat shim requires a non-empty FunctionResponse.name. The name was only recoverable through a reverse id->name lookup over assistant tool_calls still present in the request window, which fails after pruning, truncation or tool_call collapse and produces HTTP 400 "Name cannot be empty" on any follow-up iteration. Add Message.ToolName, set it at every tool-result creation site and preserve it through context pruning, then prefer it when serializing. Fall back to the existing index for history persisted before the field, so old sessions keep working without a reset. When neither source resolves a name, drop the unlabelled tool result instead of emitting an empty one: an empty name is a guaranteed 400 and a synthetic name would match no prior functionCall. Gated behind the existing Gemini detection so other OpenAI-compat hosts, which pair by tool_call_id, are unaffected. Field is JSON-optional, so session history needs no migration and older binaries ignore it on rollback.
vault_read is a singleton wired once at boot with the global workspace root, then joins doc.Path (stored relative to the TENANT root) onto it. For master tenant both roots coincide, so the bug stayed invisible; every other tenant reads ENOENT because its files live under tenants/<slug>/. vault_search is DB-only and keeps returning the doc, so the symptom is "search finds it, read fails" rather than an obvious missing file. Resolve the tenant-scoped root per request via TenantLayer, which is a no-op for master and preserves existing single-tenant behaviour. Passing the tenant root into resolvePath also narrows the boundary check, so it now doubles as cross-tenant isolation. Runs originating from channels or cron may not carry the tenant slug in context; without it TenantLayer falls back to a UUID-named directory that does not match the slug-named one the write path creates. Add a nil-safe TenantStore lookup for that case only.
…ments
isAllCapsPlaceholder() classified ANY string of >=3 chars made only of A-Z and
underscore as an LLM placeholder, and stripEmptyOptionalArgs() then removed it
from the outbound CallTool arguments. Real business identifiers match that shape:
bank codes BKASH, NAGAD, ROCKET, VCB
currency codes BDT, VND, USD, PKR, MMK
enum values SUCCESS, PENDING, ALL
Impact is silent and severe: dropping an optional filter turns a filtered query
into an unfiltered one. The MCP server still answers 200 with the full result set,
so nothing errors — the model reports totals for the entire dataset as if they were
the filtered result, with full confidence.
Observed in production: asking "how many BKASH proxies are active" returned 6,610
(all banks) instead of 1,263. Three different bank codes returned byte-identical
results, which also drove the model into a retry loop until the loop guard fired.
Diagnosis was slow because the removal is invisible: the outbound log records
args_len BEFORE stripping, and stripEmptyOptionalArgs logs nothing at all — while
normalizeArgsForSchema right below it does emit mcp.tool.args.coerced. Confirmed
the argument never reaches the server by echoing the raw JSON-RPC arguments from
RequestContext[CallToolRequestParams] on the server side.
Changes:
- isAllCapsPlaceholder: require multi-word (contains "_") or membership in a known
single-word placeholder list. Keeps the original intent — the doc comment's own
examples (SHOULD_NOT_BE_HERE, DO_NOT_SEND, NOT_APPLICABLE) are all multi-word —
while letting business codes through.
- stripEmptyOptionalArgs: emit slog.Warn("mcp.tool.args.stripped") whenever an
argument is dropped, so this class of bug is one grep away instead of a
multi-hour investigation.
- Tests: guard both layers against regression.
Verified: go build ./... clean; go test ./internal/mcp/ passes.
The vault_read fix changes what the injected workspace means: it is the global root wired once at boot, not a per-tenant root, since one tool instance serves every tenant. These tests seeded a non-master tenant but wrote fixtures straight to the injected root, so they only passed while reads ignored the tenant layer. Write fixtures to config.TenantWorkspace(base, tenantID, slug) and carry the slug in context, matching a real run. tenantSlug is now the single source of truth shared with seedTenantAgent, so on-disk layout in a test cannot drift from the seed.
…ndary The text-only gate sniffs the first 8192 bytes of a document and rejects the read when they are not valid UTF-8. The window was cut at a plain byte offset, so on non-ASCII text it regularly landed inside a multi-byte rune and left a dangling prefix. The gate then reported the document as binary even though the file was intact — measured on Vietnamese documents, about one in four files above 8 KB was blocked, depending on where the accented characters happen to fall. Trim up to 3 bytes off the window when, and only when, the content was actually cut there, then judge. A file that fits inside the window is left alone, so genuinely broken trailing bytes are still caught, and binary content stays blocked because its invalid bytes are spread over the whole window. readCapped had the same byte-offset cut at max_bytes, which put a broken character at the end of every truncated non-ASCII document.
…ni-tool-result-name fix(providers): carry tool name on tool results for Gemini
…t-read-tenant-workspace fix(vault): resolve vault_read paths under tenant workspace root
…allcaps-business-codes-stripped fix(mcp): stop stripping all-caps business identifiers from tool arguments
…t-read-utf8-rune-boundary fix(vault): stop vault_read rejecting non-ASCII text at the sniff boundary
…d thread context handleMessage prepended the thread parent's raw text (mention tags included) to content via fetchThreadParentContext, then evaluated isBotMentioned(content). In any thread whose root message @mentions the bot, every reply therefore inherited the root's mention and passed the gate — the bot auto-replied to human-to-human thread messages even with require_mention: true and thread_ttl: 0. Extract the gate into shouldRespondInGroup(eventText, channelID, threadTS), called with ev.Text before any context injection. Thread participation TTL behavior (auto-reply window, stale-entry eviction) is preserved unchanged. Add table tests covering the mention path, the disabled-TTL regression, and participation freshness/eviction.
Author
|
FYI: we are now running this patch in production (built as upstream v3.14.0 + this commit cherry-picked). It deterministically fixed the reported behavior — thread replies without a mention no longer trigger the bot in channels with require_mention: true, while explicit mentions and the thread-participation TTL window keep working. Happy to adjust anything if you'd like changes. |
clark-cant
requested changes
Aug 16, 2026
clark-cant
left a comment
clark-cant
left a comment
Contributor
There was a problem hiding this comment.
Review — scope blockernnThe described Slack mention-gate fix is valuable and the production note is useful, but this PR cannot be reviewed or merged as submitted: it changes 1,330 files (+138,935/-7,005) and is currently DIRTY. That is far beyond the stated single-handler fix and makes correctness, regression risk, and provenance impossible to assess safely.nnPlease rebase onto the intended base branch and split this into a minimal PR containing only the Slack gate change plus focused regression tests. Once the diff is narrow and mergeable, it can receive a substantive review.nnMandatory gatesn- Duplicate/prior implementation: no merged replacement identified for this scoped fix.n- Project standards: repository conventions cannot be validated across this unrelated oversized diff.n- Strategic necessity: clear for the Slack bug, not justified for the unrelated bulk changes.nnVerdict: Request changes — narrow scope and resolve conflicts before review.nnPosted by /ck:review-pr at 2026-08-16T05:15:51Z
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
20 participants
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.
Fixes #1502
Problem
handleMessageprepends the thread parent's raw text (mention tags included) tocontentviafetchThreadParentContext, then evaluatesisBotMentioned(content). In any thread whose root message @mentions the bot, every reply inherits the root's mention and passes the gate — the bot auto-replies to human-to-human thread messages even withrequire_mention: trueandthread_ttl: 0.Fix
shouldRespondInGroup(eventText, channelID, threadTS)inhandlers_mention.go, called withev.Textbefore any context injection.contentstill carries the quoted parent context downstream for the agent — only the gating source changes.The
message_changedpath already gates onev.Message.Textand promotes it toev.Textbefore this point, so it is unaffected.file_sharemessages now gate on the user's caption (ev.Text) rather than extracted document text, which is consistent with the same principle.Tests
TestShouldRespondInGrouptable tests: mention in event text (channel + thread), no mention at channel level, the regression case (thread reply without its own mention,thread_ttl=0, participation entry present → must NOT respond), fresh participation within TTL, stale participation.TestShouldRespondInGroupEvictsStaleParticipationpins stale-entry eviction.go build ./...,go vet,gofmtand the fullinternal/channels/slacksuite pass.