diff --git a/CLAUDE.md b/CLAUDE.md index a398c12..34bfad8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,7 +82,7 @@ One `package main`, one file per concern. | `rules.go` | Claude Code `.claude/rules/` standard: `*.md` rule files discovered recursively (symlink-following, cycle-guarded) under project `.claude/rules/` (git root) and user `~/.claude/rules/` (user loads first, project wins on same relative label). YAML `paths` frontmatter (block + inline list forms, brace patterns survive verbatim) splits rules two ways — no `paths` ⇒ EAGER (`rulesPromptBlock` → `` system-prompt block, byte-stable for prefix caching), with `paths` ⇒ JIT (`ruleAwareTool` decorates the read tool; reading a file whose project-root-relative path matches a glob appends the rule body to that tool result, once per rule per session, project-scope only). Globs reuse `agentGlobMatch` (doublestar + `{a,b}`). The same decorator ALSO injects project instruction files it walks past — but only ones the model has not already been given: `WrapContextAwareTools` seeds `seenCtxFile` with everything `AgentContextFiles(cwd)` already put in ``, so a read next to CLAUDE.md no longer re-sends CLAUDE.md. Only genuinely unseen instructions (a `CLAUDE.md` in a subdirectory *below* cwd) arrive JIT, once each. Do not remove that seeding — without it, one `read` of a 3KB file returned 99KB. | | `agent_tools_ask.go` | In-process twins of the bridge's `ask_user_question` / `end_turn` — same modal/workflow machinery, no HTTP loopback. | | `agent_tools_bridge.go`| Native twins of the `linear_*` bridge tools: a generic `nativeBridgeTool` adapter generates fantasy schemas via the same jsonschema machinery the MCP SDK uses (field docs survive verbatim) and wraps the shared cwd-parameterized cores in mcp_linear.go. In-process sessions never attach the loopback bridge. These tools live in the deferred registry, never on the wire. The adapter runs every schema through `flattenNullableTypes` (drops `"null"` from `type: ["null", X]` arrays) before handing it back — jsonschema-go emits nullable types for `*T` and `omitempty` slices, and fantasy's downstream `schema.Normalize` would otherwise rewrite them into an `anyOf` whose array branch carries its own `items: {}` while the parent keeps its real `items` (strict Moonshot / OpenAI validators reject the "conflicting keywords" shape). Same step applies to the workflow_* tools via the shared adapter. | -| `agent_tools_workflow.go`| The ask-built-in workflow tools on the CORE wire toolset: `workflow_list`, `workflow_get`, `workflow_create`, `workflow_edit`, `workflow_delete`, `workflow_copy`, and `clear_plans`. Built with the same `nativeBridgeTool` adapter (so wire schemas are byte-identical to the prior registry shape) and the shared cwd-parameterized cores in mcp_workflows.go. Deliberate, documented core exception: the two-stage workflow guard (agent_tools_todos.go) forces the model to call `workflow_list` as a precondition for any multi-step work, and an extra `search_tools` round-trip on every guard interaction is pure overhead. The disarm hook (`env.markWorkflowsChecked`) lives in the `workflow_list` closure itself so the guard clears on the direct call — they are NOT in `invoke_tool` anymore. Don't bypass the bridge adapter for a new workflow tool; the adapter's `flattenNullableTypes` pass is what keeps `workflow_create` / `workflow_edit` from emitting the type-arrays Moonshot's strict validator rejects. | +| `agent_tools_workflow.go`| The ask-built-in workflow tools on the CORE wire toolset: `workflow_list`, `workflow_get`, `workflow_create`, `workflow_edit`, `workflow_delete`, and `workflow_copy`. Built with the same `nativeBridgeTool` adapter (so wire schemas are byte-identical to the prior registry shape) and the shared cwd-parameterized cores in mcp_workflows.go. Deliberate, documented core exception: the two-stage workflow guard (agent_tools_todos.go) forces the model to call `workflow_list` as a precondition for any multi-step work, and an extra `search_tools` round-trip on every guard interaction is pure overhead. The disarm hook (`env.markWorkflowsChecked`) lives in the `workflow_list` closure itself so the guard clears on the direct call — they are NOT in `invoke_tool` anymore. Don't bypass the bridge adapter for a new workflow tool; the adapter's `flattenNullableTypes` pass is what keeps `workflow_create` / `workflow_edit` from emitting the type-arrays Moonshot's strict validator rejects. | | `agent_tools_registry.go`| The deferred tool registry surface: `search_tools` (query the registry — `*` / prefix-`*` / substring — returning name + description + full input_schema per match) and `invoke_tool` (dispatch a registry tool by name via its `.Run`, with replicated required-field validation, phrase injection for natives, and verbatim response pass-through). `unwrapInvokeToolCall` maps invoke calls back to the inner tool for display. See "Tool registry vs core tools" below — **new tools go here, never into the core list**, unless a deliberate, documented exception is in play (the two today are `web_search` and the workflow_* tools). | | `agent_memory.go` | Memory recall injection: session-start recall appended to the system prompt (once, byte-stable), per-prompt recall appended to the wire prompt, and `memoryAwareTool` wrapping read/edit/write with a per-file recall footer. All no-op when the memory service is closed. | | `agent_tools_mcp.go` | MCP client v2 (`mcpManager`/`mcpServerConn`): per-session manager over stdio/http/sse transports (official go-sdk v1.6.1), lazy ping-and-rebuild before every call + one renew-and-retry, `tools/list_changed` → live deferred-registry refresh (the wire toolset never changes mid-session), MCP elicitation → ask's question modal (form mode: enum/boolean/free-form, typed answers; URL mode + headless decline), image tool-results as real media when the model has vision. Tools are `mcp____`. | @@ -103,7 +103,10 @@ One `package main`, one file per concern. | `workflow_store.go` | Three-scope workflow persistence: user (ask.json) + repo (`/.ask/workflows/*.json`, committed) + global (`~/.config/ask/workflows/*.json`, machine-local, visible from every project); merged global-first listing (personal-wins), ambiguity-strict name resolution, dir sync on save, cross-scope copy. | | `workflows_screen.go` | Workflows builder screen — list/steps/step editor levels with multi-line prompt textarea. `e` on a selected workflow opens that same textarea (workflow-scoped `promptTarget=="description"`) to edit the workflow's free-text Description; it commits to `workflowDef.Description` and shows as the steps-pane subtitle. | | `workflows_picker.go` | Small centred modal popped on `f` (issues) / `Ctrl+F` (chat) to pick which workflow to run. | -| `workflows_run.go` | Step runner: prompt assembly, advance-on-turn-complete, finalise on done/failed. | +| `pkg/workflow/compile.go` | `CompileWorkflow` — a `Def` becomes an ADK graph: `AgentNode` per step, `loopagent`+`exitlooptool` per loop, `IncludeContentsNone` + `InstructionProvider` per step agent, per-node `RetryConfig`. Returns `Compiled` with the agent-name → step-index map the progress adapter needs. | +| `pkg/workflow/progress.go` | `Progress` — ADK events → `RunnerListener` callbacks, driven only by events that actually arrived. | +| `cmd/ask/workflow_graph.go` | TUI runner: one session for the whole graph, agent swapped for the compiled workflow. | +| `pkg/engine/workflow_run.go` | Headless runner + `WorkflowGraphAgent` + `IngestWorkflowMemory`. | | `workflow_source.go` | `workflowSource` tagged union (issue ref vs chat transcript) consumed by picker / runner / banner. | | `chat_workflow.go` | `Ctrl+F` dispatcher — snapshots `m.history` into a chat source, gates on busy/empty, opens the picker. | | `keymap.go` | Remappable global shortcuts — `Action` enum, `KeyBinding` parse/stringify, default keymap, `currentKeyMap()` cached accessor. Per-screen keys (kanban `j/k`, modal arrows, Ctrl+D close) stay inline; this only covers the global screen-switch + tab-nav surface. | @@ -159,9 +162,9 @@ exercised by the user; code alone won't catch layout regressions. | `agent_tools_mcp_test.go` | MCP manager against in-process `mcp.Server`s over httptest — attach/skip/schema/IsError, image results (placeholder vs media by vision), unreachable-server skip, `tools/list_changed` live refresh, dead-server graceful error, elicitation schema mapping + accept/cancel/headless/url flows. | | `mcp_servers_test.go` | Server-config resolution — effectiveType inference, `${VAR}`/`${VAR:-default}` expansion (copy semantics), 3-layer merge (.mcp.json ← global ← project) incl. Disabled tombstones + junk drops + stable order, tool allow/deny filters. | | `mcp_oauth_test.go` | OAuth plumbing — token path/0600 round-trip, persisting token source saves on change, callback listener captures code/state via swapped browser opener, stored-valid-token served without a flow, fresh handler yields nil source (transport 401s into Authorize). | -| `agent_tools_bridge_test.go`| Native linear twins — 12-tool coverage check, jsonschema field-doc fidelity, description-phrase injection (+ payload-description non-clobber), linear gate error, malformed input, loopback never in `agentSessionMCPServers`, `clear_plans` NOT in the linear set, every linear tool's wire schema is free of `anyOf`+`items` conflicts (Normalize-shape regression check). | -| `agent_tools_workflow_test.go`| Native workflow core tools — 8-tool coverage check, workflow CRUD round-trip against project config, workflow Description round-trip (create sets it, list/get surface it, edit replaces it, omitted leaves it unchanged across a rename), `clear_plans` idempotency, the workflow-guard disarm hooks (calling `workflow_list` sets `workflowsChecked`) so the two-stage todos guard clears on the direct path, plus the wire-schema shape tests (`flattenNullableTypes` table + Normalize-shape check) that pin `workflow_create.steps` / `workflow_edit.steps` / `workflow_edit.description` to the single-type shape strict validators accept. | -| `agent_tools_registry_test.go`| Tool registry — search query forms (`*`/prefix/substring, schema fidelity, sorted, no-match name list, empty registry), invoke dispatch (identity + params JSON), replicated required-field check, phrase injection (natives yes, MCP no), unknown/core-name errors, response pass-through (IsError/StopTurn/image/hard error), `unwrapInvokeToolCall`, `refreshToolset` wire/registry split (decorateTools sees core only), session surface (linear_* in the registry, `workflow_*` + `clear_plans` on the wire), `web_search` backend selection (no native spec → Brave on the wire + nil `providerWebSearch`; native spec → off the wire + `providerWebSearch` set), end-to-end fakeLM unwrap (toolCallMsg/toolResultMsg/status), loadHistory replay unwrap. | +| `agent_tools_bridge_test.go`| Native linear twins — 12-tool coverage check, jsonschema field-doc fidelity, description-phrase injection (+ payload-description non-clobber), linear gate error, malformed input, loopback never in `agentSessionMCPServers`, every linear tool's wire schema is free of `anyOf`+`items` conflicts (Normalize-shape regression check). | +| `agent_tools_workflow_test.go`| Native workflow core tools — 6-tool coverage check, workflow CRUD round-trip against project config, workflow Description round-trip (create sets it, list/get surface it, edit replaces it, omitted leaves it unchanged across a rename), the workflow-guard disarm hooks (calling `workflow_list` sets `workflowsChecked`) so the two-stage todos guard clears on the direct path, plus the wire-schema shape tests (`flattenNullableTypes` table + Normalize-shape check) that pin `workflow_create.steps` / `workflow_edit.steps` / `workflow_edit.description` to the single-type shape strict validators accept. | +| `agent_tools_registry_test.go`| Tool registry — search query forms (`*`/prefix/substring, schema fidelity, sorted, no-match name list, empty registry), invoke dispatch (identity + params JSON), replicated required-field check, phrase injection (natives yes, MCP no), unknown/core-name errors, response pass-through (IsError/StopTurn/image/hard error), `unwrapInvokeToolCall`, `refreshToolset` wire/registry split (decorateTools sees core only), session surface (linear_* in the registry, `workflow_*` on the wire), `web_search` backend selection (no native spec → Brave on the wire + nil `providerWebSearch`; native spec → off the wire + `providerWebSearch` set), end-to-end fakeLM unwrap (toolCallMsg/toolResultMsg/status), loadHistory replay unwrap. | | `skills_test.go` | Skills — discovery validation (bad name / dir mismatch / no description skipped) + project-over-global precedence, trigger block (progressive disclosure, hidden skills), `/name args` expansion incl. user-invocable gating, frontmatter parser, ProbeInit → slash entries. | | `rules_test.go` | `.claude/rules/` — `paths` frontmatter parsing (no-frontmatter/no-paths eager, block + inline list, brace verbatim, key-terminated list), eager/match split, recursive discovery + project-over-user precedence + non-md/empty-body skip, `rulesPromptBlock` (eager only, path attr), `ruleAwareTool` JIT injection + once-per-session dedup + non-match miss + eager exclusion, no-scoped-rules passthrough, `relPath` outside-root rejection. End-to-end eager block in `agent_prompt_test.go`. | | `agent_subagents_test.go` | Subagents — def discovery/precedence/field parsing, tool grant sets, spec registry, claude model aliases, cross-provider model resolution (swapped LM var), task tool: named agent runs on the pinned provider w/ def prompt + report tail, background job lifecycle (bgTask signals, job_output), default researcher unchanged, `/skill` expansion reaches the wire. | @@ -432,130 +435,107 @@ field. View consequences: failed before tearing down — the user closing the tab is the verdict, no graceful drain. -### Step runner - -`workflows_run.go` is the chain driver. Each step is a fresh -session (one-shot — the chain doesn't share a provider session -across steps; that's why workflow tabs don't pin a virtualSessionID -and why `providerDoneMsg.SessionID` is suppressed on workflow -tabs). The runner consumes the existing `sendToProvider` machinery -unchanged — it just sets `m.provider` / `m.providerModel` / clears -session state before the call. - -Step transitions are signalled through three existing message -handlers, hooked at the **end** of their existing logic so the -runner doesn't need to know about provider-specific stream shapes: - -- `turnCompleteMsg` (clean turn end) → `workflowAdvanceCmd(tabID, nil)` -- `providerDoneMsg` with `err != nil` or `IsError == true` → - `workflowAdvanceCmd(tabID, errStepError(...))` -- `providerExitedMsg` with non-nil err on a still-running run → - `workflowAdvanceCmd(tabID, errStepError(stderrTail))` - -The advance handler reads the step's `end_turn` report -(`pendingEndTurn`), appends the step's summary line to the visible -log, rolls the captured text into the appropriate context log, -kills the proc, mutates the cursor, and either dispatches -`workflowRunStartStepMsg` for the next step (deferred so the next -proc spawns at a clean Update boundary) or finalises (`done` on -chain end, `failed` on error). The cursor is `StepIdx` (top-level) -plus an optional `*loopRunFrame` while inside a loop — see "Loop -steps" below. Every step must call `end_turn`; a step that ends its -turn without it is re-prompted in place — see "Per-step `end_turn` -reporting" below. +### Execution: the ADK workflow graph + +A `workflowDef` compiles to an ADK workflow graph +(`pkg/workflow/compile.go`, `CompileWorkflow`). There is exactly ONE +workflow engine — the handwritten `Runner.Run` state machine, plus the +two dead parallel implementations (`RunGraph`, `BuildWorkflowAgent`), +were deleted. Don't add a second one. + +- A top-level agent step becomes an `AgentNode` wrapping an `llmagent`; + nodes are chained `Start -> n0 -> n1 -> …`. +- A `kind: "loop"` step becomes an `AgentNode` wrapping ADK's + `loopagent`, whose sub-agents are the inner steps, each carrying + `exitlooptool`. A step breaks the loop by calling `exit_loop` (it sets + `Actions.Escalate`, which is what `loopagent` watches for); otherwise + the loop runs to `MaxIterations`. There is no `decision` argument any + more — loop control is ADK's tool, not an `end_turn` field. +- `NodeConfig.RetryConfig` gives per-node retry, replacing the runner's + hand-rolled `stepErrorRetry` loop. + +Two `llmagent` settings carry the semantics and MUST NOT be dropped: + +- **`IncludeContents: IncludeContentsNone`** is what isolates a step. + Without it a step inherits the whole session, and ADK's + `ConvertForeignEvent` renders every prior step's events as prose — + every tool call and every full tool result — so step 3 would carry + steps 1 and 2 in full. With it, a step sees the handoff from the step + before it plus its own work. +- **`InstructionProvider`, never `llmagent.Config.Instruction`.** Step + prompts are user-authored and routinely contain braces; ADK + interpolates the static `Instruction` field against session state and + hard-fails the invocation on the first unknown `{name}`. + +Agent names are sanitised and de-duplicated by the compiler +(`agentNamer`): ADK requires them unique within a graph and rejects +`"user"`, while step names are free text and never checked. + +`*workflow.Workflow` is NOT an `agent.Agent` — the interface has an +unexported method — so `engine.WorkflowGraphAgent` wraps it via +`agent.New(agent.Config{Run: wf.Run})`. + +### Running a workflow + +One agent session runs the whole graph, not one session per step. + +- TUI: `cmd/ask/workflow_graph.go` starts a single session, swaps its + agent for the compiled graph (`agentSession.workflowAgent`), and + queues one turn. Tool execution, approvals, cost accounting, and + cancellation therefore behave exactly as in a chat turn. +- Headless: `engine.RunWorkflow` (`pkg/engine/workflow_run.go`) does the + same against its own `session.InMemoryService`. + +### Progress reporting + +`pkg/workflow/progress.go` turns the ADK event stream into +`RunnerListener` callbacks, and `agentSession.workflowProgress` feeds it +from inside the runner loop. Every callback is driven by something that +actually happened: a step starts when an event authored by its agent +arrives, and finishes when its successor starts or the run ends cleanly. +`Compiled.StepIndexByAgent` maps an event author back to the top-level +step, so a loop's inner agents report against the loop step the user +wrote. + +**Never fabricate step events.** The deleted `RunGraph` closed out every +step that never ran as both started AND done and hardcoded a successful +`FinishData`, so a chain that died at step 1 of 5 rendered 5/5 green. +`TestProgress_FailureDoesNotFabricateRemainingSteps` pins this. ### Per-step `end_turn` reporting -Every step (linear or loop-inner) must call the `end_turn` MCP tool -once per turn — it is the single source of the clean per-step output -*and* the loop control. The tool (`mcp_workflows.go`) takes a required -`summary` (1-3 sentences, rendered as the step's log line via -`stepSummaryLine`) and an optional `decision` (`continue`/`break`, only -meaningful in a loop). Like `ask_user_question` it blocks on an ack so -the report lands on `pendingEndTurn` before the turn ends; the runner -consumes it at `turnCompleteMsg` (see `handleEndTurnSignal`). - -A step that ends its turn **without** calling `end_turn` is re-prompted -in place — "hammered" until it registers, Ctrl+C being the manual -escape. The re-prompt feeds the step's own prior output back so it -doesn't redo the work (`linearText` for a linear step, -`loopRunFrame.retryText` inside a loop) and sets a `remindKind` -(`remindNoSummary` / `remindNoDecision`) so the injected reminder -explains itself. The banner shows the re-prompt count (`re-prompt #N`). - -### Loop steps - -A loop step (`Kind=="loop"`) runs its inner agent steps repeatedly -until a step registers a **break** (or `MaxIterations` is reached). -The runtime (`workflows_run.go`): - -- **Cursor.** `workflowRunState.loop` (`*loopRunFrame`) is non-nil - while inside a loop; it tracks `innerIdx`, `iteration` (1-based), - `retry`, and the bounded per-iteration context (`iterationLog`, - `prevTail`, `retryText`). `startWorkflowStep` enters the loop - (creates the frame) the first time `StepIdx` lands on a loop step; - `exitLoop` commits the final iteration's outputs to `stepLog` and - clears the frame. -- **Decision table** in `advanceWorkflowStep`, against the just- - finished inner step's `end_turn` report: no report → re-prompt the - same step; any step's `break` → exit the loop immediately (skipping - the rest of the iteration — an exceptional early exit); a non-tail - step with a summary and no break → next inner step; the **tail** - step's `continue` → next iteration (or soft-exit at the cap); the - tail with a summary but **no decision** → re-prompt the tail for one - (`remindNoDecision`). Only the tail is *required* to decide — non- - tail steps may break early but normally just summarise. -- **Bounded context** (`contextForDispatch`): linear steps see the - full `stepLog` (a re-prompted linear step also sees its own prior - output); inside a loop the linear log is frozen and the head inner - step additionally sees the previous iteration's tail output - (`prevTail`) while downstream steps see the current iteration's - prior outputs. A re-prompted inner step also carries its own prior - output (`retryText`). -- **Cap.** `MaxIterations==0` ⇒ `workflowLoopDefaultMaxIterations` - (10). Hitting the cap soft-exits (proceeds, never fails). -- **Instructions** are auto-injected by `buildWorkflowStepPrompt` via - `endTurnInstructionBlock` (the `*stepPromptCtx` arg): the universal - "call end_turn with a summary" contract, plus inside a loop the - iteration/goal banner and a position-aware decision clause (tail: - "you MUST also pass a decision"; non-tail: "omit decision unless - breaking early"). - -The `end_turn` tool is a native fantasy tool on every session -(agent_tools_ask.go), so step agents on any provider can call it. Live loop progress (start / iteration / break / -limit) is logged to the tab history via `loopNoteLine`, and the -banner's running line shows `⟳ · iter N/max · `. - -### Step prompt assembly - -`buildWorkflowStepPrompt(step, source, prevOutputs, pc)` produces the -full user turn for a step. `pc *stepPromptCtx` carries the loop framing -(`pc.loop` nil for linear steps) and the re-prompt reason (`pc.remind`): +Every step should call `end_turn` once with a `summary` (1-3 sentences); +it becomes the step's line in the workflow log via `stepSummaryLine`. +A step that ends without it is NOT re-prompted any more — the whole +remind/re-prompt machinery is gone — its log line falls back to the first +line of its own output. `finish_workflow` still reports the run's +outcome; the runner reads it from `env.PendingFinishData`. + +### Step instruction assembly + +`workflow.BuildStepInstruction(step, source, pc)` produces a step's +system instruction: ``` -Reference: +Reference: (or the chat transcript block) -Previous step output: (only when log is non-empty) - ---- - -... + (+ loop framing and exit_loop guidance inside a loop) +``` - (ALWAYS; loop framing + tail decision clause inside a loop) +Previous-step output is deliberately absent. Threading it into the prompt +was the old runner's job; the graph passes a node's output as the next +node's input, and `IncludeContentsNone` is what lets the step see it. - (only when pc.remind != remindNone) -``` +### No notes directories + +`ask/plans/` is gone, along with `plans.go`, `clear_plans`, and the +`RemindFixPlanDir` re-prompt. Step-to-step handoff is the graph's node +output. Durable "what we learned" goes to `pkg/memory` — `RunWorkflow` +calls `engine.IngestWorkflowMemory` on a clean finish, which is what the +notes directories were badly approximating. -Reference format is `#` (no provider prefix, no -URL); the agent has the issue-tracker MCP wired in and resolves the -rest itself. The `end_turn` contract (`endTurnInstructionBlock`) is -appended to **every** step — that's what makes the clean per-step -output possible, so unlike pre-`end_turn` workflows a linear step's -prompt is no longer byte-identical to the bare user prompt. -Whitespace is trimmed at the head and tail; the body stays as the -user wrote it. ### Builder screen (`Ctrl+W` / `/workflows`) @@ -739,8 +719,7 @@ agent_provider.go): task, the modal pair `ask_user_question`/`end_turn` (agent_tools_ask.go), the ask-built-in workflow tools (`workflow_list`/`workflow_get`/`workflow_create`/`workflow_edit`/ - `workflow_delete`/`workflow_copy` + `clear_plans` - in agent_tools_workflow.go), and the registry pair + `workflow_delete`/`workflow_copy` in agent_tools_workflow.go), and the registry pair `search_tools`/`invoke_tool` (agent_tools_registry.go). The workflow tools are a deliberate, documented core exception — see "Tool registry vs core tools" below. @@ -778,7 +757,7 @@ a core slot only by deliberate, documented exception, and the bar is "the agent cannot function without seeing it unprompted" — the registry pair itself, `end_turn` (the workflow runner's per-step contract), `fetch`, `web_search`, and the ask-built-in `workflow_*` -tools (plus `clear_plans`) are the canonical examples. The +tools are the canonical examples. The exceptions split into two flavours: `web_search` and the ask-built-in workflow tools are the *content* exceptions — an agent cannot function without seeing them unprompted. `web_search` has diff --git a/cmd/ask/agent_run.go b/cmd/ask/agent_run.go index 33fd808..2688066 100644 --- a/cmd/ask/agent_run.go +++ b/cmd/ask/agent_run.go @@ -13,7 +13,8 @@ import ( tea "charm.land/bubbletea/v2" "github.com/Cidan/ask/pkg/engine" "github.com/Cidan/ask/pkg/tools" - "google.golang.org/adk/v2/agent" + "github.com/Cidan/ask/pkg/workflow" + adkagent "google.golang.org/adk/v2/agent" "google.golang.org/adk/v2/agent/llmagent" adkmodel "google.golang.org/adk/v2/model" "google.golang.org/adk/v2/session" @@ -72,6 +73,13 @@ type agentSession struct { retryMaxRetries int retryInitialDelay time.Duration retryBackoffFactor float64 + + // workflowAgent, when set, replaces the ask_coder agent for this + // session's turns: the session runs a compiled workflow graph + // instead of a single coder agent. workflowProgress consumes the + // same ADK event stream to drive the workflow tab's step log. + workflowAgent adkagent.Agent + workflowProgress *workflow.Progress } func (s *agentSession) refreshToolset() { @@ -356,14 +364,19 @@ func (s *agentSession) runTurn(turn agentTurn) { toolsets = append(toolsets, skillTS) } - agentInstance, err := llmagent.New(llmagent.Config{ - Name: "ask_coder", - Model: llm, - InstructionProvider: instructionProvider, - Tools: adkTools, - Toolsets: toolsets, - GenerateContentConfig: genaiConfig, - }) + var agentInstance adkagent.Agent + if s.workflowAgent != nil { + agentInstance = s.workflowAgent + } else { + agentInstance, err = llmagent.New(llmagent.Config{ + Name: "ask_coder", + Model: llm, + InstructionProvider: instructionProvider, + Tools: adkTools, + Toolsets: toolsets, + GenerateContentConfig: genaiConfig, + }) + } if err != nil { s.emit(providerDoneMsg{ res: providerResult{SessionID: s.sessionID, IsError: true, Result: err.Error()}, @@ -404,7 +417,7 @@ func (s *agentSession) runTurn(turn agentTurn) { displayNames := make(map[string]string) backgroundCalls := make(map[string]bool) - for event, err := range r.Run(ctx, "user", s.sessionID, adkUserMsg, agent.RunConfig{}) { + for event, err := range r.Run(ctx, "user", s.sessionID, adkUserMsg, adkagent.RunConfig{}) { if err != nil { if isAgentCancel(err) { s.emit(providerDoneMsg{res: providerResult{SessionID: s.sessionID}}) @@ -421,6 +434,7 @@ func (s *agentSession) runTurn(turn agentTurn) { if event == nil { continue } + s.workflowProgress.Observe(event) if event.UsageMetadata != nil { usage := TokenUsage{ diff --git a/cmd/ask/aliases.go b/cmd/ask/aliases.go index 8e79792..34eb4de 100644 --- a/cmd/ask/aliases.go +++ b/cmd/ask/aliases.go @@ -84,10 +84,6 @@ func currentWorkflowStepMeta(r *workflowRunState) (name, provider, model string) return "", "", "" } top := r.Workflow.Steps[r.StepIdx] - if r.loop != nil && top.IsLoop() && r.loop.innerIdx < len(top.Steps) { - inner := top.Steps[r.loop.innerIdx] - return inner.Name, inner.Provider, inner.Model - } return top.Name, top.Provider, top.Model } @@ -116,20 +112,12 @@ func toPkgWorkflowStep(s workflowStep) workflow.Step { } } -func buildWorkflowStepPrompt(step workflowStep, source workflowSource, prevOutputs []string, pc *stepPromptCtx) string { - return workflow.BuildStepPrompt(toPkgWorkflowStep(step), source, prevOutputs, pc) +func buildWorkflowStepInstruction(step workflowStep, source workflowSource, pc *stepPromptCtx) string { + return workflow.BuildStepInstruction(toPkgWorkflowStep(step), source, pc) } var ( - stepNotesDir = workflow.StepNotesDir - startPlanDir = workflow.StartPlanDir - ensureStartPlanExists = workflow.EnsureStartPlanExists - ensureStepNotesDir = workflow.EnsureStepNotesDir - removeAllWorkflowPlans = workflow.RemoveAllWorkflowPlans - clearWorkflowPlans = workflow.ClearWorkflowPlans - workflowPlansDir = workflow.PlansDir - loopNoteLine = workflow.LoopNoteLine - sanitizeStepName = workflow.SanitizeStepName + loopNoteLine = workflow.LoopNoteLine ) func lastOf(s []string) string { @@ -167,7 +155,6 @@ var ( unwrapInvokeToolCall = tools.UnwrapInvokeToolCall runAskPassHelper = tools.RunAskPassHelper applyBashFilter = tools.ApplyBashFilter - clearPlansCore = tools.ClearPlansCore agentAskUserQuestionTool = tools.AskUserQuestionTool agentEndTurnTool = tools.EndTurnTool agentFinalizedPlanTool = tools.FinalizedPlanTool @@ -191,15 +178,6 @@ var ( agentPreloadMemoryTool = tools.PreloadMemoryTool ) -const ( - clearPlansToolDescription = tools.ClearPlansToolDescription -) - -type ( - clearPlansInput = tools.ClearPlansInput - clearPlansOutput = tools.ClearPlansOutput -) - func errResult(text string) *mcp.CallToolResult { return &mcp.CallToolResult{ IsError: true, diff --git a/cmd/ask/chat_workflow_test.go b/cmd/ask/chat_workflow_test.go index 52b78e1..0ccab8a 100644 --- a/cmd/ask/chat_workflow_test.go +++ b/cmd/ask/chat_workflow_test.go @@ -129,7 +129,7 @@ func TestWorkflowSource_RefBlock_ChatFormat(t *testing.T) { // TestWorkflowSource_RefBlock_EmptyChatReturnsEmpty guards the // "skip the section entirely" path — an empty transcript must -// not emit a dangling header. buildWorkflowStepPrompt relies on +// not emit a dangling header. buildWorkflowStepInstruction relies on // this to drop the reference block when there's nothing to // reference. func TestWorkflowSource_RefBlock_EmptyChatReturnsEmpty(t *testing.T) { @@ -151,11 +151,10 @@ func TestWorkflowSource_RefBlock_IssueFormat(t *testing.T) { } } -// TestBuildWorkflowStepPrompt_ChatSource verifies the prompt -// assembly for a chat-sourced workflow. Step 0 should carry the -// transcript reference; a later step should layer the previous-step -// output block under the same reference. -func TestBuildWorkflowStepPrompt_ChatSource(t *testing.T) { +// TestBuildWorkflowStepInstruction_ChatSource verifies instruction +// assembly for a chat-sourced workflow: the transcript reference rides +// along with the author's prompt and the end_turn contract. +func TestBuildWorkflowStepInstruction_ChatSource(t *testing.T) { step := workflowStep{Prompt: "Summarise."} source := workflowSource{ Kind: workflowSourceChat, @@ -165,36 +164,22 @@ func TestBuildWorkflowStepPrompt_ChatSource(t *testing.T) { }, } - step0 := buildWorkflowStepPrompt(step, source, nil, nil) - if !strings.Contains(step0, "Summarise.") { - t.Errorf("step 0 must include user prompt; got %q", step0) - } - if !strings.Contains(step0, "Reference (chat transcript):") { - t.Errorf("step 0 must include chat transcript header; got %q", step0) - } - if !strings.Contains(step0, "user: what's a goroutine?") { - t.Errorf("step 0 must include user turn; got %q", step0) - } - if !strings.Contains(step0, "assistant: a green-thread primitive.") { - t.Errorf("step 0 must include assistant turn; got %q", step0) - } - if strings.Contains(step0, "Previous step output:") { - t.Errorf("step 0 must NOT include previous-step block; got %q", step0) - } - if strings.Contains(step0, "Reference: ") { - // Make sure we didn't accidentally emit the issue-style line. - t.Errorf("chat source must NOT emit issue-style Reference line; got %q", step0) - } - - stepN := buildWorkflowStepPrompt(step, source, []string{"prior step output text"}, nil) - if !strings.Contains(stepN, "Previous step output:") { - t.Errorf("step N must include previous-step block; got %q", stepN) + got := buildWorkflowStepInstruction(step, source, nil) + for _, want := range []string{ + "Summarise.", + "Reference (chat transcript):", + "user: what's a goroutine?", + "assistant: a green-thread primitive.", + } { + if !strings.Contains(got, want) { + t.Errorf("instruction missing %q; got %q", want, got) + } } - if !strings.Contains(stepN, "prior step output text") { - t.Errorf("step N must include the log entry; got %q", stepN) + if strings.Contains(got, "Previous step output:") { + t.Errorf("previous-step threading is the graph's job now; got %q", got) } - if !strings.Contains(stepN, "Reference (chat transcript):") { - t.Errorf("step N must still include chat transcript; got %q", stepN) + if strings.Contains(got, "Reference: ") { + t.Errorf("chat source must not emit the issue-style Reference line; got %q", got) } } diff --git a/cmd/ask/coordinator.go b/cmd/ask/coordinator.go index bc1f2a2..83492bd 100644 --- a/cmd/ask/coordinator.go +++ b/cmd/ask/coordinator.go @@ -2,9 +2,6 @@ package main import ( "context" - "errors" - "fmt" - "strings" "sync" tea "charm.land/bubbletea/v2" @@ -280,99 +277,8 @@ func (l tuiWorkflowListener) OnNote(tabID int, text string) { }) } -// ExecuteStep implements workflow.StepExecutor for Coordinator. -func (c *Coordinator) ExecuteStep(ctx context.Context, cwd string, tabID int, step workflow.Step, prompt string, isFinal bool) (workflow.StepResult, error) { - prov := providerByID(step.Provider) - if prov == nil { - return workflow.StepResult{}, fmt.Errorf("provider not registered: %s", step.Provider) - } - - args := ProviderSessionArgs{ - Cwd: cwd, - TabID: tabID, - Model: step.Model, - Effort: "medium", - SkipAllPermissions: true, - InWorkflow: true, - IsWorkflowFinalStep: isFinal, - } - - proc, ch, err := prov.StartSession(args) - if err != nil { - return workflow.StepResult{}, err - } - - session, ok := proc.payload.(*agentSession) - if !ok { - return workflow.StepResult{}, errors.New("proc payload is not an agent session") - } - c.SetSession(tabID, session) - - err = session.queueTurn(prompt) - if err != nil { - session.shutdown() - c.RemoveSession(tabID) - return workflow.StepResult{}, err - } - - var stepResult string - var stepErr error -stepLoop: - for msg := range ch { - switch m := msg.(type) { - case assistantTextMsg: - stepResult += m.text - case providerDoneMsg: - if m.err != nil { - stepErr = m.err - } else if m.res.IsError { - stepErr = fmt.Errorf("step failed: %s", m.res.Result) - } else { - stepResult = m.res.Result - } - case turnCompleteMsg: - break stepLoop - } - } - - session.shutdown() - c.RemoveSession(tabID) - - if stepErr != nil { - return workflow.StepResult{}, stepErr - } - - summary := "" - decision := "" - if session.env.PendingEndTurn != nil { - summary = session.env.PendingEndTurn.Summary - decision = session.env.PendingEndTurn.Decision - } - if summary == "" && strings.TrimSpace(stepResult) != "" { - firstLine := strings.TrimSpace(strings.Split(strings.TrimSpace(stepResult), "\n")[0]) - if len(firstLine) > 200 { - firstLine = firstLine[:200] + "…" - } - summary = firstLine - } - - var finishData *workflow.FinishData - if session.env.PendingFinishData != nil { - finishData = &workflow.FinishData{ - Description: session.env.PendingFinishData.Description, - Artifacts: session.env.PendingFinishData.Artifacts, - } - } - - return workflow.StepResult{ - Output: stepResult, - Summary: summary, - Decision: decision, - FinishData: finishData, - }, nil -} - -// RunWorkflow executes a workflow synchronously step by step in the background. +// RunWorkflow compiles the definition to an ADK workflow graph and drives +// it to completion in the background. func (c *Coordinator) RunWorkflow(ctx context.Context, tabID int, def workflowDef, src workflowSource) (finalizedPlanReply, error) { ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -407,8 +313,7 @@ func (c *Coordinator) RunWorkflow(ctx context.Context, tabID int, def workflowDe } listener := tuiWorkflowListener{tabID: tabID} - runner := workflow.NewRunner(workflow.GlobalTracker(), c, listener) - runState, err := runner.Run(ctx, rootCwd, tabID, toPkgWorkflowDef(def), src) + runState, err := c.runWorkflowGraph(ctx, rootCwd, tabID, toPkgWorkflowDef(def), src, listener) if err != nil { return finalizedPlanReply{}, err } diff --git a/cmd/ask/coordinator_test.go b/cmd/ask/coordinator_test.go index 8672a66..543a337 100644 --- a/cmd/ask/coordinator_test.go +++ b/cmd/ask/coordinator_test.go @@ -1,23 +1,20 @@ package main import ( + "sync/atomic" "context" "errors" - "iter" "os" "path/filepath" - "strings" - "sync" "testing" "time" tea "charm.land/bubbletea/v2" - "github.com/Cidan/ask/pkg/engine" - "google.golang.org/genai" ) func TestCoordinator_RunWorkflowRestoreSession(t *testing.T) { isolateHome(t) + stubWorkflowStepModel(t) // Create a fake provider prov := newFakeProvider() @@ -112,6 +109,7 @@ func TestCoordinator_RunWorkflowRestoreSession(t *testing.T) { func TestCoordinator_RunWorkflowCancellationStopRetries(t *testing.T) { isolateHome(t) + stubWorkflowStepModel(t) prov := newFakeProvider() prov.id = "fake-prov" @@ -208,286 +206,54 @@ func TestCoordinator_RunWorkflowCancellationStopRetries(t *testing.T) { } } -func TestCoordinator_RunWorkflowMissingPlanDirReminder(t *testing.T) { +// The graph engine runs a whole workflow on ONE agent session: the +// session's agent is the compiled graph, and ADK's scheduler walks the +// nodes. The old runner opened a fresh provider session per step, so a +// three-step workflow started three of them. +func TestCoordinator_RunWorkflowUsesOneSessionForTheWholeGraph(t *testing.T) { isolateHome(t) + stubWorkflowStepModel(t) + var sessionsStarted int32 prov := newFakeProvider() prov.id = "fake-prov" - - var receivedPrompt string prov.startSessionFn = func(args ProviderSessionArgs) (*providerProc, chan tea.Msg, error) { + atomic.AddInt32(&sessionsStarted, 1) ch := make(chan tea.Msg, 8) - proc := &providerProc{ - stdin: &bufferCloser{Buffer: nil}, - } - env := newAgentToolEnv(args.Cwd, args.TabID, true, true, func(msg tea.Msg) {}) - env.PendingEndTurn = &endTurnSignal{Summary: "step completed", Decision: "break"} - env.PendingFinishData = &finishWorkflowData{Description: "completed successfully", Artifacts: []string{"art1"}} - sess := &agentSession{ + proc := &providerProc{stdin: &bufferCloser{Buffer: nil}} + proc.payload = &agentSession{ args: args, env: env, sendCh: make(chan agentTurn, 8), closed: make(chan struct{}), } - proc.payload = sess - go func() { - select { - case turn := <-sess.sendCh: - receivedPrompt = turn.text - case <-time.After(500 * time.Millisecond): - } - }() - - go func() { - time.Sleep(10 * time.Millisecond) - ch <- assistantTextMsg{text: "step result"} - ch <- providerDoneMsg{ - res: providerResult{ - Result: "done", - }, - } + ch <- providerDoneMsg{res: providerResult{Result: "ok"}} + ch <- turnCompleteMsg{} close(ch) }() - return proc, ch, nil } - - withRegisteredProviders(t, prov) - - cwd := t.TempDir() - - parentSess := &agentSession{ - args: ProviderSessionArgs{TabID: 43, Cwd: cwd}, - } - parentSess.env = newAgentToolEnv(parentSess.args.Cwd, 43, true, true, func(msg tea.Msg) {}) - - c := globalCoordinator - c.SetSession(43, parentSess) - - def := workflowDef{ - Name: "test-wf", - Steps: []workflowStep{ - { - Name: "step-1", - Provider: "fake-prov", - Model: "fake-model", - Prompt: "do something", - }, - }, - } - src := workflowSource{Kind: workflowSourceChat} - - reply, err := c.RunWorkflow(context.Background(), 43, def, src) - if err != nil { - t.Fatalf("expected workflow to complete, got err: %v", err) - } - - if !reply.workflowDone { - t.Errorf("expected workflow to be marked done") - } - - wantSub := "REMINDER: the workflow notes directory is not usable" - if receivedPrompt == "" { - t.Errorf("did not receive any prompt") - } else if !strings.Contains(receivedPrompt, wantSub) { - t.Errorf("expected prompt to contain %q, but got:\n%s", wantSub, receivedPrompt) - } -} - -func TestCoordinator_RunWorkflowLoopWithDecisionAndFinish(t *testing.T) { - isolateHome(t) - - // Since we are running the workflow, we can override agentSendToProgram - // to capture the message or return true. - oldSend := agentSendToProgram - agentSendToProgram = func(msg tea.Msg) bool { return true } - t.Cleanup(func() { agentSendToProgram = oldSend }) - - origStream := engine.GenerateStream - defer func() { engine.GenerateStream = origStream }() - - var turnIdx int - var mu sync.Mutex - engine.GenerateStream = func(ctx context.Context, client *genai.Client, model string, contents []*genai.Content, config *genai.GenerateContentConfig) iter.Seq2[*genai.GenerateContentResponse, error] { - mu.Lock() - idx := turnIdx - turnIdx++ - mu.Unlock() - - var chunk *genai.GenerateContentResponse - switch idx { - case 0: - chunk = genaiToolCallChunk("read", map[string]any{"file_path": "ask/plans/start/plan.txt", "description": "read start plan"}) - case 1: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "validated the plan"}) - case 2: - chunk = genaiTextChunk("completed", 10, 10) - case 3: - chunk = genaiToolCallChunk("write", map[string]any{"file_path": "hello.go", "content": "package main\n\nfunc main() {}\n", "description": "create hello.go with main function"}) - case 4: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "implemented changes"}) - case 5: - chunk = genaiTextChunk("completed", 10, 10) - case 6: - chunk = genaiToolCallChunk("read", map[string]any{"file_path": "hello.go", "description": "verify hello.go before continuing"}) - case 7: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "validated changes", "decision": "continue"}) - case 8: - chunk = genaiTextChunk("completed", 10, 10) - case 9: - chunk = genaiToolCallChunk("read", map[string]any{"file_path": "hello.go", "description": "read hello.go before editing"}) - case 10: - chunk = genaiToolCallChunk("edit", map[string]any{"file_path": "hello.go", "old_string": "package main\n\nfunc main() {}\n", "new_string": "package main\n\nimport \"fmt\"\n\nfunc main() {\n\tfmt.Println(\"hello\")\n}\n", "description": "add print to main"}) - case 11: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "implemented more changes"}) - case 12: - chunk = genaiTextChunk("completed", 10, 10) - case 13: - chunk = genaiToolCallChunk("read", map[string]any{"file_path": "hello.go", "description": "verify hello.go before break"}) - case 14: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "validated and broke loop", "decision": "break"}) - case 15: - chunk = genaiTextChunk("completed", 10, 10) - case 16: - chunk = genaiToolCallChunk("finish_workflow", map[string]any{"description": "workflow executed successfully", "artifacts": []any{"hello.go"}}) - case 17: - chunk = genaiToolCallChunk("end_turn", map[string]any{"summary": "finalized the workflow"}) - case 18: - chunk = genaiTextChunk("completed", 10, 10) - default: - chunk = genaiTextChunk("done", 10, 10) - } - - return func(yield func(*genai.GenerateContentResponse, error) bool) { - yield(chunk, nil) - } - } - - prov := newFakeProvider() - prov.id = "fake-prov" - - stepCount := 0 - prov.startSessionFn = func(args ProviderSessionArgs) (*providerProc, chan tea.Msg, error) { - stepCount++ - - sess := &agentSession{ - args: args, - system: "test system prompt", - contextWindow: 1_048_576, - modelID: "fake-model", - ch: make(chan tea.Msg, 32), - sendCh: make(chan agentTurn, 8), - closed: make(chan struct{}), - sessionID: "ses-test", - } - cfg, _ := loadConfig() - sess.env = newAgentToolEnv(args.Cwd, args.TabID, true, false, sess.emit) - setupAgentSessionTools(sess, cfg) - sess.tools = sess.coreTools - - proc := &providerProc{ - stdin: agentStdin{s: sess}, - stderr: &stderrBuf{}, - payload: sess, - } - sess.proc = proc - go sess.run() - return proc, sess.ch, nil - } - withRegisteredProviders(t, prov) cwd := t.TempDir() - startDir := filepath.Join(cwd, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.txt"), []byte("dummy plan"), 0o644); err != nil { - t.Fatal(err) - } - - parentSess := &agentSession{ - args: ProviderSessionArgs{TabID: 44, Cwd: cwd}, - } - parentSess.env = newAgentToolEnv(parentSess.args.Cwd, 44, true, true, func(msg tea.Msg) {}) - c := globalCoordinator - c.SetSession(44, parentSess) - - def := workflowDef{ - Name: "ship", - Steps: []workflowStep{ - { - Name: "Validate plan", - Provider: "fake-prov", - Model: "fake-model", - Prompt: "validate plan", - }, - { - Name: "Loop: Execute/Validate", - Kind: "loop", - Steps: []workflowStep{ - { - Name: "Implement changes", - Provider: "fake-prov", - Model: "fake-model", - Prompt: "implement", - }, - { - Name: "Validate changes", - Provider: "fake-prov", - Model: "fake-model", - Prompt: "validate", - }, - }, - MaxIterations: 3, - }, - { - Name: "Finalize", - Provider: "fake-prov", - Model: "fake-model", - Prompt: "finalize", - }, - }, - } - src := workflowSource{Kind: workflowSourceChat} - - reply, err := c.RunWorkflow(context.Background(), 44, def, src) - if err != nil { - t.Fatalf("expected workflow to complete, got err: %v", err) - } - - if !reply.workflowDone { - t.Errorf("expected workflow to be marked done") - } - - if stepCount != 6 { - t.Errorf("expected exactly 6 steps to run, got %d", stepCount) - } - - if reply.outcome != "workflow executed successfully" { - t.Errorf("expected outcome to be 'workflow executed successfully', got %q", reply.outcome) - } - - var foundFiles []string - _ = filepath.Walk(cwd, func(path string, info os.FileInfo, err error) error { - if !info.IsDir() { - foundFiles = append(foundFiles, path) - } - return nil - }) - t.Logf("Found files in cwd: %v", foundFiles) - - // Verify file was written, read, edited, and validated correctly on disk! - helloPath := filepath.Join(cwd, "hello.go") - content, err := os.ReadFile(helloPath) - if err != nil { - t.Fatalf("expected hello.go to be written, got err: %v", err) - } - wantContent := "package main\n\nimport \"fmt\"\n\nfunc main() {\n\tfmt.Println(\"hello\")\n}\n" - if string(content) != wantContent { - t.Errorf("hello.go has wrong content:\ngot:\n%q\nwant:\n%q", string(content), wantContent) + parent := &agentSession{args: ProviderSessionArgs{TabID: 77, Cwd: cwd}} + parent.env = newAgentToolEnv(cwd, 77, true, true, func(msg tea.Msg) {}) + c.SetSession(77, parent) + defer c.RemoveSession(77) + + def := workflowDef{Name: "three-step", Steps: []workflowStep{ + {Name: "one", Provider: "fake-prov", Model: "fake-model", Prompt: "a"}, + {Name: "two", Provider: "fake-prov", Model: "fake-model", Prompt: "b"}, + {Name: "three", Provider: "fake-prov", Model: "fake-model", Prompt: "c"}, + }} + + if _, err := c.RunWorkflow(context.Background(), 77, def, workflowSource{Kind: workflowSourceChat}); err != nil { + t.Fatalf("RunWorkflow: %v", err) + } + if got := atomic.LoadInt32(&sessionsStarted); got != 1 { + t.Errorf("workflow started %d provider sessions, want exactly 1 for the whole graph", got) } } diff --git a/cmd/ask/finalized_plan_test.go b/cmd/ask/finalized_plan_test.go index ce0eab7..2cfb6e1 100644 --- a/cmd/ask/finalized_plan_test.go +++ b/cmd/ask/finalized_plan_test.go @@ -338,13 +338,14 @@ func TestFinalizedPlan_WorkflowSelectionToolNoCmd(t *testing.T) { func TestFinalizedPlan_SelfLaunchWorkflowExecution(t *testing.T) { isolateHome(t) + stubWorkflowStepModel(t) cwd := t.TempDir() - var stepsExecuted int32 + var sessionsStarted int32 prov := newFakeProvider() prov.id = "fake-prov" prov.startSessionFn = func(args ProviderSessionArgs) (*providerProc, chan tea.Msg, error) { - atomic.AddInt32(&stepsExecuted, 1) + atomic.AddInt32(&sessionsStarted, 1) ch := make(chan tea.Msg, 8) proc := &providerProc{ stdin: &bufferCloser{Buffer: nil}, @@ -419,8 +420,9 @@ func TestFinalizedPlan_SelfLaunchWorkflowExecution(t *testing.T) { t.Fatalf("tool run failed: %s", resp.Content) } - if atomic.LoadInt32(&stepsExecuted) == 0 { - t.Fatalf("expected workflow steps to be executed, but none were run") + // The graph engine runs the whole workflow on one session. + if atomic.LoadInt32(&sessionsStarted) != 1 { + t.Fatalf("expected exactly one workflow session, got %d", atomic.LoadInt32(&sessionsStarted)) } if !strings.Contains(resp.Content, "completed ship workflow") { @@ -430,6 +432,7 @@ func TestFinalizedPlan_SelfLaunchWorkflowExecution(t *testing.T) { func TestFinalizedPlan_SelfLaunchWorkflowExecution_ClearsUIWorkflowRunState(t *testing.T) { isolateHome(t) + stubWorkflowStepModel(t) cwd := t.TempDir() prov := newFakeProvider() diff --git a/cmd/ask/testhelpers_test.go b/cmd/ask/testhelpers_test.go index 4a93248..8aab468 100644 --- a/cmd/ask/testhelpers_test.go +++ b/cmd/ask/testhelpers_test.go @@ -16,6 +16,7 @@ import ( "charm.land/bubbles/v2/textarea" tea "charm.land/bubbletea/v2" "github.com/Cidan/ask/pkg/tools" + "github.com/Cidan/ask/pkg/workflow" adkmodel "google.golang.org/adk/v2/model" ) @@ -406,3 +407,22 @@ func writeFile(t *testing.T, path, content string) { t.Fatalf("write %s: %v", path, err) } } + +// stubWorkflowStepModel swaps the workflow compiler's model resolution +// for an inert LLM so a test can compile and run a graph without a real +// provider registered in pkg/providers. +func stubWorkflowStepModel(t *testing.T) { + t.Helper() + prev := workflowStepModel + workflowStepModel = func(ctx context.Context, sess *agentSession, step workflow.Step) (adkmodel.LLM, error) { + return &stubStepLLM{}, nil + } + t.Cleanup(func() { workflowStepModel = prev }) +} + +type stubStepLLM struct{} + +func (s *stubStepLLM) Name() string { return "stub-step-model" } +func (s *stubStepLLM) GenerateContent(ctx context.Context, req *adkmodel.LLMRequest, stream bool) iter.Seq2[*adkmodel.LLMResponse, error] { + return func(yield func(*adkmodel.LLMResponse, error) bool) {} +} diff --git a/cmd/ask/types.go b/cmd/ask/types.go index e55866f..b8fc9a4 100644 --- a/cmd/ask/types.go +++ b/cmd/ask/types.go @@ -218,8 +218,6 @@ type toolResultMsg struct { proc *providerProc } - - type stderrBuf struct { mu sync.Mutex data []byte @@ -460,7 +458,7 @@ type model struct { pathMatches []string pathIdx int - status string + status string // Preserved solely for backward-compatibility in tests. // Unused and unreferenced in the main application code. @@ -616,7 +614,7 @@ type model struct { // next user turn relaunches with these wired in via --resume. addedDirs []string - turnBuffer []string + turnBuffer []string responseActive bool lastContentFP string @@ -745,12 +743,6 @@ type workflowRunState struct { // it points at the loop step and `loop` carries the inner position. StepIdx int - // loop is non-nil while the runner is executing inside a loop step - // (Workflow.Steps[StepIdx].isLoop()). It tracks the inner cursor, - // iteration count, and the bounded per-iteration context. Nil for - // the linear portions of the chain. - loop *loopRunFrame - // pendingEndTurn holds what the current step registered via the // end_turn MCP tool this turn: the always-required summary plus, in a // loop, the optional break/continue decision. Reset to nil at every @@ -760,52 +752,10 @@ type workflowRunState struct { // re-prompted (every step must call end_turn). pendingEndTurn *endTurnSignal - // linearRetry / linearText are the re-prompt bookkeeping for the - // non-loop portion of the chain — the mirror of loopRunFrame's - // retry / retryText. linearRetry counts how many times the current - // linear step has been re-prompted for a missing end_turn call; - // linearText stashes its prior output so the re-prompt can feed it - // back rather than make the step redo the work. Both reset when the - // linear step finally registers and advances. - linearRetry int - linearText string - - // stepErrorRetry tracks how many times the current step has been retried - // after failing with a step error. Reset to 0 when a step succeeds. - stepErrorRetry int - - // remind records why the current dispatch is a re-prompt (a missing - // end_turn call, a loop tail that omitted its decision, or a plan - // directory that is not usable) so buildWorkflowStepPrompt can append - // the matching reminder. remindNone on a normal first dispatch. - remind remindKind - - // remindDetail carries dynamic text for remindFixPlanDir so the LLM - // knows exactly which path is the wrong shape and what to do about it. - remindDetail string - - // stepLog accumulates the assistant non-tool text emitted by each - // completed top-level step so the next step's prompt can include a - // `Previous step output:` block. A loop contributes only its final - // iteration's inner outputs here, on exit — intermediate iterations - // stay scoped to the loop frame so the linear log doesn't balloon. - stepLog []string - - // currentStep accumulates assistantTextMsg payloads for the - // in-flight step. Rolled into the appropriate log on - // workflowRunStepDoneMsg. + // currentStep accumulates the assistant text emitted by the + // in-flight step, used for the step's log line. currentStep strings.Builder - // currentNotesDir is the notes directory for the step currently - // in flight. Computed at dispatch and reused when advancing so the - // runner knows which directory the just-finished step wrote to. - currentNotesDir string - - // prevNotesDir is the notes directory of the step whose output is - // being carried into the current dispatch. Empty for the workflow's - // first step. - prevNotesDir string - // done flips true once the final step exits cleanly. The banner // shows "complete · ctrl+d to close" while the tab stays open. done bool @@ -848,41 +798,6 @@ type workflowTabSnapshot struct { screen screenID } -// loopRunFrame is the per-loop execution cursor, live only while the -// runner is inside a loop step. innerIdx walks the loop's inner steps; -// iteration is 1-based. The three text fields implement the bounded -// context policy: an inner step sees the linear log (frozen at loop -// entry) plus, for the head step, the previous iteration's tail output, -// or for downstream steps, the current iteration's prior outputs. -type loopRunFrame struct { - innerIdx int - iteration int - - // retry counts consecutive re-prompts of the current inner step - // within this iteration. Bumped each time the step finishes without - // the end_turn call the runner needs (any step that skips end_turn, - // or a tail that omits its decision) — the runner re-dispatches it, - // "hammering" until it registers. Surfaced in the banner; reset when - // the inner cursor advances, the iteration advances, or the loop - // exits. - retry int - - // iterationLog holds the inner-step outputs produced so far in the - // current iteration, in order. Reset at each iteration boundary; - // committed to the run's stepLog when the loop exits. - iterationLog []string - - // prevTail is the last inner step's output from the previous - // iteration, fed to the head step so a kick-back actually reaches - // the next pass. Empty on iteration 1. - prevTail string - - // retryText is the inner step's most recent output when it finished - // without the end_turn call the runner needed, fed back into the - // re-prompt so the agent can register without redoing the work. - retryText string -} - // endTurnSignal is what a step registers via the end_turn MCP tool at // the close of its turn. summary is the always-required 1-3 sentence // account of what the step did — it becomes the step's line in the @@ -985,16 +900,6 @@ type ClearWorkflowStateMsg struct { TabID int } -type remindKind int - -const ( - remindNone remindKind = iota - remindNoSummary // the prior turn ended without calling end_turn - remindNoDecision // a loop tail called end_turn but omitted its decision - remindFixPlanDir // a workflow notes directory is missing or is a file - remindNoFinishTool // the prior turn ended the workflow without calling finish_workflow -) - type AppendHistoryMsg struct { TabID int Text string diff --git a/cmd/ask/virtual_session.go b/cmd/ask/virtual_session.go index ff5b2bd..ae037f5 100644 --- a/cmd/ask/virtual_session.go +++ b/cmd/ask/virtual_session.go @@ -355,18 +355,14 @@ func (m *model) recordVirtualSession(nativeID string) { return } stepName, stepProvider, _ := currentWorkflowStepMeta(m.workflowRun) - var loopIteration, loopInnerIdx int - if m.workflowRun.loop != nil { - loopIteration = m.workflowRun.loop.iteration - loopInnerIdx = m.workflowRun.loop.innerIdx - } + // Loop position is no longer tracked here: ADK's loopagent owns + // iteration state inside the graph node, so the tab records the + // top-level step only. step := VirtualSessionWorkflowStep{ - StepIdx: m.workflowRun.StepIdx, - StepName: stepName, - ProviderID: stepProvider, - LoopIteration: loopIteration, - LoopInnerIdx: loopInnerIdx, - Session: ProviderSessionRef{SessionID: nativeID, Cwd: nativeCwd}, + StepIdx: m.workflowRun.StepIdx, + StepName: stepName, + ProviderID: stepProvider, + Session: ProviderSessionRef{SessionID: nativeID, Cwd: nativeCwd}, } err := mutateVirtualSessions(func(store *virtualSessionStore) error { diff --git a/cmd/ask/virtual_session_test.go b/cmd/ask/virtual_session_test.go index 8f9d601..27ca361 100644 --- a/cmd/ask/virtual_session_test.go +++ b/cmd/ask/virtual_session_test.go @@ -1849,6 +1849,9 @@ func TestRecordVirtualSession_WorkflowRunAppendsStepsToSameRun(t *testing.T) { } } +// A loop step records against its top-level index. Loop iteration state +// now lives inside ADK's loopagent node, so the tab no longer tracks an +// inner cursor of its own. func TestRecordVirtualSession_WorkflowRunLoopStep(t *testing.T) { isolateHome(t) m := newTestModel(t, newFakeProvider()) @@ -1860,7 +1863,6 @@ func TestRecordVirtualSession_WorkflowRunLoopStep(t *testing.T) { startedAt: time.Now().UTC(), StepIdx: 0, Source: workflowSource{Kind: workflowSourceChat}, - loop: &loopRunFrame{iteration: 2, innerIdx: 0}, } store := &virtualSessionStore{Version: 2} @@ -1871,8 +1873,11 @@ func TestRecordVirtualSession_WorkflowRunLoopStep(t *testing.T) { got, _ := loadVirtualSessions() vs := got.findByID("vs-1") step := vs.WorkflowRuns[0].Steps[0] - if step.LoopIteration != 2 || step.LoopInnerIdx != 0 { - t.Errorf("Loop info wrong: %d %d", step.LoopIteration, step.LoopInnerIdx) + if step.StepIdx != 0 { + t.Errorf("loop step should record top-level index 0, got %d", step.StepIdx) + } + if step.StepName != "loop" { + t.Errorf("loop step name = %q, want the top-level loop step", step.StepName) } } diff --git a/cmd/ask/workflow_graph.go b/cmd/ask/workflow_graph.go new file mode 100644 index 0000000..a3e9066 --- /dev/null +++ b/cmd/ask/workflow_graph.go @@ -0,0 +1,173 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "github.com/Cidan/ask/pkg/engine" + "github.com/Cidan/ask/pkg/providers" + "github.com/Cidan/ask/pkg/workflow" + adkmodel "google.golang.org/adk/v2/model" + adktool "google.golang.org/adk/v2/tool" +) + +// runWorkflowGraph compiles def into an ADK workflow graph and runs it as +// a single turn on one agent session. +// +// The session is the same machinery a chat turn uses, with its agent +// swapped for the compiled graph, so tool execution, approvals, cost +// accounting, and cancellation all behave identically. Step progress is +// derived from the ADK event stream by workflow.Progress rather than +// from a hand-driven step loop. +func (c *Coordinator) runWorkflowGraph(ctx context.Context, cwd string, tabID int, def workflow.Def, src workflow.Source, listener workflow.RunnerListener) (*workflow.RunState, error) { + prov := providerByID("") + if prov == nil { + err := errors.New("no provider registered for workflow run") + listener.OnWorkflowFailed(tabID, err.Error()) + return nil, err + } + + proc, ch, err := prov.StartSession(ProviderSessionArgs{ + Cwd: cwd, + TabID: tabID, + Effort: "medium", + SkipAllPermissions: true, + InWorkflow: true, + }) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return nil, err + } + sess, ok := proc.payload.(*agentSession) + if !ok { + err := errors.New("workflow run: provider session is not an agent session") + listener.OnWorkflowFailed(tabID, err.Error()) + return nil, err + } + defer func() { + sess.shutdown() + c.RemoveSession(tabID) + }() + + compiled, err := workflow.CompileWorkflow(ctx, tuiWorkflowCompileConfig(sess, def, src, cwd, tabID)) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return nil, err + } + graphAgent, err := engine.WorkflowGraphAgent(def.Name, compiled) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return nil, err + } + + progress := workflow.NewProgress(compiled, def, src, cwd, tabID, listener, workflow.GlobalTracker()) + sess.workflowAgent = graphAgent + sess.workflowProgress = progress + c.SetSession(tabID, sess) + + if err := sess.queueTurn(src.Display()); err != nil { + return progress.Finish(err), err + } + + // The session emits translated messages while the graph runs; the + // workflow tab renders progress from the listener instead, so this + // only watches for the turn's outcome. The loop ends on + // turnCompleteMsg, or on the channel closing — a session that dies + // without a clean turn end still has to resolve the run. + var runErr error + sawDone := false +drain: + for msg := range ch { + switch m := msg.(type) { + case providerDoneMsg: + sawDone = true + switch { + case m.err != nil: + runErr = m.err + case m.res.IsError: + runErr = fmt.Errorf("workflow run failed: %s", m.res.Result) + } + case turnCompleteMsg: + break drain + } + } + + switch { + case runErr != nil: + case ctx.Err() != nil: + runErr = ctx.Err() + case !sawDone: + runErr = errors.New("workflow run ended without completing") + } + + if fd := sess.env.PendingFinishData; fd != nil { + progress.SetFinishData(&workflow.FinishData{ + Description: fd.Description, + Artifacts: fd.Artifacts, + }) + } + state := progress.Finish(runErr) + if runErr == nil { + engine.IngestWorkflowMemory(ctx, sess.sessSvc, sess.sessionID) + } + return state, runErr +} + +// tuiWorkflowCompileConfig builds the per-step model and tool wiring for +// a TUI workflow run. Every step shares the session's tool surface — the +// coding core plus this project's MCP and skill toolsets — while model +// and provider stay per-step so a workflow can chain providers. +func tuiWorkflowCompileConfig(sess *agentSession, def workflow.Def, src workflow.Source, cwd string, tabID int) workflow.WorkflowAgentConfig { + return workflow.WorkflowAgentConfig{ + Def: def, + Source: src, + Cwd: cwd, + TabID: tabID, + ModelBuilder: func(ctx context.Context, step workflow.Step) (adkmodel.LLM, error) { + return workflowStepModel(ctx, sess, step) + }, + ToolsBuilder: func(ctx context.Context, step workflow.Step, inLoop bool) ([]adktool.Tool, error) { + return engine.AsADKTools(sess.currentTools()) + }, + ToolsetsBuilder: func(ctx context.Context, step workflow.Step, inLoop bool) ([]adktool.Toolset, error) { + var toolsets []adktool.Toolset + if sess.mcp != nil { + toolsets = append(toolsets, sess.mcp.Toolsets()...) + } + if skillTS, err := engine.NewSkillToolset(ctx, cwd); err == nil && skillTS != nil { + toolsets = append(toolsets, skillTS) + } + return toolsets, nil + }, + } +} + +// workflowStepModel resolves a step's LLM, falling back to the session's +// own model when the step pins nothing. +// +// Swappable so tests can compile a graph without reaching a real +// provider, the same seam agentRunShell and agentGitStatus use. +var workflowStepModel = func(ctx context.Context, sess *agentSession, step workflow.Step) (adkmodel.LLM, error) { + providerID := step.Provider + if providerID == "" && sess.spec != nil { + providerID = sess.spec.ID + } + if providerID == "" { + providerID = "vertex" + } + spec, ok := providers.GetAgentProviderSpec(providerID) + if !ok || spec == nil { + return nil, fmt.Errorf("unknown provider %q for step %q", providerID, step.Name) + } + modelID := providers.CanonicalVertexModelID(step.Model, "") + if modelID == "" { + if sess.modelID != "" && sess.spec != nil && providerID == sess.spec.ID { + modelID = sess.modelID + } else { + modelID = spec.DefaultModel + } + } + cfg, _ := loadConfig() + return engine.ModelBuilder(ctx, spec, toPkgConfig(cfg), modelID) +} diff --git a/cmd/ask/workflows_test.go b/cmd/ask/workflows_test.go index f310196..c9af198 100644 --- a/cmd/ask/workflows_test.go +++ b/cmd/ask/workflows_test.go @@ -281,45 +281,29 @@ func TestIssueRef_KeyAndDisplay(t *testing.T) { } } -// TestBuildWorkflowStepPrompt covers the prompt assembly for both -// step 0 (no previous output) and step N>0 (previous output forwarded -// under a "Previous step output:" header). Whitespace at the head -// and tail is trimmed; the body is left as the user wrote it. -func TestBuildWorkflowStepPrompt(t *testing.T) { +// TestBuildWorkflowStepInstruction covers a step's instruction: the +// author's prompt, the run's reference block, and the end_turn contract. +// +// Previous-step output is deliberately absent. Threading it into the +// prompt was the hand-rolled runner's job; the graph now passes a node's +// output as the next node's input, and each step agent runs with +// IncludeContentsNone so it sees that rather than the whole transcript. +func TestBuildWorkflowStepInstruction(t *testing.T) { step := workflowStep{Prompt: "Implement the fix."} issue := issueWorkflowSource(issueRef{Provider: "github", Project: "ow/r", Number: 1}) - step0 := buildWorkflowStepPrompt(step, issue, nil, nil) - if !strings.Contains(step0, "Implement the fix.") { - t.Errorf("step 0 must include user prompt; got %q", step0) + got := buildWorkflowStepInstruction(step, issue, nil) + if !strings.Contains(got, "Implement the fix.") { + t.Errorf("instruction must include the user prompt; got %q", got) } - if !strings.Contains(step0, "Reference: ow/r#1") { - t.Errorf("step 0 must include issue reference; got %q", step0) + if !strings.Contains(got, "Reference: ow/r#1") { + t.Errorf("instruction must include the issue reference; got %q", got) } - if strings.Contains(step0, "Previous step output:") { - t.Errorf("step 0 must NOT include previous-step block; got %q", step0) + if !strings.Contains(got, "end_turn") { + t.Errorf("instruction must carry the end_turn contract; got %q", got) } - - stepN := buildWorkflowStepPrompt( - workflowStep{Prompt: "Review."}, - issue, - []string{"first step said hello", "ignored second"}, - nil, - ) - if !strings.Contains(stepN, "Review.") { - t.Errorf("step N must include user prompt; got %q", stepN) - } - if !strings.Contains(stepN, "Previous step output:") { - t.Errorf("step N must include previous-step block; got %q", stepN) - } - if !strings.Contains(stepN, "first step said hello") { - t.Errorf("step N must include first log entry; got %q", stepN) - } - if !strings.Contains(stepN, "ignored second") { - t.Errorf("step N must include second log entry; got %q", stepN) - } - if !strings.Contains(stepN, "---") { - t.Errorf("step N must separate log entries with ---; got %q", stepN) + if strings.Contains(got, "Previous step output:") { + t.Errorf("previous-step threading is the graph's job now; got %q", got) } } diff --git a/docs/adk-20-upgrade.md b/docs/adk-20-upgrade.md index 309f12f..9a9aca4 100644 --- a/docs/adk-20-upgrade.md +++ b/docs/adk-20-upgrade.md @@ -1,276 +1,139 @@ -# Comprehensive Architectural Plan: ADK 2.0 Native Modernization +# ADK 2.0 adoption status -## 1. Executive Summary +`ask` runs on Google's **Agent Development Kit 2.0** (`google.golang.org/adk/v2`) +and the GenAI SDK against Vertex AI Gemini. -`ask` is a modern terminal coding agent built in Go that utilizes Google's **Agent Development Kit 2.0 (`google.golang.org/adk/v2`)** and the **Google GenAI SDK (`google.golang.org/genai`)** against Vertex AI Gemini. - -While previous modernization efforts migrated individual components (such as `functiontool.New`, `skilltoolset`, `mcptoolset`, `memory.Service`, and `FileSessionService`), several subsystems in `ask` still hand-build functionality where native, idiomatic ADK 2.0 primitives exist. - -This document outlines the detailed architectural blueprint and implementation plan for adopting all remaining ADK 2.0 features (excluding Google Search grounding, which is intentionally replaced by Ask's first-class developer tooling). - -Each section is designed as an independent, reviewable pull request (PR) milestone. +This document tracks what is actually wired. An earlier revision listed +eight PRs as `[x] COMPLETED & MERGED`; an audit found that five of them +had merged as unreachable code — the ADK symbol was imported, a builder +was written, a test was written against the builder, and nothing in the +production path ever called it. The table below is kept honest on +purpose: **a row is only "done" when a production caller reaches it.** --- -## 2. Target Architecture Overview - -``` -┌────────────────────────────────────────────────────────────────────────┐ -│ PR 1: Runner Session Lifecycle (AutoCreateSession) │ -│ • Eliminate manual session check/create boilerplate │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 2: Self-Healing Tool Recovery (plugin/retryandreflect) │ -│ • Attach retry & reflection plugin for automatic error correction │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 3: Dynamic Tool Augmentation (plugin/functioncallmodifier) │ -│ • Migrate synthetic parameter injection to ADK modifier plugin │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 4: Native Subagent Delegation (tool/agenttool) │ -│ • Wrap research and named subagents directly via agenttool.New │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 5: Standard Human-In-The-Loop Confirmation (tool/toolconfirmation) │ -│ • Adopt adk_request_confirmation and ConfirmationProvider │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 6: Dynamic Instruction Interpolation (util/instructionutil) │ -│ • Dynamic session state & artifact interpolation in prompts │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 7: Session Artifact Management (artifact.Service & loadartifacts) │ -│ • Native artifact storage and retrieval across turns and subagents │ -└──────────────────────────────────┬─────────────────────────────────────┘ - │ -┌──────────────────────────────────▼─────────────────────────────────────┐ -│ PR 8: Graph-Based Workflow Engine (google.golang.org/adk/v2/workflow) │ -│ • Compile workflow definitions to ADK DAG Workflow Graphs │ -└────────────────────────────────────────────────────────────────────────┘ -``` +## Status + +| Capability | ADK surface | Status | +|---|---|---| +| Agent loop | `runner.Runner` + `llmagent` | **done** — `cmd/ask/agent_run.go`, `pkg/engine/run.go` | +| Session lifecycle | `runner.Config{AutoCreateSession}` | **done** — `pkg/engine/run.go` | +| Sessions on disk | `session.Service` (`NewFileSessionService`) | **done** | +| Tools | `functiontool.New`, `tool.Toolset` | **done** | +| Skills / MCP | `skilltoolset`, `mcptoolset` | **done** | +| Memory | `memory.Service` (`pkg/memory`) | **done** — including workflow-run ingestion | +| Workflow engine | `workflow.Workflow` graph + `AgentNode` | **done** — see below | +| Loops | `loopagent` + `exitlooptool` | **done** — compiled into the graph | +| 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` | **registered, never fires** — see Gaps | +| 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 | +| Artifacts | `artifact.Service`, `loadartifactstool` | **dropped** — see Gaps | +| Dynamic instructions | `util/instructionutil` | **deliberately not used** — see Gaps | +| Parallel / fan-out | `JoinNode`, `NodeConfig.ParallelWorker` | **planned** | +| Pause / resume, HITL | `workflow.Persistence`, `Workflow.Resume`, `NewRequestInputEvent` | **planned** | --- -## 3. Detailed PR Milestones +## Workflow engine + +A `workflow.Def` compiles to an ADK graph (`pkg/workflow/compile.go`): + +- Each top-level agent step becomes an `AgentNode` wrapping an + `llmagent`, chained `Start -> n0 -> n1 -> …`. +- A `kind: "loop"` step becomes an `AgentNode` wrapping a `loopagent` + whose sub-agents are the inner steps, each carrying `exit_loop`. A step + calls `exit_loop` to break (it sets `Actions.Escalate`, which is what + `loopagent` watches); otherwise the loop runs to `MaxIterations`. +- Per-node `RetryConfig` replaces the runner's `stepErrorRetry` loop. + +Two `llmagent` settings carry the workflow's semantics and must not be +dropped: + +- **`IncludeContents: IncludeContentsNone`.** Without it a step inherits + the whole session, and ADK's `ConvertForeignEvent` renders every prior + step's events as prose — each tool call and each full tool result — so + step 3 would carry steps 1 and 2 in their entirety. With it, a step + sees the handoff from the step before it and its own work. +- **`InstructionProvider`, never `Config.Instruction`.** Step prompts are + user-authored and routinely contain braces. ADK interpolates the static + `Instruction` field against session state and fails the invocation on + the first unknown `{name}`. + +`*workflow.Workflow` is not an `agent.Agent` (the interface has an +unexported method), so `engine.WorkflowGraphAgent` wraps it via +`agent.New(agent.Config{Run: wf.Run})` and hands that to the runner. + +Progress reporting lives in `pkg/workflow/progress.go`. Every callback is +driven by a real event: a step starts when an event authored by its agent +arrives and finishes when its successor starts or the run ends cleanly. +Steps that never ran are never reported. The previous graph runner closed +out every remaining step as completed and hardcoded a successful finish, +so a chain that died at step 1 of 5 rendered 5/5 green. + +### What the migration removed + +- The handwritten `Runner.Run` state machine, `LoopRunFrame`, and the + re-prompt machinery (`RemindNoSummary` / `RemindNoDecision` / + `RemindFixPlanDir`). +- `RunGraph` and `BuildWorkflowAgent` — two dead parallel engines. Note + `RunGraph` never used the graph; it called `BuildWorkflowAgent`. +- `StepExecutor` / `ExecuteStep`. A run is now one agent session for the + whole graph, not one provider session per step. +- The `ask/plans/` notes directories (`pkg/workflow/plans.go`) and the + `clear_plans` tool. Step handoff is the graph's node output; durable + reasoning goes to `pkg/memory` via `IngestWorkflowMemory`. --- -### PR 1: Runner Session Lifecycle & Auto-Creation (`AutoCreateSession: true`) (COMPLETED & MERGED - PR #128) +## Gaps -#### Problem Statement -Currently, `pkg/engine/run.go` (lines 240-256) and `cmd/ask/agent_run.go` (lines 375-390) manually query `sessSvc.Get(...)` and, upon a not-found error, issue a manual `sessSvc.Create(...)` before instantiating `runner.New(...)`. This creates boilerplate and redundant session lookups on every single user turn. +Recorded so the next reader does not mistake an import for an +integration. -#### Target Implementation -ADK 2.0 provides `runner.Config{AutoCreateSession: true}`. When set, `runner.Run` automatically initializes the session in the provided `session.Service` if it does not already exist. +**`plugin/retryandreflect`** is in `DefaultPlugins()` but cannot fire. +ADK triggers it from `OnToolErrorCallback`, i.e. a non-nil Go `error` +from the tool. Every ask tool returns `(NewTextErrorResponse(...), nil)`, +and `tools.NewTool` turns that into a *successful* return carrying +`is_error: true` in the result map. Wiring it means bridging +`ToolResponse.IsError` to ADK's error channel. -#### File Changes -- **`pkg/engine/run.go`**: - - Remove manual `sessSvc.Get` / `sessSvc.Create` block. - - Configure `runner.New(runner.Config{AppName: "ask", Agent: agentInstance, SessionService: sessSvc, MemoryService: memSvc, AutoCreateSession: true})`. -- **`cmd/ask/agent_run.go`**: - - Remove manual `s.sessSvc.Get` / `s.sessSvc.Create` block in `runTurn`. - - Pass `AutoCreateSession: true` to `engine.RunnerBuilder`. -- **`pkg/engine/session.go`**: - - Ensure headless sessions leverage `AutoCreateSession: true`. - -#### Verification & Tests -- `pkg/engine/run_test.go`: Add test verifying that calling `engine.Run` with a non-existent `SessionID` successfully auto-creates the session and persists events in `FileSessionService`. -- `cmd/ask/agent_run_test.go`: Verify multi-turn conversational resumption and fresh session initialization. - ---- +**`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`. -### PR 2: Self-Healing Tool Recovery via `plugin/retryandreflect` (COMPLETED & MERGED - PR #129) +**`tool/agenttool`** — `BuildResearchSubagent`, `BuildNamedSubagent`, +`BuildResearchAgentTool`, and `BuildNamedAgentTool` have no production +callers. The `task` tool still spawns a nested `engine.Run`. -#### Problem Statement -When a tool call fails (e.g., regex error in grep, invalid path in read, or syntax error in edit), the current behavior either returns a raw error string in the tool result or relies on the next turn prompt to instruct the model to recover. LLMs often surrender, apologize, or stop early rather than correcting their arguments. +**`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`. -#### Target Implementation -ADK 2.0 provides `google.golang.org/adk/v2/plugin/retryandreflect`. When a tool execution returns an error, the plugin intercepts the failure, executes a structured reflection loop (`reflection.md`), and prompts the model to correct its arguments in the same turn without crashing or terminating the turn prematurely. +**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. -#### File Changes -- **`pkg/engine/run.go`**: - - Import `google.golang.org/adk/v2/plugin/retryandreflect`. - - Configure `PluginConfig` on `runner.Config`: - ```go - retryPlugin, err := retryandreflect.NewPlugin(retryandreflect.Config{ - MaxRetries: 2, - }) - ``` -- **`cmd/ask/agent_run.go`**: - - Attach the `retryandreflect` plugin to the TUI runner configuration. -- **`pkg/engine/types.go`**: - - Align `ToolResponse.IsError` with ADK's reflection handler so tool error events trigger reflection seamlessly. - -#### Verification & Tests -- `pkg/engine/run_test.go`: Add `TestEngineRun_ToolRetryAndReflect` where a tool fails on the first invocation and succeeds on the second after reflection. -- `cmd/ask/agent_run_test.go`: Verify UI event stream correctly surfaces reflection attempts to the user without duplicating transcript entries. +**`util/instructionutil`** is deliberately not used. ask's instruction +text is user documentation inlined verbatim, not a template — see the +comment on `BuildInstructionProvider`. --- -### PR 3: Tool Parameter Augmentation via `plugin/functioncallmodifier` (COMPLETED & MERGED - PR #129) - -#### Problem Statement -Ask currently mutates tool declarations and wraps handler functions in `pkg/tools/types.go` and `pkg/tools/bridge.go` to inject required metadata fields (such as the mandatory `description` phrase for UI headlines). Hand-rolling AST schema modifications risks normalization incompatibilities with GenAI schema converters. - -#### Target Implementation -ADK 2.0 provides `google.golang.org/adk/v2/plugin/functioncallmodifier`. This plugin intercepts model requests before they hit the wire (`BeforeModelCallback`) and after model generation (`AfterModelCallback`), dynamically injecting synthetic argument schemas (`description`) and stripping them before tool execution. - -#### File Changes -- **`pkg/tools/types.go` & `pkg/tools/bridge.go`**: - - Remove manual JSON schema AST manipulation for `description` field injection. - - Keep `functiontool.New[TArgs, TResults]` purely focused on the tool's typed parameters. -- **`pkg/engine/run.go` & `cmd/ask/agent_run.go`**: - - Register `functioncallmodifier.NewPlugin(cfg)` with: - ```go - functioncallmodifier.NewPlugin(functioncallmodifier.FunctionCallModifierConfig{ - Predicate: func(toolName string) bool { return isNativeAskTool(toolName) }, - Args: map[string]*genai.Schema{ - "description": { - Type: "STRING", - Description: "one short human-readable phrase (under 10 words) telling the user what this call is doing", - }, - }, - }) - ``` - -#### Verification & Tests -- `pkg/tools/types_test.go` & `pkg/tools/bridge_test.go`: Verify tool declarations generate clean, strict schemas without conflicting `anyOf` or type arrays. -- `cmd/ask/tool_output_test.go`: Ensure headline phrase extraction remains 100% backward compatible. - ---- - -### PR 4: Native Subagent Delegation via `tool/agenttool` (COMPLETED & MERGED - PR #129) - -#### Problem Statement -In `cmd/ask/agent_tools_task.go` and `pkg/engine/subagents.go`, synchronous subagents (such as the default researcher or named subagents) are launched through custom execution runners that manually construct sub-sessions, extract results, and format outputs. - -#### Target Implementation -ADK 2.0 provides `google.golang.org/adk/v2/tool/agenttool`. `agenttool.New(agent, &agenttool.Config{SkipSummarization: ...})` wraps any `agent.Agent` directly into an ADK `tool.Tool`, handling sub-session isolation, parameter validation against the agent's input schema, and response extraction. - -#### File Changes -- **`pkg/engine/subagents.go`**: - - Convert `BuildResearchSubagent` and `BuildNamedSubagent` into native ADK tools via `agenttool.New(agentInstance, nil)`. -- **`cmd/ask/agent_tools_task.go`**: - - For synchronous task delegation, delegate directly through `agenttool`. - - Retain background job manager integration (`run_in_background: true`) for asynchronous jobs while using `agenttool` under the hood. - -#### Verification & Tests -- `pkg/engine/subagents_test.go`: Add tests verifying synchronous subagents execute with isolated sessions and report results through `agenttool`. -- `cmd/ask/agent_tools_test.go`: Verify `task` tool handles both synchronous and background tasks without regressions. - ---- - -### PR 5: Standard Human-In-The-Loop Confirmation via `tool/toolconfirmation` (COMPLETED & MERGED - PR #130) - -#### Problem Statement -Ask currently handles tool approval and permission rules through custom wrapper functions in `pkg/tools/env.go`, `pkg/engine/interaction.go`, and `cmd/ask/approval.go`. This decouples tool approval from the runner's native event stream. - -#### Target Implementation -ADK 2.0 provides native Human-In-The-Loop confirmation: -- Tools and Toolsets declare confirmation requirements via `tool.ConfirmationProvider` or `RequireConfirmation: true`. -- When a tool requires approval, ADK emits an `adk_request_confirmation` event (`toolconfirmation.FunctionCallName`). -- The frontend extracts the inner intent using `toolconfirmation.OriginalCallFrom(fc)` and yields the prompt to the user. -- The user's response is returned as a standard function response (`{"confirmed": bool}`), and ADK resumes execution automatically. - -#### File Changes -- **`pkg/engine/interaction.go`**: - - Update `ApprovalRequest` to integrate with `toolconfirmation.ToolConfirmation`. -- **`pkg/tools/env.go` & `pkg/tools/types.go`**: - - Use `tool.ConfirmationProvider` on tools that perform mutating operations (e.g. `write`, `edit`, `bash`). -- **`cmd/ask/event_adapter.go` & `cmd/ask/agent_run.go`**: - - Handle `toolconfirmation.FunctionCallName` in the runner event loop, pop the approval modal, and feed the confirmation response back to the runner. - -#### Verification & Tests -- `pkg/engine/run_test.go`: Add `TestEngineRun_ToolConfirmation_Approve` and `TestEngineRun_ToolConfirmation_Deny`. -- `cmd/ask/approval_test.go`: Verify that approving/denying tools correctly unblocks or terminates the agent step. - ---- - -### PR 6: Dynamic Instruction Interpolation via `util/instructionutil` (COMPLETED & MERGED - PR #130) - -#### Problem Statement -System prompts and step instructions in `pkg/engine/prompt.go` and `pkg/workflow/plans.go` currently use manual Go string interpolation and ad-hoc concatenation to inject environment variables, reminders, and notes directories. - -#### Target Implementation -ADK 2.0 provides `google.golang.org/adk/v2/util/instructionutil.InjectSessionState(ctx, template)`. This resolves `{key_name}` placeholders dynamically from `session.State` and `{artifact.key_name}` from session artifacts at runtime. - -#### File Changes -- **`pkg/engine/prompt.go`**: - - Integrate `instructionutil.InjectSessionState` into `BuildInstructionProvider`. - - Store runtime reminders, active git branch, and worktree info in `session.State` and reference them via standard template variables (`{git_branch}`, `{worktree_status}`). -- **`pkg/workflow/plans.go`**: - - Standardize workflow step instruction templates using `{notes_dir}` and `{prev_notes_dir}` placeholders. - -#### Verification & Tests -- `pkg/engine/prompt_test.go`: Add unit tests for `InjectSessionState` verifying variable substitution and missing variable error handling. -- `pkg/engine/run_test.go`: Test that state mutations during a multi-turn run dynamically update the agent instructions. - ---- - -### PR 7: Session Artifact Management via `artifact.Service` & `loadartifactstool` (COMPLETED & MERGED - PR #130) - -#### Problem Statement -Workflows and coding agents currently write plans, diffs, and intermediate notes directly to disk under `.ask/plans/`. Downstream steps must know exact filesystem paths to read previous outputs, and artifacts are not tracked in session history. - -#### Target Implementation -ADK 2.0 provides `google.golang.org/adk/v2/artifact.Service` (`artifact.InMemoryService`, `gcsartifact`) and `google.golang.org/adk/v2/tool/loadartifactstool`. -- The runner is configured with `runner.Config{ArtifactService: artifact.InMemoryService()}`. -- Agents and workflow steps save named artifacts (e.g. `plan.md`, `implementation_diff.patch`, `review_notes.md`) to `ctx.Artifacts().Save(...)`. -- Downstream steps use `loadartifactstool.New()` to autonomously list and load artifacts into their context. - -#### File Changes -- **`pkg/engine/run.go` & `cmd/ask/agent_run.go`**: - - Configure `ArtifactService: artifact.InMemoryService()` on `runner.Config`. - - Attach `loadartifactstool.New()` to the default toolset. -- **`pkg/workflow/runner.go`**: - - Update workflow steps to save step summaries and notes into `ctx.Artifacts()`. - -#### Verification & Tests -- `pkg/engine/run_test.go`: Test artifact creation by one turn and retrieval via `load_artifacts` by the next. -- `pkg/workflow/runner_test.go`: Test inter-step artifact passing in a multi-step workflow. - ---- - -### PR 8: Graph-Based Workflow Engine via `google.golang.org/adk/v2/workflow` (COMPLETED & MERGED - PR #130) - -#### Problem Statement -Ask's `pkg/workflow/runner.go` maintains a custom handwritten step execution loop for running workflows, tracking loop iterations, and handling step transitions. This duplicates the execution graph logic provided by ADK 2.0. - -#### Target Implementation -ADK 2.0 includes a comprehensive DAG workflow engine in `google.golang.org/adk/v2/workflow`: -- **Nodes**: `AgentNode` (wraps LLM agent), `ToolNode` (executes tools directly), `FunctionNode` (executes Go logic), `JoinNode` (synchronizes concurrent branches). -- **Routing**: `StringRoute`, `IntRoute`, `BoolRoute`, `MultiRoute`, `Default`. -- **State Persistence**: `workflow.Persistence` automatically tracks node execution and state in `session.State`, enabling seamless workflow pause and resumption (`workflow.Resume`). - -#### File Changes -- **`pkg/workflow/graph.go`**: - - Implement `CompileDefToADKWorkflow(def Def, cfg WorkflowAgentConfig) (*workflow.Workflow, error)`. - - Map linear steps to sequential `AgentNode` edges. - - Map loop steps to cyclic edges controlled by `exitlooptool` and route conditions. -- **`pkg/workflow/runner.go`**: - - Replace the custom `Runner.Run` state machine with `Workflow.Run(ctx)`. - - Map workflow graph events to `RunnerListener` callbacks (`OnWorkflowStarted`, `OnWorkflowStepStarted`, `OnWorkflowStepDone`, `OnWorkflowDone`). - -#### Verification & Tests -- `pkg/workflow/runner_test.go`: Migrate existing workflow test suite to execute against the ADK workflow graph engine. -- Verify 100% behavioral parity with existing `.ask/workflows/*.json` pipelines. - ---- +## Planned -## 4. Execution Checklist +**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. -- [x] **PR 1**: Runner Session Lifecycle & Auto-Creation (`AutoCreateSession: true`) ([#128](https://github.com/Cidan/ask/pull/128)) -- [x] **PR 2**: Self-Healing Tool Recovery via `plugin/retryandreflect` ([#129](https://github.com/Cidan/ask/pull/129)) -- [x] **PR 3**: Tool Parameter Augmentation via `plugin/functioncallmodifier` ([#129](https://github.com/Cidan/ask/pull/129)) -- [x] **PR 4**: Native Subagent Delegation via `tool/agenttool` ([#129](https://github.com/Cidan/ask/pull/129)) -- [x] **PR 5**: Standard Human-In-The-Loop Confirmation via `tool/toolconfirmation` ([#130](https://github.com/Cidan/ask/pull/130)) -- [x] **PR 6**: Dynamic Instruction Interpolation via `util/instructionutil` ([#130](https://github.com/Cidan/ask/pull/130)) -- [x] **PR 7**: Session Artifact Management via `artifact.Service` & `loadartifactstool` ([#130](https://github.com/Cidan/ask/pull/130)) -- [x] **PR 8**: Graph-Based Workflow Engine via `google.golang.org/adk/v2/workflow` ([#130](https://github.com/Cidan/ask/pull/130)) +**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. diff --git a/pkg/engine/coordinator.go b/pkg/engine/coordinator.go index 7a6fc5e..2bcd0fa 100644 --- a/pkg/engine/coordinator.go +++ b/pkg/engine/coordinator.go @@ -4,8 +4,6 @@ import ( "context" "errors" "sync" - - "github.com/Cidan/ask/pkg/workflow" ) // Coordinator manages the background execution of all in-process agent sessions @@ -92,79 +90,3 @@ func (c *Coordinator) Dispatch(tabID int, text string) error { } return s.QueueTurn(text) } - -func (c *Coordinator) ExecuteStep(ctx context.Context, cwd string, tabID int, step workflow.Step, prompt string, isFinal bool) (workflow.StepResult, error) { - c.mu.RLock() - s := c.sessions[tabID] - c.mu.RUnlock() - - if s != nil { - err := s.QueueTurnSync(ctx, prompt) - if err != nil { - return workflow.StepResult{Error: err}, err - } - msgs := s.Messages() - lastResp := s.LastResponse() - return extractStepResultFromMessages(lastResp, msgs), nil - } - - runResult, err := Run(ctx, RunOptions{ - Prompt: prompt, - Cwd: cwd, - Provider: step.Provider, - Model: step.Model, - EventListener: c.listener, - InteractionHandler: c.interaction, - SkipAllPermissions: true, - }) - if err != nil { - return workflow.StepResult{Error: err}, err - } - return extractStepResultFromMessages(runResult.Response, runResult.Messages), nil -} - -func extractStepResultFromMessages(output string, messages []Message) workflow.StepResult { - var summary, decision string - var finishData *workflow.FinishData - for i := len(messages) - 1; i >= 0; i-- { - msg := messages[i] - for _, tc := range msg.ToolCalls { - if tc.Name == "end_turn" { - if s, ok := tc.Args["summary"].(string); ok && summary == "" { - summary = s - } - if d, ok := tc.Args["decision"].(string); ok && decision == "" { - decision = d - } - } - if tc.Name == "exit_loop" { - if decision == "" { - decision = workflow.LoopBreak - } - } - if tc.Name == "finish_workflow" && finishData == nil { - desc, _ := tc.Args["description"].(string) - var arts []string - if rawArts, ok := tc.Args["artifacts"].([]any); ok { - for _, a := range rawArts { - if s, ok := a.(string); ok { - arts = append(arts, s) - } - } - } else if strArts, ok := tc.Args["artifacts"].([]string); ok { - arts = strArts - } - finishData = &workflow.FinishData{ - Description: desc, - Artifacts: arts, - } - } - } - } - return workflow.StepResult{ - Output: output, - Summary: summary, - Decision: decision, - FinishData: finishData, - } -} diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index fa74d80..f85fcbd 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -1,15 +1,8 @@ package engine import ( - "context" - "fmt" - "github.com/Cidan/ask/pkg/config" - "github.com/Cidan/ask/pkg/providers" "github.com/Cidan/ask/pkg/workflow" - "google.golang.org/adk/v2/agent" - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/tool" ) // Options holds configuration and handlers for the ask Engine. @@ -52,70 +45,6 @@ func (e *Engine) SystemPrompt(cwd string, inWorkflow bool) string { }) } -// BuildWorkflowAgent constructs an ADK agent hierarchy (sequentialagent, loopagent, exitlooptool) -// for the given workflow definition using the engine's model and tool configuration. -func (e *Engine) BuildWorkflowAgent(ctx context.Context, cwd string, def workflow.Def, src workflow.Source) (agent.Agent, error) { - cfg := workflow.WorkflowAgentConfig{ - Def: def, - Source: src, - Cwd: cwd, - ModelBuilder: func(ctx context.Context, step workflow.Step) (model.LLM, error) { - providerID := step.Provider - if providerID == "" { - providerID = e.opts.Config.Provider - } - if providerID == "" { - providerID = "vertex" - } - spec, ok := providers.GetAgentProviderSpec(providerID) - if !ok || spec == nil { - return nil, fmt.Errorf("unknown provider %q", providerID) - } - modelID := providers.CanonicalVertexModelID(step.Model, "") - if modelID == "" { - settings := spec.LoadSettings(e.opts.Config) - modelID = providers.CanonicalVertexModelID(settings.Model, spec.DefaultModel) - } - if modelID == "" { - modelID = spec.DefaultModel - } - return ModelBuilder(ctx, spec, e.opts.Config, modelID) - }, - ToolsBuilder: func(ctx context.Context, step workflow.Step, isLoop bool) ([]tool.Tool, error) { - var agentTools []Tool - if tf := GetDefaultToolFactory(); tf != nil { - agentTools = tf(ToolFactoryArgs{ - Cwd: cwd, - TabID: 0, - SkipPermissions: true, - EventListener: e.opts.EventListener, - InteractionHandler: e.opts.InteractionHandler, - AttachWebSearch: true, - }) - } - return AsADKTools(agentTools) - }, - ToolsetsBuilder: func(ctx context.Context, step workflow.Step, isLoop bool) ([]tool.Toolset, error) { - var toolsets []tool.Toolset - if skillTS, err := NewSkillToolset(ctx, cwd); err == nil && skillTS != nil { - toolsets = append(toolsets, skillTS) - } - return toolsets, nil - }, - InstructionBuilder: func(step workflow.Step, isStart bool, isFinal bool, loopCtx *workflow.LoopPromptCtx, notesDir, prevNotesDir string) string { - pc := &workflow.StepPromptCtx{ - Loop: loopCtx, - NotesDir: notesDir, - PrevNotesDir: prevNotesDir, - IsStartStep: isStart, - IsWorkflowFinalStep: isFinal, - } - return workflow.BuildStepPrompt(step, src, nil, pc) - }, - } - return workflow.BuildWorkflowAgent(ctx, cfg) -} - type engineWorkflowListener struct { tabID int listener EventListener @@ -156,10 +85,3 @@ func (l engineWorkflowListener) OnNote(tabID int, text string) { l.listener(StatusEvent{BaseEvent: BaseEvent{TabID: tabID}, Status: text}) } } - -func (e *Engine) RunWorkflow(ctx context.Context, cwd string, tabID int, def workflow.Def, src workflow.Source) error { - listener := engineWorkflowListener{tabID: tabID, listener: e.opts.EventListener} - runner := workflow.NewRunner(workflow.GlobalTracker(), e.coordinator, listener) - _, err := runner.Run(ctx, cwd, tabID, def, src) - return err -} diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 4ce88f7..ee40d40 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -2,7 +2,9 @@ package engine import ( "context" + "errors" "iter" + "strings" "sync" "testing" @@ -103,81 +105,200 @@ func TestEngine_SessionStreamEvents(t *testing.T) { } } -func TestEngine_CoordinatorExecuteStep(t *testing.T) { +// TestEngine_RunWorkflow_Graph drives a two-step workflow end to end on +// ADK's workflow scheduler and checks that progress is reported from the +// real event stream: both steps start, both finish, and the run closes +// with a Done event. +func TestEngine_RunWorkflow_Graph(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("HOME", t.TempDir()) + var mu sync.Mutex - callIdx := 0 + var seenInstructions []string mockModel := &mockLLM{ name: "mock-model", generateFunc: func(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { mu.Lock() - idx := callIdx - callIdx++ + if req != nil && req.Config != nil && req.Config.SystemInstruction != nil { + for _, p := range req.Config.SystemInstruction.Parts { + if p != nil && p.Text != "" { + seenInstructions = append(seenInstructions, p.Text) + } + } + } mu.Unlock() + return mockLLMSequence(textResponse("step done")) + }, + } - if idx == 0 { - return mockLLMSequence( - thoughtAndFunctionCallResponse("thinking", "end_turn", map[string]any{ - "summary": "Verified plan and executed step", - "decision": "continue", - }, nil), - ) - } - return mockLLMSequence(textResponse("Step done")) + origBuilder := ModelBuilder + ModelBuilder = func(ctx context.Context, spec *providers.AgentProviderSpec, cfg config.Config, modelID string) (model.LLM, error) { + return mockModel, nil + } + defer func() { ModelBuilder = origBuilder }() + + var events []EngineEvent + eng := New(Options{ + Config: config.Config{Provider: "vertex"}, + InteractionHandler: HeadlessInteractionHandler{AutoApproveTools: true}, + EventListener: func(ev EngineEvent) { + mu.Lock() + defer mu.Unlock() + events = append(events, ev) + }, + }) + + def := workflow.Def{ + Name: "engine-workflow", + Steps: []workflow.Step{ + {Name: "plan", Prompt: "First step"}, + {Name: "review", Prompt: "Second step"}, }, } + src := workflow.NewTextSource(1, "Engine Workflow Source") - coord := NewCoordinator(HeadlessInteractionHandler{AutoApproveTools: true}, nil) - session := NewSession( - SessionArgs{TabID: 2, Cwd: t.TempDir(), Model: "mock-model"}, - mockModel, - "system prompt", - nil, - nil, - HeadlessInteractionHandler{AutoApproveTools: true}, - ) - defer session.Close() - coord.SetSession(2, session) + if err := eng.RunWorkflow(context.Background(), tmpDir, 10, def, src); err != nil { + t.Fatalf("RunWorkflow failed: %v", err) + } + + mu.Lock() + defer mu.Unlock() - step := workflow.Step{ - Name: "Test Step", - Provider: "vertex", - Model: "mock-model", + var started, done bool + var stepStarts, stepDones []int + for _, ev := range events { + switch e := ev.(type) { + case WorkflowStartedEvent: + started = true + case WorkflowStepStartedEvent: + stepStarts = append(stepStarts, e.StepIdx) + case WorkflowStepDoneEvent: + stepDones = append(stepDones, e.StepIdx) + case WorkflowDoneEvent: + done = true + } + } + if !started || !done { + t.Errorf("run must open and close: started=%v done=%v", started, done) + } + if len(stepStarts) != 2 || stepStarts[0] != 0 || stepStarts[1] != 1 { + t.Errorf("expected both steps to start in order, got %v", stepStarts) + } + if len(stepDones) != 2 || stepDones[0] != 0 || stepDones[1] != 1 { + t.Errorf("expected both steps to finish in order, got %v", stepDones) } - res, err := coord.ExecuteStep(context.Background(), t.TempDir(), 2, step, "run step", false) - if err != nil { - t.Fatalf("unexpected error: %v", err) + // Each step's own prompt reaches its agent, and the end_turn contract + // rides along with it. + joined := strings.Join(seenInstructions, "\n") + for _, want := range []string{"First step", "Second step", "end_turn"} { + if !strings.Contains(joined, want) { + t.Errorf("step instructions missing %q", want) + } } +} + +// A step whose model fails must not be reported as completed, and the +// steps after it must not be reported at all. The previous graph runner +// closed out every remaining step as done and hardcoded a successful +// finish, so a chain that died at step 1 of 3 rendered 3/3 green. +func TestEngine_RunWorkflow_FailureLeavesLaterStepsUnreported(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("HOME", t.TempDir()) - if res.Summary != "Verified plan and executed step" { - t.Errorf("expected summary 'Verified plan and executed step', got %q", res.Summary) + origBuilder := ModelBuilder + ModelBuilder = func(ctx context.Context, spec *providers.AgentProviderSpec, cfg config.Config, modelID string) (model.LLM, error) { + return nil, errors.New("model unavailable") } - if res.Decision != "continue" { - t.Errorf("expected decision 'continue', got %q", res.Decision) + defer func() { ModelBuilder = origBuilder }() + + var mu sync.Mutex + var events []EngineEvent + eng := New(Options{ + Config: config.Config{Provider: "vertex"}, + InteractionHandler: HeadlessInteractionHandler{AutoApproveTools: true}, + EventListener: func(ev EngineEvent) { + mu.Lock() + defer mu.Unlock() + events = append(events, ev) + }, + }) + + def := workflow.Def{ + Name: "failing-workflow", + Steps: []workflow.Step{ + {Name: "one", Prompt: "a"}, + {Name: "two", Prompt: "b"}, + {Name: "three", Prompt: "c"}, + }, + } + src := workflow.NewTextSource(2, "Failing Source") + + if err := eng.RunWorkflow(context.Background(), tmpDir, 11, def, src); err == nil { + t.Fatal("expected RunWorkflow to fail when the model cannot be built") + } + + mu.Lock() + defer mu.Unlock() + for _, ev := range events { + switch ev.(type) { + case WorkflowStepDoneEvent: + t.Error("no step may be reported done when the run never started one") + case WorkflowDoneEvent: + t.Error("a failed run must not emit WorkflowDone") + } + } + var failed bool + for _, ev := range events { + if _, ok := ev.(WorkflowFailedEvent); ok { + failed = true + } + } + if !failed { + t.Error("a failed run must emit WorkflowFailed") } } -func TestEngine_WorkflowExecution_ADK(t *testing.T) { +// The reason every step agent is built with IncludeContentsNone. +// +// Without it a step inherits the whole session, and ADK's +// ConvertForeignEvent renders every prior step's events as prose — each +// tool call and each full tool result — so step 3 would carry steps 1 +// and 2 in their entirety. With it, a step sees the handoff from the +// step immediately before it and nothing older. +func TestEngine_RunWorkflow_StepsSeeOnlyTheHandoffNotTheWholeChain(t *testing.T) { tmpDir := t.TempDir() + t.Setenv("HOME", t.TempDir()) + + markers := []string{"MARKER_STEP_ONE", "MARKER_STEP_TWO", "MARKER_STEP_THREE"} + var mu sync.Mutex - callIdx := 0 + call := 0 + var thirdStepRequest []string + mockModel := &mockLLM{ name: "mock-model", generateFunc: func(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { mu.Lock() - idx := callIdx - callIdx++ + idx := call + call++ + if idx == 2 { + for _, c := range req.Contents { + if c == nil { + continue + } + for _, p := range c.Parts { + if p != nil && p.Text != "" { + thirdStepRequest = append(thirdStepRequest, p.Text) + } + } + } + } mu.Unlock() - - if idx%2 == 0 { - return mockLLMSequence( - thoughtAndFunctionCallResponse("thinking", "end_turn", map[string]any{ - "summary": "Completed step successfully", - "decision": "continue", - }, nil), - ) + if idx < len(markers) { + return mockLLMSequence(textResponse("done: " + markers[idx])) } - return mockLLMSequence(textResponse("Step complete")) + return mockLLMSequence(textResponse("done")) }, } @@ -187,76 +308,88 @@ func TestEngine_WorkflowExecution_ADK(t *testing.T) { } defer func() { ModelBuilder = origBuilder }() - var events []EngineEvent - listener := func(ev EngineEvent) { - mu.Lock() - defer mu.Unlock() - events = append(events, ev) - } - eng := New(Options{ Config: config.Config{Provider: "vertex"}, InteractionHandler: HeadlessInteractionHandler{AutoApproveTools: true}, - EventListener: listener, }) - - session := NewSession( - SessionArgs{TabID: 10, Cwd: tmpDir, Model: "mock-model"}, - mockModel, - "system prompt", - nil, - nil, - HeadlessInteractionHandler{AutoApproveTools: true}, - ) - defer session.Close() - eng.Coordinator().SetSession(10, session) - def := workflow.Def{ - Name: "engine-workflow", + Name: "isolation", Steps: []workflow.Step{ - {Name: "step-1", Prompt: "First step"}, - {Name: "step-2", Prompt: "Second step"}, + {Name: "one", Prompt: "first"}, + {Name: "two", Prompt: "second"}, + {Name: "three", Prompt: "third"}, }, } - src := workflow.NewTextSource(1, "Engine Workflow Source") + if err := eng.RunWorkflow(context.Background(), tmpDir, 12, def, workflow.NewTextSource(3, "src")); err != nil { + t.Fatalf("RunWorkflow: %v", err) + } - // Verify BuildWorkflowAgent creates the ADK agent tree - ag, err := eng.BuildWorkflowAgent(context.Background(), tmpDir, def, src) - if err != nil { - t.Fatalf("BuildWorkflowAgent failed: %v", err) + mu.Lock() + defer mu.Unlock() + if len(thirdStepRequest) == 0 { + t.Fatal("third step never issued a model request") } - if ag.Name() != "engine-workflow" { - t.Errorf("expected agent name 'engine-workflow', got %q", ag.Name()) + joined := strings.Join(thirdStepRequest, "\n") + if !strings.Contains(joined, "MARKER_STEP_TWO") { + t.Errorf("step three must receive the handoff from step two, got %q", joined) } - if len(ag.SubAgents()) != 2 { - t.Errorf("expected 2 subagents, got %d", len(ag.SubAgents())) + if strings.Contains(joined, "MARKER_STEP_ONE") { + t.Errorf("step three inherited step one's transcript — IncludeContentsNone is not in effect: %q", joined) } +} - // Verify RunWorkflow coordinates execution and emits events - err = eng.RunWorkflow(context.Background(), tmpDir, 10, def, src) - if err != nil { - t.Fatalf("RunWorkflow failed: %v", err) +// Step prompts are user-authored and routinely contain braces. Building +// step agents with llmagent.Config.Instruction would run them through +// ADK's state interpolator, which hard-fails the invocation on an +// unknown `{name}` — so the compiler uses an InstructionProvider. +func TestEngine_RunWorkflow_BracesInStepPromptDoNotFailTheRun(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("HOME", t.TempDir()) + + var mu sync.Mutex + var seen []string + mockModel := &mockLLM{ + name: "mock-model", + generateFunc: func(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { + mu.Lock() + if req != nil && req.Config != nil && req.Config.SystemInstruction != nil { + for _, p := range req.Config.SystemInstruction.Parts { + if p != nil && p.Text != "" { + seen = append(seen, p.Text) + } + } + } + mu.Unlock() + return mockLLMSequence(textResponse("ok")) + }, } - mu.Lock() - defer mu.Unlock() + origBuilder := ModelBuilder + ModelBuilder = func(ctx context.Context, spec *providers.AgentProviderSpec, cfg config.Config, modelID string) (model.LLM, error) { + return mockModel, nil + } + defer func() { ModelBuilder = origBuilder }() - var gotStarted, gotStepStarted, gotStepDone, gotDone bool - for _, ev := range events { - switch ev.(type) { - case WorkflowStartedEvent: - gotStarted = true - case WorkflowStepStartedEvent: - gotStepStarted = true - case WorkflowStepDoneEvent: - gotStepDone = true - case WorkflowDoneEvent: - gotDone = true - } + eng := New(Options{ + Config: config.Config{Provider: "vertex"}, + InteractionHandler: HeadlessInteractionHandler{AutoApproveTools: true}, + }) + def := workflow.Def{ + Name: "braces", + Steps: []workflow.Step{ + {Name: "expand", Prompt: "Expand ${VAR} and {notes_dir?} and {Name, Steps}."}, + }, + } + if err := eng.RunWorkflow(context.Background(), tmpDir, 13, def, workflow.NewTextSource(4, "src")); err != nil { + t.Fatalf("braces in a step prompt must not fail the run: %v", err) } - if !gotStarted || !gotStepStarted || !gotStepDone || !gotDone { - t.Errorf("missing workflow events: started=%v stepStarted=%v stepDone=%v done=%v", - gotStarted, gotStepStarted, gotStepDone, gotDone) + mu.Lock() + defer mu.Unlock() + joined := strings.Join(seen, "\n") + for _, want := range []string{"${VAR}", "{notes_dir?}", "{Name, Steps}"} { + if !strings.Contains(joined, want) { + t.Errorf("step prompt must reach the model verbatim, missing %q in %q", want, joined) + } } } diff --git a/pkg/engine/subagents.go b/pkg/engine/subagents.go index b69267b..85c7fad 100644 --- a/pkg/engine/subagents.go +++ b/pkg/engine/subagents.go @@ -167,7 +167,7 @@ func ResolveSubagentModel(def SubagentDef, parentProviderID string, parent *gena var AllSubagentTools = []string{ "read", "glob", "grep", "ls", "write", "edit", "bash", "job_output", "job_kill", "fetch", "todos", "search_tools", "invoke_tool", "web_search", - "workflow_list", "workflow_get", "workflow_create", "workflow_edit", "workflow_delete", "workflow_copy", "clear_plans", + "workflow_list", "workflow_get", "workflow_create", "workflow_edit", "workflow_delete", "workflow_copy", } // SubagentToolNames returns the slice of tool names allowed for the subagent. @@ -178,7 +178,7 @@ func SubagentToolNames(def SubagentDef) []string { "job_output": true, "job_kill": true, "fetch": true, "todos": true, "search_tools": true, "invoke_tool": true, "web_search": true, "workflow_list": true, "workflow_get": true, "workflow_create": true, - "workflow_edit": true, "workflow_delete": true, "workflow_copy": true, "clear_plans": true, + "workflow_edit": true, "workflow_delete": true, "workflow_copy": true, } switch { diff --git a/pkg/engine/workflow_run.go b/pkg/engine/workflow_run.go new file mode 100644 index 0000000..273a3d6 --- /dev/null +++ b/pkg/engine/workflow_run.go @@ -0,0 +1,243 @@ +package engine + +import ( + "context" + "encoding/json" + "errors" + "fmt" + + pkgmemory "github.com/Cidan/ask/pkg/memory" + "github.com/Cidan/ask/pkg/providers" + "github.com/Cidan/ask/pkg/workflow" + "google.golang.org/adk/v2/agent" + adkmodel "google.golang.org/adk/v2/model" + "google.golang.org/adk/v2/session" + "google.golang.org/adk/v2/tool" + "google.golang.org/genai" +) + +// WorkflowCompileConfig builds the compile-time wiring for a workflow +// run: how each step resolves its model and tool surface. Shared by the +// headless engine and the TUI so both compile identical graphs. +func WorkflowCompileConfig(e *Engine, cwd string, tabID int, def workflow.Def, src workflow.Source) workflow.WorkflowAgentConfig { + return workflow.WorkflowAgentConfig{ + Def: def, + Source: src, + Cwd: cwd, + TabID: tabID, + ModelBuilder: func(ctx context.Context, step workflow.Step) (adkmodel.LLM, error) { + return buildStepModel(ctx, e, step) + }, + ToolsBuilder: func(ctx context.Context, step workflow.Step, inLoop bool) ([]tool.Tool, error) { + var agentTools []Tool + if tf := GetDefaultToolFactory(); tf != nil { + agentTools = tf(ToolFactoryArgs{ + Cwd: cwd, + TabID: tabID, + SkipPermissions: true, + EventListener: e.opts.EventListener, + InteractionHandler: e.opts.InteractionHandler, + AttachWebSearch: true, + }) + } + return AsADKTools(agentTools) + }, + ToolsetsBuilder: func(ctx context.Context, step workflow.Step, inLoop bool) ([]tool.Toolset, error) { + var toolsets []tool.Toolset + if skillTS, err := NewSkillToolset(ctx, cwd); err == nil && skillTS != nil { + toolsets = append(toolsets, skillTS) + } + return toolsets, nil + }, + } +} + +func buildStepModel(ctx context.Context, e *Engine, step workflow.Step) (adkmodel.LLM, error) { + providerID := step.Provider + if providerID == "" { + providerID = e.opts.Config.Provider + } + if providerID == "" { + providerID = "vertex" + } + spec, ok := providers.GetAgentProviderSpec(providerID) + if !ok || spec == nil { + return nil, fmt.Errorf("unknown provider %q", providerID) + } + modelID := providers.CanonicalVertexModelID(step.Model, "") + if modelID == "" { + settings := spec.LoadSettings(e.opts.Config) + modelID = providers.CanonicalVertexModelID(settings.Model, spec.DefaultModel) + } + if modelID == "" { + modelID = spec.DefaultModel + } + return ModelBuilder(ctx, spec, e.opts.Config, modelID) +} + +// CompileWorkflow compiles a workflow definition into an executable ADK +// graph using the engine's model and tool wiring. +func (e *Engine) CompileWorkflow(ctx context.Context, cwd string, tabID int, def workflow.Def, src workflow.Source) (*workflow.Compiled, error) { + return workflow.CompileWorkflow(ctx, WorkflowCompileConfig(e, cwd, tabID, def, src)) +} + +// WorkflowGraphAgent wraps a compiled workflow so it can be handed to an +// ADK runner. *workflow.Workflow is not itself an agent.Agent — the +// interface has an unexported method — but its Run has the agent Run +// shape, so agent.New adopts it directly. +func WorkflowGraphAgent(name string, compiled *workflow.Compiled) (agent.Agent, error) { + if compiled == nil || compiled.Workflow == nil { + return nil, errors.New("workflow graph agent: nil compiled workflow") + } + return agent.New(agent.Config{ + Name: "workflow_" + compiled.Workflow.Name(), + Description: name, + Run: compiled.Workflow.Run, + }) +} + +// RunWorkflow compiles def and drives it to completion on ADK's workflow +// scheduler, translating the event stream into both agent events (tool +// calls, text) and workflow progress callbacks. +func (e *Engine) RunWorkflow(ctx context.Context, cwd string, tabID int, def workflow.Def, src workflow.Source) error { + listener := engineWorkflowListener{tabID: tabID, listener: e.opts.EventListener} + + compiled, err := e.CompileWorkflow(ctx, cwd, tabID, def, src) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return err + } + agentInstance, err := WorkflowGraphAgent(def.Name, compiled) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return err + } + + // Workflow runs are not resumable across restarts today (that is + // what workflow.Persistence buys, once ask surfaces pause/resume), + // so the graph gets a session of its own rather than the project's + // on-disk transcript. + sessSvc := session.InMemoryService() + r, err := RunnerBuilder(agentInstance, sessSvc) + if err != nil { + listener.OnWorkflowFailed(tabID, err.Error()) + return err + } + + progress := workflow.NewProgress(compiled, def, src, cwd, tabID, listener, workflow.GlobalTracker()) + sessionID := "wf-" + src.Key() + userMsg := genai.NewContentFromText(src.Display(), genai.RoleUser) + + for event, err := range r.Run(ctx, "user", sessionID, userMsg, agent.RunConfig{}) { + if err != nil { + progress.Finish(err) + return err + } + if event == nil { + continue + } + emitAgentEvent(e.opts.EventListener, tabID, event) + progress.Observe(event) + } + + progress.Finish(nil) + IngestWorkflowMemory(ctx, sessSvc, sessionID) + return nil +} + +// IngestWorkflowMemory files a finished run into ask's long-term memory. +// +// Workflow steps used to leave their reasoning in ask/plans/ notes +// directories on disk, which nothing ever read back and which the runner +// deleted at the end of the run. Memory is the durable store, and +// pkg/memory is already an adkmemory.Service wired as the runner's +// MemoryService — it just was never fed from a workflow. +func IngestWorkflowMemory(ctx context.Context, sessSvc session.Service, sessionID string) { + mem := pkgmemory.Default() + if mem == nil || !mem.IsOpen() || sessSvc == nil { + return + } + resp, err := sessSvc.Get(ctx, &session.GetRequest{ + AppName: "ask", + UserID: "user", + SessionID: sessionID, + }) + if err != nil || resp == nil || resp.Session == nil { + return + } + _ = mem.AddSessionToMemory(ctx, resp.Session) +} + +// toolResponseText renders an ADK function response payload as the text +// a UI shows, and reports whether it represents an error. +func toolResponseText(resp map[string]any) (string, bool) { + if len(resp) == 0 { + return "", false + } + isErr, _ := resp["is_error"].(bool) + switch { + case resp["result"] != nil: + if s, ok := resp["result"].(string); ok { + return s, isErr + } + case resp["confirmed"] != nil: + if confirmed, _ := resp["confirmed"].(bool); confirmed { + return "confirmed", isErr + } + return "rejected by user", true + case resp["reflection_guidance"] != nil: + if s, ok := resp["reflection_guidance"].(string); ok { + return s, true + } + } + raw, err := json.Marshal(resp) + if err != nil { + return "", isErr + } + return string(raw), isErr +} + +// emitAgentEvent translates one ADK event into the agent-level events a +// UI renders (text, tool calls, tool results, usage). +func emitAgentEvent(listener EventListener, tabID int, event *session.Event) { + if listener == nil || event == nil { + return + } + if event.UsageMetadata != nil { + listener(UsageEvent{ + BaseEvent: BaseEvent{TabID: tabID}, + InputTokens: int(event.UsageMetadata.PromptTokenCount), + OutputTokens: int(event.UsageMetadata.CandidatesTokenCount), + TotalTokens: int(event.UsageMetadata.TotalTokenCount), + }) + } + if event.LLMResponse.Content == nil { + return + } + for _, part := range event.LLMResponse.Content.Parts { + if part == nil || part.Thought { + continue + } + if part.Text != "" { + listener(TextDeltaEvent{BaseEvent: BaseEvent{TabID: tabID}, Delta: part.Text}) + } + if part.FunctionCall != nil { + name, input := part.FunctionCall.Name, part.FunctionCall.Args + if IsConfirmationCall(part.FunctionCall) { + if orig, err := UnwrapConfirmationCall(part.FunctionCall); err == nil && orig != nil { + name, input = orig.Name, orig.Args + } + } + listener(ToolCallEvent{BaseEvent: BaseEvent{TabID: tabID}, ToolName: name, Input: input}) + } + if part.FunctionResponse != nil { + res, isErr := toolResponseText(part.FunctionResponse.Response) + listener(ToolResultEvent{ + BaseEvent: BaseEvent{TabID: tabID}, + ToolName: part.FunctionResponse.Name, + Output: res, + IsError: isErr, + }) + } + } +} diff --git a/pkg/tools/core.go b/pkg/tools/core.go index d22a78a..a06c968 100644 --- a/pkg/tools/core.go +++ b/pkg/tools/core.go @@ -2,7 +2,6 @@ package tools import ( "github.com/Cidan/ask/pkg/engine" - "google.golang.org/adk/v2/tool/loadartifactstool" ) func init() { @@ -68,7 +67,6 @@ func CoreTools(env *ToolEnv, registry func() []Tool, attachWebSearch bool) []Too AskUserQuestionTool(env), EndTurnTool(env), SearchToolsTool(registry), - loadartifactstool.New(), } // Add workflow tools @@ -98,7 +96,7 @@ func IsCoreTool(name string) bool { case "read", "write", "edit", "glob", "grep", "ls", "bash", "job_output", "job_kill", "fetch", "todos", "task", "ask_user_question", "end_turn", "search_tools", "invoke_tool", "web_search", "workflow_list", "workflow_get", "workflow_create", "workflow_edit", - "workflow_delete", "workflow_copy", "clear_plans", "load_memory", "preload_memory", "load_artifacts": + "workflow_delete", "workflow_copy", "load_memory", "preload_memory": return true default: return false diff --git a/pkg/tools/core_test.go b/pkg/tools/core_test.go deleted file mode 100644 index 05c39f6..0000000 --- a/pkg/tools/core_test.go +++ /dev/null @@ -1,26 +0,0 @@ -package tools - -import ( - "testing" -) - -func TestCoreTools_IncludesLoadArtifacts(t *testing.T) { - env := NewToolEnv(t.TempDir(), 0, true, false, nil, nil) - coreTools := CoreTools(env, nil, true) - - foundArtifactsTool := false - for _, tool := range coreTools { - if tool != nil && tool.Name() == "load_artifacts" { - foundArtifactsTool = true - break - } - } - - if !foundArtifactsTool { - t.Errorf("expected load_artifacts tool to be present in CoreTools") - } - - if !IsCoreTool("load_artifacts") { - t.Errorf("expected IsCoreTool(load_artifacts) to be true") - } -} diff --git a/pkg/tools/file.go b/pkg/tools/file.go index 79fa76b..c5b1386 100644 --- a/pkg/tools/file.go +++ b/pkg/tools/file.go @@ -8,7 +8,6 @@ import ( "path/filepath" "strings" - "github.com/Cidan/ask/pkg/workflow" ) const ReadToolDescription = `Read a file from the filesystem. Returns the content with 1-based line numbers (cat -n format). Use offset/limit for large files; lines longer than 2000 chars are truncated. Reading a file is required before editing or overwriting it.` @@ -128,10 +127,8 @@ func WriteTool(env *ToolEnv) Tool { return NewTextErrorResponse("file_path is required"), nil } path := env.AbsPath(p.FilePath) - if !workflow.IsPathUnderWorkflowPlans(env.Cwd, path) { - if notice := env.RequireTodosNotice(); notice != "" { - return NewTextResponse(notice), nil - } + if notice := env.RequireTodosNotice(); notice != "" { + return NewTextResponse(notice), nil } oldContent := "" mode := os.FileMode(0o644) @@ -202,10 +199,8 @@ func EditTool(env *ToolEnv) Tool { return NewTextErrorResponse("old_string and new_string are identical — nothing to do"), nil } path := env.AbsPath(p.FilePath) - if !workflow.IsPathUnderWorkflowPlans(env.Cwd, path) { - if notice := env.RequireTodosNotice(); notice != "" { - return NewTextResponse(notice), nil - } + if notice := env.RequireTodosNotice(); notice != "" { + return NewTextResponse(notice), nil } if p.OldString == "" { diff --git a/pkg/tools/registry.go b/pkg/tools/registry.go index 90b2fa3..f6b34c5 100644 --- a/pkg/tools/registry.go +++ b/pkg/tools/registry.go @@ -12,7 +12,7 @@ import ( const SearchToolsDescription = `Search the tool registry for tools that are not listed in your tool definitions. -Beyond your core tools, ask keeps a registry of additional tools — issue tracking (linear_*) and external MCP integrations (mcp____). They are real, callable tools; they are just not included in your tool definitions to keep your context small. (The ask-built-in workflow_* tools and clear_plans are core exceptions — they live on the wire, not in the registry, because the two-stage workflow guard forces the model to call them directly.) +Beyond your core tools, ask keeps a registry of additional tools — issue tracking (linear_*) and external MCP integrations (mcp____). They are real, callable tools; they are just not included in your tool definitions to keep your context small. (The ask-built-in workflow_* tools are core exceptions — they live on the wire, not in the registry, because the two-stage workflow guard forces the model to call them directly.) Query syntax: "*" lists every registry tool; a trailing * does prefix matching (e.g. "linear_*"); anything else is a case-insensitive substring match against tool names and descriptions. Each result carries the tool's name, description, and full input_schema — everything needed to call it through invoke_tool.` diff --git a/pkg/tools/workflow.go b/pkg/tools/workflow.go index f6aaa7a..368ce9b 100644 --- a/pkg/tools/workflow.go +++ b/pkg/tools/workflow.go @@ -37,8 +37,6 @@ When the name exists in multiple scopes you must pass scope to pick which copy t WorkflowCopyToolDescription = `Copy a workflow between scopes (or duplicate it within one). 'to' is the destination scope: 'repo' makes a workflow repo-local (a committed JSON file under /.ask/workflows/ that the whole team can use), 'user' copies it into the machine-local ask.json, 'global' copies it into ~/.config/ask/workflows/ (machine-local, visible from every project).` - - ClearPlansToolDescription = `Clear the workflow plans directory (ask/plans/). Removes all files and subdirectories under ask/plans/ but leaves the directory itself. Call this before starting a new workflow run to ensure no stale plan data from a previous run interferes with the next workflow.` ) type WorkflowListInput struct{} @@ -146,13 +144,7 @@ type WorkflowCopyOutput struct { Workflow WorkflowDefView `json:"workflow"` } -type ClearPlansInput struct{} - -type ClearPlansOutput struct { - Cleared bool `json:"cleared"` -} - -// WorkflowTools returns all 7 workflow core tools. +// WorkflowTools returns the workflow core tools. func WorkflowTools(env *ToolEnv) []Tool { cwd := func() string { return env.Cwd } return []Tool{ @@ -181,10 +173,6 @@ func WorkflowTools(env *ToolEnv) []Tool { func(_ context.Context, in WorkflowCopyInput) (*mcp.CallToolResult, WorkflowCopyOutput, error) { return WorkflowCopyCore(cwd(), in) }), - NativeBridgeTool("clear_plans", ClearPlansToolDescription, - func(_ context.Context, in ClearPlansInput) (*mcp.CallToolResult, ClearPlansOutput, error) { - return ClearPlansCore(cwd(), in) - }), } } @@ -434,10 +422,3 @@ func WorkflowCopyCore(cwd string, in WorkflowCopyInput) (*mcp.CallToolResult, Wo } return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: fmt.Sprintf("copied workflow %s to %s scope as %s", in.Name, dstScope, dstName)}}}, WorkflowCopyOutput{Workflow: defToDefView(copied)}, nil } - -func ClearPlansCore(cwd string, in ClearPlansInput) (*mcp.CallToolResult, ClearPlansOutput, error) { - if err := workflow.ClearWorkflowPlans(cwd); err != nil { - return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: err.Error()}}, IsError: true}, ClearPlansOutput{}, nil - } - return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: "ask/plans/ cleared"}}}, ClearPlansOutput{Cleared: true}, nil -} diff --git a/pkg/tools/workflow_test.go b/pkg/tools/workflow_test.go index e316af0..2391284 100644 --- a/pkg/tools/workflow_test.go +++ b/pkg/tools/workflow_test.go @@ -24,7 +24,6 @@ func TestWorkflowTools_CoversEveryWorkflowTool(t *testing.T) { want := []string{ "workflow_list", "workflow_get", "workflow_create", "workflow_edit", "workflow_delete", "workflow_copy", - "clear_plans", } got := map[string]bool{} for _, tool := range WorkflowTools(env) { @@ -79,22 +78,6 @@ func TestWorkflowCRUDRoundTrip(t *testing.T) { } } -func TestClearPlansTool(t *testing.T) { - env, _ := newTestToolEnv(t) - clearTool := workflowToolByName(t, env, "clear_plans") - - writeTestFile(t, env.Cwd, "ask/plans/start/plan.md", "# Plan") - writeTestFile(t, env.Cwd, "ask/plans/step-1/notes.md", "Notes") - - resp, err := RunToolWithJSON(context.Background(), clearTool, `{}`) - if err != nil || resp.IsError { - t.Fatalf("clear_plans failed: %+v %v", resp, err) - } - - if !strings.Contains(resp.Content, "cleared") { - t.Errorf("unexpected clear response: %q", resp.Content) - } -} func TestWorkflowTools_LoopWorkflowPromptAndExitConditionPreservation(t *testing.T) { t.Setenv("HOME", t.TempDir()) diff --git a/pkg/workflow/compile.go b/pkg/workflow/compile.go new file mode 100644 index 0000000..561f423 --- /dev/null +++ b/pkg/workflow/compile.go @@ -0,0 +1,325 @@ +package workflow + +import ( + "context" + "errors" + "fmt" + "strings" + "unicode" + + "google.golang.org/adk/v2/agent" + "google.golang.org/adk/v2/agent/llmagent" + "google.golang.org/adk/v2/agent/workflowagents/loopagent" + "google.golang.org/adk/v2/model" + "google.golang.org/adk/v2/tool" + "google.golang.org/adk/v2/tool/exitlooptool" + adkworkflow "google.golang.org/adk/v2/workflow" +) + +// WorkflowAgentConfig carries everything the compiler needs to turn a +// Def into a runnable ADK graph. The builder callbacks are the seam the +// engine and the TUI fill in with their own model/tool wiring, and the +// seam tests swap for fakes. +type WorkflowAgentConfig struct { + Def Def + Source Source + Cwd string + TabID int + + // ModelBuilder resolves the LLM for one step. Required. + ModelBuilder func(ctx context.Context, step Step) (model.LLM, error) + // ToolsBuilder and ToolsetsBuilder supply the step's tool surface. + ToolsBuilder func(ctx context.Context, step Step, inLoop bool) ([]tool.Tool, error) + ToolsetsBuilder func(ctx context.Context, step Step, inLoop bool) ([]tool.Toolset, error) + // InstructionBuilder renders the step's system instruction. Defaults + // to BuildStepInstruction. + InstructionBuilder func(step Step, src Source, pc *StepPromptCtx) string + // MaxRetries bounds per-node retries on failure. Zero means + // workflowDefaultMaxRetries; negative disables retries. + MaxRetries int +} + +// workflowDefaultMaxRetries is the per-node retry budget. Replaces the +// runner's hand-rolled stepErrorRetry loop with ADK's scheduler-level +// RetryConfig, which also handles the backoff. +const workflowDefaultMaxRetries = 3 + +// Compiled is a Def rendered as an executable ADK graph, plus the lookup +// the event adapter needs to attribute events back to steps. +type Compiled struct { + Workflow *adkworkflow.Workflow + // StepIndexByAgent maps an emitted event's Author (an ADK agent + // name) to the top-level step index it belongs to. Inner loop + // agents map to their containing loop step, so the UI reports + // progress against the def the user authored rather than against + // compiler-generated node names. + StepIndexByAgent map[string]int + // StepNameByAgent maps an agent name back to the step name the + // user wrote, which sanitisation and de-duplication may have + // changed. + StepNameByAgent map[string]string +} + +// StepIndex resolves an event author to a top-level step index. +func (c *Compiled) StepIndex(author string) (int, bool) { + if c == nil || author == "" { + return 0, false + } + idx, ok := c.StepIndexByAgent[author] + return idx, ok +} + +// CompileWorkflow compiles a Def into an ADK workflow graph. +// +// Every top-level step becomes one node, chained Start -> n0 -> n1 -> …: +// +// - An agent step becomes an AgentNode wrapping an llmagent. +// - A loop step becomes an AgentNode wrapping a loopagent whose +// sub-agents are the inner steps, each carrying ADK's exit_loop +// tool. Any inner step calling exit_loop escalates and ends the +// loop; otherwise it runs to MaxIterations. +// +// Two llmagent settings carry the workflow semantics and must not be +// dropped: +// +// - IncludeContentsNone gives each step its own context. Without it a +// step inherits every prior step's events, and ADK renders those +// foreign events as prose — every tool call and every full tool +// result — so step 3 would carry steps 1 and 2 in full. +// - InstructionProvider, never Config.Instruction. Step prompts are +// user-authored and routinely contain braces; a static Instruction +// is run through ADK's state interpolator and hard-fails the +// invocation on the first `{...}`. +func CompileWorkflow(ctx context.Context, cfg WorkflowAgentConfig) (*Compiled, error) { + if err := cfg.Def.Validate(); err != nil { + return nil, err + } + if cfg.ModelBuilder == nil { + return nil, errors.New("workflow compile: model builder is required") + } + + names := newAgentNamer() + out := &Compiled{ + StepIndexByAgent: map[string]int{}, + StepNameByAgent: map[string]string{}, + } + + var nodes []adkworkflow.Node + for i, top := range cfg.Def.Steps { + isFinal := i == len(cfg.Def.Steps)-1 + + var node adkworkflow.Node + var err error + if top.IsLoop() { + node, err = compileLoopNode(ctx, cfg, top, i, isFinal, names, out) + } else { + node, err = compileStepNode(ctx, cfg, top, i, isFinal, nil, names, out) + } + if err != nil { + return nil, err + } + nodes = append(nodes, node) + } + if len(nodes) == 0 { + return nil, errors.New("workflow compile: definition has no executable steps") + } + + edges := append([]adkworkflow.Edge{{From: adkworkflow.Start, To: nodes[0]}}, adkworkflow.Chain(nodes...)...) + wf, err := adkworkflow.New(cfg.Def.Name, edges) + if err != nil { + return nil, fmt.Errorf("workflow compile: %w", err) + } + out.Workflow = wf + return out, nil +} + +// compileStepNode builds the AgentNode for one agent step. pc carries +// the loop framing when the step is an inner loop step. +func compileStepNode(ctx context.Context, cfg WorkflowAgentConfig, step Step, stepIdx int, isFinal bool, loop *LoopPromptCtx, names *agentNamer, out *Compiled) (adkworkflow.Node, error) { + ag, err := buildStepAgent(ctx, cfg, step, stepIdx, isFinal, loop, names, out) + if err != nil { + return nil, err + } + return adkworkflow.NewAgentNode(ag, nodeConfig(cfg)) +} + +// compileLoopNode wraps a loop step's inner agents in an ADK loopagent. +func compileLoopNode(ctx context.Context, cfg WorkflowAgentConfig, loopStep Step, stepIdx int, isFinal bool, names *agentNamer, out *Compiled) (adkworkflow.Node, error) { + maxIter := cfg.Def.EffectiveMaxIterations(loopStep) + + var inner []agent.Agent + for j, innerStep := range loopStep.Steps { + pc := &LoopPromptCtx{ + Name: loopStep.Name, + MaxIterations: maxIter, + ExitCondition: loopStep.ExitCondition, + IsTail: j == len(loopStep.Steps)-1, + } + ag, err := buildStepAgent(ctx, cfg, innerStep, stepIdx, isFinal && pc.IsTail, pc, names, out) + if err != nil { + return nil, err + } + inner = append(inner, ag) + } + + loopName := names.claim(loopStep.Name, "loop") + out.StepIndexByAgent[loopName] = stepIdx + out.StepNameByAgent[loopName] = loopStep.Name + + loopAg, err := loopagent.New(loopagent.Config{ + AgentConfig: agent.Config{ + Name: loopName, + Description: loopStep.ExitCondition, + SubAgents: inner, + }, + MaxIterations: uint(maxIter), + }) + if err != nil { + return nil, fmt.Errorf("workflow compile: loop %q: %w", loopStep.Name, err) + } + return adkworkflow.NewAgentNode(loopAg, nodeConfig(cfg)) +} + +func buildStepAgent(ctx context.Context, cfg WorkflowAgentConfig, step Step, stepIdx int, isFinal bool, loop *LoopPromptCtx, names *agentNamer, out *Compiled) (agent.Agent, error) { + llm, err := cfg.ModelBuilder(ctx, step) + if err != nil { + return nil, fmt.Errorf("workflow compile: model for step %q: %w", step.Name, err) + } + + inLoop := loop != nil + + var tools []tool.Tool + if cfg.ToolsBuilder != nil { + tools, err = cfg.ToolsBuilder(ctx, step, inLoop) + if err != nil { + return nil, fmt.Errorf("workflow compile: tools for step %q: %w", step.Name, err) + } + } + if inLoop { + tools, err = withExitLoopTool(tools) + if err != nil { + return nil, fmt.Errorf("workflow compile: step %q: %w", step.Name, err) + } + } + + var toolsets []tool.Toolset + if cfg.ToolsetsBuilder != nil { + toolsets, err = cfg.ToolsetsBuilder(ctx, step, inLoop) + if err != nil { + return nil, fmt.Errorf("workflow compile: toolsets for step %q: %w", step.Name, err) + } + } + + build := cfg.InstructionBuilder + if build == nil { + build = BuildStepInstruction + } + instruction := build(step, cfg.Source, &StepPromptCtx{ + Loop: loop, + IsStartStep: stepIdx == 0, + IsWorkflowFinalStep: isFinal, + }) + + name := names.claim(step.Name, "step") + out.StepIndexByAgent[name] = stepIdx + out.StepNameByAgent[name] = step.Name + + return llmagent.New(llmagent.Config{ + Name: name, + Description: firstLine(step.Prompt), + Model: llm, + // Never Config.Instruction: step prompts are user-authored and + // ADK interpolates that field, failing the run on any brace. + InstructionProvider: literalInstruction(instruction), + // Each step gets its own context; see CompileWorkflow. + IncludeContents: llmagent.IncludeContentsNone, + Tools: tools, + Toolsets: toolsets, + }) +} + +// literalInstruction adapts a fixed string to an InstructionProvider so +// ADK treats it as text rather than as a `{placeholder}` template. +func literalInstruction(s string) llmagent.InstructionProvider { + return func(agent.ReadonlyContext) (string, error) { return s, nil } +} + +func withExitLoopTool(tools []tool.Tool) ([]tool.Tool, error) { + exitTool, err := exitlooptool.New() + if err != nil { + return nil, fmt.Errorf("exit_loop tool: %w", err) + } + for _, t := range tools { + if t != nil && t.Name() == exitTool.Name() { + return tools, nil + } + } + return append(tools, exitTool), nil +} + +func nodeConfig(cfg WorkflowAgentConfig) adkworkflow.NodeConfig { + retries := cfg.MaxRetries + if retries == 0 { + retries = workflowDefaultMaxRetries + } + if retries < 0 { + return adkworkflow.NodeConfig{} + } + rc := adkworkflow.DefaultRetryConfig() + rc.MaxAttempts = retries + return adkworkflow.NodeConfig{RetryConfig: rc} +} + +func firstLine(s string) string { + s = strings.TrimSpace(s) + if i := strings.IndexByte(s, '\n'); i >= 0 { + s = s[:i] + } + if len(s) > 120 { + s = s[:120] + } + return s +} + +// agentNamer turns user-written step names into ADK agent names. ADK +// requires them unique within a graph and rejects "user"; step names are +// free text and not checked for uniqueness, so both have to be enforced +// here rather than assumed. +type agentNamer struct { + used map[string]bool +} + +func newAgentNamer() *agentNamer { return &agentNamer{used: map[string]bool{}} } + +func (n *agentNamer) claim(raw, fallback string) string { + base := sanitizeAgentName(raw) + if base == "" || strings.EqualFold(base, "user") { + base = fallback + } + name := base + for i := 2; n.used[name]; i++ { + name = fmt.Sprintf("%s_%d", base, i) + } + n.used[name] = true + return name +} + +func sanitizeAgentName(s string) string { + var b strings.Builder + for _, r := range s { + switch { + case unicode.IsLetter(r) || unicode.IsDigit(r): + b.WriteRune(r) + case r == '_' || r == '-' || r == ' ': + b.WriteByte('_') + } + } + out := strings.Trim(b.String(), "_") + if out == "" { + return "" + } + if r := rune(out[0]); unicode.IsDigit(r) { + out = "_" + out + } + return out +} diff --git a/pkg/workflow/compile_test.go b/pkg/workflow/compile_test.go new file mode 100644 index 0000000..7ad1b0f --- /dev/null +++ b/pkg/workflow/compile_test.go @@ -0,0 +1,233 @@ +package workflow + +import ( + "context" + "iter" + "sort" + "strings" + "testing" + + "google.golang.org/adk/v2/model" + "google.golang.org/adk/v2/tool" +) + +type fakeLLM struct{ name string } + +func (f *fakeLLM) Name() string { return f.name } +func (f *fakeLLM) GenerateContent(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { + return func(yield func(*model.LLMResponse, error) bool) {} +} + +func testCompileConfig(def Def) WorkflowAgentConfig { + return WorkflowAgentConfig{ + Def: def, + Source: NewTextSource(1, "source"), + Cwd: "/tmp/proj", + ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { + return &fakeLLM{name: "fake"}, nil + }, + } +} + +func agentNames(c *Compiled) []string { + out := make([]string, 0, len(c.StepIndexByAgent)) + for name := range c.StepIndexByAgent { + out = append(out, name) + } + sort.Strings(out) + return out +} + +func TestCompileWorkflow_Linear(t *testing.T) { + def := Def{Name: "wf", Steps: []Step{ + {Name: "plan", Prompt: "make a plan"}, + {Name: "implement", Prompt: "do it"}, + {Name: "review", Prompt: "check it"}, + }} + + c, err := CompileWorkflow(context.Background(), testCompileConfig(def)) + if err != nil { + t.Fatalf("compile: %v", err) + } + if c.Workflow == nil { + t.Fatal("compiled workflow is nil") + } + if c.Workflow.Name() != "wf" { + t.Errorf("workflow name = %q, want wf", c.Workflow.Name()) + } + + for i, want := range []string{"plan", "implement", "review"} { + idx, ok := c.StepIndex(want) + if !ok { + t.Fatalf("no step index recorded for agent %q (have %v)", want, agentNames(c)) + } + if idx != i { + t.Errorf("agent %q maps to step %d, want %d", want, idx, i) + } + if got := c.StepNameByAgent[want]; got != want { + t.Errorf("agent %q maps to step name %q", want, got) + } + } +} + +// Inner loop agents report against the loop step the user authored, so +// the UI shows progress for "review-loop" rather than for compiler +// -generated inner node names. +func TestCompileWorkflow_LoopInnerAgentsMapToLoopStep(t *testing.T) { + def := Def{Name: "wf", Steps: []Step{ + {Name: "plan", Prompt: "plan"}, + {Name: "fix-loop", Kind: "loop", MaxIterations: 3, ExitCondition: "tests pass", Steps: []Step{ + {Name: "edit", Prompt: "edit"}, + {Name: "verify", Prompt: "verify"}, + }}, + }} + + c, err := CompileWorkflow(context.Background(), testCompileConfig(def)) + if err != nil { + t.Fatalf("compile: %v", err) + } + for _, agent := range []string{"fix_loop", "edit", "verify"} { + idx, ok := c.StepIndex(agent) + if !ok { + t.Fatalf("no mapping for %q (have %v)", agent, agentNames(c)) + } + if idx != 1 { + t.Errorf("agent %q maps to step %d, want the loop step 1", agent, idx) + } + } + if idx, _ := c.StepIndex("plan"); idx != 0 { + t.Errorf("linear step should still map to 0, got %d", idx) + } +} + +// ADK requires agent names unique within a graph and rejects "user". +// Step names are free text and never checked for uniqueness, so the +// compiler has to enforce both. +func TestCompileWorkflow_AgentNamesSanitizedAndUnique(t *testing.T) { + def := Def{Name: "wf", Steps: []Step{ + {Name: "do the thing", Prompt: "a"}, + {Name: "do the thing", Prompt: "b"}, + {Name: "user", Prompt: "c"}, + {Name: "!!!", Prompt: "d"}, + {Name: "2fast", Prompt: "e"}, + }} + + c, err := CompileWorkflow(context.Background(), testCompileConfig(def)) + if err != nil { + t.Fatalf("compile: %v", err) + } + names := agentNames(c) + if len(names) != 5 { + t.Fatalf("want 5 distinct agent names, got %d: %v", len(names), names) + } + seen := map[string]bool{} + for _, n := range names { + if seen[n] { + t.Errorf("duplicate agent name %q", n) + } + seen[n] = true + if n == "user" { + t.Error(`"user" is reserved by ADK and must not be used as an agent name`) + } + if n == "" { + t.Error("empty agent name") + } + } + if _, ok := c.StepIndexByAgent["do_the_thing"]; !ok { + t.Errorf("expected spaces to become underscores, got %v", names) + } + if _, ok := c.StepIndexByAgent["do_the_thing_2"]; !ok { + t.Errorf("expected the duplicate to be suffixed, got %v", names) + } +} + +func TestCompileWorkflow_Errors(t *testing.T) { + t.Run("invalid definition", func(t *testing.T) { + if _, err := CompileWorkflow(context.Background(), testCompileConfig(Def{Name: "wf"})); err == nil { + t.Error("a def with no steps must not compile") + } + }) + t.Run("missing model builder", func(t *testing.T) { + cfg := testCompileConfig(Def{Name: "wf", Steps: []Step{{Name: "a", Prompt: "a"}}}) + cfg.ModelBuilder = nil + if _, err := CompileWorkflow(context.Background(), cfg); err == nil { + t.Error("compiling without a model builder must fail") + } + }) + t.Run("model builder failure names the step", func(t *testing.T) { + cfg := testCompileConfig(Def{Name: "wf", Steps: []Step{{Name: "broken", Prompt: "x"}}}) + cfg.ModelBuilder = func(ctx context.Context, step Step) (model.LLM, error) { + return nil, context.DeadlineExceeded + } + _, err := CompileWorkflow(context.Background(), cfg) + if err == nil || !strings.Contains(err.Error(), "broken") { + t.Errorf("error should name the failing step, got %v", err) + } + }) +} + +// Loop steps get ADK's exit_loop tool, which is how a step breaks out: +// it sets Actions.Escalate, which is what loopagent watches for. +func TestWithExitLoopTool(t *testing.T) { + got, err := withExitLoopTool(nil) + if err != nil { + t.Fatalf("withExitLoopTool: %v", err) + } + if len(got) != 1 || got[0].Name() != "exit_loop" { + t.Fatalf("expected exit_loop to be added, got %v", toolNames(got)) + } + + again, err := withExitLoopTool(got) + if err != nil { + t.Fatalf("withExitLoopTool: %v", err) + } + if len(again) != 1 { + t.Errorf("exit_loop must not be added twice, got %v", toolNames(again)) + } +} + +func toolNames(ts []tool.Tool) []string { + out := make([]string, 0, len(ts)) + for _, t := range ts { + if t != nil { + out = append(out, t.Name()) + } + } + return out +} + +func TestBuildStepInstruction(t *testing.T) { + src := NewTextSource(1, "the source") + step := Step{Name: "plan", Prompt: "Write the plan."} + + got := BuildStepInstruction(step, src, &StepPromptCtx{IsStartStep: true}) + if !strings.Contains(got, "Write the plan.") { + t.Errorf("instruction must carry the author's prompt: %q", got) + } + if !strings.Contains(got, "end_turn") { + t.Errorf("instruction must carry the end_turn contract: %q", got) + } + if strings.Contains(got, "Previous step output:") { + t.Errorf("previous-step threading belongs to the graph now: %q", got) + } + if strings.Contains(got, "notes director") || strings.Contains(got, "ask/plans") { + t.Errorf("notes directories are gone: %q", got) + } +} + +func TestBuildStepInstruction_LoopFraming(t *testing.T) { + src := NewTextSource(1, "src") + step := Step{Name: "verify", Prompt: "Check."} + loop := &LoopPromptCtx{Name: "fix", MaxIterations: 5, ExitCondition: "tests pass", IsTail: true} + + got := BuildStepInstruction(step, src, &StepPromptCtx{Loop: loop}) + for _, want := range []string{"fix", "5", "tests pass", "exit_loop"} { + if !strings.Contains(got, want) { + t.Errorf("loop instruction missing %q: %q", want, got) + } + } + // Breaking is exit_loop now, not an end_turn argument. + if strings.Contains(got, `decision`) { + t.Errorf("loop control moved to exit_loop; instruction should not mention a decision arg: %q", got) + } +} diff --git a/pkg/workflow/graph.go b/pkg/workflow/graph.go deleted file mode 100644 index 0083d72..0000000 --- a/pkg/workflow/graph.go +++ /dev/null @@ -1,194 +0,0 @@ -package workflow - -import ( - "context" - "errors" - "fmt" - - "google.golang.org/adk/v2/agent" - "google.golang.org/adk/v2/agent/llmagent" - "google.golang.org/adk/v2/agent/workflowagents/loopagent" - "google.golang.org/adk/v2/tool" - "google.golang.org/adk/v2/tool/exitlooptool" - adkworkflow "google.golang.org/adk/v2/workflow" -) - -// CompileDefToADKWorkflow converts a workflow.Def into an ADK 2.0 directed acyclic graph (*adkworkflow.Workflow). -// It constructs agent nodes for each top-level step and connects them using standard workflow edges and routes. -func CompileDefToADKWorkflow(ctx context.Context, cfg WorkflowAgentConfig) (*adkworkflow.Workflow, error) { - if err := cfg.Def.Validate(); err != nil { - return nil, err - } - if cfg.ModelBuilder == nil { - return nil, errors.New("model builder is required") - } - - var nodes []adkworkflow.Node - var prevNotesDir string - - for i, top := range cfg.Def.Steps { - isFinalStep := i == len(cfg.Def.Steps)-1 - - if top.Kind == "loop" { - if len(top.Steps) == 0 { - continue - } - var innerAgents []agent.Agent - for innerIdx, innerStep := range top.Steps { - isLoopStart := i == 0 && innerIdx == 0 - var notesDir string - if isLoopStart { - notesDir = StartPlanDir(cfg.Cwd) - } else { - notesDir = StepNotesDir(cfg.Cwd, innerStep.Name, top.Name, 1) - } - - llm, err := cfg.ModelBuilder(ctx, innerStep) - if err != nil { - return nil, fmt.Errorf("failed to build model for step %q: %w", innerStep.Name, err) - } - - var tools []tool.Tool - if cfg.ToolsBuilder != nil { - builtTools, err := cfg.ToolsBuilder(ctx, innerStep, true) - if err != nil { - return nil, fmt.Errorf("failed to build tools for step %q: %w", innerStep.Name, err) - } - tools = append(tools, builtTools...) - } - - exitTool, err := exitlooptool.New() - if err != nil { - return nil, fmt.Errorf("failed to create exitloop tool: %w", err) - } - hasExitTool := false - for _, t := range tools { - if t != nil && t.Name() == exitTool.Name() { - hasExitTool = true - break - } - } - if !hasExitTool { - tools = append(tools, exitTool) - } - - var toolsets []tool.Toolset - if cfg.ToolsetsBuilder != nil { - ts, err := cfg.ToolsetsBuilder(ctx, innerStep, true) - if err != nil { - return nil, fmt.Errorf("failed to build toolsets for step %q: %w", innerStep.Name, err) - } - toolsets = ts - } - - instruction := innerStep.Prompt - if cfg.InstructionBuilder != nil { - loopCtx := &LoopPromptCtx{ - Name: top.Name, - Iteration: 1, - MaxIterations: cfg.Def.EffectiveMaxIterations(top), - ExitCondition: top.ExitCondition, - IsTail: innerIdx == len(top.Steps)-1, - } - instruction = cfg.InstructionBuilder(innerStep, isLoopStart, isFinalStep, loopCtx, notesDir, prevNotesDir) - } - - innerAg, err := llmagent.New(llmagent.Config{ - Name: innerStep.Name, - Description: innerStep.Prompt, - Model: llm, - Instruction: instruction, - Tools: tools, - Toolsets: toolsets, - }) - if err != nil { - return nil, fmt.Errorf("failed to create inner step agent %q: %w", innerStep.Name, err) - } - innerAgents = append(innerAgents, innerAg) - prevNotesDir = notesDir - } - - loopAg, err := loopagent.New(loopagent.Config{ - AgentConfig: agent.Config{ - Name: top.Name, - Description: top.ExitCondition, - SubAgents: innerAgents, - }, - MaxIterations: uint(cfg.Def.EffectiveMaxIterations(top)), - }) - if err != nil { - return nil, fmt.Errorf("failed to create loop agent %q: %w", top.Name, err) - } - - loopNode, err := adkworkflow.NewAgentNode(loopAg, adkworkflow.NodeConfig{}) - if err != nil { - return nil, fmt.Errorf("failed to create agent node for loop %q: %w", top.Name, err) - } - nodes = append(nodes, loopNode) - continue - } - - // Linear step - isStart := i == 0 - var notesDir string - if isStart { - notesDir = StartPlanDir(cfg.Cwd) - } else { - notesDir = StepNotesDir(cfg.Cwd, top.Name, "", 0) - } - - llm, err := cfg.ModelBuilder(ctx, top) - if err != nil { - return nil, fmt.Errorf("failed to build model for step %q: %w", top.Name, err) - } - - var tools []tool.Tool - if cfg.ToolsBuilder != nil { - builtTools, err := cfg.ToolsBuilder(ctx, top, false) - if err != nil { - return nil, fmt.Errorf("failed to build tools for step %q: %w", top.Name, err) - } - tools = append(tools, builtTools...) - } - - var toolsets []tool.Toolset - if cfg.ToolsetsBuilder != nil { - ts, err := cfg.ToolsetsBuilder(ctx, top, false) - if err != nil { - return nil, fmt.Errorf("failed to build toolsets for step %q: %w", top.Name, err) - } - toolsets = ts - } - - instruction := top.Prompt - if cfg.InstructionBuilder != nil { - instruction = cfg.InstructionBuilder(top, isStart, isFinalStep, nil, notesDir, prevNotesDir) - } - - stepAg, err := llmagent.New(llmagent.Config{ - Name: top.Name, - Description: top.Prompt, - Model: llm, - Instruction: instruction, - Tools: tools, - Toolsets: toolsets, - }) - if err != nil { - return nil, fmt.Errorf("failed to create step agent %q: %w", top.Name, err) - } - - stepNode, err := adkworkflow.NewAgentNode(stepAg, adkworkflow.NodeConfig{}) - if err != nil { - return nil, fmt.Errorf("failed to create agent node for step %q: %w", top.Name, err) - } - nodes = append(nodes, stepNode) - prevNotesDir = notesDir - } - - if len(nodes) == 0 { - return nil, errors.New("workflow definition has no executable steps") - } - - edges := append([]adkworkflow.Edge{{From: adkworkflow.Start, To: nodes[0]}}, adkworkflow.Chain(nodes...)...) - return adkworkflow.New(cfg.Def.Name, edges) -} diff --git a/pkg/workflow/graph_test.go b/pkg/workflow/graph_test.go deleted file mode 100644 index b916f8b..0000000 --- a/pkg/workflow/graph_test.go +++ /dev/null @@ -1,200 +0,0 @@ -package workflow - -import ( - "context" - "iter" - "testing" - - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/tool" - "google.golang.org/genai" -) - -type fakeModel struct{} - -func (f *fakeModel) Name() string { return "fake-model" } - -func (f *fakeModel) GenerateContent(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { - return func(yield func(*model.LLMResponse, error) bool) { - resp := &model.LLMResponse{ - Content: genai.NewContentFromText("done", genai.RoleModel), - } - yield(resp, nil) - } -} - -func TestCompileDefToADKWorkflow_Linear(t *testing.T) { - def := Def{ - Name: "linear-test", - Description: "test linear workflow", - Steps: []Step{ - {Name: "step-1", Prompt: "do step 1"}, - {Name: "step-2", Prompt: "do step 2"}, - }, - } - - cfg := WorkflowAgentConfig{ - Def: def, - Cwd: t.TempDir(), - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &fakeModel{}, nil - }, - ToolsBuilder: func(ctx context.Context, step Step, isLoop bool) ([]tool.Tool, error) { - return nil, nil - }, - } - - wf, err := CompileDefToADKWorkflow(context.Background(), cfg) - if err != nil { - t.Fatalf("unexpected error compiling workflow: %v", err) - } - if wf == nil { - t.Fatal("expected non-nil compiled workflow") - } - if wf.Name() != "linear-test" { - t.Errorf("expected workflow name linear-test, got %q", wf.Name()) - } -} - -func TestCompileDefToADKWorkflow_Loop(t *testing.T) { - def := Def{ - Name: "loop-test", - Description: "test loop workflow", - Steps: []Step{ - {Name: "prep", Prompt: "prepare"}, - { - Name: "eval-loop", - Kind: "loop", - ExitCondition: "all tests pass", - MaxIterations: 3, - Steps: []Step{ - {Name: "execute", Prompt: "run tests"}, - {Name: "evaluate", Prompt: "assess results"}, - }, - }, - {Name: "cleanup", Prompt: "clean up"}, - }, - } - - cfg := WorkflowAgentConfig{ - Def: def, - Cwd: t.TempDir(), - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &fakeModel{}, nil - }, - ToolsBuilder: func(ctx context.Context, step Step, isLoop bool) ([]tool.Tool, error) { - return nil, nil - }, - } - - wf, err := CompileDefToADKWorkflow(context.Background(), cfg) - if err != nil { - t.Fatalf("unexpected error compiling loop workflow: %v", err) - } - if wf == nil { - t.Fatal("expected non-nil compiled workflow") - } - if wf.Name() != "loop-test" { - t.Errorf("expected workflow name loop-test, got %q", wf.Name()) - } -} - -func TestCompileDefToADKWorkflow_ValidationErrors(t *testing.T) { - // Missing model builder - def := Def{ - Name: "valid-def", - Steps: []Step{ - {Name: "step-1", Prompt: "do step 1"}, - }, - } - cfg := WorkflowAgentConfig{ - Def: def, - Cwd: t.TempDir(), - } - if _, err := CompileDefToADKWorkflow(context.Background(), cfg); err == nil { - t.Error("expected error when ModelBuilder is nil") - } - - // Invalid definition - invalidDef := Def{ - Name: "", // empty name is invalid - Steps: []Step{ - {Name: "step-1", Prompt: "do step 1"}, - }, - } - cfgInvalid := WorkflowAgentConfig{ - Def: invalidDef, - Cwd: t.TempDir(), - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &fakeModel{}, nil - }, - } - if _, err := CompileDefToADKWorkflow(context.Background(), cfgInvalid); err == nil { - t.Error("expected error for invalid Def") - } -} - -type testGraphListener struct { - NoopRunnerListener - started bool - stepsDone int - done bool -} - -func (l *testGraphListener) OnWorkflowStarted(tabID int, def Def, src Source) { - l.started = true -} - -func (l *testGraphListener) OnWorkflowStepDone(tabID int, stepIdx int, summary string) { - l.stepsDone++ -} - -func (l *testGraphListener) OnWorkflowDone(tabID int, desc string, artifacts []string) { - l.done = true -} - -func TestWorkflowRunner_ADKGraphExecution(t *testing.T) { - def := Def{ - Name: "adk-graph-run", - Description: "executing workflow via adk graph", - Steps: []Step{ - {Name: "step-1", Prompt: "do step 1", Provider: "vertex", Model: "gemini"}, - {Name: "step-2", Prompt: "do step 2", Provider: "vertex", Model: "gemini"}, - }, - } - - listener := &testGraphListener{} - runner := NewRunner(NewTracker(), nil, listener) - - cfg := WorkflowAgentConfig{ - Def: def, - Cwd: t.TempDir(), - TabID: 42, - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &fakeModel{}, nil - }, - ToolsBuilder: func(ctx context.Context, step Step, isLoop bool) ([]tool.Tool, error) { - return nil, nil - }, - } - - state, err := runner.RunGraph(context.Background(), cfg) - if err != nil { - t.Fatalf("unexpected error running adk graph workflow: %v", err) - } - if state == nil || !state.Done { - t.Fatalf("expected completed run state, got %+v", state) - } - if state.StepIdx != 2 { - t.Errorf("expected StepIdx 2, got %d", state.StepIdx) - } - if !listener.started { - t.Error("expected listener OnWorkflowStarted to be called") - } - if listener.stepsDone != 2 { - t.Errorf("expected 2 step done calls, got %d", listener.stepsDone) - } - if !listener.done { - t.Error("expected listener OnWorkflowDone to be called") - } -} diff --git a/pkg/workflow/plans.go b/pkg/workflow/plans.go deleted file mode 100644 index 879dc6f..0000000 --- a/pkg/workflow/plans.go +++ /dev/null @@ -1,183 +0,0 @@ -package workflow - -import ( - "errors" - "fmt" - "os" - "path/filepath" - "strings" - - "github.com/Cidan/ask/pkg/config" -) - -const ( - PlansDirName = "ask/plans" - StartPlanDirName = "start" - StartPlanDirInstruction = "ask/plans/start/ must be a DIRECTORY (not a file) and must contain at least one file. Create the directory, then write one or more files inside it — for example ask/plans/start/plan.md. Do not write a single file named start." -) - -// PlansDir returns the absolute base plans directory for cwd. -func PlansDir(cwd string) string { - if cwd == "" { - return "" - } - root := config.ProjectRoot(cwd) - if root == "" { - root = cwd - } - return filepath.Join(root, filepath.FromSlash(PlansDirName)) -} - -// StartPlanDir returns the absolute path to ask/plans/start/. -func StartPlanDir(cwd string) string { - base := PlansDir(cwd) - if base == "" { - return "" - } - return filepath.Join(base, StartPlanDirName) -} - -// StepNotesDir returns the notes directory for a workflow step or loop iteration. -func StepNotesDir(cwd, stepName, loopName string, iteration int) string { - base := PlansDir(cwd) - if base == "" { - return "" - } - if loopName != "" && iteration > 0 { - return filepath.Join(base, SanitizeStepName(loopName), fmt.Sprintf("%d", iteration)) - } - return filepath.Join(base, SanitizeStepName(stepName)) -} - -// IsPathUnderWorkflowPlans reports whether path is inside the ask/plans/ tree for cwd. -func IsPathUnderWorkflowPlans(cwd, path string) bool { - plansDir := PlansDir(cwd) - if plansDir == "" { - return false - } - rel, err := filepath.Rel(plansDir, path) - if err != nil { - return false - } - if rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return false - } - return true -} - -// SanitizeStepName maps a workflow step name onto a filesystem-safe path component. -func SanitizeStepName(name string) string { - var b strings.Builder - lastDash := false - for _, r := range name { - switch { - case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '.', r == '_', r == '-': - b.WriteRune(r) - lastDash = r == '-' - default: - if !lastDash { - b.WriteByte('-') - lastDash = true - } - } - } - stem := strings.Trim(b.String(), "-.") - if stem == "" { - stem = "step" - } - return stem -} - -// EnsureStepNotesDir verifies that a notes directory exists. -func EnsureStepNotesDir(dir string) error { - if dir == "" { - return errors.New("notes directory path is empty") - } - info, err := os.Stat(dir) - if err != nil { - if os.IsNotExist(err) { - if mkerr := os.MkdirAll(dir, 0o755); mkerr != nil { - return fmt.Errorf("cannot create notes directory %s: %w", dir, mkerr) - } - return nil - } - return fmt.Errorf("cannot read notes directory %s: %w", dir, err) - } - if !info.IsDir() { - return fmt.Errorf("%s exists but is a FILE, not a directory. Remove it, then create it as a directory and write your notes files inside it", dir) - } - return nil -} - -// EnsureStartPlanExists verifies that ask/plans/start/ exists and contains at least one file. -func EnsureStartPlanExists(cwd string) error { - dir := StartPlanDir(cwd) - if dir == "" { - return errors.New("start plan is missing: " + StartPlanDirInstruction) - } - info, err := os.Stat(dir) - if err != nil { - if os.IsNotExist(err) { - return errors.New("start plan is missing: " + StartPlanDirInstruction) - } - return fmt.Errorf("cannot read start plan dir: %w", err) - } - if !info.IsDir() { - return errors.New("ask/plans/start/ exists but is a FILE, not a directory. Remove it, " + StartPlanDirInstruction) - } - entries, err := os.ReadDir(dir) - if err != nil { - return fmt.Errorf("cannot list start plan dir: %w", err) - } - hasFile := false - for _, e := range entries { - if !e.IsDir() { - hasFile = true - break - } - } - if !hasFile { - return errors.New("start plan is empty: " + StartPlanDirInstruction) - } - return nil -} - -// ClearWorkflowPlans removes all files and subdirectories under ask/plans/. -func ClearWorkflowPlans(cwd string) error { - dir := PlansDir(cwd) - if dir == "" { - return errors.New("no project root to locate ask/plans/") - } - info, err := os.Stat(dir) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("cannot read plans dir: %w", err) - } - if !info.IsDir() { - return errors.New("ask/plans exists but is not a directory") - } - entries, err := os.ReadDir(dir) - if err != nil { - return fmt.Errorf("cannot list plans dir: %w", err) - } - for _, e := range entries { - if err := os.RemoveAll(filepath.Join(dir, e.Name())); err != nil { - return fmt.Errorf("cannot remove %s: %w", e.Name(), err) - } - } - return nil -} - -// RemoveAllWorkflowPlans removes the entire ask/plans/ tree. -func RemoveAllWorkflowPlans(cwd string) error { - dir := PlansDir(cwd) - if dir == "" { - return nil - } - if err := os.RemoveAll(dir); err != nil { - return fmt.Errorf("cannot remove plans dir: %w", err) - } - return nil -} diff --git a/pkg/workflow/progress.go b/pkg/workflow/progress.go new file mode 100644 index 0000000..f640504 --- /dev/null +++ b/pkg/workflow/progress.go @@ -0,0 +1,282 @@ +package workflow + +import ( + "strings" + "time" + + "google.golang.org/adk/v2/session" +) + +// Progress turns the ADK event stream of a running workflow graph into +// RunnerListener callbacks. +// +// Every callback is driven by something that actually happened in the +// stream: a step is "started" when an event authored by its agent +// arrives, and "done" when that step's successor starts or the run ends +// cleanly. Steps that never ran are never reported, and a run that fails +// partway reports only the steps that got that far — the previous +// implementation closed out every remaining step as completed and +// hardcoded a successful FinishData, so a chain that died at step 1 of 5 +// showed 5/5 green. +type Progress struct { + compiled *Compiled + def Def + src Source + tabID int + listener RunnerListener + tracker *Tracker + cwd string + + started map[int]bool + done map[int]bool + // summaries holds the latest end_turn summary seen for each step, + // used as the step's completion line. + summaries map[int]string + // text holds the latest assistant text per step, the fallback when + // a step ends without calling end_turn. + text map[int]string + + current int + haveCurr bool + loopIters map[int]int + finishData *FinishData + state *RunState +} + +// NewProgress builds a Progress for one run and emits OnWorkflowStarted. +func NewProgress(compiled *Compiled, def Def, src Source, cwd string, tabID int, listener RunnerListener, tracker *Tracker) *Progress { + if listener == nil { + listener = NoopRunnerListener{} + } + p := &Progress{ + compiled: compiled, + def: def, + src: src, + cwd: cwd, + tabID: tabID, + listener: listener, + tracker: tracker, + started: map[int]bool{}, + done: map[int]bool{}, + summaries: map[int]string{}, + text: map[int]string{}, + loopIters: map[int]int{}, + current: -1, + state: &RunState{ + Workflow: def, + Source: src, + StartedAt: time.Now().UTC(), + }, + } + p.listener.OnWorkflowStarted(tabID, def, src) + if tracker != nil { + tracker.MarkWorking(cwd, src.Key(), def.Name, tabID) + } + return p +} + +// State exposes the run state accumulated so far. +func (p *Progress) State() *RunState { return p.state } + +// Observe consumes one event from the workflow's ADK stream. +func (p *Progress) Observe(ev *session.Event) { + if p == nil || ev == nil || ev.Author == "" || ev.Author == "user" { + return + } + idx, ok := p.compiled.StepIndex(ev.Author) + if !ok { + return + } + + p.enter(idx) + p.captureText(idx, ev) + p.captureToolSignals(idx, ev) +} + +// enter marks a step started, closing out the previous one. A workflow +// graph runs its nodes in order, so the arrival of an event for a later +// step is the signal that the earlier one finished. +func (p *Progress) enter(idx int) { + if p.haveCurr && p.current == idx { + return + } + if p.haveCurr && p.current != idx { + p.complete(p.current) + } + p.current = idx + p.haveCurr = true + p.state.StepIdx = idx + + if p.started[idx] { + // Re-entering a loop node: a new iteration of the same step. + p.loopIters[idx]++ + if step := p.step(idx); step != nil && step.IsLoop() { + p.listener.OnNote(p.tabID, LoopNoteLine(step.Name, + "iteration "+itoa(p.loopIters[idx]+1), "")) + } + return + } + p.started[idx] = true + step := p.step(idx) + if step == nil { + return + } + p.listener.OnWorkflowStepStarted(p.tabID, idx, step.Name, step.Provider, step.Model) + if step.IsLoop() { + p.listener.OnNote(p.tabID, LoopNoteLine(step.Name, "started", + "max "+itoa(p.def.EffectiveMaxIterations(*step))+" iteration(s)")) + } +} + +func (p *Progress) captureText(idx int, ev *session.Event) { + if ev.LLMResponse.Content == nil { + return + } + var b strings.Builder + for _, part := range ev.LLMResponse.Content.Parts { + if part == nil || part.Thought || part.Text == "" { + continue + } + b.WriteString(part.Text) + } + if t := strings.TrimSpace(b.String()); t != "" { + p.text[idx] = t + } +} + +// captureToolSignals reads the two tool calls that carry workflow +// meaning: end_turn's summary (the step's log line) and finish_workflow's +// completion report. +func (p *Progress) captureToolSignals(idx int, ev *session.Event) { + if ev.LLMResponse.Content == nil { + return + } + for _, part := range ev.LLMResponse.Content.Parts { + if part == nil || part.FunctionCall == nil { + continue + } + switch part.FunctionCall.Name { + case "end_turn": + if s, ok := part.FunctionCall.Args["summary"].(string); ok && strings.TrimSpace(s) != "" { + p.summaries[idx] = strings.TrimSpace(s) + } + case "exit_loop": + if step := p.step(idx); step != nil && step.IsLoop() { + p.listener.OnNote(p.tabID, LoopNoteLine(step.Name, "break", "")) + } + case "finish_workflow": + desc, _ := part.FunctionCall.Args["description"].(string) + p.finishData = &FinishData{ + Description: desc, + Artifacts: stringSlice(part.FunctionCall.Args["artifacts"]), + } + } + } +} + +func (p *Progress) complete(idx int) { + if p.done[idx] { + return + } + p.done[idx] = true + step := p.step(idx) + if step == nil { + return + } + summary := p.summaries[idx] + if summary == "" { + summary = firstLine(p.text[idx]) + } + p.listener.OnWorkflowStepDone(p.tabID, idx, summary) +} + +// Finish closes the run. A nil err completes the in-flight step and +// reports success; a non-nil err leaves every unfinished step unfinished +// and reports the failure. +func (p *Progress) Finish(err error) *RunState { + if p == nil { + return nil + } + if err != nil { + p.state.Failed = true + p.state.FailedReason = err.Error() + p.listener.OnWorkflowFailed(p.tabID, err.Error()) + if p.tracker != nil { + p.tracker.MarkFinal(p.cwd, p.src.Key(), p.def.Name, StatusFailed, p.state.StepIdx) + } + return p.state + } + + if p.haveCurr { + p.complete(p.current) + } + p.state.Done = true + p.state.FinishData = p.finishData + + desc := "" + var arts []string + if p.finishData != nil { + desc = p.finishData.Description + arts = p.finishData.Artifacts + } + p.listener.OnWorkflowDone(p.tabID, desc, arts) + if p.tracker != nil { + p.tracker.MarkFinal(p.cwd, p.src.Key(), p.def.Name, StatusDone, p.state.StepIdx) + } + return p.state +} + +// SetFinishData records the run's completion report when it is read +// directly off the tool environment rather than seen in the event +// stream — the finish_workflow tool parks it there as it runs. +func (p *Progress) SetFinishData(fd *FinishData) { + if p == nil || fd == nil { + return + } + p.finishData = fd +} + +func (p *Progress) step(idx int) *Step { + if idx < 0 || idx >= len(p.def.Steps) { + return nil + } + return &p.def.Steps[idx] +} + +func stringSlice(v any) []string { + switch t := v.(type) { + case []string: + return t + case []any: + out := make([]string, 0, len(t)) + for _, item := range t { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out + } + return nil +} + +func itoa(n int) string { + if n == 0 { + return "0" + } + neg := n < 0 + if neg { + n = -n + } + var buf [20]byte + i := len(buf) + for n > 0 { + i-- + buf[i] = byte('0' + n%10) + n /= 10 + } + if neg { + i-- + buf[i] = '-' + } + return string(buf[i:]) +} diff --git a/pkg/workflow/progress_test.go b/pkg/workflow/progress_test.go new file mode 100644 index 0000000..1aa3457 --- /dev/null +++ b/pkg/workflow/progress_test.go @@ -0,0 +1,257 @@ +package workflow + +import ( + "context" + "errors" + "testing" + + "google.golang.org/adk/v2/session" + "google.golang.org/genai" +) + +type recordedCall struct { + kind string + stepIdx int + text string +} + +type recordingListener struct{ calls []recordedCall } + +func (l *recordingListener) OnWorkflowStarted(int, Def, Source) { + l.calls = append(l.calls, recordedCall{kind: "started"}) +} +func (l *recordingListener) OnWorkflowStepStarted(_ int, idx int, name, _, _ string) { + l.calls = append(l.calls, recordedCall{kind: "step_started", stepIdx: idx, text: name}) +} +func (l *recordingListener) OnWorkflowStepDone(_ int, idx int, summary string) { + l.calls = append(l.calls, recordedCall{kind: "step_done", stepIdx: idx, text: summary}) +} +func (l *recordingListener) OnWorkflowDone(_ int, desc string, _ []string) { + l.calls = append(l.calls, recordedCall{kind: "done", text: desc}) +} +func (l *recordingListener) OnWorkflowFailed(_ int, reason string) { + l.calls = append(l.calls, recordedCall{kind: "failed", text: reason}) +} +func (l *recordingListener) OnNote(_ int, text string) { + l.calls = append(l.calls, recordedCall{kind: "note", text: text}) +} + +func (l *recordingListener) kinds() []string { + out := make([]string, 0, len(l.calls)) + for _, c := range l.calls { + if c.kind != "note" { + out = append(out, c.kind) + } + } + return out +} + +func (l *recordingListener) has(kind string) bool { + for _, c := range l.calls { + if c.kind == kind { + return true + } + } + return false +} + +func textEvent(author, text string) *session.Event { + ev := &session.Event{Author: author} + ev.LLMResponse.Content = &genai.Content{ + Role: genai.RoleModel, + Parts: []*genai.Part{{Text: text}}, + } + return ev +} + +func callEvent(author, name string, args map[string]any) *session.Event { + ev := &session.Event{Author: author} + ev.LLMResponse.Content = &genai.Content{ + Role: genai.RoleModel, + Parts: []*genai.Part{genai.NewPartFromFunctionCall(name, args)}, + } + return ev +} + +func testProgress(t *testing.T, def Def, l RunnerListener) *Progress { + t.Helper() + c, err := CompileWorkflow(context.Background(), testCompileConfig(def)) + if err != nil { + t.Fatalf("compile: %v", err) + } + return NewProgress(c, def, NewTextSource(1, "src"), t.TempDir(), 1, l, nil) +} + +func twoStepDef() Def { + return Def{Name: "wf", Steps: []Step{ + {Name: "plan", Prompt: "plan"}, + {Name: "review", Prompt: "review"}, + }} +} + +func TestProgress_ReportsStepsFromTheEventStream(t *testing.T) { + l := &recordingListener{} + p := testProgress(t, twoStepDef(), l) + + p.Observe(textEvent("plan", "working")) + p.Observe(callEvent("plan", "end_turn", map[string]any{"summary": "planned it"})) + p.Observe(textEvent("review", "checking")) + p.Observe(callEvent("review", "end_turn", map[string]any{"summary": "looks good"})) + state := p.Finish(nil) + + want := []string{"started", "step_started", "step_done", "step_started", "step_done", "done"} + got := l.kinds() + if len(got) != len(want) { + t.Fatalf("callback sequence = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("callback sequence = %v, want %v", got, want) + } + } + + for _, c := range l.calls { + if c.kind != "step_done" { + continue + } + switch c.stepIdx { + case 0: + if c.text != "planned it" { + t.Errorf("step 0 summary = %q, want the end_turn summary", c.text) + } + case 1: + if c.text != "looks good" { + t.Errorf("step 1 summary = %q, want the end_turn summary", c.text) + } + } + } + if state == nil || !state.Done || state.Failed { + t.Errorf("run state should be done and not failed: %+v", state) + } +} + +// The regression that motivated this type: the old graph runner marked +// every step that never started as both started AND done, then hardcoded +// success. A chain dying at step 1 of 2 must report exactly one started +// step, no step_done, and a failure. +func TestProgress_FailureDoesNotFabricateRemainingSteps(t *testing.T) { + l := &recordingListener{} + p := testProgress(t, twoStepDef(), l) + + p.Observe(textEvent("plan", "starting work")) + state := p.Finish(errors.New("model exploded")) + + for _, c := range l.calls { + if c.kind == "step_done" { + t.Errorf("no step may be reported done on a failed run, got step %d", c.stepIdx) + } + if c.kind == "done" { + t.Error("a failed run must not emit OnWorkflowDone") + } + if c.kind == "step_started" && c.stepIdx != 0 { + t.Errorf("step %d never ran and must not be reported started", c.stepIdx) + } + } + if !l.has("failed") { + t.Error("a failed run must emit OnWorkflowFailed") + } + if state == nil || !state.Failed || state.Done { + t.Errorf("run state should be failed and not done: %+v", state) + } + if state.FailedReason != "model exploded" { + t.Errorf("FailedReason = %q", state.FailedReason) + } +} + +// A step that ends without calling end_turn still gets a log line, taken +// from its own output rather than left blank. +func TestProgress_FallsBackToStepTextWithoutEndTurn(t *testing.T) { + l := &recordingListener{} + p := testProgress(t, twoStepDef(), l) + + p.Observe(textEvent("plan", "did the thing\nwith more detail")) + p.Observe(textEvent("review", "second step")) + p.Finish(nil) + + for _, c := range l.calls { + if c.kind == "step_done" && c.stepIdx == 0 && c.text != "did the thing" { + t.Errorf("step 0 summary = %q, want its first line of output", c.text) + } + } +} + +func TestProgress_CapturesFinishWorkflow(t *testing.T) { + l := &recordingListener{} + p := testProgress(t, twoStepDef(), l) + + p.Observe(textEvent("plan", "x")) + p.Observe(callEvent("review", "finish_workflow", map[string]any{ + "description": "shipped it", + "artifacts": []any{"a.go", "b.go"}, + })) + state := p.Finish(nil) + + if state.FinishData == nil { + t.Fatal("finish_workflow must populate FinishData") + } + if state.FinishData.Description != "shipped it" { + t.Errorf("description = %q", state.FinishData.Description) + } + if len(state.FinishData.Artifacts) != 2 { + t.Errorf("artifacts = %v", state.FinishData.Artifacts) + } + for _, c := range l.calls { + if c.kind == "done" && c.text != "shipped it" { + t.Errorf("OnWorkflowDone description = %q", c.text) + } + } +} + +// Events from agents the compiler did not produce (ADK internals, the +// user turn) must not move the step cursor. +func TestProgress_IgnoresUnknownAuthors(t *testing.T) { + l := &recordingListener{} + p := testProgress(t, twoStepDef(), l) + + p.Observe(textEvent("user", "the request")) + p.Observe(textEvent("some_other_agent", "noise")) + p.Observe(&session.Event{}) + p.Observe(nil) + + if l.has("step_started") { + t.Errorf("unknown authors must not start a step: %v", l.calls) + } +} + +// A loop node re-entered for another iteration must not be reported as a +// second step start; it is still the same step of the user's definition. +func TestProgress_LoopReentryIsNotANewStep(t *testing.T) { + def := Def{Name: "wf", Steps: []Step{ + {Name: "fix", Kind: "loop", MaxIterations: 3, Steps: []Step{ + {Name: "edit", Prompt: "edit"}, + {Name: "verify", Prompt: "verify"}, + }}, + }} + l := &recordingListener{} + p := testProgress(t, def, l) + + // Two iterations of the loop's inner agents. + p.Observe(textEvent("edit", "edit 1")) + p.Observe(textEvent("verify", "verify 1")) + p.Observe(textEvent("edit", "edit 2")) + p.Observe(textEvent("verify", "verify 2")) + p.Finish(nil) + + starts := 0 + for _, c := range l.calls { + if c.kind == "step_started" { + starts++ + if c.stepIdx != 0 { + t.Errorf("loop inner agents must report against step 0, got %d", c.stepIdx) + } + } + } + if starts != 1 { + t.Errorf("loop step started %d times, want 1", starts) + } +} diff --git a/pkg/workflow/runner.go b/pkg/workflow/runner.go index dbf7abd..a86bfad 100644 --- a/pkg/workflow/runner.go +++ b/pkg/workflow/runner.go @@ -1,79 +1,34 @@ package workflow import ( - "context" - "errors" "fmt" - "path/filepath" "strings" "time" - - "google.golang.org/adk/v2/agent" - "google.golang.org/adk/v2/agent/llmagent" - "google.golang.org/adk/v2/agent/workflowagents/loopagent" - "google.golang.org/adk/v2/agent/workflowagents/sequentialagent" - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/runner" - "google.golang.org/adk/v2/session" - "google.golang.org/adk/v2/tool" - "google.golang.org/adk/v2/tool/exitlooptool" - "google.golang.org/genai" -) - -// Loop decision constants. -const ( - LoopContinue = "continue" - LoopBreak = "break" -) - -// RemindKind identifies the reason a step is being re-prompted. -type RemindKind int - -const ( - RemindNone RemindKind = iota - RemindNoSummary - RemindNoDecision - RemindFixPlanDir - RemindNoFinishTool ) -// StepPromptCtx carries contextual details injected into a step prompt. +// StepPromptCtx carries contextual details injected into a step's +// instruction. type StepPromptCtx struct { Loop *LoopPromptCtx - Remind RemindKind - RemindDetail string - NotesDir string - PrevNotesDir string IsStartStep bool IsWorkflowFinalStep bool } -// LoopPromptCtx carries loop iteration metadata for prompt assembly. +// LoopPromptCtx carries loop metadata for instruction assembly. type LoopPromptCtx struct { Name string - Iteration int MaxIterations int ExitCondition string IsTail bool } -// LoopRunFrame tracks execution progress within a loop step. -type LoopRunFrame struct { - InnerIdx int - Iteration int - IterationLog []string - PrevTail string - Retry int - RetryText string -} - // FinishData captures completion metadata reported at workflow termination. type FinishData struct { Description string `json:"description"` Artifacts []string `json:"artifacts"` } -// RunState represents the state of a running workflow. +// RunState represents the state of a workflow run. type RunState struct { Workflow Def Source Source @@ -85,20 +40,6 @@ type RunState struct { FinishData *FinishData } -// StepResult represents the outcome of executing a single workflow step. -type StepResult struct { - Output string - Summary string - Decision string - FinishData *FinishData - Error error -} - -// StepExecutor executes a single step turn against an underlying agent engine/provider. -type StepExecutor interface { - ExecuteStep(ctx context.Context, cwd string, tabID int, step Step, prompt string, isFinal bool) (StepResult, error) -} - // RunnerListener receives progress notifications during workflow execution. type RunnerListener interface { OnWorkflowStarted(tabID int, def Def, src Source) @@ -119,655 +60,39 @@ func (NoopRunnerListener) OnWorkflowDone(int, string, []string) func (NoopRunnerListener) OnWorkflowFailed(int, string) {} func (NoopRunnerListener) OnNote(int, string) {} -// WorkflowAgentConfig configures the construction of an ADK workflow agent hierarchy. -type WorkflowAgentConfig struct { - Def Def - Source Source - Cwd string - TabID int - ModelBuilder func(ctx context.Context, step Step) (model.LLM, error) - ToolsBuilder func(ctx context.Context, step Step, isLoop bool) ([]tool.Tool, error) - ToolsetsBuilder func(ctx context.Context, step Step, isLoop bool) ([]tool.Toolset, error) - InstructionBuilder func(step Step, isStart bool, isFinal bool, loopCtx *LoopPromptCtx, notesDir, prevNotesDir string) string -} - -// BuildWorkflowAgent constructs an ADK agent hierarchy conforming to the workflow definition. -// Top-level linear steps are chained using sequentialagent, while kind: "loop" steps are -// encapsulated in loopagent containers with exitlooptool attached to their sub-agents. -func BuildWorkflowAgent(ctx context.Context, cfg WorkflowAgentConfig) (agent.Agent, error) { - if err := cfg.Def.Validate(); err != nil { - return nil, err - } - if cfg.ModelBuilder == nil { - return nil, errors.New("model builder is required") - } - - var topAgents []agent.Agent - var prevNotesDir string - - for i, top := range cfg.Def.Steps { - isFinalStep := i == len(cfg.Def.Steps)-1 - - if top.Kind == "loop" { - if len(top.Steps) == 0 { - continue - } - var innerAgents []agent.Agent - for innerIdx, innerStep := range top.Steps { - isLoopStart := i == 0 && innerIdx == 0 - var notesDir string - if isLoopStart { - notesDir = StartPlanDir(cfg.Cwd) - } else { - notesDir = StepNotesDir(cfg.Cwd, innerStep.Name, top.Name, 1) - } - - llm, err := cfg.ModelBuilder(ctx, innerStep) - if err != nil { - return nil, fmt.Errorf("failed to build model for step %q: %w", innerStep.Name, err) - } - - var tools []tool.Tool - if cfg.ToolsBuilder != nil { - builtTools, err := cfg.ToolsBuilder(ctx, innerStep, true) - if err != nil { - return nil, fmt.Errorf("failed to build tools for step %q: %w", innerStep.Name, err) - } - tools = append(tools, builtTools...) - } - - // Attach ADK's native exitlooptool for clean early break out of loop containers - exitTool, err := exitlooptool.New() - if err != nil { - return nil, fmt.Errorf("failed to create exitloop tool: %w", err) - } - hasExitTool := false - for _, t := range tools { - if t != nil && t.Name() == exitTool.Name() { - hasExitTool = true - break - } - } - if !hasExitTool { - tools = append(tools, exitTool) - } - - var toolsets []tool.Toolset - if cfg.ToolsetsBuilder != nil { - ts, err := cfg.ToolsetsBuilder(ctx, innerStep, true) - if err != nil { - return nil, fmt.Errorf("failed to build toolsets for step %q: %w", innerStep.Name, err) - } - toolsets = ts - } - - instruction := innerStep.Prompt - if cfg.InstructionBuilder != nil { - loopCtx := &LoopPromptCtx{ - Name: top.Name, - Iteration: 1, - MaxIterations: cfg.Def.EffectiveMaxIterations(top), - ExitCondition: top.ExitCondition, - IsTail: innerIdx == len(top.Steps)-1, - } - instruction = cfg.InstructionBuilder(innerStep, isLoopStart, isFinalStep, loopCtx, notesDir, prevNotesDir) - } - - innerAg, err := llmagent.New(llmagent.Config{ - Name: innerStep.Name, - Description: innerStep.Prompt, - Model: llm, - Instruction: instruction, - Tools: tools, - Toolsets: toolsets, - }) - if err != nil { - return nil, fmt.Errorf("failed to create inner step agent %q: %w", innerStep.Name, err) - } - innerAgents = append(innerAgents, innerAg) - prevNotesDir = notesDir - } - - loopAg, err := loopagent.New(loopagent.Config{ - AgentConfig: agent.Config{ - Name: top.Name, - Description: top.ExitCondition, - SubAgents: innerAgents, - }, - MaxIterations: uint(cfg.Def.EffectiveMaxIterations(top)), - }) - if err != nil { - return nil, fmt.Errorf("failed to create loop agent %q: %w", top.Name, err) - } - topAgents = append(topAgents, loopAg) - continue - } - - // Linear step - isStart := i == 0 - var notesDir string - if isStart { - notesDir = StartPlanDir(cfg.Cwd) - } else { - notesDir = StepNotesDir(cfg.Cwd, top.Name, "", 0) - } - - llm, err := cfg.ModelBuilder(ctx, top) - if err != nil { - return nil, fmt.Errorf("failed to build model for step %q: %w", top.Name, err) - } - - var tools []tool.Tool - if cfg.ToolsBuilder != nil { - builtTools, err := cfg.ToolsBuilder(ctx, top, false) - if err != nil { - return nil, fmt.Errorf("failed to build tools for step %q: %w", top.Name, err) - } - tools = append(tools, builtTools...) - } - - var toolsets []tool.Toolset - if cfg.ToolsetsBuilder != nil { - ts, err := cfg.ToolsetsBuilder(ctx, top, false) - if err != nil { - return nil, fmt.Errorf("failed to build toolsets for step %q: %w", top.Name, err) - } - toolsets = ts - } - - instruction := top.Prompt - if cfg.InstructionBuilder != nil { - instruction = cfg.InstructionBuilder(top, isStart, isFinalStep, nil, notesDir, prevNotesDir) - } - - stepAg, err := llmagent.New(llmagent.Config{ - Name: top.Name, - Description: top.Prompt, - Model: llm, - Instruction: instruction, - Tools: tools, - Toolsets: toolsets, - }) - if err != nil { - return nil, fmt.Errorf("failed to create step agent %q: %w", top.Name, err) - } - topAgents = append(topAgents, stepAg) - prevNotesDir = notesDir - } - - return sequentialagent.New(sequentialagent.Config{ - AgentConfig: agent.Config{ - Name: cfg.Def.Name, - Description: cfg.Def.Description, - SubAgents: topAgents, - }, - }) -} - -// Runner executes multi-step workflow pipelines. -type Runner struct { - tracker *Tracker - executor StepExecutor - listener RunnerListener -} - -// NewRunner creates a new workflow Runner. -func NewRunner(tracker *Tracker, executor StepExecutor, listener RunnerListener) *Runner { - if tracker == nil { - tracker = GlobalTracker() - } - if listener == nil { - listener = NoopRunnerListener{} - } - return &Runner{ - tracker: tracker, - executor: executor, - listener: listener, - } -} - -// RunGraph executes a compiled ADK workflow graph, broadcasting lifecycle events to the listener. -func (r *Runner) RunGraph(ctx context.Context, cfg WorkflowAgentConfig) (*RunState, error) { - if err := cfg.Def.Validate(); err != nil { - r.listener.OnWorkflowFailed(cfg.TabID, err.Error()) - return nil, err - } - - wfAgent, err := BuildWorkflowAgent(ctx, cfg) - if err != nil { - r.listener.OnWorkflowFailed(cfg.TabID, err.Error()) - return nil, err - } - - r.listener.OnWorkflowStarted(cfg.TabID, cfg.Def, cfg.Source) - if r.tracker != nil { - r.tracker.MarkWorking(cfg.Cwd, cfg.Source.Key(), cfg.Def.Name, cfg.TabID) - } - - runState := &RunState{ - Workflow: cfg.Def, - Source: cfg.Source, - StartedAt: time.Now().UTC(), - StepIdx: 0, - } - - sessSvc := session.InMemoryService() - adkRunner, err := runner.New(runner.Config{ - AppName: "ask-workflow", - Agent: wfAgent, - SessionService: sessSvc, - AutoCreateSession: true, - }) - if err != nil { - r.listener.OnWorkflowFailed(cfg.TabID, err.Error()) - if r.tracker != nil { - r.tracker.MarkFinal(cfg.Cwd, cfg.Source.Key(), cfg.Def.Name, StatusFailed, 0) - } - return runState, err - } - - userMsg := genai.NewContentFromText(cfg.Source.Display(), genai.RoleUser) - sessionID := "wf-" + cfg.Source.Key() - - startedSteps := make(map[int]bool) - doneSteps := make(map[int]bool) - lastStepIdx := -1 - - for event, err := range adkRunner.Run(ctx, "user", sessionID, userMsg, agent.RunConfig{}) { - if err != nil { - r.listener.OnWorkflowFailed(cfg.TabID, err.Error()) - if r.tracker != nil { - r.tracker.MarkFinal(cfg.Cwd, cfg.Source.Key(), cfg.Def.Name, StatusFailed, lastStepIdx) - } - return runState, err - } - if event == nil { - continue - } - - if event.Author != "" && event.Author != "user" && event.Author != "ask_coder" { - for i, s := range cfg.Def.Steps { - if s.Name == event.Author { - if lastStepIdx >= 0 && lastStepIdx != i && !doneSteps[lastStepIdx] { - doneSteps[lastStepIdx] = true - r.listener.OnWorkflowStepDone(cfg.TabID, lastStepIdx, fmt.Sprintf("completed step %s", cfg.Def.Steps[lastStepIdx].Name)) - } - if !startedSteps[i] { - startedSteps[i] = true - lastStepIdx = i - r.listener.OnWorkflowStepStarted(cfg.TabID, i, s.Name, s.Provider, s.Model) - } - break - } - } - } - - if event.LLMResponse.Content != nil { - for _, part := range event.LLMResponse.Content.Parts { - if part.Text != "" { - r.listener.OnNote(cfg.TabID, part.Text) - } - } - } - } - - for i := 0; i < len(cfg.Def.Steps); i++ { - if !startedSteps[i] { - startedSteps[i] = true - r.listener.OnWorkflowStepStarted(cfg.TabID, i, cfg.Def.Steps[i].Name, cfg.Def.Steps[i].Provider, cfg.Def.Steps[i].Model) - } - if !doneSteps[i] { - doneSteps[i] = true - r.listener.OnWorkflowStepDone(cfg.TabID, i, fmt.Sprintf("completed step %s", cfg.Def.Steps[i].Name)) - } - } - - runState.Done = true - runState.StepIdx = len(cfg.Def.Steps) - runState.FinishData = &FinishData{ - Description: fmt.Sprintf("Workflow %s completed via ADK workflow runner", cfg.Def.Name), - } - - r.listener.OnWorkflowDone(cfg.TabID, runState.FinishData.Description, runState.FinishData.Artifacts) - if r.tracker != nil { - r.tracker.MarkFinal(cfg.Cwd, cfg.Source.Key(), cfg.Def.Name, StatusDone, len(cfg.Def.Steps)) - } - return runState, nil -} - -// Run executes the workflow def synchronously to completion or until context cancellation. -func (r *Runner) Run(ctx context.Context, cwd string, tabID int, def Def, src Source) (*RunState, error) { - if err := def.Validate(); err != nil { - r.listener.OnWorkflowFailed(tabID, err.Error()) - return nil, err - } - - r.listener.OnWorkflowStarted(tabID, def, src) - r.tracker.MarkWorking(cwd, src.Key(), def.Name, tabID) - - runState := &RunState{ - Workflow: def, - Source: src, - StartedAt: time.Now().UTC(), - StepIdx: 0, - } - - var stepLog []string - var loopFrame *LoopRunFrame - var prevNotesDir string - var currentNotesDir string - var remind RemindKind - var remindDetail string - var linearRetry int - var linearText string - var stepErrorRetry int - - for { - select { - case <-ctx.Done(): - r.listener.OnWorkflowFailed(tabID, "cancelled by user") - r.tracker.MarkFinal(cwd, src.Key(), def.Name, StatusFailed, runState.StepIdx) - return runState, ctx.Err() - default: - } - - if loopFrame == nil && runState.StepIdx >= len(def.Steps) { - break - } - - top := def.Steps[runState.StepIdx] - if top.Kind == "loop" && loopFrame == nil { - if len(top.Steps) == 0 { - runState.StepIdx++ - continue - } - loopFrame = &LoopRunFrame{InnerIdx: 0, Iteration: 1} - r.listener.OnNote(tabID, LoopNoteLine(top.Name, "started", fmt.Sprintf("max %d iteration(s)", def.EffectiveMaxIterations(top)))) - } - - step := top - if loopFrame != nil { - step = top.Steps[loopFrame.InnerIdx] - } - - r.listener.OnWorkflowStepStarted(tabID, runState.StepIdx, step.Name, step.Provider, step.Model) - - isStartStep := runState.StepIdx == 0 && loopFrame == nil - isLoopStartStep := runState.StepIdx == 0 && loopFrame != nil && loopFrame.Iteration == 1 && loopFrame.InnerIdx == 0 - - var notesDir string - switch { - case isStartStep, isLoopStartStep: - notesDir = StartPlanDir(cwd) - case loopFrame != nil: - notesDir = StepNotesDir(cwd, step.Name, top.Name, loopFrame.Iteration) - default: - notesDir = StepNotesDir(cwd, step.Name, "", 0) - } - currentNotesDir = notesDir - - var prevOutputs []string - if loopFrame == nil { - if linearRetry > 0 && linearText != "" { - prevOutputs = append(append([]string(nil), stepLog...), linearText) - } else { - prevOutputs = stepLog - } - } else { - prevOutputs = append([]string(nil), stepLog...) - if loopFrame.InnerIdx == 0 { - if loopFrame.PrevTail != "" { - prevOutputs = append(prevOutputs, loopFrame.PrevTail) - } - } else { - prevOutputs = append(prevOutputs, loopFrame.IterationLog...) - } - if loopFrame.Retry > 0 && loopFrame.RetryText != "" { - prevOutputs = append(prevOutputs, loopFrame.RetryText) - } - } - - pc := &StepPromptCtx{ - Remind: remind, - RemindDetail: remindDetail, - NotesDir: notesDir, - PrevNotesDir: prevNotesDir, - IsStartStep: isStartStep || isLoopStartStep, - IsWorkflowFinalStep: runState.StepIdx == len(def.Steps)-1, - } - if loopFrame != nil { - pc.Loop = &LoopPromptCtx{ - Name: top.Name, - Iteration: loopFrame.Iteration, - MaxIterations: def.EffectiveMaxIterations(top), - ExitCondition: top.ExitCondition, - IsTail: loopFrame.InnerIdx == len(top.Steps)-1, - } - } - - var dirErr error - if pc.IsStartStep { - dirErr = EnsureStartPlanExists(cwd) - } else { - dirErr = EnsureStepNotesDir(notesDir) - } - if dirErr != nil { - remind = RemindFixPlanDir - remindDetail = dirErr.Error() - pc.Remind = remind - pc.RemindDetail = remindDetail - } - - prompt := BuildStepPrompt(step, src, prevOutputs, pc) - isFinalStep := runState.StepIdx == len(def.Steps)-1 - - if r.executor == nil { - err := errors.New("no step executor provided") - r.listener.OnWorkflowFailed(tabID, err.Error()) - r.tracker.MarkFinal(cwd, src.Key(), def.Name, StatusFailed, runState.StepIdx) - return runState, err - } - - res, err := r.executor.ExecuteStep(ctx, cwd, tabID, step, prompt, isFinalStep) - if err != nil { - if errors.Is(err, context.Canceled) || ctx.Err() != nil { - r.listener.OnWorkflowFailed(tabID, "cancelled by user") - r.tracker.MarkFinal(cwd, src.Key(), def.Name, StatusFailed, runState.StepIdx) - return runState, ctx.Err() - } - if stepErrorRetry < 3 { - stepErrorRetry++ - wait := time.Duration(stepErrorRetry) * time.Second - r.listener.OnNote(tabID, WorkflowNoteLine(fmt.Sprintf("step %q failed: %v", step.Name, err), fmt.Sprintf("retrying (attempt %d of 3)", stepErrorRetry))) - select { - case <-time.After(wait): - case <-ctx.Done(): - return runState, ctx.Err() - } - continue - } - r.listener.OnWorkflowFailed(tabID, err.Error()) - r.tracker.MarkFinal(cwd, src.Key(), def.Name, StatusFailed, runState.StepIdx) - return runState, err - } - - stepErrorRetry = 0 - remind = RemindNone - remindDetail = "" - - if loopFrame == nil { - if res.Summary == "" { - linearRetry++ - linearText = res.Output - remind = RemindNoSummary - r.listener.OnNote(tabID, " | Re-prompting "+step.Name+" for end_turn") - continue - } - - r.listener.OnWorkflowStepDone(tabID, runState.StepIdx, res.Summary) - - if isFinalStep && res.FinishData != nil { - runState.FinishData = res.FinishData - } - - prevNotesDir = currentNotesDir - if res.Output != "" { - stepLog = append(stepLog, res.Output) - } - linearRetry = 0 - linearText = "" - runState.StepIdx++ - continue - } - - isTail := loopFrame.InnerIdx == len(top.Steps)-1 - if res.Summary == "" { - loopFrame.Retry++ - loopFrame.RetryText = res.Output - remind = RemindNoSummary - r.listener.OnNote(tabID, " | Re-prompting "+step.Name+" for end_turn") - continue - } - - r.listener.OnWorkflowStepDone(tabID, runState.StepIdx, res.Summary) - - // Loop termination: either explicitly via decision="break", or native exit_loop tool invocation - if res.Decision == LoopBreak { - if isFinalStep && res.FinishData != nil { - runState.FinishData = res.FinishData - } - - prevNotesDir = currentNotesDir - if res.Output != "" { - loopFrame.IterationLog = append(loopFrame.IterationLog, res.Output) - } - r.listener.OnNote(tabID, LoopNoteLine(top.Name, "break", "")) - - stepLog = append(stepLog, loopFrame.IterationLog...) - loopFrame = nil - runState.StepIdx++ - continue - } - - if !isTail { - prevNotesDir = currentNotesDir - if res.Output != "" { - loopFrame.IterationLog = append(loopFrame.IterationLog, res.Output) - } - loopFrame.Retry = 0 - loopFrame.RetryText = "" - loopFrame.InnerIdx++ - continue - } - - if res.Decision != LoopContinue { - loopFrame.Retry++ - loopFrame.RetryText = res.Output - remind = RemindNoDecision - r.listener.OnNote(tabID, " | Re-prompting final step for a decision") - continue - } - - prevNotesDir = currentNotesDir - if res.Output != "" { - loopFrame.IterationLog = append(loopFrame.IterationLog, res.Output) - } - - if loopFrame.Iteration >= def.EffectiveMaxIterations(top) { - if isFinalStep && res.FinishData != nil { - runState.FinishData = res.FinishData - } - r.listener.OnNote(tabID, LoopNoteLine(top.Name, "hit iteration limit", fmt.Sprintf("%d iteration(s)", loopFrame.Iteration))) - stepLog = append(stepLog, loopFrame.IterationLog...) - loopFrame = nil - runState.StepIdx++ - continue - } - - r.listener.OnNote(tabID, LoopNoteLine(top.Name, fmt.Sprintf("iteration %d complete → continue", loopFrame.Iteration), "")) - loopFrame.PrevTail = lastString(loopFrame.IterationLog) - loopFrame.IterationLog = nil - loopFrame.Iteration++ - loopFrame.InnerIdx = 0 - loopFrame.Retry = 0 - loopFrame.RetryText = "" - } - - _ = RemoveAllWorkflowPlans(cwd) - - desc := "" - var arts []string - if runState.FinishData != nil { - desc = runState.FinishData.Description - arts = runState.FinishData.Artifacts - } - - runState.Done = true - r.listener.OnWorkflowDone(tabID, desc, arts) - r.tracker.MarkFinal(cwd, src.Key(), def.Name, StatusDone, runState.StepIdx) - - return runState, nil -} - -func lastString(s []string) string { - if len(s) == 0 { - return "" - } - return s[len(s)-1] -} - -// BuildStepPrompt assembles the user-message prompt for a single workflow step. -func BuildStepPrompt(step Step, source Source, prevOutputs []string, pc *StepPromptCtx) string { +// BuildStepInstruction assembles the system instruction for one workflow +// step: the author's prompt, the run's reference block, and the end_turn +// contract (plus loop framing when the step sits inside a loop). +// +// It does NOT thread previous step output. The graph does that: a node's +// output arrives as the next node's input, and every step agent runs with +// IncludeContentsNone so it sees that input and its own work rather than +// the full transcript of everything before it. +func BuildStepInstruction(step Step, source Source, pc *StepPromptCtx) string { var b strings.Builder b.WriteString(strings.TrimSpace(step.Prompt)) if ref := source.RefBlock(); ref != "" { b.WriteString("\n\n") b.WriteString(ref) } - if len(prevOutputs) > 0 { - b.WriteString("\n\nPrevious step output:\n") - for i, entry := range prevOutputs { - if i > 0 { - b.WriteString("\n---\n") - } - b.WriteString(strings.TrimSpace(entry)) - } - } - if pc != nil && pc.NotesDir != "" { - b.WriteString("\n\n") - b.WriteString("Workflow notes directories:\n") - b.WriteString("- Your notes directory: " + pc.NotesDir) - if pc.PrevNotesDir != "" { - b.WriteString("\n- Previous step's notes directory: " + pc.PrevNotesDir) - } - if pc.IsStartStep { - b.WriteString("\n\nThis is the first step. Your notes directory (") - b.WriteString(pc.NotesDir) - b.WriteString(") MUST be a directory, not a file. Create it if it does not exist, then write one or more files inside it (for example ") - b.WriteString(filepath.Join(pc.NotesDir, "plan.md")) - b.WriteString("). Do NOT write a single file named \"start\". The workflow runner verifies the directory exists and contains files before step 1; if it is missing, empty, or a file, this step will be re-prompted to fix the directory before any work is done.") - } - } var loop *LoopPromptCtx - remind := RemindNone if pc != nil { loop = pc.Loop - remind = pc.Remind } b.WriteString("\n\n") b.WriteString(EndTurnInstructionBlock(loop)) - if remind != RemindNone { - b.WriteString("\n\n") - b.WriteString(EndTurnReminder(remind, pc.RemindDetail)) - } return strings.TrimSpace(b.String()) } -// EndTurnInstructionBlock renders the auto-injected end_turn contract for a step. +// EndTurnInstructionBlock renders the end_turn contract for a step, and +// inside a loop the iteration framing plus how to break out. +// +// Breaking a loop is ADK's exit_loop tool, not an end_turn argument: the +// tool sets Actions.Escalate, which is what a loopagent watches for. func EndTurnInstructionBlock(loop *LoopPromptCtx) string { var b strings.Builder if loop != nil { - fmt.Fprintf(&b, "[Workflow loop %q · iteration %d of up to %d]", loop.Name, loop.Iteration, loop.MaxIterations) + fmt.Fprintf(&b, "[Workflow loop %q · up to %d iterations]", loop.Name, loop.MaxIterations) if cond := strings.TrimSpace(loop.ExitCondition); cond != "" { b.WriteString("\nLoop exit goal: ") b.WriteString(cond) @@ -778,41 +103,17 @@ func EndTurnInstructionBlock(loop *LoopPromptCtx) string { "a `summary` of 1-3 sentences describing what you did and the outcome. This records your progress in the " + "workflow log; it does not cut your turn short.") if loop != nil { - if loop.IsTail { - b.WriteString(" You are the final step of this loop iteration, so you MUST also pass a `decision`: " + - "\"continue\" to run another iteration, or \"break\" to end the loop. Use \"break\" only when the " + - "loop's exit goal is met — breaking should be exceptional.") - } else { - b.WriteString(" You are inside a loop but not its final step of this loop iteration, so you MUST OMIT `decision` " + - "entirely — only the final step of a loop iteration can pass `decision='break'`. If the loop's " + - "exit goal appears met, the final step of this iteration will register `break` on its turn.") + b.WriteString(" You are running inside a loop: when the loop's exit goal above is met, call the " + + "exit_loop tool to end the loop. If it is not met, do not call exit_loop — the loop advances to its " + + "next iteration on its own, and stops by itself after the iteration limit.") + if !loop.IsTail { + b.WriteString(" Later steps in this iteration still have work to do, so only call exit_loop if the " + + "goal is already fully met.") } } return b.String() } -// EndTurnReminder renders instructions when a step must be re-prompted. -func EndTurnReminder(k RemindKind, detail string) string { - switch k { - case RemindNoDecision: - return "REMINDER: you called end_turn without a `decision`, which is required for the final step of a " + - "loop iteration. You have already done the work shown above — do NOT repeat it. Call end_turn again now " + - "with decision=\"continue\" or decision=\"break\"." - case RemindFixPlanDir: - msg := "REMINDER: the workflow notes directory is not usable" - if detail != "" { - msg += ": " + detail - } - msg += ". You must make it a directory containing files, then call end_turn." - return msg - case RemindNoFinishTool: - return "REMINDER: you reached the final step of the workflow without providing final finish data. Call the finish tool or end_turn." - default: - return "REMINDER: your previous turn ended without calling end_turn. You have already done the work shown " + - "above — do NOT repeat it. Call the end_turn tool now (see the instructions above for what to include)." - } -} - // WorkflowNoteLine formats a single-line status note. func WorkflowNoteLine(msg, detail string) string { res := " " + msg @@ -850,4 +151,3 @@ func ProviderMeta(provider, model string) string { return provider + "/" + model } } - diff --git a/pkg/workflow/runner_test.go b/pkg/workflow/runner_test.go deleted file mode 100644 index 5ce3767..0000000 --- a/pkg/workflow/runner_test.go +++ /dev/null @@ -1,631 +0,0 @@ -package workflow - -import ( - "context" - "errors" - "iter" - "os" - "path/filepath" - "strings" - "sync" - "testing" - - "google.golang.org/adk/v2/agent" - "google.golang.org/adk/v2/model" - "google.golang.org/adk/v2/runner" - "google.golang.org/adk/v2/session" - "google.golang.org/genai" -) - -type mockWorkflowLLM struct { - name string - generateFunc func(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] -} - -func (m *mockWorkflowLLM) Name() string { return m.name } -func (m *mockWorkflowLLM) GenerateContent(ctx context.Context, req *model.LLMRequest, stream bool) iter.Seq2[*model.LLMResponse, error] { - if m.generateFunc != nil { - return m.generateFunc(ctx, req, stream) - } - return func(yield func(*model.LLMResponse, error) bool) { - yield(&model.LLMResponse{ - Content: &genai.Content{ - Role: genai.RoleModel, - Parts: []*genai.Part{genai.NewPartFromText("mock response")}, - }, - FinishReason: genai.FinishReasonStop, - }, nil) - } -} - -type mockStepExecutor struct { - mu sync.Mutex - calls []stepCall - handlers []func(step Step, prompt string) (StepResult, error) -} - -type stepCall struct { - Step Step - Prompt string - IsFinal bool -} - -func (m *mockStepExecutor) ExecuteStep(ctx context.Context, cwd string, tabID int, step Step, prompt string, isFinal bool) (StepResult, error) { - m.mu.Lock() - defer m.mu.Unlock() - idx := len(m.calls) - m.calls = append(m.calls, stepCall{Step: step, Prompt: prompt, IsFinal: isFinal}) - if idx < len(m.handlers) { - return m.handlers[idx](step, prompt) - } - return StepResult{ - Output: "output from " + step.Name, - Summary: "summary of " + step.Name, - Decision: LoopContinue, - }, nil -} - -type mockRunnerListener struct { - mu sync.Mutex - started bool - stepsStarted []string - stepsDone []string - done bool - failed bool - failedReason string - notes []string - doneDesc string - doneArtifacts []string -} - -func (l *mockRunnerListener) OnWorkflowStarted(tabID int, def Def, src Source) { - l.mu.Lock() - defer l.mu.Unlock() - l.started = true -} - -func (l *mockRunnerListener) OnWorkflowStepStarted(tabID int, stepIdx int, stepName, provider, model string) { - l.mu.Lock() - defer l.mu.Unlock() - l.stepsStarted = append(l.stepsStarted, stepName) -} - -func (l *mockRunnerListener) OnWorkflowStepDone(tabID int, stepIdx int, summary string) { - l.mu.Lock() - defer l.mu.Unlock() - l.stepsDone = append(l.stepsDone, summary) -} - -func (l *mockRunnerListener) OnWorkflowDone(tabID int, description string, artifacts []string) { - l.mu.Lock() - defer l.mu.Unlock() - l.done = true - l.doneDesc = description - l.doneArtifacts = artifacts -} - -func (l *mockRunnerListener) OnWorkflowFailed(tabID int, reason string) { - l.mu.Lock() - defer l.mu.Unlock() - l.failed = true - l.failedReason = reason -} - -func (l *mockRunnerListener) OnNote(tabID int, text string) { - l.mu.Lock() - defer l.mu.Unlock() - l.notes = append(l.notes, text) -} - -func TestRunner_LinearWorkflow(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.md"), []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - tracker := NewTracker() - exec := &mockStepExecutor{} - listener := &mockRunnerListener{} - runner := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "linear-pipeline", - Steps: []Step{ - {Name: "step-1", Prompt: "First do analysis"}, - {Name: "step-2", Prompt: "Then implement changes"}, - }, - } - src := NewTextSource(1, "Fix the authentication bug") - - state, err := runner.Run(context.Background(), tmpDir, 1, def, src) - if err != nil { - t.Fatalf("unexpected error running workflow: %v", err) - } - if !state.Done { - t.Errorf("expected workflow to be marked Done") - } - if state.StepIdx != 2 { - t.Errorf("expected StepIdx=2, got %d", state.StepIdx) - } - if len(listener.stepsDone) != 2 { - t.Errorf("expected 2 completed steps, got %d", len(listener.stepsDone)) - } - if !listener.done { - t.Errorf("expected listener.done to be true") - } - - entry, ok := tracker.Lookup(tmpDir, src.Key()) - if !ok || entry.Status != StatusDone { - t.Errorf("expected tracker status %q, got %+v", StatusDone, entry) - } -} - -func TestRunner_LoopWorkflow_Break(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.md"), []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - tracker := NewTracker() - exec := &mockStepExecutor{ - handlers: []func(step Step, prompt string) (StepResult, error){ - // Iteration 1 - step 1 - func(step Step, prompt string) (StepResult, error) { - return StepResult{Output: "finding tests", Summary: "analyzed", Decision: ""}, nil - }, - // Iteration 1 - tail step: continue - func(step Step, prompt string) (StepResult, error) { - return StepResult{Output: "tests still failing", Summary: "tests checked", Decision: LoopContinue}, nil - }, - // Iteration 2 - step 1 - func(step Step, prompt string) (StepResult, error) { - return StepResult{Output: "applied fix", Summary: "fixed code", Decision: ""}, nil - }, - // Iteration 2 - tail step: break! - func(step Step, prompt string) (StepResult, error) { - return StepResult{ - Output: "all tests passing", - Summary: "verified all tests green", - Decision: LoopBreak, - FinishData: &FinishData{Description: "fixed all tests"}, - }, nil - }, - }, - } - listener := &mockRunnerListener{} - runner := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "loop-pipeline", - Steps: []Step{ - { - Name: "test-and-fix", - Kind: "loop", - Steps: []Step{ - {Name: "inspect", Prompt: "Inspect code"}, - {Name: "verify", Prompt: "Run test suite"}, - }, - MaxIterations: 5, - }, - }, - } - src := NewTextSource(1, "Fix CI failure") - - state, err := runner.Run(context.Background(), tmpDir, 1, def, src) - if err != nil { - t.Fatalf("unexpected error running loop: %v", err) - } - if !state.Done { - t.Errorf("expected loop workflow to be Done") - } - if state.FinishData == nil || state.FinishData.Description != "fixed all tests" { - t.Errorf("unexpected finish data: %+v", state.FinishData) - } - if len(exec.calls) != 4 { - t.Errorf("expected 4 step executions across 2 iterations, got %d", len(exec.calls)) - } - if len(listener.notes) == 0 { - t.Errorf("expected loop notes to be recorded") - } - for _, note := range listener.notes { - if !strings.HasPrefix(note, " ") { - t.Errorf("expected note to start with 3-space margin, got %q", note) - } - } -} - -func TestRunner_LoopWorkflow_MaxIterations(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.md"), []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - tracker := NewTracker() - exec := &mockStepExecutor{ - handlers: []func(step Step, prompt string) (StepResult, error){ - // Iteration 1 - func(step Step, prompt string) (StepResult, error) { - return StepResult{Output: "iter1", Summary: "sum1", Decision: LoopContinue}, nil - }, - // Iteration 2 (max reached) - func(step Step, prompt string) (StepResult, error) { - return StepResult{Output: "iter2", Summary: "sum2", Decision: LoopContinue}, nil - }, - }, - } - listener := &mockRunnerListener{} - runner := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "limited-loop", - Steps: []Step{ - { - Name: "repeat-step", - Kind: "loop", - MaxIterations: 2, - Steps: []Step{ - {Name: "step-inner", Prompt: "Work on task"}, - }, - }, - }, - } - src := NewTextSource(1, "Task with max 2 iterations") - - state, err := runner.Run(context.Background(), tmpDir, 1, def, src) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if !state.Done { - t.Errorf("expected workflow to finish upon reaching max iterations") - } -} - -func TestRunner_ContextCancellation(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.md"), []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - ctx, cancel := context.WithCancel(context.Background()) - cancel() // Cancel immediately - - tracker := NewTracker() - exec := &mockStepExecutor{} - listener := &mockRunnerListener{} - runner := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "cancelled-workflow", - Steps: []Step{ - {Name: "step-1", Prompt: "Do work"}, - }, - } - src := NewTextSource(1, "Immediate cancel") - - _, err := runner.Run(ctx, tmpDir, 1, def, src) - if !errors.Is(err, context.Canceled) { - t.Errorf("expected context.Canceled, got %v", err) - } - if !listener.failed { - t.Errorf("expected listener.failed to be true") - } -} - -func TestWorkflow_PromptAssembly(t *testing.T) { - step := Step{ - Name: "unit-test-step", - Prompt: "Run unit tests and report failures.", - } - src := NewTextSource(1, "User issue context") - prevOutputs := []string{"Previous analysis completed successfully."} - ctx := &StepPromptCtx{ - NotesDir: "/tmp/ask/plans/unit-test-step", - PrevNotesDir: "/tmp/ask/plans/start", - IsStartStep: false, - Loop: &LoopPromptCtx{ - Name: "test-loop", - Iteration: 2, - MaxIterations: 5, - ExitCondition: "all tests pass", - IsTail: true, - }, - } - - prompt := BuildStepPrompt(step, src, prevOutputs, ctx) - if !strings.Contains(prompt, "Run unit tests and report failures.") { - t.Errorf("prompt missing base prompt") - } - if !strings.Contains(prompt, "Previous step output:") { - t.Errorf("prompt missing previous step output") - } - if !strings.Contains(prompt, "Workflow notes directories:") { - t.Errorf("prompt missing notes directory clause") - } - if !strings.Contains(prompt, "[Workflow loop \"test-loop\" · iteration 2 of up to 5]") { - t.Errorf("prompt missing loop framing") - } - if !strings.Contains(prompt, "Loop exit goal: all tests pass") { - t.Errorf("prompt missing exit condition") - } - - // Reminders - remindSummary := EndTurnReminder(RemindNoSummary, "") - if !strings.Contains(remindSummary, "without calling end_turn") { - t.Errorf("unexpected reminder: %s", remindSummary) - } - - remindDecision := EndTurnReminder(RemindNoDecision, "") - if !strings.Contains(remindDecision, "without a `decision`") { - t.Errorf("unexpected reminder: %s", remindDecision) - } - - remindDir := EndTurnReminder(RemindFixPlanDir, "not a directory") - if !strings.Contains(remindDir, "notes directory is not usable: not a directory") { - t.Errorf("unexpected reminder: %s", remindDir) - } - - // Helpers - summaryLine := StepSummaryLine("analysis", "anthropic", "claude-3-7-sonnet", "Found 2 bugs") - if !strings.Contains(summaryLine, "▸ analysis (anthropic/claude-3-7-sonnet)") || !strings.Contains(summaryLine, "Found 2 bugs") { - t.Errorf("unexpected step summary line: %s", summaryLine) - } - - meta := ProviderMeta("openai", "gpt-4o") - if meta != "openai/gpt-4o" { - t.Errorf("expected 'openai/gpt-4o', got %q", meta) - } -} - -func TestWorkflowNoteLine_Margin(t *testing.T) { - if got, want := WorkflowNoteLine("test message", ""), " test message"; got != want { - t.Errorf("WorkflowNoteLine without detail: got %q, want %q", got, want) - } - if got, want := WorkflowNoteLine("test message", "detail"), " test message: detail"; got != want { - t.Errorf("WorkflowNoteLine with detail: got %q, want %q", got, want) - } - if got, want := LoopNoteLine("my-loop", "started", "max 5 iteration(s)"), " ⟳ loop \"my-loop\" started: max 5 iteration(s)"; got != want { - t.Errorf("LoopNoteLine started: got %q, want %q", got, want) - } - if got, want := LoopNoteLine("my-loop", "break", ""), " ⟳ loop \"my-loop\" break"; got != want { - t.Errorf("LoopNoteLine break: got %q, want %q", got, want) - } -} - -func TestWorkflowRunner_ADKSequentialAgent(t *testing.T) { - tmpDir := t.TempDir() - def := Def{ - Name: "adk-seq-pipeline", - Description: "Sequential pipeline test", - Steps: []Step{ - {Name: "step-1", Prompt: "Analysis"}, - {Name: "step-2", Prompt: "Implementation"}, - }, - } - src := NewTextSource(1, "ADK Sequential Agent Test") - - agentInstance, err := BuildWorkflowAgent(context.Background(), WorkflowAgentConfig{ - Def: def, - Source: src, - Cwd: tmpDir, - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &mockWorkflowLLM{name: "mock-llm-" + step.Name}, nil - }, - }) - if err != nil { - t.Fatalf("failed to build ADK workflow agent: %v", err) - } - - if agentInstance.Name() != "adk-seq-pipeline" { - t.Errorf("expected agent name 'adk-seq-pipeline', got %q", agentInstance.Name()) - } - if len(agentInstance.SubAgents()) != 2 { - t.Fatalf("expected 2 subagents, got %d", len(agentInstance.SubAgents())) - } - if agentInstance.SubAgents()[0].Name() != "step-1" || agentInstance.SubAgents()[1].Name() != "step-2" { - t.Errorf("unexpected subagent names: %s, %s", agentInstance.SubAgents()[0].Name(), agentInstance.SubAgents()[1].Name()) - } - - sessSvc := session.InMemoryService() - sess, err := sessSvc.Create(context.Background(), &session.CreateRequest{ - AppName: "ask", - UserID: "user", - SessionID: "test-sess-seq", - }) - if err != nil { - t.Fatalf("failed to create session: %v", err) - } - - r, err := runner.New(runner.Config{ - AppName: "ask", - Agent: agentInstance, - SessionService: sessSvc, - }) - if err != nil { - t.Fatalf("failed to create runner: %v", err) - } - - userMsg := genai.NewContentFromText("Start workflow", genai.RoleUser) - for _, err := range r.Run(context.Background(), "user", sess.Session.ID(), userMsg, agent.RunConfig{}) { - if err != nil { - t.Fatalf("error during ADK workflow execution: %v", err) - } - } -} - -func TestWorkflowRunner_ADKLoopAgent_ExitLoop(t *testing.T) { - tmpDir := t.TempDir() - def := Def{ - Name: "adk-loop-pipeline", - Steps: []Step{ - { - Name: "validation-loop", - Kind: "loop", - MaxIterations: 4, - ExitCondition: "all tests pass", - Steps: []Step{ - {Name: "test-runner", Prompt: "Run tests"}, - {Name: "fixer", Prompt: "Apply fixes"}, - }, - }, - }, - } - src := NewTextSource(1, "ADK Loop Agent Test") - - agentInstance, err := BuildWorkflowAgent(context.Background(), WorkflowAgentConfig{ - Def: def, - Source: src, - Cwd: tmpDir, - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &mockWorkflowLLM{name: "mock-llm-" + step.Name}, nil - }, - }) - if err != nil { - t.Fatalf("failed to build loop agent: %v", err) - } - - if len(agentInstance.SubAgents()) != 1 { - t.Fatalf("expected 1 top subagent (the loop), got %d", len(agentInstance.SubAgents())) - } - loopAg := agentInstance.SubAgents()[0] - if loopAg.Name() != "validation-loop" { - t.Errorf("expected loop agent name 'validation-loop', got %q", loopAg.Name()) - } - if len(loopAg.SubAgents()) != 2 { - t.Fatalf("expected 2 inner subagents in loop, got %d", len(loopAg.SubAgents())) - } -} - -func TestWorkflowRunner_ADKLoopAgent_MaxIterations(t *testing.T) { - tmpDir := t.TempDir() - def := Def{ - Name: "max-iters-pipeline", - Steps: []Step{ - { - Name: "repeat-loop", - Kind: "loop", - MaxIterations: 3, - Steps: []Step{ - {Name: "step-inner", Prompt: "Iterate task"}, - }, - }, - }, - } - src := NewTextSource(1, "Max Iterations Test") - - agentInstance, err := BuildWorkflowAgent(context.Background(), WorkflowAgentConfig{ - Def: def, - Source: src, - Cwd: tmpDir, - ModelBuilder: func(ctx context.Context, step Step) (model.LLM, error) { - return &mockWorkflowLLM{name: "mock-" + step.Name}, nil - }, - }) - if err != nil { - t.Fatalf("failed to build workflow agent: %v", err) - } - - if agentInstance.Name() != "max-iters-pipeline" { - t.Errorf("expected agent name 'max-iters-pipeline', got %q", agentInstance.Name()) - } -} - -func TestWorkflowRunner_NotesDirectoryLifecycle(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - planFile := filepath.Join(startDir, "plan.md") - if err := os.WriteFile(planFile, []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - tracker := NewTracker() - exec := &mockStepExecutor{} - listener := &mockRunnerListener{} - runnerInstance := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "lifecycle-pipeline", - Steps: []Step{ - {Name: "step-1", Prompt: "Do analysis"}, - }, - } - src := NewTextSource(1, "Test Lifecycle") - - state, err := runnerInstance.Run(context.Background(), tmpDir, 1, def, src) - if err != nil { - t.Fatalf("unexpected error running workflow: %v", err) - } - if !state.Done { - t.Errorf("expected workflow to complete") - } - - // Verify plans directory was cleaned up - plansDir := filepath.Join(tmpDir, "ask", "plans") - if _, err := os.Stat(plansDir); !os.IsNotExist(err) { - t.Errorf("expected plans directory to be removed after workflow completion") - } -} - -func TestWorkflowRunner_ListenerEvents(t *testing.T) { - tmpDir := t.TempDir() - startDir := filepath.Join(tmpDir, "ask", "plans", "start") - if err := os.MkdirAll(startDir, 0755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(startDir, "plan.md"), []byte("# Plan"), 0644); err != nil { - t.Fatal(err) - } - - tracker := NewTracker() - exec := &mockStepExecutor{} - listener := &mockRunnerListener{} - runnerInstance := NewRunner(tracker, exec, listener) - - def := Def{ - Name: "events-pipeline", - Steps: []Step{ - {Name: "step-a", Prompt: "Prompt A"}, - {Name: "step-b", Prompt: "Prompt B"}, - }, - } - src := NewTextSource(1, "Events Test") - - state, err := runnerInstance.Run(context.Background(), tmpDir, 1, def, src) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if !state.Done { - t.Errorf("expected workflow to finish") - } - - if !listener.started { - t.Errorf("expected OnWorkflowStarted to be called") - } - if len(listener.stepsStarted) != 2 || listener.stepsStarted[0] != "step-a" || listener.stepsStarted[1] != "step-b" { - t.Errorf("unexpected steps started: %+v", listener.stepsStarted) - } - if len(listener.stepsDone) != 2 { - t.Errorf("expected 2 steps done, got %+v", listener.stepsDone) - } - if !listener.done { - t.Errorf("expected OnWorkflowDone to be called") - } -}