diff --git a/cmd/gateway_builtin_tools.go b/cmd/gateway_builtin_tools.go index 04c1e92b..8a417ea2 100644 --- a/cmd/gateway_builtin_tools.go +++ b/cmd/gateway_builtin_tools.go @@ -24,6 +24,7 @@ func builtinToolSeedData() []store.BuiltinToolDef { {Name: "exec", DisplayName: "Execute Command", Description: "Execute a shell command in the workspace and return stdout/stderr", Category: "runtime", Enabled: true, Metadata: json.RawMessage(`{"config_hint":"Config → Tools → Exec Approval"}`), }, + {Name: "wait", DisplayName: "Wait", Description: "Pause the current agent tool sequence for a bounded number of milliseconds", Category: "runtime", Enabled: true}, // web {Name: "web_search", DisplayName: "Web Search", Description: "Search the web for information using a search engine (Brave or DuckDuckGo)", Category: "web", Enabled: true, diff --git a/cmd/gateway_builtin_tools_test.go b/cmd/gateway_builtin_tools_test.go new file mode 100644 index 00000000..a4beea53 --- /dev/null +++ b/cmd/gateway_builtin_tools_test.go @@ -0,0 +1,20 @@ +package cmd + +import "testing" + +func TestBuiltinToolSeedDataIncludesWait(t *testing.T) { + t.Parallel() + for _, def := range builtinToolSeedData() { + if def.Name != "wait" { + continue + } + if def.Category != "runtime" { + t.Fatalf("wait category = %q, want runtime", def.Category) + } + if !def.Enabled { + t.Fatal("wait should be enabled by default") + } + return + } + t.Fatal("builtinToolSeedData() missing wait") +} diff --git a/cmd/gateway_tools_wiring.go b/cmd/gateway_tools_wiring.go index 53d70ee2..85a0e001 100644 --- a/cmd/gateway_tools_wiring.go +++ b/cmd/gateway_tools_wiring.go @@ -39,6 +39,7 @@ func wireExtraTools( // DateTime tool (precise time for cron scheduling, memory timestamps, etc.) toolsReg.Register(tools.NewDateTimeTool()) + toolsReg.Register(tools.NewWaitTool()) // Cron tool (agent-facing) toolsReg.Register(tools.NewCronTool(pgStores.Cron)) @@ -261,4 +262,3 @@ func wireWorkstationTools( } return func() {} } - diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 7f1ba5e9..6afba041 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,20 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-18 +### Tools: built-in wait delay + +**Features** + +- Added a built-in `wait` tool with bounded millisecond delays, cancellation support, per-agent min/max settings, and runtime policy visibility. +- Preserved same-response ordering by making `wait` a sequential tool-call barrier. +- Added Web agent settings controls so per-agent wait limits are not dropped on save. + +**Tests** + +- Added focused wait validation, cancellation, policy, builtin seed, config parsing, and tool-stage ordering coverage. + +--- + ### Providers: ChatGPT OAuth GPT-5.5 default **Changed** diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index d7b69af3..287bc883 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -108,6 +108,11 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup if l.memoryCfg != nil { ctx = tools.WithMemoryConfig(ctx, l.memoryCfg) } + var waitToolCfg *config.WaitToolPolicy + if l.agentToolPolicy != nil && l.agentToolPolicy.Wait != nil { + waitToolCfg = l.agentToolPolicy.Wait + ctx = tools.WithWaitToolConfig(ctx, waitToolCfg) + } if l.sandboxCfg != nil { ctx = tools.WithSandboxConfig(ctx, l.sandboxCfg) } @@ -371,6 +376,7 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ParentProvider: providerName, MemoryCfg: l.memoryCfg, SandboxCfg: l.sandboxCfg, + WaitToolCfg: waitToolCfg, ShellDenyGroups: l.shellDenyGroups, Workspace: tools.ToolWorkspaceFromCtx(ctx), TeamWorkspace: tools.ToolTeamWorkspaceFromCtx(ctx), diff --git a/internal/agent/loop_pipeline_adapter.go b/internal/agent/loop_pipeline_adapter.go index ae6449a5..3c381b0e 100644 --- a/internal/agent/loop_pipeline_adapter.go +++ b/internal/agent/loop_pipeline_adapter.go @@ -141,7 +141,10 @@ func (l *Loop) buildPipelineDeps(req *RunRequest, bridgeRS *runState) pipeline.P ExecuteToolCall: cb.executeToolCall, ExecuteToolRaw: cb.executeToolRaw, ProcessToolResult: cb.processToolResult, - CheckReadOnly: cb.checkReadOnly, + SequentialToolCall: func(tc providers.ToolCall) bool { + return l.resolveToolCallName(tc.Name) == "wait" + }, + CheckReadOnly: cb.checkReadOnly, // Observe: drain InjectCh DrainInjectCh: func() []providers.Message { diff --git a/internal/agent/toolloop.go b/internal/agent/toolloop.go index 94592366..c1a79778 100644 --- a/internal/agent/toolloop.go +++ b/internal/agent/toolloop.go @@ -142,7 +142,7 @@ func (s *toolLoopState) detect(toolName string, argsHash string) (level, message } // recordMutation updates the read-only streak based on tool type. -// Mutating tools reset the streak; exec/bash/mcp are neutral (ambiguous); all others increment. +// Mutating tools reset the streak; exec/bash/wait/mcp are neutral; all others increment. // team_tasks is classified by action: read-only (list/get/search), neutral (progress), // or mutating (create/complete/cancel/comment/etc.). func (s *toolLoopState) recordMutation(toolName string, args map[string]any) { @@ -168,9 +168,10 @@ func (s *toolLoopState) recordMutation(toolName string, args map[string]any) { return } // exec/bash: ambiguous (could be ls or rm). + // wait: intentional delay, neither progress nor read-only scanning. // mcp_*: user-defined external tools — GoClaw cannot determine read vs write. // Neither reset nor increment the read-only streak. - if toolName == "exec" || toolName == "bash" || strings.HasPrefix(toolName, "mcp_") { + if toolName == "exec" || toolName == "bash" || toolName == "wait" || strings.HasPrefix(toolName, "mcp_") { return } s.incrementReadOnly(toolName, args) diff --git a/internal/agent/toolloop_test.go b/internal/agent/toolloop_test.go index 2300976b..7cf1b16d 100644 --- a/internal/agent/toolloop_test.go +++ b/internal/agent/toolloop_test.go @@ -570,6 +570,17 @@ func TestReadOnlyStreak_ExecNeutral(t *testing.T) { } } +func TestReadOnlyStreak_WaitNeutral(t *testing.T) { + var s toolLoopState + for range 5 { + s.recordMutation("read_file", nil) + } + s.recordMutation("wait", map[string]any{"timeMs": 1000}) + if s.readOnlyStreak != 5 { + t.Fatalf("expected streak 5 after wait, got %d", s.readOnlyStreak) + } +} + func TestReadOnlyStreak_MCPNeutral(t *testing.T) { var s toolLoopState // 5 reads → streak = 5 diff --git a/internal/config/config_channels.go b/internal/config/config_channels.go index 9d93564e..a05d8d28 100644 --- a/internal/config/config_channels.go +++ b/internal/config/config_channels.go @@ -1,4 +1,4 @@ -package config +package config // PendingCompactionConfig configures LLM-based compaction of pending group messages. // When a group accumulates more than Threshold pending messages, older messages are @@ -444,9 +444,15 @@ type ToolPolicySpec struct { Deny []string `json:"deny,omitempty"` AlsoAllow []string `json:"alsoAllow,omitempty"` ByProvider map[string]*ToolPolicySpec `json:"byProvider,omitempty"` + Wait *WaitToolPolicy `json:"wait,omitempty"` ToolCallPrefix string `json:"toolCallPrefix,omitempty"` // prefix to strip from model's tool call names before registry lookup } +// WaitToolPolicy configures per-agent safety bounds for the wait tool. +type WaitToolPolicy struct { + MinMs int `json:"min_ms,omitempty"` + MaxMs int `json:"max_ms,omitempty"` +} // SessionsConfig controls session behavior. // Matching TS src/config/sessions/types.ts + src/config/types.base.ts. diff --git a/internal/pipeline/deps.go b/internal/pipeline/deps.go index 1c5d095d..8a2b1697 100644 --- a/internal/pipeline/deps.go +++ b/internal/pipeline/deps.go @@ -1,4 +1,4 @@ -package pipeline +package pipeline import ( "context" @@ -89,6 +89,10 @@ type PipelineDeps struct { ExecuteToolRaw func(ctx context.Context, tc providers.ToolCall) (providers.Message, any, error) // ProcessToolResult processes a raw tool result with state mutation (sequential only). ProcessToolResult func(ctx context.Context, state *RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message + // SequentialToolCall returns true for tools that must preserve same-response order. + // When any tool call in a batch matches, ToolStage uses ExecuteToolCall for the + // whole batch instead of parallel raw execution. + SequentialToolCall func(tc providers.ToolCall) bool // CheckReadOnly checks read-only streak. Returns warning message (if any) and whether to break. CheckReadOnly func(state *RunState) (*providers.Message, bool) diff --git a/internal/pipeline/stages_test.go b/internal/pipeline/stages_test.go index 94efb391..8c1204ed 100644 --- a/internal/pipeline/stages_test.go +++ b/internal/pipeline/stages_test.go @@ -5,6 +5,7 @@ import ( "errors" "os" "path/filepath" + "reflect" "strings" "sync" "sync/atomic" @@ -966,6 +967,197 @@ func TestToolStage_MultipleTools_ParallelPath_InvokesRawAndProcessForEach(t *tes } } +func TestToolStage_MultipleTools_SequentialBarrierSkipsParallelRawPath(t *testing.T) { + t.Parallel() + calls := []string{} + deps := &PipelineDeps{ + ExecuteToolCall: func(_ context.Context, _ *RunState, tc providers.ToolCall) ([]providers.Message, error) { + calls = append(calls, tc.Name) + return []providers.Message{{Role: "tool", Content: "result:" + tc.Name, ToolCallID: tc.ID}}, nil + }, + ExecuteToolRaw: func(_ context.Context, _ providers.ToolCall) (providers.Message, any, error) { + t.Fatal("ExecuteToolRaw must not be called when a sequential barrier is present") + return providers.Message{}, nil, nil + }, + ProcessToolResult: func(_ context.Context, _ *RunState, _ providers.ToolCall, _ providers.Message, _ any) []providers.Message { + t.Fatal("ProcessToolResult must not be called when a sequential barrier is present") + return nil + }, + SequentialToolCall: func(tc providers.ToolCall) bool { + return tc.Name == "wait" + }, + } + stage := NewToolStage(deps) + state := defaultState() + state.Think.LastResponse = &providers.ChatResponse{ + ToolCalls: []providers.ToolCall{ + {ID: "1", Name: "message"}, + {ID: "2", Name: "wait"}, + {ID: "3", Name: "message"}, + }, + } + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + want := []string{"message", "wait", "message"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("ExecuteToolCall order = %v, want %v", calls, want) + } +} + +func TestToolStage_MultipleTools_PrefixedSequentialBarrierSkipsParallelRawPath(t *testing.T) { + t.Parallel() + calls := []string{} + deps := &PipelineDeps{ + ExecuteToolCall: func(_ context.Context, _ *RunState, tc providers.ToolCall) ([]providers.Message, error) { + calls = append(calls, tc.Name) + return []providers.Message{{Role: "tool", Content: "result:" + tc.Name, ToolCallID: tc.ID}}, nil + }, + ExecuteToolRaw: func(_ context.Context, _ providers.ToolCall) (providers.Message, any, error) { + t.Fatal("ExecuteToolRaw must not be called when a prefixed sequential barrier is present") + return providers.Message{}, nil, nil + }, + ProcessToolResult: func(_ context.Context, _ *RunState, _ providers.ToolCall, _ providers.Message, _ any) []providers.Message { + t.Fatal("ProcessToolResult must not be called when a prefixed sequential barrier is present") + return nil + }, + SequentialToolCall: func(tc providers.ToolCall) bool { + return strings.TrimPrefix(tc.Name, "proxy_") == "wait" + }, + } + stage := NewToolStage(deps) + state := defaultState() + state.Think.LastResponse = &providers.ChatResponse{ + ToolCalls: []providers.ToolCall{ + {ID: "1", Name: "proxy_message"}, + {ID: "2", Name: "proxy_wait"}, + {ID: "3", Name: "proxy_message"}, + }, + } + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + want := []string{"proxy_message", "proxy_wait", "proxy_message"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("ExecuteToolCall order = %v, want %v", calls, want) + } +} + +func TestToolStage_SequentialBatchStopsAfterContextCancellation(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + calls := []string{} + deps := &PipelineDeps{ + ExecuteToolCall: func(_ context.Context, _ *RunState, tc providers.ToolCall) ([]providers.Message, error) { + calls = append(calls, tc.Name) + if tc.Name == "wait" { + cancel() + } + return []providers.Message{{Role: "tool", Content: "result:" + tc.Name, ToolCallID: tc.ID}}, nil + }, + ExecuteToolRaw: func(_ context.Context, _ providers.ToolCall) (providers.Message, any, error) { + t.Fatal("ExecuteToolRaw must not be called when a sequential barrier is present") + return providers.Message{}, nil, nil + }, + ProcessToolResult: func(_ context.Context, _ *RunState, _ providers.ToolCall, _ providers.Message, _ any) []providers.Message { + t.Fatal("ProcessToolResult must not be called when a sequential barrier is present") + return nil + }, + SequentialToolCall: func(tc providers.ToolCall) bool { + return tc.Name == "wait" + }, + } + stage := NewToolStage(deps) + state := defaultState() + state.Think.LastResponse = &providers.ChatResponse{ + ToolCalls: []providers.ToolCall{ + {ID: "1", Name: "message"}, + {ID: "2", Name: "wait", Arguments: map[string]any{"timeMs": 1000}}, + {ID: "3", Name: "message"}, + }, + } + + if err := stage.Execute(ctx, state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + want := []string{"message", "wait"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("ExecuteToolCall order = %v, want %v", calls, want) + } + if stage.Result() != AbortRun { + t.Fatalf("Result() = %v, want AbortRun", stage.Result()) + } +} + +func TestToolStage_SequentialBatchEnforcesToolBudgetBeforeEachCall(t *testing.T) { + t.Parallel() + calls := []string{} + deps := &PipelineDeps{ + Config: PipelineConfig{MaxToolCalls: 2}, + ExecuteToolCall: func(_ context.Context, _ *RunState, tc providers.ToolCall) ([]providers.Message, error) { + calls = append(calls, tc.Name) + return []providers.Message{{Role: "tool", Content: "result:" + tc.Name, ToolCallID: tc.ID}}, nil + }, + SequentialToolCall: func(tc providers.ToolCall) bool { + return tc.Name == "wait" + }, + } + stage := NewToolStage(deps) + state := defaultState() + state.Think.LastResponse = &providers.ChatResponse{ + ToolCalls: []providers.ToolCall{ + {ID: "1", Name: "message"}, + {ID: "2", Name: "wait", Arguments: map[string]any{"timeMs": 1000}}, + {ID: "3", Name: "message"}, + }, + } + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + want := []string{"message", "wait"} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("ExecuteToolCall order = %v, want %v", calls, want) + } + if stage.Result() != BreakLoop { + t.Fatalf("Result() = %v, want BreakLoop", stage.Result()) + } +} + +func TestToolStage_SequentialWaitBatchEnforcesCumulativeWaitCap(t *testing.T) { + t.Parallel() + calls := []string{} + deps := &PipelineDeps{ + ExecuteToolCall: func(_ context.Context, _ *RunState, tc providers.ToolCall) ([]providers.Message, error) { + calls = append(calls, tc.ID) + return []providers.Message{{Role: "tool", Content: "result:" + tc.Name, ToolCallID: tc.ID}}, nil + }, + SequentialToolCall: func(tc providers.ToolCall) bool { + return tc.Name == "wait" + }, + } + stage := NewToolStage(deps) + state := defaultState() + state.Think.LastResponse = &providers.ChatResponse{ + ToolCalls: []providers.ToolCall{ + {ID: "1", Name: "wait", Arguments: map[string]any{"timeMs": 300000}}, + {ID: "2", Name: "wait", Arguments: map[string]any{"timeMs": 300000}}, + }, + } + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + if !reflect.DeepEqual(calls, []string{"1"}) { + t.Fatalf("ExecuteToolCall calls = %v, want [1]", calls) + } + if stage.Result() != AbortRun { + t.Fatalf("Result() = %v, want AbortRun", stage.Result()) + } +} + func TestToolStage_LoopKilled_ReturnsBreakLoop(t *testing.T) { t.Parallel() deps := &PipelineDeps{ diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index 8779822b..ac6c7a1e 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -2,7 +2,9 @@ package pipeline import ( "context" + "encoding/json" "fmt" + "strconv" "sync" "github.com/google/uuid" @@ -11,6 +13,8 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/store" ) +const maxSequentialWaitBatchMs = 300000 + // ToolStage runs per iteration after PruneStage. Executes tool calls from // ThinkState.LastResponse, checks exit conditions (loop kill, read-only streak, budget). type ToolStage struct { @@ -42,12 +46,24 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { // Parallel path: separate I/O (parallel) from state mutation (sequential). // Requires both ExecuteToolRaw and ProcessToolResult callbacks. - if len(toolCalls) > 1 && s.deps.ExecuteToolRaw != nil && s.deps.ProcessToolResult != nil { + if len(toolCalls) > 1 && s.deps.ExecuteToolRaw != nil && s.deps.ProcessToolResult != nil && !s.requiresSequential(toolCalls) { return s.executeParallel(ctx, state, toolCalls) } // Sequential fallback: ExecuteToolCall handles both I/O and state mutation. + cumulativeWaitMs := 0 for _, tc := range toolCalls { + if s.shouldStopBeforeTool(ctx, state) { + return nil + } + if s.deps.SequentialToolCall != nil && s.deps.SequentialToolCall(tc) { + cumulativeWaitMs += toolCallTimeMs(tc) + if cumulativeWaitMs > maxSequentialWaitBatchMs { + s.result = AbortRun + return nil + } + } + // Hook: sync PreToolUse — block if hook denies. Builtin-source hooks may // rewrite tc.Arguments via UpdatedToolInput (e.g. path-sanitizer); apply // before ExecuteToolCall so the rewrite is authoritative. @@ -99,12 +115,61 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { s.result = BreakLoop return nil } + if ctx.Err() != nil { + s.result = AbortRun + return nil + } } s.checkExitConditions(state) return nil } +func (s *ToolStage) requiresSequential(toolCalls []providers.ToolCall) bool { + if s.deps.SequentialToolCall == nil { + return false + } + for _, tc := range toolCalls { + if s.deps.SequentialToolCall(tc) { + return true + } + } + return false +} + +func (s *ToolStage) shouldStopBeforeTool(ctx context.Context, state *RunState) bool { + if ctx.Err() != nil { + s.result = AbortRun + return true + } + if s.deps.Config.MaxToolCalls > 0 && state.Tool.TotalToolCalls >= s.deps.Config.MaxToolCalls { + s.result = BreakLoop + return true + } + return false +} + +func toolCallTimeMs(tc providers.ToolCall) int { + v, ok := tc.Arguments["timeMs"] + if !ok { + return 0 + } + switch n := v.(type) { + case int: + return n + case int64: + return int(n) + case float64: + return int(n) + case json.Number: + i, err := strconv.Atoi(n.String()) + if err == nil { + return i + } + } + return 0 +} + // executeParallel runs tool I/O concurrently, then processes results sequentially. func (s *ToolStage) executeParallel(ctx context.Context, state *RunState, toolCalls []providers.ToolCall) error { type rawResult struct { diff --git a/internal/store/agent_store_test.go b/internal/store/agent_store_test.go index 4a143e1e..022ccb87 100644 --- a/internal/store/agent_store_test.go +++ b/internal/store/agent_store_test.go @@ -439,3 +439,24 @@ func TestParseAllowImageGeneration_UnrelatedKeys_DefaultsTrue(t *testing.T) { t.Error("other_config without allow_image_generation key must default to true") } } + +func TestParseToolsConfigWaitPolicy(t *testing.T) { + t.Parallel() + agent := AgentData{ + ToolsConfig: json.RawMessage(`{"profile":"coding","wait":{"min_ms":500,"max_ms":60000},"toolCallPrefix":"proxy_"}`), + } + + got := agent.ParseToolsConfig() + if got == nil { + t.Fatal("ParseToolsConfig() = nil") + } + if got.Wait == nil { + t.Fatal("Wait policy was not parsed") + } + if got.Wait.MinMs != 500 || got.Wait.MaxMs != 60000 { + t.Fatalf("Wait = %#v, want min=500 max=60000", got.Wait) + } + if got.ToolCallPrefix != "proxy_" { + t.Fatalf("ToolCallPrefix = %q", got.ToolCallPrefix) + } +} diff --git a/internal/store/run_context.go b/internal/store/run_context.go index 98674803..2e05a356 100644 --- a/internal/store/run_context.go +++ b/internal/store/run_context.go @@ -45,6 +45,7 @@ type RunContext struct { ParentProvider string MemoryCfg *config.MemoryConfig SandboxCfg *sandbox.Config + WaitToolCfg *config.WaitToolPolicy ShellDenyGroups map[string]bool // Workspace diff --git a/internal/tools/capability.go b/internal/tools/capability.go index 1b1f4284..5ff9908d 100644 --- a/internal/tools/capability.go +++ b/internal/tools/capability.go @@ -46,7 +46,7 @@ func inferMetadata(name string) ToolMetadata { name == "memory_search" || name == "memory_get" || name == "memory_expand" || name == "skill_search" || name == "knowledge_graph_search" || name == "sessions_list" || name == "session_status" || name == "sessions_history" || - name == "datetime" || name == "web_search" || name == "web_fetch": + name == "datetime" || name == "wait" || name == "web_search" || name == "web_fetch": meta.Capabilities = []ToolCapability{CapReadOnly} case name == "spawn": meta.Capabilities = []ToolCapability{CapAsync} diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index f30a81a4..923ac901 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -358,6 +358,24 @@ func MemoryConfigFromCtx(ctx context.Context) *config.MemoryConfig { return nil } +// --- Per-agent wait tool config override --- + +const ctxWaitToolCfg toolContextKey = "tool_wait_config" + +func WithWaitToolConfig(ctx context.Context, cfg *config.WaitToolPolicy) context.Context { + return context.WithValue(ctx, ctxWaitToolCfg, cfg) +} + +func WaitToolConfigFromCtx(ctx context.Context) *config.WaitToolPolicy { + if v, _ := ctx.Value(ctxWaitToolCfg).(*config.WaitToolPolicy); v != nil { + return v + } + if rc := store.RunContextFromCtx(ctx); rc != nil { + return rc.WaitToolCfg + } + return nil +} + // --- Team ID propagation (task dispatch → workspace tools) --- const ctxTeamID toolContextKey = "tool_team_id" diff --git a/internal/tools/policy.go b/internal/tools/policy.go index 20725a99..7d83dc3b 100644 --- a/internal/tools/policy.go +++ b/internal/tools/policy.go @@ -16,7 +16,7 @@ var builtinToolGroups = map[string][]string{ "memory": {"memory_search", "memory_get"}, "web": {"web_search", "web_fetch"}, "fs": {"read_file", "write_file", "list_files", "edit"}, - "runtime": {"exec"}, + "runtime": {"exec", "wait"}, "sessions": {"sessions_list", "sessions_history", "sessions_send", "spawn", "session_status"}, "ui": {"browser"}, "automation": {"cron"}, @@ -25,7 +25,7 @@ var builtinToolGroups = map[string][]string{ "vault": {"vault_search", "vault_read"}, // Composite group: all goclaw native tools (excludes MCP/custom plugins). "goclaw": { - "read_file", "write_file", "list_files", "edit", "exec", + "read_file", "write_file", "list_files", "edit", "exec", "wait", "web_search", "web_fetch", "browser", "memory_search", "memory_get", "memory_expand", "knowledge_graph_search", "vault_search", "vault_read", @@ -48,7 +48,7 @@ var builtinToolGroups = map[string][]string{ var toolProfiles = map[string][]string{ "minimal": {"session_status"}, "coding": {"group:fs", "group:runtime", "group:sessions", "group:memory", "group:web", "group:vault", "read_image", "create_image", "skill_search"}, - "messaging": {"group:messaging", "group:web", "group:vault", "sessions_list", "sessions_history", "sessions_send", "session_status", "read_image", "skill_search"}, + "messaging": {"group:messaging", "wait", "group:web", "group:vault", "sessions_list", "sessions_history", "sessions_send", "session_status", "read_image", "skill_search"}, "full": {}, // empty = no restrictions } diff --git a/internal/tools/policy_race_test.go b/internal/tools/policy_race_test.go index c07978d2..abd6bd62 100644 --- a/internal/tools/policy_race_test.go +++ b/internal/tools/policy_race_test.go @@ -160,6 +160,14 @@ func TestToolGroups_BuiltinGroups_Seeded(t *testing.T) { if !containsTool(web, "web_search") || !containsTool(web, "web_fetch") { t.Errorf("web group should contain web_search and web_fetch, got: %v", web) } + + runtime, ok := reg.GetToolGroup("runtime") + if !ok { + t.Fatal("expected 'runtime' builtin group to exist") + } + if !containsTool(runtime, "wait") { + t.Errorf("runtime group should contain wait, got: %v", runtime) + } } func containsTool(tools []string, name string) bool { diff --git a/internal/tools/wait.go b/internal/tools/wait.go new file mode 100644 index 00000000..b965205f --- /dev/null +++ b/internal/tools/wait.go @@ -0,0 +1,128 @@ +package tools + +import ( + "context" + "encoding/json" + "fmt" + "math" + "strconv" + "strings" + "time" +) + +const ( + defaultWaitMinMs = 100 + defaultWaitMaxMs = 300000 +) + +// WaitTool pauses the current agent tool sequence for a bounded duration. +type WaitTool struct{} + +func NewWaitTool() *WaitTool { return &WaitTool{} } + +func (t *WaitTool) Name() string { return "wait" } + +func (t *WaitTool) Description() string { + return "Pause execution before the next tool call. Use for rate-limit spacing or waiting for async work to complete." +} + +func (t *WaitTool) Parameters() map[string]any { + return map[string]any{ + "type": "object", + "required": []string{"timeMs"}, + "properties": map[string]any{ + "timeMs": map[string]any{ + "type": "integer", + "description": "Duration to wait in milliseconds.", + "minimum": defaultWaitMinMs, + "maximum": defaultWaitMaxMs, + }, + "reason": map[string]any{ + "type": "string", + "description": "Optional reason for logging and debugging.", + }, + }, + } +} + +func (t *WaitTool) Execute(ctx context.Context, args map[string]any) *Result { + timeMs, err := parseWaitMillis(args["timeMs"]) + if err != nil { + return ErrorResult(err.Error()) + } + + minMs, maxMs := waitLimits(ctx) + if timeMs < minMs { + return ErrorResult(fmt.Sprintf("timeMs must be at least %dms", minMs)) + } + if timeMs > maxMs { + return ErrorResult(fmt.Sprintf("timeMs must be at most %dms", maxMs)) + } + + timer := time.NewTimer(time.Duration(timeMs) * time.Millisecond) + defer timer.Stop() + + select { + case <-timer.C: + reason, _ := args["reason"].(string) + reason = strings.TrimSpace(reason) + if reason != "" { + return SilentResult(fmt.Sprintf("Waited %dms. Reason: %s", timeMs, reason)) + } + return SilentResult(fmt.Sprintf("Waited %dms.", timeMs)) + case <-ctx.Done(): + return ErrorResult("wait cancelled: " + ctx.Err().Error()) + } +} + +func parseWaitMillis(value any) (int, error) { + if value == nil { + return 0, fmt.Errorf("timeMs is required") + } + switch v := value.(type) { + case int: + return v, nil + case int64: + return int(v), nil + case float64: + if math.IsNaN(v) || math.IsInf(v, 0) || math.Trunc(v) != v { + return 0, fmt.Errorf("timeMs must be an integer number of milliseconds") + } + return int(v), nil + case json.Number: + i, err := strconv.Atoi(v.String()) + if err != nil { + return 0, fmt.Errorf("timeMs must be an integer number of milliseconds") + } + return i, nil + default: + return 0, fmt.Errorf("timeMs must be an integer number of milliseconds") + } +} + +func waitLimits(ctx context.Context) (int, int) { + minMs := defaultWaitMinMs + maxMs := defaultWaitMaxMs + if cfg := WaitToolConfigFromCtx(ctx); cfg != nil { + if cfg.MinMs > 0 { + minMs = clampWaitLimit(cfg.MinMs) + } + if cfg.MaxMs > 0 { + maxMs = clampWaitLimit(cfg.MaxMs) + } + } + if maxMs < minMs { + maxMs = minMs + } + return minMs, maxMs +} + +func clampWaitLimit(v int) int { + if v < defaultWaitMinMs { + return defaultWaitMinMs + } + if v > defaultWaitMaxMs { + return defaultWaitMaxMs + } + return v +} diff --git a/internal/tools/wait_test.go b/internal/tools/wait_test.go new file mode 100644 index 00000000..367d3ea6 --- /dev/null +++ b/internal/tools/wait_test.go @@ -0,0 +1,95 @@ +package tools + +import ( + "context" + "encoding/json" + "strings" + "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +func TestWaitToolValidation(t *testing.T) { + t.Parallel() + tool := NewWaitTool() + + tests := []struct { + name string + args map[string]any + want string + }{ + {name: "missing", args: map[string]any{}, want: "timeMs is required"}, + {name: "below minimum", args: map[string]any{"timeMs": 99}, want: "at least 100ms"}, + {name: "above maximum", args: map[string]any{"timeMs": 300001}, want: "at most 300000ms"}, + {name: "fractional", args: map[string]any{"timeMs": 100.5}, want: "integer"}, + {name: "string", args: map[string]any{"timeMs": "100"}, want: "integer"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := tool.Execute(context.Background(), tt.args) + if got == nil || !got.IsError || !strings.Contains(got.ForLLM, tt.want) { + t.Fatalf("Execute() = %#v, want error containing %q", got, tt.want) + } + }) + } +} + +func TestWaitToolSuccess(t *testing.T) { + t.Parallel() + tool := NewWaitTool() + + start := time.Now() + got := tool.Execute(context.Background(), map[string]any{ + "timeMs": json.Number("100"), + "reason": "rate limit spacing", + }) + if got == nil || got.IsError { + t.Fatalf("Execute() error = %#v", got) + } + if elapsed := time.Since(start); elapsed < 90*time.Millisecond { + t.Fatalf("wait returned too early after %s", elapsed) + } + if !strings.Contains(got.ForLLM, "Waited 100ms") || !strings.Contains(got.ForLLM, "rate limit spacing") { + t.Fatalf("ForLLM = %q", got.ForLLM) + } +} + +func TestWaitToolContextCancellation(t *testing.T) { + t.Parallel() + tool := NewWaitTool() + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + start := time.Now() + got := tool.Execute(ctx, map[string]any{"timeMs": 300000}) + if got == nil || !got.IsError || !strings.Contains(got.ForLLM, "wait cancelled") { + t.Fatalf("Execute() = %#v, want cancellation error", got) + } + if elapsed := time.Since(start); elapsed > 100*time.Millisecond { + t.Fatalf("cancelled wait took %s", elapsed) + } +} + +func TestWaitToolPerAgentBounds(t *testing.T) { + t.Parallel() + tool := NewWaitTool() + ctx := WithWaitToolConfig(context.Background(), &config.WaitToolPolicy{MinMs: 250, MaxMs: 500}) + + if got := tool.Execute(ctx, map[string]any{"timeMs": 200}); got == nil || !got.IsError || !strings.Contains(got.ForLLM, "at least 250ms") { + t.Fatalf("below custom min = %#v", got) + } + if got := tool.Execute(ctx, map[string]any{"timeMs": 600}); got == nil || !got.IsError || !strings.Contains(got.ForLLM, "at most 500ms") { + t.Fatalf("above custom max = %#v", got) + } +} + +func TestInferMetadataWaitReadOnly(t *testing.T) { + t.Parallel() + meta := inferMetadata("wait") + if !meta.HasCapability(CapReadOnly) || meta.HasCapability(CapMutating) { + t.Fatalf("wait metadata = %#v, want read-only and not mutating", meta) + } +} diff --git a/plans/260518-0000-built-in-wait-tool/phase-01-research-and-tdd-design.md b/plans/260518-0000-built-in-wait-tool/phase-01-research-and-tdd-design.md new file mode 100644 index 00000000..264bdbfa --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/phase-01-research-and-tdd-design.md @@ -0,0 +1,42 @@ +--- +phase: 1 +title: "Research and TDD Design" +status: complete +effort: "1h" +--- + +# Phase 1: Research and TDD Design + +## Overview + +Verify current tool registration and policy paths before coding. Write tests first for the new tool contract, policy visibility, and per-agent configuration. + +## Context Links + +- Issue: nextlevelbuilder/goclaw#1097 +- Tool interface: `internal/tools/types.go` +- Tool registry execution and cancellation path: `internal/tools/registry.go` +- Built-in groups/profiles: `internal/tools/policy.go` +- Per-agent `tools_config` parsing: `internal/store/agent_store.go` +- Agent context injection: `internal/agent/loop_context.go` + +## Key Insights + +- `browser` already supports `act.kind=wait`, but only through `pkg/browser/tool.go`. +- General tools are `internal/tools.Tool` implementations and are registered into `tools.Registry`. +- Built-in DB visibility is separate from runtime registration; `cmd/gateway_builtin_tools.go` must seed `wait`. +- Per-agent knobs can fit existing `agents.tools_config` JSON without a migration by extending `config.ToolPolicySpec`. +- `ToolStage` currently runs multi-tool model responses through the parallel raw-tool path. A same-turn `message, wait, message` sequence must force sequential execution or both messages can run before the sleep completes. + +## Implementation Steps + +1. Add tests for `wait` validation: missing `timeMs`, below 100ms, above 300000ms, fractional numbers, success message, and context cancellation. +2. Add policy tests showing `wait` belongs to `group:runtime`, `group:goclaw`, and coding/full visibility. +3. Add config parsing test for `tools_config.wait.min_ms/max_ms`. +4. Re-run grep for `runtime` group and `builtinToolSeedData` before implementation to avoid missing catalog surfaces. + +## Success Criteria + +- [x] Tests fail before implementation for the missing `wait` tool. +- [x] Plan cites only live files and existing extension points. +- [x] No DB migration is required for per-agent settings. diff --git a/plans/260518-0000-built-in-wait-tool/phase-02-implement-wait-tool.md b/plans/260518-0000-built-in-wait-tool/phase-02-implement-wait-tool.md new file mode 100644 index 00000000..39bac18e --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/phase-02-implement-wait-tool.md @@ -0,0 +1,88 @@ +--- +phase: 2 +title: "Implement Wait Tool" +status: complete +effort: "2h" +--- + +# Phase 2: Implement Wait Tool + +## Overview + +Implement the smallest production-safe `wait` tool and wire it through every runtime visibility layer. + +## Requirements + +- Functional: `wait({timeMs, reason?})` delays the current agent action sequence and then returns a concise success result. +- Bounds: default minimum 100ms, default maximum 300000ms. +- Per-agent override: `agents.tools_config` may include `{"wait":{"min_ms":500,"max_ms":60000}}`; invalid overrides are ignored or clamped to absolute safety bounds. +- Cancellation: if the run context is cancelled while waiting, return an error quickly. +- Concurrency: no package-level locks or shared timers. +- Ordering: if any resolved tool call in the model response is `wait`, execute that tool-call batch sequentially so `message -> wait -> message` preserves order. +- Cancellation: after a cancelled wait, the sequential batch aborts before later side-effecting calls. +- Abuse guard: a same-response wait batch is capped to 300000ms cumulative wait time. + +## Related Code Files + +- Modify: `internal/tools/wait.go` +- Modify: `internal/tools/policy.go` +- Modify: `internal/tools/capability.go` +- Modify: `internal/tools/context_keys.go` +- Modify: `internal/pipeline/deps.go` +- Modify: `internal/pipeline/tool_stage.go` +- Modify: `internal/agent/loop_pipeline_adapter.go` +- Modify: `internal/config/config_channels.go` +- Modify: `internal/agent/loop_context.go` +- Modify: `internal/store/run_context.go` +- Modify: `cmd/gateway_tools_wiring.go` +- Modify: `cmd/gateway_builtin_tools.go` +- Modify: `ui/web/src/types/agent.ts` +- Modify: `ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx` +- Modify: `ui/web/src/pages/agents/agent-detail/config-sections/tool-policy-section.tsx` +- Modify: `ui/web/src/i18n/locales/{en,vi,zh}/agents.json` +- Tests: `internal/tools/wait_test.go`, focused existing tests as needed + +## Architecture + +Agent loop injects per-agent wait limits into context. The registry calls `WaitTool.Execute(ctx,args)`. `Execute` validates `timeMs`, applies limits, waits on `time.NewTimer`, and selects on `ctx.Done()` for interruption. + +`ToolStage` treats resolved `wait` tool calls as a sequential barrier. This disables the multi-tool parallel raw path for that assistant response, preserving order for same-turn tool batches. + +## Implementation Steps + +1. Define `config.WaitToolPolicy` and add `Wait *WaitToolPolicy` to `ToolPolicySpec`. +2. Add `tools.WithWaitToolConfig` / `WaitToolConfigFromCtx`, with RunContext fallback. +3. Inject `l.agentToolPolicy.Wait` in `Loop.injectContext`. +4. Add a pipeline dependency hook that can mark a resolved tool call as sequential-only, and wire it from the agent loop using `resolveToolCallName`. +5. Add `WaitTool` in `internal/tools/wait.go`. +6. Register `tools.NewWaitTool()` next to `datetime` in `wireExtraTools`. +7. Seed `wait` in `builtinToolSeedData` as runtime enabled by default. +8. Add `wait` to `runtime`, `goclaw`, coding profile if needed, and neutral metadata as appropriate. +9. Mark `wait` neutral in agent tool-loop detection so intentional delay sequences do not count as read-only no-progress loops. +10. Update Web agent settings types/save path and add compact wait min/max controls under tool policy so UI edits do not drop `tools_config.wait`. + +## Tests Before + +- `go test ./internal/tools -run "TestWaitTool|TestToolGroups|TestInferMetadata"` +- `go test ./internal/store -run TestParseToolsConfig` +- `go test ./internal/pipeline -run TestToolStage` +- `pnpm -C ui/web build` if frontend settings are changed + +## Tests After + +- Same focused tests plus `go test ./cmd -run BuiltinTool` if existing cmd tests cover seed data. + +## Success Criteria + +- [x] `wait` appears in provider definitions when policy allows runtime tools. +- [x] `wait` is absent when globally disabled by builtin tool settings. +- [x] A same-response `message, wait, message` batch uses sequential tool execution. +- [x] Cancellation returns before the requested delay. +- [x] No sleeping test exceeds a few hundred milliseconds. + +## Risk Assessment + +- Long sleeps can tie up one agent run goroutine; bounded max and cancellation prevent indefinite hangs. +- Rate limiting might count wait as a tool execution; accepted for v1 because it prevents abusing wait as a rate-limit bypass. +- Progress notifications for >1 minute are deferred because no existing low-risk tool callback emits user progress. +- UI save path currently reconstructs `tools_config`; missing `wait` in that object would silently erase agent-specific wait limits. diff --git a/plans/260518-0000-built-in-wait-tool/phase-03-validate-and-ship.md b/plans/260518-0000-built-in-wait-tool/phase-03-validate-and-ship.md new file mode 100644 index 00000000..265a7b10 --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/phase-03-validate-and-ship.md @@ -0,0 +1,38 @@ +--- +phase: 3 +title: "Validate and Ship" +status: complete +effort: "1h" +--- + +# Phase 3: Validate and Ship + +## Overview + +Validate the implementation with focused Go tests, compile checks, adversarial review, scoped commit/push, and beta PR against `dev`. + +## Implementation Steps + +1. Run focused tests: + - `go test ./internal/tools -run "TestWaitTool|TestToolGroups|TestInferMetadata"` + - `go test ./internal/store -run TestParseToolsConfig` + - `go test ./cmd -run BuiltinTool` +2. Run compile checks: + - `go build ./...` + - `go build -tags sqliteonly ./...` +3. Run code review on changed files and address correctness findings. +4. Update docs/changelog only if implementation changes user-facing admin docs. +5. Stage only plan + implementation files; run `git diff --cached --check` and staged secret scan. +6. Commit with conventional message, push `codex/feat-wait-tool`, and create PR to `digitopvn/goclaw:dev`. + +## Success Criteria + +- [x] Focused tests pass. +- [x] Both PG and SQLite builds pass, or unrelated baseline failures are documented with evidence. +- [x] Code review has no unresolved critical/high correctness findings. +- [x] PR links issue #1097 and targets `dev`. + +## Unresolved Questions + +- Should long waits emit user-facing progress later? Deferred for v1 unless reviewer finds existing status callback. +- Should `wait_until` be a separate future tool? Deferred. diff --git a/plans/260518-0000-built-in-wait-tool/plan.md b/plans/260518-0000-built-in-wait-tool/plan.md new file mode 100644 index 00000000..7f92d89b --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/plan.md @@ -0,0 +1,37 @@ +--- +title: "Built-in Wait Tool with Delay Parameter" +description: "Add a general-purpose wait tool with bounded millisecond delays and per-agent limit overrides." +status: complete +priority: P1 +issue: 1097 +branch: "codex/feat-wait-tool" +tags: [tools, runtime, tdd, issue-1097] +blockedBy: [] +blocks: [] +created: "2026-05-18T14:29:45.859Z" +createdBy: "ck:plan" +source: skill +--- + +# Built-in Wait Tool with Delay Parameter + +## Overview + +Add a built-in `wait` tool so agents can pause between actions without using browser-only wait, polling loops, or cron handoffs. + +Scope is intentionally narrow: bounded sleep inside tool execution, context cancellation support, gateway registration, builtin-tool seed visibility, and focused tests. `wait_until` and progress notifications stay out of v1 unless code review finds an existing event surface that makes them trivial. + +## Phases + +| Phase | Name | Status | +|-------|------|--------| +| 1 | [Research and TDD Design](./phase-01-research-and-tdd-design.md) | Complete | +| 2 | [Implement Wait Tool](./phase-02-implement-wait-tool.md) | Complete | +| 3 | [Validate and Ship](./phase-03-validate-and-ship.md) | Complete | + +## Dependencies + +- Related issue: nextlevelbuilder/goclaw#1097 +- Existing tool contract: `internal/tools/types.go`, `internal/tools/registry.go` +- Existing registration surfaces: `cmd/gateway_setup.go`, `cmd/gateway_tools_wiring.go`, `cmd/gateway_builtin_tools.go` +- Ordering barrier: `internal/pipeline/tool_stage.go` parallelizes multi-tool responses unless a tool opts out diff --git a/plans/260518-0000-built-in-wait-tool/reports/red-team-report.md b/plans/260518-0000-built-in-wait-tool/reports/red-team-report.md new file mode 100644 index 00000000..92eda3ca --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/reports/red-team-report.md @@ -0,0 +1,30 @@ +# Red Team Report: Built-in Wait Tool + +## Findings + +### Critical: same-turn tool batches can bypass wait ordering + +Evidence: `internal/pipeline/tool_stage.go` sends any response with more than one tool call through `executeParallel` when raw/process callbacks are present. The raw path starts all tool I/O goroutines before sequential result processing. A model response containing `message`, `wait`, `message` can therefore send both messages before the wait completes. + +Disposition: Accept. Add a sequential-only dependency hook in the pipeline and wire it from the agent loop using the resolved registry name. Any batch containing resolved `wait` must use the existing sequential `ExecuteToolCall` path. + +### High: UI save can erase per-agent wait config + +Evidence: `ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx` rebuilds `tools_config` with only `profile`, `allow`, `deny`, `alsoAllow`, and `byProvider`. `ui/web/src/types/agent.ts` has no wait config field. If backend accepts `tools_config.wait`, the next agent settings save drops it. + +Disposition: Accept. Add UI type, save mapping, and compact controls. + +### Medium: repeated wait calls can look like read-only no-progress + +Evidence: `internal/agent/toolloop.go` treats only `exec`, `bash`, and `mcp_*` as neutral. All non-mutating, non-neutral tools increment read-only streak. A message/wait/message pattern is fine, but wait-only polling or long staged waits can trigger irrelevant warnings. + +Disposition: Accept. Classify `wait` as neutral. + +## Rejected / Deferred + +- `wait_until` deferred. Issue asks to consider it, not include v1. +- Long-wait progress notification deferred. Existing tool event path is generic; adding user-facing progress now increases scope. + +## Unresolved Questions + +- None. diff --git a/plans/260518-0000-built-in-wait-tool/reports/reviewer-20260518-built-in-wait-tool.md b/plans/260518-0000-built-in-wait-tool/reports/reviewer-20260518-built-in-wait-tool.md new file mode 100644 index 00000000..2eff7a47 --- /dev/null +++ b/plans/260518-0000-built-in-wait-tool/reports/reviewer-20260518-built-in-wait-tool.md @@ -0,0 +1,70 @@ +# Built-in Wait Tool Review + +## Scope + +- Files: `internal/tools/wait.go`, `internal/pipeline/tool_stage.go`, agent context/policy wiring, Web agent settings, locale files, plan docs. +- LOC: tracked diff 193 additions / 20 deletions across 20 files, plus new `wait.go` 114 lines and `wait_test.go` 83 lines. +- Focus: correctness, security, concurrency, cancellation, policy/UI regression. +- Scout findings: order barrier wired through resolved tool name; aggregate wait budget and cancellation-in-batch are the risky paths. + +## Overall Assessment + +Implementation is mostly coherent. Same-response `message -> wait -> message` ordering is fixed for the normal path by forcing the whole batch through sequential `ExecuteToolCall` when any resolved call is `wait`. Per-agent bounds are parsed, injected, clamped, and UI save now preserves `wait`. + +Blocking issue: cancellation during `wait` does not stop later same-batch tool side effects. + +## Critical Issues + +- [internal/tools/wait.go:73] `wait` returns `ErrorResult` on `ctx.Done()`, but [internal/pipeline/tool_stage.go:50] continues the sequential batch and can execute later calls such as `message` before the pipeline checks `ctx.Err()` at [internal/pipeline/pipeline.go:96]. This breaks cancellation safety and can send messages after user abort. + Fix: in `ToolStage`, check `ctx.Err()` before and after each sequential tool call, set `s.result = AbortRun`, and return before executing subsequent calls. Add regression test: `message, wait(cancelled), message` must not run the second message. + +## High Priority + +- [internal/pipeline/tool_stage.go:50] A single assistant response can contain many `wait` calls; budget is only checked after the whole batch at [internal/pipeline/tool_stage.go:104] and [internal/pipeline/tool_stage.go:194]. With default `max_tool_calls=25` and max wait 300000ms, one response can occupy an agent lane for up to 125 minutes unless manually cancelled. + Fix: enforce remaining tool-call budget before each call, or pre-truncate/reject batch calls that exceed remaining budget. For sequential wait batches, consider a per-response cumulative wait cap. + +## Medium Priority + +- [internal/agent/loop_pipeline_adapter.go:144] Prefixed wait calls are handled through `resolveToolCallName`, so behavior appears correct, but [internal/pipeline/stages_test.go:970] only tests raw `wait`. + Fix: add a focused adapter/stage regression test with `ToolCallPrefix: "proxy_"` and calls `proxy_message, proxy_wait, proxy_message`. + +## Low Priority + +- [plans/260518-0000-built-in-wait-tool/phase-01-research-and-tdd-design.md] Phase 1 is marked complete, but its success checklist is still unchecked. Phase 2 remains in progress and Phase 3 pending. Plan status should be synced after fixes. + +## Edge Cases Found by Scout + +- Cancellation inside a wait batch can allow later side effects. +- Per-call max wait is bounded, but cumulative same-response waits are not. +- Prefix resolution is implemented, but missing direct regression coverage. +- UI now preserves `wait` and `toolCallPrefix` when saving enabled tool policy. + +## Positive Observations + +- No shared timer or package-level mutable state in `WaitTool`. +- Wait bounds clamp to absolute safety limits. +- Runtime/coding/messaging/full visibility paths are covered through policy groups/profiles. +- Focused Go tests and Web build pass. + +## Recommended Actions + +1. Block landing until cancellation stops the rest of a sequential batch. +2. Enforce max tool-call budget before every tool execution, not after a batch. +3. Add prefixed wait-ordering regression test. +4. Sync plan checkboxes/status after fixes. + +## Metrics + +- Type Coverage: not measured. +- Test Coverage: focused tests pass; coverage percentage not measured. +- Linting Issues: `git diff --check` clean for reviewed files; CRLF warnings only for `internal/config/config_channels.go` and `internal/pipeline/deps.go`. + +## Verification + +- `go test ./internal/tools ./internal/pipeline ./internal/agent ./internal/store ./cmd` passed. +- `pnpm build` in `ui/web` passed with existing Vite chunk-size warnings. +- `git diff --check` passed for reviewed files. + +## Unresolved Questions + +- Should wait have a stricter cumulative per-response cap than generic tool-call budget? diff --git a/ui/web/src/i18n/locales/en/agents.json b/ui/web/src/i18n/locales/en/agents.json index e04f3eb9..a0d28566 100644 --- a/ui/web/src/i18n/locales/en/agents.json +++ b/ui/web/src/i18n/locales/en/agents.json @@ -762,7 +762,11 @@ "selectToolsDeny": "Select tools to deny...", "selectToolsAlsoAllow": "Select additional tools...", "toolCallPrefix": "Tool Call Prefix", - "toolCallPrefixHint": "Strips this prefix from model's tool call names before registry lookup." + "toolCallPrefixHint": "Strips this prefix from model's tool call names before registry lookup.", + "waitLimits": "Wait Tool Bounds", + "waitMinPlaceholder": "Min ms", + "waitMaxPlaceholder": "Max ms", + "waitLimitsHint": "Optional per-agent bounds. Server safety limits still clamp waits to 100-300000ms." }, "workspaceSharing": { "title": "Workspace Sharing", diff --git a/ui/web/src/i18n/locales/vi/agents.json b/ui/web/src/i18n/locales/vi/agents.json index eb84c04d..8bb5c2a5 100644 --- a/ui/web/src/i18n/locales/vi/agents.json +++ b/ui/web/src/i18n/locales/vi/agents.json @@ -747,7 +747,11 @@ "selectToolsDeny": "Chọn công cụ để từ chối...", "selectToolsAlsoAllow": "Chọn công cụ bổ sung...", "toolCallPrefix": "Tiền tố Tool Call", - "toolCallPrefixHint": "Loại bỏ tiền tố này từ tên tool call của model trước khi tra cứu registry." + "toolCallPrefixHint": "Loại bỏ tiền tố này từ tên tool call của model trước khi tra cứu registry.", + "waitLimits": "Giới hạn công cụ wait", + "waitMinPlaceholder": "Min ms", + "waitMaxPlaceholder": "Max ms", + "waitLimitsHint": "Giới hạn riêng cho agent. Server vẫn kẹp wait trong khoảng 100-300000ms." }, "workspaceSharing": { "title": "Chia sẻ Workspace", diff --git a/ui/web/src/i18n/locales/zh/agents.json b/ui/web/src/i18n/locales/zh/agents.json index fab6c365..fef22a44 100644 --- a/ui/web/src/i18n/locales/zh/agents.json +++ b/ui/web/src/i18n/locales/zh/agents.json @@ -747,7 +747,11 @@ "selectToolsDeny": "选择要拒绝的工具...", "selectToolsAlsoAllow": "选择附加工具...", "toolCallPrefix": "工具调用前缀", - "toolCallPrefixHint": "从模型的工具调用名称中去除此前缀后再查找注册表。" + "toolCallPrefixHint": "从模型的工具调用名称中去除此前缀后再查找注册表。", + "waitLimits": "Wait 工具边界", + "waitMinPlaceholder": "最小毫秒", + "waitMaxPlaceholder": "最大毫秒", + "waitLimitsHint": "可选的 Agent 专属边界。服务端仍会将等待限制在 100-300000 毫秒内。" }, "workspaceSharing": { "title": "工作区共享", diff --git a/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx b/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx index 5e68ebcd..b96d6b2d 100644 --- a/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx +++ b/ui/web/src/pages/agents/agent-detail/agent-overview-tab.tsx @@ -82,7 +82,15 @@ export function AgentOverviewTab({ agent, onUpdate, heartbeat, onManageCodexPool memory_config: mem, subagents_config: subEnabled ? sub : null, tools_config: toolsEnabled - ? { profile: tools.profile, allow: tools.allow, deny: tools.deny, alsoAllow: tools.alsoAllow, byProvider: tools.byProvider } + ? { + profile: tools.profile, + allow: tools.allow, + deny: tools.deny, + alsoAllow: tools.alsoAllow, + byProvider: tools.byProvider, + wait: tools.wait, + toolCallPrefix: tools.toolCallPrefix, + } : {}, // Promoted fields sent at top level (NOT NULL columns — send "" not null) emoji: emoji.trim(), diff --git a/ui/web/src/pages/agents/agent-detail/config-sections/tool-policy-section.tsx b/ui/web/src/pages/agents/agent-detail/config-sections/tool-policy-section.tsx index 4c825cff..b208862a 100644 --- a/ui/web/src/pages/agents/agent-detail/config-sections/tool-policy-section.tsx +++ b/ui/web/src/pages/agents/agent-detail/config-sections/tool-policy-section.tsx @@ -21,6 +21,23 @@ interface ToolPolicySectionProps { export function ToolPolicySection({ enabled, value, onToggle, onChange }: ToolPolicySectionProps) { const { t } = useTranslation("agents"); const s = "configSections.toolPolicy"; + + const updateWaitLimit = (field: "min_ms" | "max_ms", raw: string) => { + const nextWait = { ...(value.wait ?? {}) }; + if (raw === "") { + delete nextWait[field]; + } else { + const parsed = Number(raw); + if (Number.isFinite(parsed) && parsed > 0) { + nextWait[field] = Math.trunc(parsed); + } + } + onChange({ + ...value, + wait: Object.keys(nextWait).length > 0 ? nextWait : undefined, + }); + }; + return ( onChange({ ...value, toolCallPrefix: e.target.value.replace(/[^a-z0-9_{}/]/g, "") || undefined })} placeholder="e.g. proxy_" - className="font-mono text-sm" + className="font-mono text-base md:text-sm" />

{t(`${s}.toolCallPrefixHint`)}

+
+ {t(`${s}.waitLimits`)} +
+ updateWaitLimit("min_ms", e.target.value)} + placeholder={t(`${s}.waitMinPlaceholder`)} + className="text-base md:text-sm" + /> + updateWaitLimit("max_ms", e.target.value)} + placeholder={t(`${s}.waitMaxPlaceholder`)} + className="text-base md:text-sm" + /> +
+

{t(`${s}.waitLimitsHint`)}

+
{t(`${s}.allow`)} ; + byProvider?: Record; + wait?: { + min_ms?: number; + max_ms?: number; + }; toolCallPrefix?: string; // prefix to strip from model's tool call names }