diff --git a/docs/adk-20-upgrade.md b/docs/adk-20-upgrade.md index bb2e15a..9334f03 100644 --- a/docs/adk-20-upgrade.md +++ b/docs/adk-20-upgrade.md @@ -27,9 +27,9 @@ purpose: **a row is only "done" when a production caller reaches it.** | Per-node retry | `workflow.NodeConfig.RetryConfig` | **done** — replaced the hand-rolled retry loop | | Step context isolation | `llmagent.IncludeContentsNone` | **done** | | Self-healing tool errors | `plugin/retryandreflect` | **done** — tools return Go errors, so `OnToolErrorCallback` fires | -| Parameter injection | `plugin/functioncallmodifier` | **inert** — see Gaps | -| Subagent delegation | `tool/agenttool` | **not wired** — see Gaps | -| Human-in-the-loop | `tool/toolconfirmation` | **not wired** — see Gaps | +| Parameter injection | `plugin/functioncallmodifier` | **not needed** — `description` is a static params field | +| Subagent delegation | `tool/agenttool` | **rejected** — see Deliberate departures | +| Human-in-the-loop | `tool/toolconfirmation` | **deferred** — needs turn suspend/resume | | Artifacts | `artifact.Service`, `loadartifactstool` | **dropped** — see Gaps | | Dynamic instructions | `util/instructionutil` | **deliberately not used** — see Gaps | | Parallel / fan-out | `JoinNode`, `NodeConfig.ParallelWorker` | **planned** | @@ -88,45 +88,53 @@ so a chain that died at step 1 of 5 rendered 5/5 green. --- -## Gaps - -Recorded so the next reader does not mistake an import for an -integration. - -**`plugin/functioncallmodifier`** is registered with a predicate that -always returns `false` (`pkg/engine/plugins.go`), so it never applies. -PR #132 disabled it to fix a proto validation error; the manual JSON -schema surgery it was meant to replace is still in -`pkg/tools/bridge.go`. - -**`tool/agenttool`** — `BuildResearchSubagent`, `BuildNamedSubagent`, -`BuildResearchAgentTool`, and `BuildNamedAgentTool` have no production -callers. The `task` tool still spawns a nested `engine.Run`. - -**`tool/toolconfirmation`** — no tool declares `RequireConfirmation` or a -`ConfirmationProvider`, so ADK never emits `adk_request_confirmation` and -the handling in `run.go` / `agent_run.go` is unreachable. Approval is the -in-tool blocking path in `pkg/tools/env.go`. - -**Artifacts** were dropped rather than wired. ADK ships only -`InMemoryService` and `gcsartifact`; ask's was rebuilt per turn and -nothing ever saved to it, so `loadartifactstool` could only return empty -while costing tokens on every request. Node outputs cover step handoff -and `pkg/memory` covers durable state. - -**`util/instructionutil`** is deliberately not used. ask's instruction -text is user documentation inlined verbatim, not a template — see the -comment on `BuildInstructionProvider`. - ---- +## Deliberate departures + +Recorded so nobody "finishes the migration" by wiring one of these. + +**`plugin/functioncallmodifier` — removed, not needed.** It injects +synthetic arguments into tool declarations at request time. ask needed +that for the required `description` phrase, which is now a static field +on every native tool's params struct, so there is nothing left to +inject. It had shipped with a predicate that always returned `false` +since PR #132. Bridge tools still get `description` added to their input +schema in `pkg/tools/bridge.go`, because their input types come from the +MCP handler cores; that is a one-time build at construction. + +**`tool/agenttool` — rejected for the task tool.** `agent_tool.go` +builds its own runner with a hardcoded config: no `PluginConfig`, so a +subagent would lose `retryandreflect`, and `MemoryService: +memory.InMemoryService()`, so it would lose ask's memory. It also +produces one tool per agent, replacing `task(agent: "foo")` with a tool +named `foo`. The task tool's nested `engine.Run` goes through +`RunnerBuilder` — ask's plugins, memory, and file session service — and +keeps the background-job path and the subagent UI events. The four +builders added for this migration had no production callers and are +deleted. + +**`tool/toolconfirmation` — deferred.** It emits an +`adk_request_confirmation` function call, pauses the run, and resumes on +a function response. ask has no suspend/resume path for a chat turn, so +adopting it means building one. Approval is `ToolEnv.ApprovalDenied`, +which blocks on the TUI modal and returns the denial inline. +`IsConfirmationCall` / `UnwrapConfirmationCall` stay wired in the event +loops because an MCP server can declare confirmation on its own tools. + +**`util/instructionutil` — not used.** ask's instruction text is user +documentation inlined verbatim, not a template. See the comment on +`BuildInstructionProvider`. + +**Artifacts — dropped.** ADK ships only `InMemoryService` and +`gcsartifact`; ask's was rebuilt per turn and nothing ever saved to it. +Node outputs cover step handoff and `pkg/memory` covers durable state. ## Planned -**Parallel / fan-out.** A `parallel` step kind alongside `loop`, compiled -to fan-out edges plus a `JoinNode`, with `NodeConfig.ParallelWorker` for -list-typed inputs. Needs builder UI and a store schema addition. +See [follow-ups.md](follow-ups.md). Two items: -**Pause / resume and HITL.** `workflow.Persistence` plus -`Workflow.Resume`, and `NewRequestInputEvent` routed to ask's question -modal — replacing today's behaviour where workflow tabs auto-decline -every prompt. +- **Pause and resume** — non-blocking approvals and resumable workflows. + One project, not two: both need the ability to pause a turn and pick it + up later. Covers `workflow.Persistence`, `Workflow.Resume`, + `NewRequestInputEvent`, and `tool/toolconfirmation`. +- **Parallel / fan-out** — a `parallel` step kind using `JoinNode` and + `NodeConfig.ParallelWorker`. Independent of the above. diff --git a/docs/follow-ups.md b/docs/follow-ups.md new file mode 100644 index 0000000..0f10eb7 --- /dev/null +++ b/docs/follow-ups.md @@ -0,0 +1,80 @@ +# Follow-ups + +Work that is deliberately not done yet, with the reasoning, so it can be +picked up without re-deriving it. Written 2026-08-21, after the ADK +migration audit. + +--- + +## 1. Pause and resume — the big one + +**The problem today.** When the agent wants to write a file or run a +command, ask pops a modal and the agent freezes. You have to be at that +machine, at that moment. Walk away and it sits there. Close the app and +the work is gone. + +Worse: workflow tabs **auto-deny every approval**, because no human is +attached to answer. That is why workflows only really work for +pre-cleared operations. + +**What changes.** The agent stops cleanly, saves where it was, and the +approval becomes a message instead of a blocking popup. You answer +whenever. The run picks up where it left off. + +**What it unlocks, in rough order of value:** + +1. **Workflows that can ask.** A pipeline that hits an approval pauses + and waits instead of being denied. This is the difference between + "workflows only do pre-cleared work" and "workflows do real work." +2. **Approvals that survive a restart.** Close the laptop, come back, + approve, continue. +3. **Approve from somewhere else.** Once an approval is data rather than + a modal, it can go to a phone, a web page, Slack. Prerequisite for ask + running anywhere but the terminal in front of you. +4. **Long-running agents.** Something that runs for an hour, hits one + decision point, parks, waits. + +**Why it is one project, not two.** Resumable workflows and +non-blocking approvals need the same machinery: the ability to pause a +turn and pick it up later. Doing either gets most of the other. + +**The ADK pieces that map to it:** `workflow.Persistence` and +`Workflow.Resume` for the workflow half, `NewRequestInputEvent` for +surfacing the prompt, and `tool/toolconfirmation` for approvals. ask +currently uses none of them, and `ToolEnv.ApprovalDenied` blocks on the +modal instead. + +**Size.** Not small. ask has no suspend/resume path for a chat turn at +all; that is the work. + +--- + +## 2. Parallel / fan-out + +Independent of the above. Add a `parallel` step kind alongside `loop` in +a workflow definition: fan several steps out at once, join their results, +continue. + +The graph engine already supports it — `JoinNode` for the join, +`NodeConfig.ParallelWorker` for running a node once per item of a list +input. The work is the workflow schema, the builder UI, and the +compiler mapping. It needs nothing from follow-up 1. + +--- + +## 3. Agent discoverability (maybe) + +**The symptom to watch for:** the model not using agents that are +defined. Today there is one `task` tool and the model names which agent +it wants as a parameter; the available agents are described in prose in +the system prompt. + +ADK's `tool/agenttool` would make every agent its own tool, which models +pick from more reliably than from a prose list. It was rejected — +see "Deliberate departures" in `adk-20-upgrade.md` — because it would +cost every agent's definition on every request, and because subagents +would lose memory access, tool-error recovery, and background execution. + +If this turns out to be a real complaint, cheaper fixes first: better +trigger descriptions on agent definitions, or surfacing agents through +the same registry search that MCP tools use. diff --git a/pkg/engine/interaction.go b/pkg/engine/interaction.go index c9443e1..b9489ff 100644 --- a/pkg/engine/interaction.go +++ b/pkg/engine/interaction.go @@ -114,6 +114,19 @@ func (h HeadlessInteractionHandler) RequestSudoPassword(ctx context.Context, tab return SudoPasswordResponse{Cancelled: true}, nil } +// Approval is NOT ADK's tool/toolconfirmation flow, deliberately. +// +// That flow emits an adk_request_confirmation function call, pauses the +// run, and resumes when a function response arrives. ask has no +// suspend/resume path for a chat turn, so adopting it means building +// one. Instead a mutating tool calls ToolEnv.ApprovalDenied, which blocks +// on the TUI modal and returns the denial message inline. +// +// The two unwrap helpers below stay because an MCP server can declare +// confirmation on its own tools; if ADK ever emits the call, the event +// loops in run.go and agent_run.go render the inner intent rather than +// the wrapper. + // IsConfirmationCall reports whether a function call is an ADK tool confirmation request. func IsConfirmationCall(fc *genai.FunctionCall) bool { if fc == nil { @@ -126,10 +139,3 @@ func IsConfirmationCall(fc *genai.FunctionCall) bool { func UnwrapConfirmationCall(fc *genai.FunctionCall) (*genai.FunctionCall, error) { return toolconfirmation.OriginalCallFrom(fc) } - -// FormatConfirmationResponse constructs the standard ADK confirmation response payload. -func FormatConfirmationResponse(confirmed bool) map[string]any { - return map[string]any{ - "confirmed": confirmed, - } -} diff --git a/pkg/engine/interaction_test.go b/pkg/engine/interaction_test.go index 956b585..198a039 100644 --- a/pkg/engine/interaction_test.go +++ b/pkg/engine/interaction_test.go @@ -52,14 +52,4 @@ func TestInteraction_ConfirmationHelpers(t *testing.T) { t.Errorf("expected unwrapped command 'rm -rf tmp', got %v", origCall.Args["command"]) } - // FormatConfirmationResponse - respTrue := FormatConfirmationResponse(true) - if confirmed, ok := respTrue["confirmed"].(bool); !ok || !confirmed { - t.Errorf("expected confirmed: true in response, got %v", respTrue) - } - - respFalse := FormatConfirmationResponse(false) - if confirmed, ok := respFalse["confirmed"].(bool); !ok || confirmed { - t.Errorf("expected confirmed: false in response, got %v", respFalse) - } } diff --git a/pkg/engine/plugins.go b/pkg/engine/plugins.go index 8e00c87..042f895 100644 --- a/pkg/engine/plugins.go +++ b/pkg/engine/plugins.go @@ -2,44 +2,37 @@ package engine import ( "google.golang.org/adk/v2/plugin" - "google.golang.org/adk/v2/plugin/functioncallmodifier" "google.golang.org/adk/v2/plugin/retryandreflect" - "google.golang.org/genai" ) // DefaultPlugins returns the standard set of ADK plugins configured for ask. +// +// functioncallmodifier is deliberately absent. It exists to inject +// synthetic arguments into tool declarations at request time; ask needed +// that for the required `description` phrase, which is now a real field +// on every native tool's params struct, so there is nothing left to +// inject. It was registered here with a predicate that always returned +// false ever since PR #132 disabled it to stop it clobbering +// ParametersJsonSchema — a plugin that could never fire. +// +// Bridge tools (linear_*, workflow_*) still have `description` added to +// their input schema in pkg/tools/bridge.go, because their input types +// come from the MCP handler cores and do not carry the field. That is a +// one-time schema build at construction, not per-request AST surgery. func DefaultPlugins() []*plugin.Plugin { var plugins []*plugin.Plugin - // 1. Retry and reflect plugin for automated in-turn tool error self-healing. + // Retry and reflect: when a tool returns a Go error, hand the model + // corrective guidance and let it retry in the same turn instead of + // surrendering. Every ask tool reports failure as a real error, which + // is what OnToolErrorCallback keys on. if retryPlugin, err := NewRetryAndReflectPlugin(2); err == nil && retryPlugin != nil { plugins = append(plugins, retryPlugin) } - // 2. Function call modifier plugin for parameter injection when configured. - // Defaults to inactive so native tools using functiontool.New with ParametersJsonSchema - // are not corrupted by the plugin's decl.Parameters initialization. - if modPlugin, err := NewFunctionCallModifierPlugin(FunctionCallModifierOptions{ - Predicate: func(toolName string) bool { - return false - }, - }); err == nil && modPlugin != nil { - plugins = append(plugins, modPlugin) - } - return plugins } -func isCoreCodingTool(toolName string) bool { - switch toolName { - case "read", "write", "edit", "glob", "grep", "ls", "bash", "job_output", "job_kill", - "fetch", "todos", "ask_user_question", "end_turn", "web_search": - return true - default: - return false - } -} - // NewRetryAndReflectPlugin creates an ADK retryandreflect plugin with the specified max retries. func NewRetryAndReflectPlugin(maxRetries int) (*plugin.Plugin, error) { if maxRetries <= 0 { @@ -50,26 +43,3 @@ func NewRetryAndReflectPlugin(maxRetries int) (*plugin.Plugin, error) { retryandreflect.WithTrackingScope(retryandreflect.Invocation), ) } - -// FunctionCallModifierOptions defines configuration for the functioncallmodifier plugin. -type FunctionCallModifierOptions struct { - Predicate func(toolName string) bool - Args map[string]*genai.Schema - OverrideDescription func(originalDescription string) string -} - -// NewFunctionCallModifierPlugin creates an ADK functioncallmodifier plugin with safety guards. -func NewFunctionCallModifierPlugin(opts FunctionCallModifierOptions) (*plugin.Plugin, error) { - pred := opts.Predicate - if pred == nil { - pred = func(toolName string) bool { - return len(opts.Args) > 0 || opts.OverrideDescription != nil - } - } - cfg := functioncallmodifier.FunctionCallModifierConfig{ - Predicate: pred, - Args: opts.Args, - OverrideDescription: opts.OverrideDescription, - } - return functioncallmodifier.NewPlugin(cfg) -} diff --git a/pkg/engine/plugins_test.go b/pkg/engine/plugins_test.go index 2cf2e0c..529f48b 100644 --- a/pkg/engine/plugins_test.go +++ b/pkg/engine/plugins_test.go @@ -1,11 +1,8 @@ package engine import ( + "strings" "testing" - - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/plugin" - "google.golang.org/genai" ) func TestDefaultPlugins_Configuration(t *testing.T) { @@ -15,25 +12,19 @@ func TestDefaultPlugins_Configuration(t *testing.T) { } hasRetry := false - hasModifier := false for _, p := range plugins { if p == nil { t.Fatal("received nil plugin in DefaultPlugins()") } - switch p.Name() { - case "RetryAndReflectPlugin": + if p.Name() == "RetryAndReflectPlugin" { hasRetry = true - case "FunctionCallModifierPlugin": - hasModifier = true } } if !hasRetry { t.Error("expected RetryAndReflectPlugin to be present in DefaultPlugins") } - if !hasModifier { - t.Error("expected FunctionCallModifierPlugin to be present in DefaultPlugins") - } + } func TestNewRetryAndReflectPlugin(t *testing.T) { @@ -61,118 +52,14 @@ func TestNewRetryAndReflectPlugin(t *testing.T) { }) } -func TestNewFunctionCallModifierPlugin(t *testing.T) { - t.Run("with nil predicate and args", func(t *testing.T) { - p, err := NewFunctionCallModifierPlugin(FunctionCallModifierOptions{}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if p == nil || p.Name() != "FunctionCallModifierPlugin" { - t.Fatalf("expected valid FunctionCallModifierPlugin, got %v", p) +// The description phrase is a static field on every native tool's params +// struct, so nothing needs to inject it at request time. This pins that +// the plugin list stays free of functioncallmodifier, whose only job here +// was that injection and which shipped disabled. +func TestDefaultPlugins_NoFunctionCallModifier(t *testing.T) { + for _, p := range DefaultPlugins() { + if p != nil && strings.Contains(p.Name(), "FunctionCallModifier") { + t.Error("functioncallmodifier is redundant: description is a real params field") } - }) - - t.Run("with custom schema args and description override", func(t *testing.T) { - p, err := NewFunctionCallModifierPlugin(FunctionCallModifierOptions{ - Predicate: func(toolName string) bool { - return toolName == "read" - }, - Args: map[string]*genai.Schema{ - "description": { - Type: "STRING", - Description: "short phrase", - }, - }, - OverrideDescription: func(orig string) string { - return orig + " (modified)" - }, - }) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if p == nil || p.Name() != "FunctionCallModifierPlugin" { - t.Fatalf("expected valid FunctionCallModifierPlugin, got %v", p) - } - }) -} - -func TestFunctionCallModifier_ActiveParameterInjection(t *testing.T) { - plugins := DefaultPlugins() - var modPlugin *plugin.Plugin - for _, p := range plugins { - if p != nil && p.Name() == "FunctionCallModifierPlugin" { - modPlugin = p - break - } - } - if modPlugin == nil { - t.Fatal("expected FunctionCallModifierPlugin in DefaultPlugins") - } - - // Test isCoreCodingTool predicate - coreTools := []string{"read", "write", "edit", "glob", "grep", "ls", "bash", "job_output", "job_kill", "fetch", "todos", "ask_user_question", "end_turn", "web_search"} - for _, name := range coreTools { - if !isCoreCodingTool(name) { - t.Errorf("expected %q to be recognized as core coding tool", name) - } - } - - nonCore := []string{"custom_tool", "random_func", "unknown"} - for _, name := range nonCore { - if isCoreCodingTool(name) { - t.Errorf("expected %q not to be recognized as core coding tool", name) - } - } -} - -func TestDefaultPlugins_DoesNotCorruptParametersJsonSchema(t *testing.T) { - plugins := DefaultPlugins() - var modPlugin *plugin.Plugin - for _, p := range plugins { - if p != nil && p.Name() == "FunctionCallModifierPlugin" { - modPlugin = p - break - } - } - if modPlugin == nil { - t.Fatal("expected FunctionCallModifierPlugin in DefaultPlugins") - } - - // Create a tool with ParametersJsonSchema - decl := &genai.FunctionDeclaration{ - Name: "read", - Description: "Read file content", - ParametersJsonSchema: map[string]any{"type": "object", "properties": map[string]any{"file_path": map[string]any{"type": "string"}}}, - } - - req := &model.LLMRequest{ - Tools: map[string]any{ - "read": struct{}{}, - }, - Config: &genai.GenerateContentConfig{ - Tools: []*genai.Tool{ - { - FunctionDeclarations: []*genai.FunctionDeclaration{decl}, - }, - }, - }, - } - - // Run BeforeModelCallback - actx := NewStandaloneAgentContext(nil) - cb := modPlugin.BeforeModelCallback() - if cb != nil { - _, err := cb(actx, req) - if err != nil { - t.Fatalf("unexpected error from BeforeModelCallback: %v", err) - } - } - - // Parameters must remain nil to prevent Vertex AI proto validation failure - if decl.Parameters != nil { - t.Errorf("decl.Parameters was mutated to non-nil: %+v; this triggers proto validation error with ParametersJsonSchema", decl.Parameters) - } - if decl.ParametersJsonSchema == nil { - t.Errorf("decl.ParametersJsonSchema was unexpectedly cleared") } } diff --git a/pkg/engine/prompt.go b/pkg/engine/prompt.go index 0d4902b..8571e87 100644 --- a/pkg/engine/prompt.go +++ b/pkg/engine/prompt.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "runtime" + "sort" "strings" "time" "unicode/utf8" @@ -445,6 +446,17 @@ func ContextFileRealPath(path string) string { return filepath.Clean(path) } +// ContextScope is one directory searched for instruction files, paired +// with the root its @-links resolve against. +// +// The two differ for the user-global scope: ~/.claude/CLAUDE.md is not +// inside the project, so an @-link in it must resolve under ~/.claude, +// not under the repository that happens to be open. +type ContextScope struct { + Dir string + Root string +} + // AgentContextSearchDirs lists the directories searched for project // instruction files, in load order: the user-global ~/.claude scope // first, then every directory from the project root down to cwd, so @@ -452,12 +464,24 @@ func ContextFileRealPath(path string) string { // Mirrors RuleSearchScopes / DiscoverSkills / DiscoverSubagents, which // already walk to the project root and read the user-global scope. func AgentContextSearchDirs(cwd string) []string { - var dirs []string + scopes := AgentContextScopes(cwd) + dirs := make([]string, 0, len(scopes)) + for _, sc := range scopes { + dirs = append(dirs, sc.Dir) + } + return dirs +} + +// AgentContextScopes is AgentContextSearchDirs with each directory's +// @-link resolution root attached. +func AgentContextScopes(cwd string) []ContextScope { + var scopes []ContextScope if home, err := os.UserHomeDir(); err == nil && home != "" { - dirs = append(dirs, filepath.Join(home, ".claude")) + claude := filepath.Join(home, ".claude") + scopes = append(scopes, ContextScope{Dir: claude, Root: claude}) } if cwd == "" { - return dirs + return scopes } abs, err := filepath.Abs(cwd) if err != nil { @@ -475,9 +499,9 @@ func AgentContextSearchDirs(cwd string) []string { } } for i := len(chain) - 1; i >= 0; i-- { - dirs = append(dirs, chain[i]) + scopes = append(scopes, ContextScope{Dir: chain[i], Root: root}) } - return dirs + return scopes } // AgentContextFiles loads the project's instruction files (CLAUDE.md, @@ -493,8 +517,8 @@ func AgentContextSearchDirs(cwd string) []string { func AgentContextFiles(cwd string) []LoadedContextDoc { var docs []LoadedContextDoc seenReal := map[string]bool{} - for _, dir := range AgentContextSearchDirs(cwd) { - docs = append(docs, contextFilesInDir(dir, seenReal)...) + for _, sc := range AgentContextScopes(cwd) { + docs = append(docs, contextFilesInDir(sc.Dir, sc.Root, seenReal)...) } return docs } @@ -502,7 +526,7 @@ func AgentContextFiles(cwd string) []LoadedContextDoc { // contextFilesInDir loads the instruction files present in one // directory, skipping any whose resolved path is already in seenReal // and recording the ones it loads. -func contextFilesInDir(dir string, seenReal map[string]bool) []LoadedContextDoc { +func contextFilesInDir(dir, linkRoot string, seenReal map[string]bool) []LoadedContextDoc { var docs []LoadedContextDoc seenName := map[string]bool{} for _, name := range AgentContextFileNames { @@ -531,6 +555,7 @@ func contextFilesInDir(dir string, seenReal map[string]bool) []LoadedContextDoc Path: path, Body: strings.TrimRight(content, "\n"), Links: links, + Root: linkRoot, }) } return docs @@ -608,17 +633,38 @@ func BuildSystemPrompt(opts PromptOptions) string { } // Seed from the links each document recorded off its FULL body, not // from the capped Body that goes on the wire — otherwise an @-link - // living past the cap is never followed. - var sourceLinks []string + // living past the cap is never followed. Each document resolves its + // links against its OWN scope root, so an @-link in + // ~/.claude/CLAUDE.md looks under ~/.claude rather than under + // whichever repository happens to be open. + linksByRoot := map[string][]string{} for _, d := range ctxDocs { - sourceLinks = append(sourceLinks, d.Links...) + root := d.Root + if root == "" { + root = repoRoot + } + linksByRoot[root] = append(linksByRoot[root], d.Links...) } for _, r := range rules { - if r.Eager() { - sourceLinks = append(sourceLinks, r.Links...) + if !r.Eager() { + continue } + root := r.Root + if root == "" { + root = repoRoot + } + linksByRoot[root] = append(linksByRoot[root], r.Links...) + } + var linkRoots []string + for root := range linksByRoot { + linkRoots = append(linkRoots, root) + } + sort.Strings(linkRoots) + var linkedDocs []LoadedContextDoc + for _, root := range linkRoots { + linkedDocs = append(linkedDocs, LoadContextLinksFrom(root, linksByRoot[root])...) } - if linkedDocs := LoadContextLinksFrom(repoRoot, sourceLinks); len(linkedDocs) > 0 { + if len(linkedDocs) > 0 { if block := ContextLinksPromptBlock(linkedDocs); block != "" { b.WriteString("\n\n") b.WriteString(block) diff --git a/pkg/engine/prompt_links.go b/pkg/engine/prompt_links.go index 9797c40..09934ae 100644 --- a/pkg/engine/prompt_links.go +++ b/pkg/engine/prompt_links.go @@ -20,6 +20,9 @@ type LoadedContextDoc struct { // Body is what goes into the prompt, capped by // TruncateInstructionDoc. Body string + // Root is the directory this document's @-links resolve against — + // its own scope, not necessarily the project root. + Root string // Links are the @-references found in the document's FULL body, // before any truncation. Body alone is not a safe source for them: // a link past the cap is still a real dependency of the diff --git a/pkg/engine/prompt_test.go b/pkg/engine/prompt_test.go index e5029b1..4b07367 100644 --- a/pkg/engine/prompt_test.go +++ b/pkg/engine/prompt_test.go @@ -251,6 +251,53 @@ func TestAgentContextFileCap_FitsRealInstructionFiles(t *testing.T) { // End to end: a CLAUDE.md that overflows the cap still gets its @-linked // docs into , even when the link itself sits in the part // that was cut. +// An @-link in the user-global CLAUDE.md resolves inside ~/.claude, not +// inside whichever repository happens to be open. This is the real case: +// ~/.claude/CLAUDE.md contains "@RTK.md", which lives next to it, and +// resolving against the project root looked for /RTK.md and +// silently found nothing. +func TestBuildSystemPrompt_GlobalScopeLinksResolveInHome(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + stubGitStatus(t, "") + + writeTestDoc(t, home, ".claude/CLAUDE.md", "Global rules.\nSee @RTK.md for the CLI.\n") + writeTestDoc(t, home, ".claude/RTK.md", "# RTK\nrtk-marker\n") + + cwd := t.TempDir() + if err := os.Mkdir(filepath.Join(cwd, ".git"), 0o755); err != nil { + t.Fatal(err) + } + writeTestDoc(t, cwd, "CLAUDE.md", "Project rules.\n") + // A same-named file in the project must not be what gets picked up. + writeTestDoc(t, cwd, "RTK.md", "wrong-file-marker") + + prompt := BuildSystemPrompt(PromptOptions{Cwd: cwd}) + if !strings.Contains(prompt, "rtk-marker") { + t.Errorf("global @-link must resolve under ~/.claude:\n%s", prompt) + } + if strings.Contains(prompt, "wrong-file-marker") { + t.Error("global @-link resolved into the project root") + } +} + +// A project document's links still resolve against the project root. +func TestBuildSystemPrompt_ProjectScopeLinksResolveInProject(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + stubGitStatus(t, "") + cwd := t.TempDir() + if err := os.Mkdir(filepath.Join(cwd, ".git"), 0o755); err != nil { + t.Fatal(err) + } + writeTestDoc(t, cwd, "CLAUDE.md", "See @docs/guide.md.\n") + writeTestDoc(t, cwd, "docs/guide.md", "project-guide-marker") + + prompt := BuildSystemPrompt(PromptOptions{Cwd: cwd}) + if !strings.Contains(prompt, "project-guide-marker") { + t.Errorf("project @-link must still resolve under the project root:\n%s", prompt) + } +} + func TestBuildSystemPrompt_FollowsContextLinkPastCap(t *testing.T) { t.Setenv("HOME", t.TempDir()) stubGitStatus(t, "") diff --git a/pkg/engine/rules.go b/pkg/engine/rules.go index 584b466..5aff553 100644 --- a/pkg/engine/rules.go +++ b/pkg/engine/rules.go @@ -29,6 +29,10 @@ type Rule struct { // Links are the @-references found in the rule's FULL body, before // truncation — see LoadedContextDoc.Links. Links []string + // Root is the directory this rule's @-links resolve against: its own + // scope root, so a user-global rule links within ~/.claude rather + // than into whichever repository is open. + Root string } func (r Rule) Eager() bool { return len(r.Paths) == 0 } @@ -141,6 +145,7 @@ func ParseRuleFile(path string, scope RuleScope) (Rule, bool) { Paths: paths, Body: strings.TrimRight(body, "\n"), Links: links, + Root: scope.Root, }, true } @@ -370,7 +375,7 @@ func (ct *ContextAwareTool) Run(ctx agent.Context, args any) (map[string]any, er var dirAdd []string if !seen { ct.mu.Lock() - docs := contextFilesInDir(dir, ct.seenCtxFile) + docs := contextFilesInDir(dir, ct.root, ct.seenCtxFile) ct.mu.Unlock() for _, d := range docs { relP, err := filepath.Rel(ct.root, d.Path) @@ -410,7 +415,13 @@ func (ct *ContextAwareTool) Run(ctx agent.Context, args any) (map[string]any, er add = append(add, fmt.Sprintf("## Rule for %s (%s)\n\n%s", rel, r.Rel, r.Body)) // r.Links, not r.Body — Body is capped, Links are not. - if linked := LoadContextLinksFrom(ct.root, r.Links); len(linked) > 0 { + // r.Root, not ct.root — a user-global rule resolves its + // links inside ~/.claude. + linkRoot := r.Root + if linkRoot == "" { + linkRoot = ct.root + } + if linked := LoadContextLinksFrom(linkRoot, r.Links); len(linked) > 0 { for _, d := range linked { add = append(add, fmt.Sprintf("### Included from %s\n\n%s", d.Path, d.Body)) } diff --git a/pkg/engine/subagents.go b/pkg/engine/subagents.go index 85c7fad..f7c39ab 100644 --- a/pkg/engine/subagents.go +++ b/pkg/engine/subagents.go @@ -1,8 +1,8 @@ package engine import ( - "errors" "fmt" + "google.golang.org/genai" "os" "path/filepath" "sort" @@ -10,12 +10,6 @@ import ( "github.com/Cidan/ask/pkg/config" "github.com/Cidan/ask/pkg/providers" - "google.golang.org/adk/v2/agent" - "google.golang.org/adk/v2/agent/llmagent" - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/tool" - "google.golang.org/adk/v2/tool/agenttool" - "google.golang.org/genai" ) // SubagentDef is a named subagent definition. @@ -211,69 +205,18 @@ func SubagentTools(def SubagentDef, available map[string]Tool) []Tool { return out } -// BuildResearchSubagent constructs an ADK agent for deep research and investigation. -func BuildResearchSubagent(llm model.LLM, tools []tool.Tool, maxTokens int32) (agent.Agent, error) { - instruction := `You are a research sub-agent inside ask. Your role is to perform deep, thorough investigations on the codebase, read relevant files, execute commands if needed, search broadly, and return a comprehensive, self-contained final report to the calling agent. -State the answer and findings first, followed by concrete file_path:line_number references. Be concise, precise, and honest.` - - var genConfig *genai.GenerateContentConfig - if maxTokens > 0 { - genConfig = &genai.GenerateContentConfig{ - MaxOutputTokens: maxTokens, - } - } - - return llmagent.New(llmagent.Config{ - Name: "research_subagent", - Description: "Performs thorough code research, file reading, and investigation", - Model: llm, - Instruction: instruction, - Tools: tools, - GenerateContentConfig: genConfig, - }) -} - -// BuildNamedSubagent constructs an ADK agent from a SubagentDef. -func BuildNamedSubagent(def SubagentDef, llm model.LLM, tools []tool.Tool, maxTokens int32) (agent.Agent, error) { - prompt := def.Prompt - if prompt == "" { - prompt = fmt.Sprintf("You are subagent %s. %s", def.Name, def.Description) - } - - var genConfig *genai.GenerateContentConfig - if maxTokens > 0 { - genConfig = &genai.GenerateContentConfig{ - MaxOutputTokens: maxTokens, - } - } - - return llmagent.New(llmagent.Config{ - Name: def.Name, - Description: def.Description, - Model: llm, - Instruction: prompt, - Tools: tools, - GenerateContentConfig: genConfig, - }) -} - -// BuildResearchAgentTool wraps an ADK agent as a callable ADK tool. -func BuildResearchAgentTool(agentInstance agent.Agent) (tool.Tool, error) { - if agentInstance == nil { - return nil, errors.New("agent instance is nil") - } - return agenttool.New(agentInstance, &agenttool.Config{ - SkipSummarization: true, - }), nil -} - -// BuildNamedAgentTool constructs and wraps a named subagent as a callable ADK tool. -func BuildNamedAgentTool(def SubagentDef, llm model.LLM, tools []tool.Tool, maxTokens int32) (tool.Tool, error) { - ag, err := BuildNamedSubagent(def, llm, tools, maxTokens) - if err != nil { - return nil, err - } - return agenttool.New(ag, &agenttool.Config{ - SkipSummarization: true, - }), nil -} +// Subagents deliberately do NOT use ADK's tool/agenttool. +// +// agenttool.New wraps an agent as a tool, but it builds its own runner +// with a hardcoded config (agent_tool.go): no PluginConfig, so the +// subagent loses retryandreflect, and MemoryService is +// memory.InMemoryService(), so it loses ask's real memory. It also +// produces one tool per agent, which would replace task(agent: "foo") +// with a tool named foo. +// +// The task tool instead runs a nested engine.Run, which goes through +// RunnerBuilder — ask's plugins, memory service, and file session +// service — and keeps the background-job path and the subagent UI +// events. BuildResearchSubagent / BuildNamedSubagent / +// BuildResearchAgentTool / BuildNamedAgentTool were added for the +// agenttool migration, never called from production, and are deleted. diff --git a/pkg/engine/subagents_test.go b/pkg/engine/subagents_test.go index 9e9ecb3..f8ead7c 100644 --- a/pkg/engine/subagents_test.go +++ b/pkg/engine/subagents_test.go @@ -2,14 +2,12 @@ package engine import ( "context" - "iter" "os" "path/filepath" "strings" "testing" "github.com/Cidan/ask/pkg/config" - adkmodel "google.golang.org/adk/v2/model" "google.golang.org/genai" ) @@ -104,75 +102,10 @@ func TestSubagentTools_GrantSets(t *testing.T) { } } -func TestBuildResearchSubagent_ADKIntegration(t *testing.T) { - fakeLLM := &fakeSubagentLLM{} - subagent, err := BuildResearchSubagent(fakeLLM, nil, 4096) - if err != nil { - t.Fatalf("BuildResearchSubagent failed: %v", err) - } - if subagent == nil { - t.Fatal("expected non-nil subagent") - } - if subagent.Name() != "research_subagent" { - t.Errorf("expected name 'research_subagent', got %q", subagent.Name()) - } - - agentTool, err := BuildResearchAgentTool(subagent) - if err != nil { - t.Fatalf("BuildResearchAgentTool failed: %v", err) - } - if agentTool == nil { - t.Fatal("expected non-nil agentTool") - } -} - -func TestBuildNamedSubagent_ADKIntegration(t *testing.T) { - fakeLLM := &fakeSubagentLLM{} - def := SubagentDef{ - Name: "custom_agent", - Description: "A custom test subagent", - Prompt: "Investigate things.", - } - subagent, err := BuildNamedSubagent(def, fakeLLM, nil, 2048) - if err != nil { - t.Fatalf("BuildNamedSubagent failed: %v", err) - } - if subagent == nil { - t.Fatal("expected non-nil subagent") - } - if subagent.Name() != "custom_agent" { - t.Errorf("expected name 'custom_agent', got %q", subagent.Name()) - } - - agentTool, err := BuildNamedAgentTool(def, fakeLLM, nil, 2048) - if err != nil { - t.Fatalf("BuildNamedAgentTool failed: %v", err) - } - if agentTool == nil { - t.Fatal("expected non-nil agentTool") - } - if agentTool.Name() != "custom_agent" { - t.Errorf("expected tool name 'custom_agent', got %q", agentTool.Name()) - } -} - -type fakeSubagentLLM struct{} - -func (f *fakeSubagentLLM) Name() string { return "fake-llm" } - -func (f *fakeSubagentLLM) GenerateContent(ctx context.Context, req *adkmodel.LLMRequest, stream bool) iter.Seq2[*adkmodel.LLMResponse, error] { - return func(yield func(*adkmodel.LLMResponse, error) bool) { - resp := &adkmodel.LLMResponse{ - Content: &genai.Content{ - Role: genai.RoleModel, - Parts: []*genai.Part{ - {Text: "report"}, - }, - }, - } - yield(resp, nil) - } -} +// Subagents run through the task tool's nested engine.Run rather than +// ADK's agenttool — see the note at the top of subagents.go. The builders +// that wrapped them in agenttool had no production callers and are gone, +// so the tests that exercised them are too. func TestResolveSubagentModel(t *testing.T) { home := t.TempDir() diff --git a/pkg/tools/contract_test.go b/pkg/tools/contract_test.go index 44a894b..4ca1370 100644 --- a/pkg/tools/contract_test.go +++ b/pkg/tools/contract_test.go @@ -82,3 +82,23 @@ func TestToolFailuresAreGoErrors(t *testing.T) { t.Error("a no-op edit must return a Go error") } } + +// The description phrase is a static field on every coding tool's params +// struct. That is what makes ADK's functioncallmodifier plugin +// unnecessary — it exists to inject synthetic arguments at request time, +// and ask has nothing left to inject. +func TestCodingToolsDeclareDescriptionStatically(t *testing.T) { + coding := map[string]bool{ + "read": true, "write": true, "edit": true, "glob": true, "grep": true, + "ls": true, "bash": true, "fetch": true, "todos": true, + } + for _, tl := range allCoreTools(t) { + info := ExtractToolInfo(tl) + if !coding[info.Name] { + continue + } + if _, ok := info.Parameters["description"]; !ok { + t.Errorf("%s must declare a description parameter, got %v", info.Name, info.Parameters) + } + } +}