From 690bd684d0b5f39e60aa1280ab5350916cbebbf9 Mon Sep 17 00:00:00 2001 From: Alexandre Balmes Date: Sun, 17 May 2026 23:58:54 +0200 Subject: [PATCH] feat(roles): implement F098 agent role injection via AGENTS.md - `.go-arch-lint.yml`: register `infra-roles` component and wire dependency rules - `CHANGELOG.md`: add F098 entry to Unreleased section - `CLAUDE.md`: add two architecture rules for infra components and adapter patterns - `README.md`: add Agent Roles bullet to feature list - `docs/README.md`: add Agent Roles entry under agent-steps section - `docs/reference/error-codes.md`: add USER.INPUT.MISSING_ROLE error code documentation - `docs/user-guide/agent-steps.md`: add full Agent Roles section with examples and validation table - `docs/user-guide/workflow-syntax.md`: document `role` field in agent step syntax reference - `internal/application/conversation_manager.go`: inject role system prompt in ExecuteConversation - `internal/application/conversation_manager_helpers_test.go`: consolidate testResolverWithValues helper - `internal/application/conversation_manager_role_test.go`: add 267-line role injection test suite - `internal/application/conversation_manager_tdd_test.go`: update ExecuteConversation call signatures - `internal/application/conversation_manager_test.go`: add workflowDir parameter to test calls - `internal/application/execution_service.go`: wire role resolution in executeAgentStep and conversation path - `internal/application/execution_service_conversation_step_test.go`: update test signatures - `internal/application/execution_service_role_test.go`: add 504-line role execution test suite - `internal/application/execution_setup.go`: wire SetAgentRoleRepository in service initialization - `internal/application/role_loader.go`: add BuildRoleSystemPrompt helper and nil guard - `internal/application/role_loader_test.go`: remove duplicate resolver helper - `internal/domain/workflow/agent_role.go`: add ErrRoleEmptyContent validation code - `internal/domain/workflow/agent_role_test.go`: update tests for struct field changes - `internal/infrastructure/roles/filesystem_repository.go`: fix cross-platform path traversal check and filter empty search paths - `internal/infrastructure/xdg/xdg.go`: expose AWF agents XDG config dir - `internal/interfaces/cli/validate.go`: add role reference validation with path-traversal guard - `internal/interfaces/cli/validate_role_test.go`: add 416-line CLI validation test suite Closes #351 --- .go-arch-lint.yml | 14 + CHANGELOG.md | 1 + CLAUDE.md | 4 +- README.md | 1 + docs/README.md | 1 + docs/reference/error-codes.md | 17 + docs/user-guide/agent-steps.md | 231 ++++++++ docs/user-guide/workflow-syntax.md | 23 + internal/application/conversation_manager.go | 26 +- .../conversation_manager_helpers_test.go | 20 +- .../conversation_manager_role_test.go | 267 ++++++++++ .../conversation_manager_tdd_test.go | 24 +- .../application/conversation_manager_test.go | 10 + internal/application/execution_service.go | 46 +- ...xecution_service_conversation_step_test.go | 31 +- .../execution_service_role_test.go | 504 ++++++++++++++++++ internal/application/execution_setup.go | 13 + internal/application/role_loader.go | 125 +++++ internal/application/role_loader_test.go | 230 ++++++++ internal/domain/errors/codes.go | 3 + internal/domain/errors/codes_role_test.go | 36 ++ .../domain/ports/agent_role_repository.go | 12 + internal/domain/workflow/agent_config.go | 37 +- .../agent_config_conversation_test.go | 79 +-- .../workflow/agent_config_prompt_file_test.go | 9 - .../domain/workflow/agent_config_role_test.go | 117 ++++ internal/domain/workflow/agent_role.go | 28 + internal/domain/workflow/agent_role_test.go | 108 ++++ internal/domain/workflow/validation_errors.go | 5 + .../infrastructure/repository/yaml_mapper.go | 1 + .../repository/yaml_mapper_role_test.go | 103 ++++ .../infrastructure/repository/yaml_types.go | 7 +- .../roles/filesystem_repository.go | 116 ++++ .../roles/filesystem_repository_test.go | 432 +++++++++++++++ internal/infrastructure/xdg/xdg.go | 16 +- internal/interfaces/cli/run.go | 3 + internal/interfaces/cli/validate.go | 92 ++++ internal/interfaces/cli/validate_role_test.go | 416 +++++++++++++++ internal/testutil/mocks/mocks.go | 24 + tests/integration/api/functional_test.go | 4 +- 40 files changed, 3084 insertions(+), 152 deletions(-) create mode 100644 internal/application/conversation_manager_role_test.go create mode 100644 internal/application/execution_service_role_test.go create mode 100644 internal/application/role_loader.go create mode 100644 internal/application/role_loader_test.go create mode 100644 internal/domain/errors/codes_role_test.go create mode 100644 internal/domain/ports/agent_role_repository.go create mode 100644 internal/domain/workflow/agent_config_role_test.go create mode 100644 internal/domain/workflow/agent_role.go create mode 100644 internal/domain/workflow/agent_role_test.go create mode 100644 internal/infrastructure/repository/yaml_mapper_role_test.go create mode 100644 internal/infrastructure/roles/filesystem_repository.go create mode 100644 internal/infrastructure/roles/filesystem_repository_test.go create mode 100644 internal/interfaces/cli/validate_role_test.go diff --git a/.go-arch-lint.yml b/.go-arch-lint.yml index f803dfcc..34ab5f21 100644 --- a/.go-arch-lint.yml +++ b/.go-arch-lint.yml @@ -271,6 +271,9 @@ components: infra-otel: in: infrastructure/otel + infra-roles: + in: infrastructure/roles + infra-skills: in: infrastructure/skills @@ -347,6 +350,7 @@ deps: - infra-http - infra-notify - infra-repository + - infra-roles - infra-skills - infra-xdg canUse: @@ -539,6 +543,15 @@ deps: - go-stdlib - otel + infra-roles: + mayDependOn: + - domain-workflow + - domain-ports + - infra-skills + - infra-xdg + canUse: + - go-stdlib + infra-skills: mayDependOn: - domain-workflow @@ -575,6 +588,7 @@ deps: - infra-otel - infra-plugin - infra-repository + - infra-roles - infra-skills - infra-store - infra-tokenizer diff --git a/CHANGELOG.md b/CHANGELOG.md index c877eaa9..e0213f32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **F098**: Agent role injection via AGENTS.md — new optional `role: ` field on agent steps loads `/AGENTS.md`, strips YAML frontmatter, and injects the body as `system_prompt` (Claude native `--system-prompt`; CLI providers via existing first-turn concat). Role discovery searches in strict priority order: `AWF_AGENTS_PATH` (exclusive override), `.awf/agents/`, `.agents/`, `$XDG_CONFIG_HOME/awf/agents/`, `~/.agents/`. Explicit path references supported (relative to workflow file, absolute, tilde-expanded); `role` value is interpolated (`role: {{.inputs.persona}}`) before discovery. When combined with inline `system_prompt`, the role content is prepended (`\n\n`). Works in both `mode: single` and `mode: conversation` — three injection sites (`executeAgentStep`, `executeResumableAgentCall`, `ConversationManager.ExecuteConversation`) unified through a single `ComposeSystemPrompt` helper. `awf validate` reports hard errors for missing role directories / missing AGENTS.md and non-blocking warnings for empty bodies, `AGENTS.md` > 500KB, and combined `role + system_prompt` > 10KB. Path-traversal patterns (`..`) rejected during validation. New error code `USER.INPUT.MISSING_ROLE`. Backward compatible: agent steps without `role:` behave identically to pre-F098. Implementation mirrors the existing skills mechanism (new `AgentRole` entity, `AgentRoleRepository` port, `internal/infrastructure/roles/` adapter, `SetAgentRoleRepository()` optional wiring). Roles establish **who** the agent is (system prompt); skills define **what** it knows (user-prompt XML block) — orthogonal channels by design - **F097**: HTTP REST API server (`awf serve`) — new `internal/interfaces/api/` adapter alongside `cli/` and `tui/` exposing workflow discovery (`GET /api/workflows`, `GET /api/workflows/{name}`, `POST /api/workflows/{name}/validate`), async execution (`POST /api/workflows/{name}/run` returning 202 + `execution_id`), lifecycle control (`GET /api/executions`, `GET /api/executions/{id}`, `DELETE /api/executions/{id}`, `POST /api/executions/{id}/resume`), Server-Sent Events streaming (`GET /api/executions/{id}/events` emitting `step.started`, `step.completed`, `step.failed`, `workflow.completed`, `workflow.failed`, `output`), and history queries (`GET /api/history`, `GET /api/history/stats`); Huma v2 + chi v5 generate OpenAPI 3.1 spec served at `/openapi.json`, `/openapi.yaml`, and Swagger UI at `/docs`; Bridge adapter pattern mirrors `tui/bridge.go` with `sync.Map` tracking active executions; SSE polling at 200ms cadence matching TUI; graceful shutdown via `signal.NotifyContext` waits up to 30s for active streams; default binding `127.0.0.1:2511` (loopback-only, non-loopback opt-in via `--host`); arch-lint enforces `interfaces-api` may import only `application/` and `domain/*` (no `infrastructure/`, `cli/`, or `tui/`); see [ADR-016](docs/ADR/016-http-interface-adapter-huma-sse-streaming.md) ## [0.9.0] - 2026-05-14 diff --git a/CLAUDE.md b/CLAUDE.md index ca47c7d1..a08d22ed 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -217,8 +217,6 @@ func TestWorkflowValidation(t *testing.T) { ## Architecture Rules -- Commit generated protobuf files (.pb.go, _grpc.pb.go) to git; treat as source artifacts for build reproducibility, not ephemeral build outputs -- CLI command implementations must call infrastructure layer methods rather than reimplementing HTTP requests, parsing, or validation; avoid logic duplication - Application layer must persist source metadata (SetSourceData) after successful infrastructure installation; omitting state blocks downstream operations like updates - Use dual import aliases (e.g., infrastructurePlugin + registry) when consuming refactored packages; explicitly requalify all symbol references to prevent ambiguity - Keep thin wrapper functions in original location for backward compatibility; delegate completely to extracted packages to maintain single source of truth @@ -240,6 +238,8 @@ func TestWorkflowValidation(t *testing.T) { - Use provider name prefixes for all infrastructure provider helper methods (buildCopilot, extractCopilot, parseCopilot, validateCopilot) to prevent naming collisions across implementations - Use mutex-protected getter/setter methods (Get*/Set*) for concurrent shared state; apply consistently across all goroutine-accessed fields - Server owns background task coordination (WaitGroup); pass by pointer to handlers and coordinate shutdown: httpSrv.Shutdown() then sseWG.Wait() +- Always update `.go-arch-lint.yml` when adding new infrastructure components; register the package and document its dependency rules in the commit message +- When implementing infrastructure adapters that follow established patterns (e.g., FilesystemAgentRoleRepository mirrors FilesystemSkillRepository), reuse shared utilities (skills.StripFrontmatter) to maintain single source of truth ## Common Pitfalls diff --git a/README.md b/README.md index afd64608..954c34b5 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ A Go CLI tool for orchestrating AI agents (Claude, Gemini, Codex, GitHub Copilot - **Agent Steps** - Invoke AI agents via CLI tools (Claude, Codex, Gemini, GitHub Copilot) or direct HTTP (OpenAI, Ollama, vLLM, Groq) with prompt templates, response parsing, and accurate token tracking - **Output Formatting for Agent Steps** - Automatically strip markdown code fences and validate JSON output; human-readable streaming display controlled by `output_format` field (text vs raw NDJSON); unified display-event abstraction across all 6 providers with optional verbose mode showing tool-use markers (`[tool: Name(Arg)]`) - **Agent Skills** - Inject deterministic domain knowledge into agent steps via `skills:` declarations in workflow YAML; filesystem-based multi-directory discovery (project `.awf/skills/`, `.agents/skills/`, `.claude/skills/`, XDG global) with priority ordering; SKILL.md frontmatter stripping and agentskills.io-compliant `` structured wrapping with bundled resource enumeration; validated by `awf validate` +- **Agent Roles** - Inject reusable personas into agent steps via `role:` field referencing AGENTS.md files; filesystem-based multi-directory discovery (project `.awf/agents/`, `.agents/`, XDG global) with priority ordering; AGENTS.md frontmatter stripping and system prompt injection with optional composition via inline `system_prompt` field; validated by `awf validate` - **External Prompt Files** - Load agent prompts from `.md` files with full template interpolation, helper functions, and local override support - **External Script Files** - Load commands from external script files with shebang-based interpreter dispatch, template interpolation, path resolution, and local override support - **Conversation Mode** - Multi-turn conversations with native session resume for CLI providers (`claude`, `codex`, `gemini`, `opencode`, `github_copilot`), automatic context window management for HTTP providers, mid-conversation context injection via `inject_context` field, and token tracking across all turns diff --git a/docs/README.md b/docs/README.md index 3bcb51c8..f95ec9ad 100644 --- a/docs/README.md +++ b/docs/README.md @@ -30,6 +30,7 @@ Learn how to use AWF effectively: - [Interactive Input Collection](user-guide/interactive-inputs.md) - Automatic prompting for missing workflow inputs - [Agent Steps](user-guide/agent-steps.md) - Invoke AI agents via CLI (Claude, Codex, Gemini, GitHub Copilot) or HTTP APIs (OpenAI, Ollama, vLLM, Groq) - [Agent Skills](user-guide/agent-steps.md#agent-skills) - Inject deterministic domain knowledge into agent steps via `skills:` declarations with multi-directory discovery + - [Agent Roles](user-guide/agent-steps.md#agent-roles) - Inject reusable personas into agent steps via `role:` field referencing AGENTS.md files with system prompt composition - [Output Formatting](user-guide/agent-steps.md#output-formatting) - Automatic code fence stripping and JSON validation (`output_format: json|text`) - [Streaming Output Display & Tool Markers](user-guide/agent-steps.md#streaming-output-display--tool-markers) - Human-readable filtered output and tool-use markers for `--output streaming` and `--output buffered` modes - [External Prompt Files](user-guide/agent-steps.md#external-prompt-files) - Load prompts from Markdown files with template interpolation diff --git a/docs/reference/error-codes.md b/docs/reference/error-codes.md index 54794f9c..e8e1ca92 100644 --- a/docs/reference/error-codes.md +++ b/docs/reference/error-codes.md @@ -75,6 +75,23 @@ awf run code-review --- +### USER.INPUT.MISSING_ROLE + +**Description:** An agent role referenced in a workflow step could not be resolved. Either the role directory does not exist in any discovery path, the directory exists but contains no `AGENTS.md` file, or no `AgentRoleRepository` is wired when the step is executed. + +**Resolution:** Verify the role name matches a directory in one of the discovery paths (`.awf/agents/`, `.agents/`, `$XDG_CONFIG_HOME/awf/agents/`, `~/.agents/`) and that `/AGENTS.md` is readable. For path-based references, verify the path is correct relative to the workflow file. Set `AWF_AGENTS_PATH=` to restrict discovery to a single directory. + +**Example:** +```bash +awf run code-review +# Error [USER.INPUT.MISSING_ROLE]: role 'go-senior' not found in search paths: +# .awf/agents/, .agents/, ~/.config/awf/agents/, ~/.agents/ +``` + +**Related codes:** `USER.INPUT.MISSING_SKILL`, `USER.INPUT.MISSING_FILE`, `WORKFLOW.VALIDATION.INVALID_REFERENCE` + +--- + ### USER.INPUT.INVALID_FORMAT **Description:** The file format does not match expected structure or contains invalid syntax. diff --git a/docs/user-guide/agent-steps.md b/docs/user-guide/agent-steps.md index e7aece13..1d8e08b6 100644 --- a/docs/user-guide/agent-steps.md +++ b/docs/user-guide/agent-steps.md @@ -650,6 +650,237 @@ Validation reports: | Skill not in expected location | Wrong discovery directory | Move skill to one of the 7 standard directories or use `path:` reference | | Large skill file warning | `SKILL.md` exceeds 500KB | Consider splitting into multiple smaller skills | +## Agent Roles + +Agent roles define reusable personas that are injected into the agent's system prompt. Unlike skills (which inject knowledge into the user prompt), roles establish the agent's identity, behavior, and perspective via the system prompt. Roles are stored in AGENTS.md files following the [agents.md](https://github.com/agentsmd/agents.md) specification. + +### Basic Usage + +Create a role directory with an `AGENTS.md` file: + +**Directory structure:** +``` +.awf/agents/ +├── go-senior/ +│ └── AGENTS.md # Agent persona (frontmatter optional) +├── security-reviewer/ +│ └── AGENTS.md +└── docs-writer/ + └── AGENTS.md +``` + +**File:** `.awf/agents/go-senior/AGENTS.md` +```markdown +--- +name: go-senior +description: Senior Go engineer persona +--- + +You are a senior Go engineer with 10+ years of experience. + +When reviewing code: +- Prioritize readability and maintainability +- Suggest idiomatic Go patterns +- Consider performance implications +- Flag potential race conditions +- Reference Go proverbs and best practices +``` + +Reference the role by name in your workflow: + +```yaml +review: + type: agent + provider: claude + role: go-senior + prompt: "Review this Go code: {{.inputs.code}}" + on_success: done +``` + +When executed, the AGENTS.md content is stripped of frontmatter and injected as the system prompt to the agent, establishing the persona before the user prompt is sent. + +### Multiple Roles + +A single agent step uses one role only. To combine multiple personas, either: +1. Create a composite role that includes all relevant perspectives +2. Chain multiple agent steps with different roles + +```yaml +# ❌ Invalid: only one role per step +step: + type: agent + role: go-senior + role: security-reviewer # Error + +# ✅ Valid: chain steps for multiple perspectives +initial_review: + type: agent + provider: claude + role: go-senior + prompt: "Review this code: {{.inputs.code}}" + on_success: security_review + +security_review: + type: agent + provider: claude + role: security-reviewer + prompt: | + After this Go review: + {{.states.initial_review.Output}} + + Now analyze for security issues: {{.inputs.code}} + on_success: done +``` + +### Role Discovery + +AWF searches for roles in the following directories, in priority order: + +1. **`AWF_AGENTS_PATH` environment variable** (if set, exclusive — overrides all others) +2. **`.awf/agents/`** (project-level, AWF-specific) +3. **`.agents/`** (project-level, cross-client compatibility) +4. **`$XDG_CONFIG_HOME/awf/agents/`** (global user, AWF-specific — defaults to `~/.config/awf/agents/`) +5. **`~/.agents/`** (global user, cross-client) + +This enables shared global personas while allowing project-specific overrides. + +**Example with environment variable:** + +```bash +# Use only roles from custom directory +AWF_AGENTS_PATH=/shared/agents awf run workflow +``` + +### Role Content Format + +**Frontmatter is optional** — AWF strips YAML frontmatter (content between `---` delimiters) if present, preserving only the Markdown body: + +```markdown +--- +name: go-senior +description: Senior Go engineer persona +license: MIT +tags: [go, senior, code-review] +--- + +You are a senior Go engineer... +``` + +Only the Markdown body is injected as the system prompt. The role name comes from the directory name (e.g., `go-senior`), not from frontmatter. + +### Combining Role with Inline System Prompt + +A step can combine a role with an inline `system_prompt` field. The role content is injected first, followed by the inline prompt, separated by a blank line: + +```yaml +review: + type: agent + provider: claude + role: go-senior + system_prompt: "Focus on performance optimizations and memory leaks." + prompt: "Review this code: {{.inputs.code}}" + on_success: done +``` + +The effective system prompt sent to the agent is: + +``` + + +Focus on performance optimizations and memory leaks. +``` + +This allows you to reuse a base persona while adding step-specific context or overrides. + +### Explicit Path References + +Reference roles by explicit path instead of discovery: + +```yaml +review: + type: agent + provider: claude + role: ./custom-agents/senior-go + prompt: "Review: {{.inputs.code}}" + on_success: done +``` + +Paths can be: +- **Relative** to the workflow directory: `role: ./custom-agents/senior-go` +- **Absolute**: `role: /home/user/agents/senior-go` +- **Home-relative**: `role: ~/shared-agents/senior-go` + +### Dynamic Role Selection + +The `role` field supports template interpolation, enabling dynamic role selection based on workflow inputs or state: + +```yaml +inputs: + - name: persona + type: string + default: go-senior + +review: + type: agent + provider: claude + role: "{{.inputs.persona}}" + prompt: "Review: {{.inputs.code}}" + on_success: done +``` + +```bash +awf run workflow --input persona=security-reviewer --input code="..." +``` + +### Validation + +Use `awf validate` to check that all role references exist and are readable: + +```bash +awf validate workflow.yaml +``` + +Validation reports: +- ✓ Roles found with valid `AGENTS.md` files +- ✗ Roles referenced but not found in any discovery path +- ✗ Role directories found but missing `AGENTS.md` +- ⚠ Empty `AGENTS.md` files (warning — content will be empty) +- ⚠ `AGENTS.md` exceeds 500KB (warning — context window impact) +- ⚠ Combined `role + system_prompt` exceeds 10KB (warning — context window impact) + +### Troubleshooting + +| Issue | Cause | Solution | +|-------|-------|----------| +| `AgentRoleNotFoundError` | Referenced role not found in discovery paths | Check role directory name matches YAML declaration; verify `AGENTS.md` exists | +| Empty role content | `AGENTS.md` is 0 bytes or contains only frontmatter | Add Markdown body to `AGENTS.md` | +| Role not in expected location | Wrong discovery directory | Move role to one of the 5 standard directories or use explicit path reference | +| Large role file warning | `AGENTS.md` exceeds 500KB | Consider splitting into multiple smaller roles or removing verbose content | +| Combined prompt too large | `role + system_prompt` over 10KB | Reduce role/prompt size or split into separate steps | + +### Conversation Mode with Roles + +Roles work seamlessly in conversation mode (`mode: conversation`). The role is resolved and injected once at the start of the conversation, establishing the agent's persona for all subsequent user turns: + +```yaml +chat: + type: agent + provider: claude + mode: conversation + role: go-senior + system_prompt: "Be helpful and concise." + prompt: "{{.inputs.topic}}" + timeout: 600 + on_success: done +``` + +### Limitations + +- A single agent step uses one role only — no composition or inheritance +- No template interpolation is applied to role content — it is injected as-is +- Roles do not reference other roles; only workflow steps reference roles +- Roles establish system-level persona via the standard `system_prompt` mechanism available to all providers (Claude native `--system-prompt`, CLI-based providers via first-turn concat, HTTP providers via API field) + ## External Prompt Files Instead of inlining prompts in YAML, you can load prompts from external Markdown files using the `prompt_file` field: diff --git a/docs/user-guide/workflow-syntax.md b/docs/user-guide/workflow-syntax.md index f8440937..d593634c 100644 --- a/docs/user-guide/workflow-syntax.md +++ b/docs/user-guide/workflow-syntax.md @@ -408,6 +408,28 @@ review: See [Agent Steps — Agent Skills](agent-steps.md#agent-skills) for discovery paths, SKILL.md format, and validation. +### Agent Step with Role + +Inject a reusable persona into agent steps via `role:` field. Roles are loaded from AGENTS.md files and establish the agent's identity via system prompt. + +```yaml +review: + type: agent + provider: claude + role: go-senior + system_prompt: "Focus on performance and memory safety." + prompt: "Review this Go code: {{.states.read.Output}}" + options: + model: claude-sonnet-4-20250514 + timeout: 120 + on_success: done + on_failure: error +``` + +Roles and inline `system_prompt` are combined: role content is injected first, followed by the inline prompt, separated by a blank line. + +See [Agent Steps — Agent Roles](agent-steps.md#agent-roles) for discovery paths, AGENTS.md format, dynamic role selection, and validation. + ### Conversation Mode `mode: conversation` runs an **interactive user-driven loop**: the agent replies, AWF prompts for your next message, and the loop continues until you submit an empty line, `exit`, or `quit`. It requires a terminal (or piped stdin). @@ -438,6 +460,7 @@ For **automated cross-step session resume** (no stdin loop), use `mode: single` | `prompt` | string | Yes* | Prompt template (supports `{{.inputs.*}}` and `{{.states.*}}` interpolation); in `mode: conversation` this serves as the first user message | | `prompt_file` | string | No* | Path to external prompt template file (mutually exclusive with `prompt`; not supported in `mode: conversation`) | | `system_prompt` | string | No | System message preserved for the whole session | +| `role` | string | No | Role reference — name-based (e.g., `go-senior`) or path-based (e.g., `./custom-agents/senior`); loaded from AGENTS.md file and injected as system prompt; see [Agent Roles](agent-steps.md#agent-roles) | | `output_format` | string | No | Post-processing format: `json` (strip fences + validate JSON) or `text` (strip fences only) | | `conversation` | object | No | Session tracking sub-struct. Presence opts the step into session tracking (marker flag); contents: see [Session Tracking](#session-tracking) | | `skills` | array | No | Skill references for agent context injection — name-based (e.g., `go-conventions`) or path-based (e.g., `path: ./custom/audit`); see [Agent Skills](agent-steps.md#agent-skills) | diff --git a/internal/application/conversation_manager.go b/internal/application/conversation_manager.go index cac75175..54045579 100644 --- a/internal/application/conversation_manager.go +++ b/internal/application/conversation_manager.go @@ -36,6 +36,7 @@ type ConversationManager struct { resolver interpolation.Resolver agentRegistry ports.AgentRegistry userInputReader ports.UserInputReader + agentRoleRepo ports.AgentRoleRepository } func NewConversationManager( @@ -56,6 +57,12 @@ func (m *ConversationManager) SetUserInputReader(r ports.UserInputReader) { m.userInputReader = r } +// SetAgentRoleRepository wires the optional agent role repository for role-based +// system prompt injection. When nil, steps that reference a role will return an error. +func (m *ConversationManager) SetAgentRoleRepository(repo ports.AgentRoleRepository) { + m.agentRoleRepo = repo +} + // validateConversationInputs validates step and agent config inputs. // ConversationConfig is optional — a nil config is treated as an empty config // (no ContinueFrom reference). @@ -78,6 +85,7 @@ func (m *ConversationManager) initializeConversationState( config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + composedSystemPrompt string, ) (*workflow.ConversationState, string, error) { var state *workflow.ConversationState if config != nil && config.ContinueFrom != "" { @@ -105,8 +113,7 @@ func (m *ConversationManager) initializeConversationState( } copy(state.Turns, prior.Turns) } else { - systemPrompt := step.Agent.SystemPrompt - state = workflow.NewConversationState(systemPrompt) + state = workflow.NewConversationState(composedSystemPrompt) } intCtx := buildContext(execCtx) @@ -165,6 +172,7 @@ func (m *ConversationManager) ExecuteConversation( config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + workflowDir string, stdoutW, stderrW io.Writer, ) (*workflow.ConversationResult, error) { if err := m.validateConversationInputs(step, config); err != nil { @@ -182,7 +190,12 @@ func (m *ConversationManager) ExecuteConversation( return nil, fmt.Errorf("step %s: %w", step.Name, err) } - state, resolvedPrompt, err := m.initializeConversationState(step, resolvedProvider, config, execCtx, buildContext) + composedPrompt, roleErr := BuildRoleSystemPrompt(ctx, m.agentRoleRepo, m.resolver, step, workflowDir, intCtx) + if roleErr != nil { + return nil, fmt.Errorf("step %s: resolve role: %w", step.Name, roleErr) + } + + state, resolvedPrompt, err := m.initializeConversationState(step, resolvedProvider, config, execCtx, buildContext, composedPrompt) if err != nil { return nil, err } @@ -191,8 +204,11 @@ func (m *ConversationManager) ExecuteConversation( // and inject output_format so baseCLIProvider can route display filtering // identically between executeAgentStep and conversation mode (F082). options := cloneAndInjectOutputFormat(step.Agent.Options, step.Agent.OutputFormat) - if step.Agent.SystemPrompt != "" { - options["system_prompt"] = step.Agent.SystemPrompt + if composedPrompt != "" { + if _, exists := options["system_prompt"]; exists { + m.logger.Warn("options.system_prompt overridden by composed role+system_prompt", "step", step.Name) + } + options["system_prompt"] = composedPrompt } if m.userInputReader == nil { diff --git a/internal/application/conversation_manager_helpers_test.go b/internal/application/conversation_manager_helpers_test.go index 83f905f7..a95450f0 100644 --- a/internal/application/conversation_manager_helpers_test.go +++ b/internal/application/conversation_manager_helpers_test.go @@ -11,13 +11,23 @@ import ( "github.com/stretchr/testify/assert" ) -// Simple resolver mock for testing type testResolver struct{} func (t *testResolver) Resolve(template string, ctx *interpolation.Context) (string, error) { return template, nil } +type testResolverWithValues struct { + values map[string]string +} + +func (r *testResolverWithValues) Resolve(template string, _ *interpolation.Context) (string, error) { + if v, ok := r.values[template]; ok { + return v, nil + } + return template, nil +} + // TestValidateConversationInputs_HappyPath tests valid inputs are accepted func TestValidateConversationInputs_HappyPath(t *testing.T) { manager := &ConversationManager{} @@ -102,7 +112,7 @@ func TestInitializeConversationState_FreshStart(t *testing.T) { return interpolation.NewContext() } - state, resolvedPrompt, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext) + state, resolvedPrompt, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext, "") assert.NoError(t, err) assert.NotNil(t, state) @@ -144,7 +154,7 @@ func TestInitializeConversationState_ContinueFrom_WithSessionID(t *testing.T) { return interpolation.NewContext() } - state, resolvedPrompt, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext) + state, resolvedPrompt, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext, "") assert.NoError(t, err) assert.NotNil(t, state) @@ -178,7 +188,7 @@ func TestInitializeConversationState_ContinueFrom_StepNotFound(t *testing.T) { return interpolation.NewContext() } - state, _, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext) + state, _, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext, "") assert.Error(t, err) assert.Nil(t, state) @@ -214,7 +224,7 @@ func TestInitializeConversationState_ContinueFrom_NoConversationState(t *testing return interpolation.NewContext() } - state, _, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext) + state, _, err := manager.initializeConversationState(step, "claude", config, execCtx, buildContext, "") assert.Error(t, err) assert.Nil(t, state) diff --git a/internal/application/conversation_manager_role_test.go b/internal/application/conversation_manager_role_test.go new file mode 100644 index 00000000..6a377609 --- /dev/null +++ b/internal/application/conversation_manager_role_test.go @@ -0,0 +1,267 @@ +package application + +import ( + "context" + "io" + "testing" + + domainerrors "github.com/awf-project/cli/internal/domain/errors" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/testutil/mocks" + "github.com/awf-project/cli/pkg/interpolation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConversationManager_ExecuteConversation_ComposedSystemPrompt(t *testing.T) { + logger := mocks.NewMockLogger() + registry := mocks.NewMockAgentRegistry() + + capturedOptions := make(map[string]any) + + provider := mocks.NewMockAgentProvider("claude") + provider.SetConversationFunc(func(ctx context.Context, state *workflow.ConversationState, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.ConversationResult, error) { + capturedOptions = opts + + return &workflow.ConversationResult{ + State: state, + Provider: "claude", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "mentor" { + return &workflow.AgentRole{ + Name: "mentor", + Content: "You are a mentor", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + resolver := &testResolverWithValues{ + values: map[string]string{}, + } + + manager := NewConversationManager(logger, resolver, registry) + manager.SetAgentRoleRepository(roleRepo) + manager.SetUserInputReader(&mocks.MockUserInputReader{}) + + step := &workflow.Step{ + Name: "mentor_chat", + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "Help me learn", + Role: "mentor", + SystemPrompt: "Be patient", + }, + } + + config := &workflow.ConversationConfig{} + execCtx := workflow.NewExecutionContext("test-id", "test-workflow") + + buildContext := func(ec *workflow.ExecutionContext) *interpolation.Context { + return interpolation.NewContext() + } + + result, err := manager.ExecuteConversation( + context.Background(), + step, + config, + execCtx, + buildContext, + "", + io.Discard, + io.Discard, + ) + + require.NoError(t, err) + require.NotNil(t, result) + + systemPrompt, ok := capturedOptions["system_prompt"] + require.True(t, ok, "system_prompt should be set in cloned options") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Contains(t, composed, "You are a mentor", "should contain role content") + assert.Contains(t, composed, "Be patient", "should contain inline system prompt") +} + +func TestConversationManager_ExecuteConversation_OriginalOptionsNotMutated(t *testing.T) { + logger := mocks.NewMockLogger() + registry := mocks.NewMockAgentRegistry() + + capturedOptions := make(map[string]any) + + provider := mocks.NewMockAgentProvider("claude") + provider.SetConversationFunc(func(ctx context.Context, state *workflow.ConversationState, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.ConversationResult, error) { + capturedOptions = opts + + return &workflow.ConversationResult{ + State: state, + Provider: "claude", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "expert" { + return &workflow.AgentRole{ + Name: "expert", + Content: "Expert role", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + resolver := &testResolverWithValues{ + values: map[string]string{}, + } + + manager := NewConversationManager(logger, resolver, registry) + manager.SetAgentRoleRepository(roleRepo) + manager.SetUserInputReader(&mocks.MockUserInputReader{}) + + originalOptions := map[string]any{ + "temperature": 0.7, + } + + step := &workflow.Step{ + Name: "expert_chat", + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "Analyze this", + Role: "expert", + Options: originalOptions, + SystemPrompt: "Be analytical", + }, + } + + config := &workflow.ConversationConfig{} + execCtx := workflow.NewExecutionContext("test-id", "test-workflow") + + buildContext := func(ec *workflow.ExecutionContext) *interpolation.Context { + return interpolation.NewContext() + } + + _, err := manager.ExecuteConversation( + context.Background(), + step, + config, + execCtx, + buildContext, + "", + io.Discard, + io.Discard, + ) + + require.NoError(t, err) + + assert.NotContains(t, originalOptions, "system_prompt", + "original options map should not be mutated with system_prompt") + assert.Equal(t, 0.7, originalOptions["temperature"], + "original options should retain their values") + + assert.Contains(t, capturedOptions, "system_prompt", + "cloned options should have system_prompt") + assert.Equal(t, 0.7, capturedOptions["temperature"], + "cloned options should contain original values") +} + +func TestConversationManager_ExecuteConversation_RoleOnly(t *testing.T) { + logger := mocks.NewMockLogger() + registry := mocks.NewMockAgentRegistry() + + capturedOptions := make(map[string]any) + + provider := mocks.NewMockAgentProvider("claude") + provider.SetConversationFunc(func(ctx context.Context, state *workflow.ConversationState, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.ConversationResult, error) { + capturedOptions = opts + + return &workflow.ConversationResult{ + State: state, + Provider: "claude", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "assistant" { + return &workflow.AgentRole{ + Name: "assistant", + Content: "You are helpful", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + resolver := &testResolverWithValues{ + values: map[string]string{}, + } + + manager := NewConversationManager(logger, resolver, registry) + manager.SetAgentRoleRepository(roleRepo) + manager.SetUserInputReader(&mocks.MockUserInputReader{}) + + step := &workflow.Step{ + Name: "assistant_chat", + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "Hello", + Role: "assistant", + }, + } + + config := &workflow.ConversationConfig{} + execCtx := workflow.NewExecutionContext("test-id", "test-workflow") + + buildContext := func(ec *workflow.ExecutionContext) *interpolation.Context { + return interpolation.NewContext() + } + + result, err := manager.ExecuteConversation( + context.Background(), + step, + config, + execCtx, + buildContext, + "", + io.Discard, + io.Discard, + ) + + require.NoError(t, err) + require.NotNil(t, result) + + systemPrompt, ok := capturedOptions["system_prompt"] + require.True(t, ok, "system_prompt should be set") + + composed, ok := systemPrompt.(string) + require.True(t, ok) + + assert.Equal(t, "You are helpful", composed, + "should contain only role content when no inline SystemPrompt") +} diff --git a/internal/application/conversation_manager_tdd_test.go b/internal/application/conversation_manager_tdd_test.go index 6d5dd941..3853857b 100644 --- a/internal/application/conversation_manager_tdd_test.go +++ b/internal/application/conversation_manager_tdd_test.go @@ -87,7 +87,7 @@ func TestConversationManager_ExecuteConversation_SingleTurn_HappyPath(t *testing execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should complete successfully with StopReasonUserExit require.NoError(t, err) @@ -147,7 +147,7 @@ func TestConversationManager_ExecuteConversation_MultiTurn_HappyPath(t *testing. execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should execute 3 turns (initial + 2 user inputs), stop with UserExit require.NoError(t, err) @@ -203,7 +203,7 @@ func TestConversationManager_ExecuteConversation_WithSystemPrompt(t *testing.T) execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: system prompt should be passed to provider require.NoError(t, err) @@ -265,7 +265,7 @@ func TestConversationManager_ExecuteConversation_ContinueFrom_HappyPath(t *testi buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should resume prior session, retain session ID and prior turns require.NoError(t, err) @@ -305,7 +305,7 @@ func TestConversationManager_ExecuteConversation_MissingUserInputReader_Error(t execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should return error about missing UserInputReader assert.Error(t, err) @@ -323,7 +323,7 @@ func TestConversationManager_ExecuteConversation_NilStep_Error(t *testing.T) { execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), nil, &workflow.ConversationConfig{}, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), nil, &workflow.ConversationConfig{}, execCtx, buildContext, "", io.Discard, io.Discard) assert.Error(t, err) assert.Nil(t, result) @@ -348,7 +348,7 @@ func TestConversationManager_ExecuteConversation_ProviderNotFound_Error(t *testi execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) assert.Error(t, err) assert.Nil(t, result) @@ -396,7 +396,7 @@ func TestConversationManager_ExecuteConversation_ContextCancellation_Error(t *te execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - _, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + _, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should return error related to context cancellation assert.Error(t, err) @@ -426,7 +426,7 @@ func TestConversationManager_ExecuteConversation_ContinueFromStepNotFound_Error( // No prior step in states buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) assert.Error(t, err) assert.Contains(t, err.Error(), "not found") @@ -482,7 +482,7 @@ func TestConversationManager_ExecuteConversation_ProviderExecutionError_Preserve execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should return error but preserve last result assert.Error(t, err) @@ -531,7 +531,7 @@ func TestConversationManager_SetUserInputReader(t *testing.T) { execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager2.ExecuteConversation(context.Background(), step, &workflow.ConversationConfig{}, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager2.ExecuteConversation(context.Background(), step, &workflow.ConversationConfig{}, execCtx, buildContext, "", io.Discard, io.Discard) // If reader was properly set, call should succeed (not fail on missing UserInputReader) assert.NoError(t, err) @@ -582,7 +582,7 @@ func TestConversationManager_ExecuteConversation_EmptyInputTerminates(t *testing execCtx := newTestExecutionContext(t) buildContext := newTestBuildContext() - result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, io.Discard, io.Discard) + result, err := manager.ExecuteConversation(context.Background(), step, config, execCtx, buildContext, "", io.Discard, io.Discard) // Assertions: should exit after first turn without another provider call require.NoError(t, err) diff --git a/internal/application/conversation_manager_test.go b/internal/application/conversation_manager_test.go index 90c0e2b1..34dd4d31 100644 --- a/internal/application/conversation_manager_test.go +++ b/internal/application/conversation_manager_test.go @@ -171,6 +171,7 @@ func TestConversationManager_ExecuteConversation_MultiTurn(t *testing.T) { config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -219,6 +220,7 @@ func TestConversationManager_ExecuteConversation_Error_NoUserInputReader(t *test config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -252,6 +254,7 @@ func TestConversationManager_ExecuteConversation_Error_NilStep(t *testing.T) { &workflow.ConversationConfig{}, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -293,6 +296,7 @@ func TestConversationManager_ExecuteConversation_Error_NilConfig(t *testing.T) { nil, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -335,6 +339,7 @@ func TestConversationManager_ExecuteConversation_Error_ProviderNotFound(t *testi config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -384,6 +389,7 @@ func TestConversationManager_ExecuteConversation_Error_UserInputReaderFails(t *t config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -430,6 +436,7 @@ func TestConversationManager_ExecuteConversation_Error_ProviderExecutionFails(t config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -479,6 +486,7 @@ func TestConversationManager_ExecuteConversation_Error_ContextCancellation(t *te config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -526,6 +534,7 @@ func TestConversationManager_ExecuteConversation_Error_ContinueFromStepNotFound( config, execCtx, buildContext, + "", io.Discard, io.Discard, ) @@ -581,6 +590,7 @@ func TestConversationManager_ExecuteConversation_Error_ContinueFromNoConversatio config, execCtx, buildContext, + "", io.Discard, io.Discard, ) diff --git a/internal/application/execution_service.go b/internal/application/execution_service.go index 33b691ed..67e7281a 100644 --- a/internal/application/execution_service.go +++ b/internal/application/execution_service.go @@ -36,6 +36,7 @@ type ConversationExecutor interface { config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + workflowDir string, stdoutW, stderrW io.Writer, ) (*workflow.ConversationResult, error) } @@ -67,6 +68,7 @@ type ExecutionService struct { tracer ports.Tracer eventPublisher ports.EventPublisher skillRepo ports.SkillRepository + agentRoleRepo ports.AgentRoleRepository } // SetOutputWriters configures streaming output writers. @@ -114,6 +116,14 @@ func (s *ExecutionService) SetSkillRepository(repo ports.SkillRepository) { s.skillRepo = repo } +func (s *ExecutionService) SetAgentRoleRepository(repo ports.AgentRoleRepository) { + s.agentRoleRepo = repo +} + +func (s *ExecutionService) buildRoleAwareSystemPrompt(ctx context.Context, step *workflow.Step, workflowDir string, intCtx *interpolation.Context) (string, error) { + return BuildRoleSystemPrompt(ctx, s.agentRoleRepo, s.resolver, step, workflowDir, intCtx) +} + // SetAWFPaths configures the AWF XDG directory paths for F063 template interpolation. // Keys: prompts_dir, config_dir, data_dir, workflows_dir, plugins_dir, scripts_dir. func (s *ExecutionService) SetAWFPaths(paths map[string]string) { @@ -2278,7 +2288,7 @@ func (s *ExecutionService) executeAgentStep( // F033: Route to conversation execution if mode is "conversation" if step.Agent.Mode == "conversation" { - return s.executeConversationStep(stepCtx, step, execCtx) + return s.executeConversationStep(stepCtx, wf, step, execCtx) } // F063: Load prompt from file if prompt_file is specified @@ -2332,6 +2342,17 @@ func (s *ExecutionService) executeAgentStep( s.logger.Debug("executing agent step", "step", step.Name, "provider", resolvedProvider) opts := cloneAndInjectOutputFormat(step.Agent.Options, step.Agent.OutputFormat) + composedPrompt, roleErr := s.buildRoleAwareSystemPrompt(stepCtx, step, wf.SourceDir, intCtx) + if roleErr != nil { + return "", fmt.Errorf("step %s: resolve role: %w", step.Name, roleErr) + } + if composedPrompt != "" { + if _, exists := opts["system_prompt"]; exists { + s.logger.Warn("options.system_prompt overridden by composed role+system_prompt", "step", step.Name) + } + opts["system_prompt"] = composedPrompt + } + // Record step state state := workflow.StepState{ Name: step.Name, @@ -2348,7 +2369,7 @@ func (s *ExecutionService) executeAgentStep( var execErr error if step.Agent.Conversation != nil { var convResult *workflow.ConversationResult - convResult, execErr = s.executeResumableAgentCall(stepCtx, step, provider, resolvedProvider, resolvedPrompt, opts, execCtx) + convResult, execErr = s.executeResumableAgentCall(stepCtx, step, provider, resolvedProvider, resolvedPrompt, opts, execCtx, wf.SourceDir) if convResult != nil { state.Output = convResult.Output state.DisplayOutput = convResult.DisplayOutput @@ -2484,19 +2505,23 @@ func (s *ExecutionService) executeResumableAgentCall( resolvedPrompt string, opts map[string]any, execCtx *workflow.ExecutionContext, + workflowDir string, ) (*workflow.ConversationResult, error) { - state, err := s.buildResumableState(step, resolvedProvider, execCtx) + // Resolve composed prompt BEFORE state init so the persisted state + // carries the full role+inline system prompt (mirrors the ConversationManager fix). + composed, roleErr := s.buildRoleAwareSystemPrompt(ctx, step, workflowDir, s.buildInterpolationContext(execCtx)) + if roleErr != nil { + return nil, fmt.Errorf("resolve role: %w", roleErr) + } + state, err := s.buildResumableState(step, resolvedProvider, execCtx, composed) if err != nil { return nil, err } // Providers consume system_prompt via the options map (same convention as // ConversationManager). Inject it on fresh sessions only; on resumed // sessions the provider retains the system prompt from the prior turn. - if step.Agent.SystemPrompt != "" && state.SessionID == "" { - if opts == nil { - opts = map[string]any{} - } - opts["system_prompt"] = step.Agent.SystemPrompt + if composed != "" && state.SessionID == "" { + opts["system_prompt"] = composed } return provider.ExecuteConversation(ctx, state, resolvedPrompt, opts, s.stdoutWriter, s.stderrWriter) } @@ -2508,10 +2533,11 @@ func (s *ExecutionService) buildResumableState( step *workflow.Step, resolvedProvider string, execCtx *workflow.ExecutionContext, + composedSystemPrompt string, ) (*workflow.ConversationState, error) { cfg := step.Agent.Conversation if cfg == nil || cfg.ContinueFrom == "" { - return workflow.NewConversationState(step.Agent.SystemPrompt), nil + return workflow.NewConversationState(composedSystemPrompt), nil } priorStepState, ok := execCtx.GetStepState(cfg.ContinueFrom) @@ -2556,6 +2582,7 @@ func (s *ExecutionService) buildResumableState( // F051: T009 - Implement delegation to ConversationManager func (s *ExecutionService) executeConversationStep( ctx context.Context, + wf *workflow.Workflow, step *workflow.Step, execCtx *workflow.ExecutionContext, ) (string, error) { @@ -2584,6 +2611,7 @@ func (s *ExecutionService) executeConversationStep( step.Agent.Conversation, execCtx, buildContext, + wf.SourceDir, s.stdoutWriter, s.stderrWriter, ) diff --git a/internal/application/execution_service_conversation_step_test.go b/internal/application/execution_service_conversation_step_test.go index 08617381..cfe8bf58 100644 --- a/internal/application/execution_service_conversation_step_test.go +++ b/internal/application/execution_service_conversation_step_test.go @@ -72,7 +72,7 @@ func TestExecuteConversationStep_T009_HappyPath_SingleTurnSuccess(t *testing.T) logger: newMockLogger(), resolver: newMockResolver(), } - nextStep, err := svc.executeConversationStep(context.Background(), step, execCtx) + nextStep, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.NoError(t, err) assert.Equal(t, "", nextStep) // Empty string means use OnSuccess transition @@ -135,7 +135,7 @@ func TestExecuteConversationStep_T009_HappyPath_MultiTurnSuccess(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - nextStep, err := svc.executeConversationStep(context.Background(), step, execCtx) + nextStep, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.NoError(t, err) assert.Equal(t, "", nextStep) @@ -197,7 +197,7 @@ func TestExecuteConversationStep_T009_HappyPath_WithInputInterpolation(t *testin logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.NoError(t, err) assert.True(t, buildContextCalled, "buildContext function should have been passed and called") @@ -233,7 +233,7 @@ func TestExecuteConversationStep_T009_EdgeCase_MinimalConfig(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - nextStep, err := svc.executeConversationStep(context.Background(), step, execCtx) + nextStep, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.NoError(t, err) assert.Equal(t, "", nextStep) @@ -282,7 +282,7 @@ func TestExecuteConversationStep_T009_EdgeCase_EmptyOutput(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - nextStep, err := svc.executeConversationStep(context.Background(), step, execCtx) + nextStep, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.NoError(t, err) assert.Equal(t, "", nextStep) @@ -310,7 +310,7 @@ func TestExecuteConversationStep_T009_EdgeCase_ContextCancellation(t *testing.T) execCtx.Status = workflow.StatusRunning // Mock manager that checks context mockConvMgr := &mockConversationManagerWithContextT009{ - executeFunc: func(ctx context.Context, step *workflow.Step, config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, stdoutW, stderrW io.Writer) (*workflow.ConversationResult, error) { + executeFunc: func(ctx context.Context, step *workflow.Step, config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, workflowDir string, stdoutW, stderrW io.Writer) (*workflow.ConversationResult, error) { if ctx.Err() != nil { return nil, ctx.Err() } @@ -326,7 +326,7 @@ func TestExecuteConversationStep_T009_EdgeCase_ContextCancellation(t *testing.T) ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := svc.executeConversationStep(ctx, step, execCtx) + _, err := svc.executeConversationStep(ctx, &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.True(t, errors.Is(err, context.Canceled)) @@ -352,7 +352,7 @@ func TestExecuteConversationStep_T009_Error_NoConversationManager(t *testing.T) logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.Contains(t, err.Error(), "conversation manager not configured") @@ -375,7 +375,7 @@ func TestExecuteConversationStep_T009_Error_NilAgentConfig(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.Contains(t, err.Error(), "agent config is nil") @@ -405,7 +405,7 @@ func TestExecuteConversationStep_T009_Error_ConversationManagerFailure(t *testin logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.Contains(t, err.Error(), "provider authentication failed") @@ -435,7 +435,7 @@ func TestExecuteConversationStep_T009_Error_ProviderNotFound(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.Contains(t, err.Error(), "provider not found") @@ -465,7 +465,7 @@ func TestExecuteConversationStep_T009_Error_InterpolationFailure(t *testing.T) { logger: newMockLogger(), resolver: newMockResolver(), } - _, err := svc.executeConversationStep(context.Background(), step, execCtx) + _, err := svc.executeConversationStep(context.Background(), &workflow.Workflow{}, step, execCtx) require.Error(t, err) assert.Contains(t, err.Error(), "interpolation failed") @@ -483,6 +483,7 @@ func (m *mockConversationManagerT009) ExecuteConversation( config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + workflowDir string, stdoutW, stderrW io.Writer, ) (*workflow.ConversationResult, error) { if m.err != nil { @@ -493,7 +494,7 @@ func (m *mockConversationManagerT009) ExecuteConversation( // mockConversationManagerWithContextT009 allows testing context cancellation. type mockConversationManagerWithContextT009 struct { - executeFunc func(ctx context.Context, step *workflow.Step, config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, stdoutW, stderrW io.Writer) (*workflow.ConversationResult, error) + executeFunc func(ctx context.Context, step *workflow.Step, config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, workflowDir string, stdoutW, stderrW io.Writer) (*workflow.ConversationResult, error) } func (m *mockConversationManagerWithContextT009) ExecuteConversation( @@ -502,9 +503,10 @@ func (m *mockConversationManagerWithContextT009) ExecuteConversation( config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + workflowDir string, stdoutW, stderrW io.Writer, ) (*workflow.ConversationResult, error) { - return m.executeFunc(ctx, step, config, execCtx, buildContext, stdoutW, stderrW) + return m.executeFunc(ctx, step, config, execCtx, buildContext, workflowDir, stdoutW, stderrW) } // mockConversationManagerWithBuildContextT009 captures buildContext calls for verification. @@ -519,6 +521,7 @@ func (m *mockConversationManagerWithBuildContextT009) ExecuteConversation( config *workflow.ConversationConfig, execCtx *workflow.ExecutionContext, buildContext ContextBuilderFunc, + workflowDir string, stdoutW, stderrW io.Writer, ) (*workflow.ConversationResult, error) { if m.onExecute != nil { diff --git a/internal/application/execution_service_role_test.go b/internal/application/execution_service_role_test.go new file mode 100644 index 00000000..a5c68094 --- /dev/null +++ b/internal/application/execution_service_role_test.go @@ -0,0 +1,504 @@ +package application_test + +import ( + "context" + "io" + "testing" + + "github.com/awf-project/cli/internal/application" + domainerrors "github.com/awf-project/cli/internal/domain/errors" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/testutil/mocks" + "github.com/awf-project/cli/pkg/interpolation" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testResolverWithValues struct { + values map[string]string +} + +func (r *testResolverWithValues) Resolve(template string, _ *interpolation.Context) (string, error) { + if v, ok := r.values[template]; ok { + return v, nil + } + return template, nil +} + +func TestExecutionService_AgentStep_SingleMode_RoleAndSystemPrompt(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + Role: "expert", + SystemPrompt: "Be concise", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetExecuteFunc(func(ctx context.Context, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.AgentResult, error) { + systemPrompt, ok := opts["system_prompt"] + require.True(t, ok, "system_prompt should be set in options") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Contains(t, composed, "expert role content", "should contain role content") + assert.Contains(t, composed, "Be concise", "should contain inline system prompt") + + return &workflow.AgentResult{ + Provider: "claude", + Output: "This is an expert analysis", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "expert" { + return &workflow.AgentRole{ + Name: "expert", + Content: "expert role content", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + execSvc.SetAgentRoleRepository(roleRepo) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_SingleMode_RoleOnly(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + Role: "assistant", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetExecuteFunc(func(ctx context.Context, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.AgentResult, error) { + systemPrompt, ok := opts["system_prompt"] + require.True(t, ok, "system_prompt should be set when role is provided") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Equal(t, "assistant role content", composed, "should only contain role content") + + return &workflow.AgentResult{ + Provider: "claude", + Output: "Hello", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "assistant" { + return &workflow.AgentRole{ + Name: "assistant", + Content: "assistant role content", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + execSvc.SetAgentRoleRepository(roleRepo) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_SingleMode_NoRole_BackwardCompat(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetExecuteFunc(func(ctx context.Context, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.AgentResult, error) { + _, ok := opts["system_prompt"] + assert.False(t, ok, "system_prompt should not be set when both role and SystemPrompt are empty") + + return &workflow.AgentResult{ + Provider: "claude", + Output: "Hello", + }, nil + }) + _ = registry.Register(provider) + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_SingleMode_SystemPromptOnly(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + SystemPrompt: "Be helpful", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetExecuteFunc(func(ctx context.Context, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.AgentResult, error) { + systemPrompt, ok := opts["system_prompt"] + require.True(t, ok, "system_prompt should be set when SystemPrompt is provided") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Equal(t, "Be helpful", composed, "should equal inline SystemPrompt when no role") + + return &workflow.AgentResult{ + Provider: "claude", + Output: "Hello", + }, nil + }) + _ = registry.Register(provider) + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_SingleMode_InterpolatedRole(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + Role: "{{inputs.persona}}", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetExecuteFunc(func(ctx context.Context, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.AgentResult, error) { + systemPrompt, ok := opts["system_prompt"] + require.True(t, ok, "system_prompt should be set") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Equal(t, "designer role content", composed) + + return &workflow.AgentResult{ + Provider: "claude", + Output: "Hello", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "designer" { + return &workflow.AgentRole{ + Name: "designer", + Content: "designer role content", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + resolver := &testResolverWithValues{ + values: map[string]string{ + "{{inputs.persona}}": "designer", + }, + } + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + resolver, + nil, + ) + execSvc.SetAgentRegistry(registry) + execSvc.SetAgentRoleRepository(roleRepo) + + ctx, err := execSvc.Run(context.Background(), "role-test", map[string]any{ + "persona": "designer", + }) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_ResumableMode_WithRole(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + Role: "expert", + SystemPrompt: "Be concise", + Conversation: &workflow.ConversationConfig{}, + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + provider := mocks.NewMockAgentProvider("claude") + provider.SetConversationFunc(func(ctx context.Context, state *workflow.ConversationState, prompt string, opts map[string]any, stdout, stderr io.Writer) (*workflow.ConversationResult, error) { + systemPrompt, ok := opts["system_prompt"] + require.True(t, ok, "system_prompt should be set in resumable mode with role") + + composed, ok := systemPrompt.(string) + require.True(t, ok, "system_prompt should be a string") + + assert.Contains(t, composed, "expert role content", "should contain role content") + assert.Contains(t, composed, "Be concise", "should contain inline system prompt") + + return &workflow.ConversationResult{ + Provider: "claude", + State: state, + Output: "Expert analysis", + }, nil + }) + _ = registry.Register(provider) + + roleRepo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name == "expert" { + return &workflow.AgentRole{ + Name: "expert", + Content: "expert role content", + }, nil + } + return nil, domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + "role not found: "+name, + map[string]any{"role": name}, + nil, + ) + }, + } + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + execSvc.SetAgentRoleRepository(roleRepo) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.NoError(t, err) + assert.Equal(t, workflow.StatusCompleted, ctx.Status) +} + +func TestExecutionService_AgentStep_SingleMode_MissingRepository(t *testing.T) { + repo := newMockRepository() + repo.workflows["role-test"] = &workflow.Workflow{ + Name: "role-test", + Initial: "ask", + Steps: map[string]*workflow.Step{ + "ask": { + Name: "ask", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Prompt: "What is this?", + Role: "expert", + }, + OnSuccess: "done", + }, + "done": { + Name: "done", + Type: workflow.StepTypeTerminal, + }, + }, + } + + registry := mocks.NewMockAgentRegistry() + _ = registry.Register(mocks.NewMockAgentProvider("claude")) + + wfSvc := application.NewWorkflowService(repo, newMockStateStore(), newMockExecutor(), &mockLogger{}, nil) + execSvc := application.NewExecutionService( + wfSvc, + newMockExecutor(), + newMockParallelExecutor(), + newMockStateStore(), + &mockLogger{}, + newMockResolver(), + nil, + ) + execSvc.SetAgentRegistry(registry) + + ctx, err := execSvc.Run(context.Background(), "role-test", nil) + + require.Error(t, err) + assert.Equal(t, workflow.StatusFailed, ctx.Status) +} diff --git a/internal/application/execution_setup.go b/internal/application/execution_setup.go index 35b88b62..33bcc08e 100644 --- a/internal/application/execution_setup.go +++ b/internal/application/execution_setup.go @@ -102,6 +102,7 @@ type setupConfig struct { templatePaths []string pluginService *PluginService eventPublisher ports.EventPublisher + agentRoleRepo ports.AgentRoleRepository } // WithNotifyConfig configures notification backend defaults. @@ -171,6 +172,11 @@ func WithEventPublisher(p ports.EventPublisher) SetupOption { return func(c *setupConfig) { c.eventPublisher = p } } +// WithAgentRoleRepository injects an agent role repository for F098 role resolution. +func WithAgentRoleRepository(repo ports.AgentRoleRepository) SetupOption { + return func(c *setupConfig) { c.agentRoleRepo = repo } +} + // ExecutionSetup centralizes ExecutionService wiring. // It is the single authoritative place where all Set*() calls on ExecutionService // are performed, so both CLI runWorkflow and TUI buildBridge share an identical @@ -260,6 +266,9 @@ func (s *ExecutionSetup) Build(_ context.Context) (*SetupResult, error) { if cfg.userInputReader != nil { convMgr.SetUserInputReader(cfg.userInputReader) } + if cfg.agentRoleRepo != nil { + convMgr.SetAgentRoleRepository(cfg.agentRoleRepo) + } execSvc.SetConversationManager(convMgr) } @@ -281,6 +290,10 @@ func (s *ExecutionSetup) Build(_ context.Context) (*SetupResult, error) { execSvc.SetSkillRepository(infraskills.NewFilesystemSkillRepository(s.logger)) + if cfg.agentRoleRepo != nil { + execSvc.SetAgentRoleRepository(cfg.agentRoleRepo) + } + compositeProvider := s.buildProviders(cfg) execSvc.SetOperationProvider(compositeProvider) diff --git a/internal/application/role_loader.go b/internal/application/role_loader.go new file mode 100644 index 00000000..2f607568 --- /dev/null +++ b/internal/application/role_loader.go @@ -0,0 +1,125 @@ +package application + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + + domainerrors "github.com/awf-project/cli/internal/domain/errors" + "github.com/awf-project/cli/internal/domain/ports" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/pkg/interpolation" +) + +// ResolveAgentRole resolves a role reference to an AgentRole. +// An empty ref is a no-op and returns (nil, nil). +// References containing path separators or starting with '.', '~', '/' are treated as paths; +// everything else is resolved by name through the repository. +func ResolveAgentRole(ctx context.Context, repo ports.AgentRoleRepository, ref, workflowDir string) (*workflow.AgentRole, error) { + if ref == "" { + return nil, nil + } + if err := ctx.Err(); err != nil { + return nil, err + } + if !isRolePathRef(ref) { + role, err := repo.Load(ctx, ref) + if err != nil { + return nil, fmt.Errorf("resolve role %q: %w", ref, err) + } + return role, nil + } + absPath, err := expandRolePath(ref, workflowDir) + if err != nil { + return nil, fmt.Errorf("resolve role %q: %w", ref, err) + } + role, err := repo.LoadFromPath(ctx, absPath) + if err != nil { + return nil, fmt.Errorf("resolve role %q: %w", ref, err) + } + return role, nil +} + +// isRolePathRef reports whether ref should be treated as a filesystem path. +// Refs containing path separators or starting with '.', '~', '/' are paths; +// everything else is a symbolic name for repo.Load. +func isRolePathRef(ref string) bool { + if ref == "" { + return false + } + return strings.ContainsAny(ref, `/\`) || ref[0] == '.' || ref[0] == '~' +} + +// expandRolePath converts a path ref to an absolute path. +func expandRolePath(ref, workflowDir string) (string, error) { + if strings.HasPrefix(ref, "~/") { + homeDir, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(homeDir, ref[2:]), nil + } + if ref == "~" { + return os.UserHomeDir() + } + if filepath.IsAbs(ref) { + return filepath.Clean(ref), nil + } + return filepath.Join(workflowDir, ref), nil +} + +// BuildRoleSystemPrompt resolves the agent role template, loads role content +// from the repository, and composes the final system prompt. +func BuildRoleSystemPrompt( + ctx context.Context, + repo ports.AgentRoleRepository, + resolver interpolation.Resolver, + step *workflow.Step, + workflowDir string, + intCtx *interpolation.Context, +) (string, error) { + if step.Agent == nil { + return "", nil + } + roleRef := step.Agent.Role + if roleRef != "" { + resolved, err := resolver.Resolve(roleRef, intCtx) + if err != nil { + return "", fmt.Errorf("resolve role template: %w", err) + } + roleRef = resolved + } + if roleRef == "" { + return ComposeSystemPrompt("", step.Agent.SystemPrompt), nil + } + if repo == nil { + return "", domainerrors.NewUserError( + domainerrors.ErrorCodeUserInputMissingRole, + fmt.Sprintf("agent role %q requires a configured role repository; set one via SetAgentRoleRepository", roleRef), + map[string]any{"role": roleRef}, + nil, + ) + } + role, err := ResolveAgentRole(ctx, repo, roleRef, workflowDir) + if err != nil { + return "", err + } + if role == nil { + return ComposeSystemPrompt("", step.Agent.SystemPrompt), nil + } + return ComposeSystemPrompt(role.Content, step.Agent.SystemPrompt), nil +} + +// ComposeSystemPrompt concatenates role content and an inline system_prompt. +// Composition order: role + "\n\n" + inline. Empty parts are omitted. +func ComposeSystemPrompt(roleContent, inline string) string { + if roleContent == "" { + return inline + } + if inline == "" { + return roleContent + } + return roleContent + "\n\n" + inline +} diff --git a/internal/application/role_loader_test.go b/internal/application/role_loader_test.go new file mode 100644 index 00000000..e57360cb --- /dev/null +++ b/internal/application/role_loader_test.go @@ -0,0 +1,230 @@ +package application_test + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/awf-project/cli/internal/application" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/testutil/mocks" +) + +func TestResolveAgentRole_EmptyRef(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{} + ctx := context.Background() + + role, err := application.ResolveAgentRole(ctx, repo, "", "/some/dir") + + assert.NoError(t, err) + assert.Nil(t, role) +} + +func TestResolveAgentRole_NameResolution(t *testing.T) { + expectedRole := &workflow.AgentRole{ + Name: "go-senior", + Content: "expert in golang", + } + + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + if name != "go-senior" { + t.Errorf("LoadFunc called with wrong name: %q", name) + } + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, "go-senior", "/some/dir") + + require.NoError(t, err) + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_AbsolutePath(t *testing.T) { + expectedRole := &workflow.AgentRole{ + Name: "custom", + Content: "custom content", + } + expectedPath := "/abs/path/to/role.yaml" + + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + if absolutePath != expectedPath { + t.Errorf("LoadFromPathFunc called with wrong path: %q", absolutePath) + } + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, "/abs/path/to/role.yaml", "/some/dir") + + require.NoError(t, err) + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_RelativePath(t *testing.T) { + expectedRole := &workflow.AgentRole{ + Name: "local", + Content: "local role", + } + + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + expectedAbs := filepath.Join("/workflow/dir", "local") + if absolutePath != expectedAbs { + t.Errorf("LoadFromPathFunc called with wrong path: %q, expected %q", absolutePath, expectedAbs) + } + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, "./local", "/workflow/dir") + + require.NoError(t, err) + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_TildePath(t *testing.T) { + homeDir, err := os.UserHomeDir() + require.NoError(t, err) + + expectedRole := &workflow.AgentRole{ + Name: "home-role", + Content: "role in home", + } + expectedPath := filepath.Join(homeDir, "roles", "expert.yaml") + + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + if absolutePath != expectedPath { + t.Errorf("LoadFromPathFunc called with wrong path: %q, expected %q", absolutePath, expectedPath) + } + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, "~/roles/expert.yaml", "/workflow/dir") + + require.NoError(t, err) + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_PathWithBackslash(t *testing.T) { + expectedRole := &workflow.AgentRole{ + Name: "win-path", + Content: "windows path", + } + + pathCalled := false + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + pathCalled = true + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, `some\path\file`, "/workflow/dir") + + require.NoError(t, err) + assert.True(t, pathCalled, "should have called LoadFromPath for backslash path") + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_DotStartPath(t *testing.T) { + expectedRole := &workflow.AgentRole{ + Name: "dot-path", + Content: "path starting with dot", + } + + pathCalled := false + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + pathCalled = true + return expectedRole, nil + }, + } + + ctx := context.Background() + role, err := application.ResolveAgentRole(ctx, repo, ".hidden", "/workflow/dir") + + require.NoError(t, err) + assert.True(t, pathCalled, "should have called LoadFromPath for dot-start path") + assert.Equal(t, expectedRole, role) +} + +func TestResolveAgentRole_ErrorWrapping(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(ctx context.Context, name string) (*workflow.AgentRole, error) { + return nil, context.DeadlineExceeded + }, + } + + ctx := context.Background() + _, err := application.ResolveAgentRole(ctx, repo, "missing-role", "/some/dir") + + require.Error(t, err) + assert.Contains(t, err.Error(), "missing-role") +} + +func TestResolveAgentRole_PathErrorWrapping(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFromPathFunc: func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + return nil, os.ErrNotExist + }, + } + + ctx := context.Background() + _, err := application.ResolveAgentRole(ctx, repo, "/path/to/role", "/some/dir") + + require.Error(t, err) + // Error should mention the original unresolved ref + assert.Contains(t, err.Error(), "/path/to/role") +} + +func TestComposeSystemPrompt_BothNonEmpty(t *testing.T) { + result := application.ComposeSystemPrompt("You are an expert", "Be concise") + assert.Equal(t, "You are an expert\n\nBe concise", result) +} + +func TestComposeSystemPrompt_RoleOnly(t *testing.T) { + result := application.ComposeSystemPrompt("You are an expert", "") + assert.Equal(t, "You are an expert", result) +} + +func TestComposeSystemPrompt_InlineOnly(t *testing.T) { + result := application.ComposeSystemPrompt("", "Be concise") + assert.Equal(t, "Be concise", result) +} + +func TestComposeSystemPrompt_BothEmpty(t *testing.T) { + result := application.ComposeSystemPrompt("", "") + assert.Equal(t, "", result) +} + +func TestComposeSystemPrompt_PreservesWhitespace(t *testing.T) { + role := "Line 1\nLine 2" + inline := "Inline 1\nInline 2" + result := application.ComposeSystemPrompt(role, inline) + assert.Equal(t, "Line 1\nLine 2\n\nInline 1\nInline 2", result) +} + +func TestResolveAgentRole_ContextError(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := application.ResolveAgentRole(ctx, repo, "test", "/dir") + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) +} diff --git a/internal/domain/errors/codes.go b/internal/domain/errors/codes.go index 7ec54ae6..f213d899 100644 --- a/internal/domain/errors/codes.go +++ b/internal/domain/errors/codes.go @@ -38,6 +38,9 @@ const ( // ErrorCodeUserInputMissingSkill indicates a required skill was not found. ErrorCodeUserInputMissingSkill ErrorCode = "USER.INPUT.MISSING_SKILL" + + // ErrorCodeUserInputMissingRole indicates a required agent role was not found. + ErrorCodeUserInputMissingRole ErrorCode = "USER.INPUT.MISSING_ROLE" ) // Error code constants for WORKFLOW category (exit code 2). diff --git a/internal/domain/errors/codes_role_test.go b/internal/domain/errors/codes_role_test.go new file mode 100644 index 00000000..f68c59e0 --- /dev/null +++ b/internal/domain/errors/codes_role_test.go @@ -0,0 +1,36 @@ +package errors + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestErrorCodeUserInputMissingRole_Exists(t *testing.T) { + assert.Equal(t, ErrorCode("USER.INPUT.MISSING_ROLE"), ErrorCodeUserInputMissingRole) +} + +func TestErrorCodeUserInputMissingRole_IsValid(t *testing.T) { + assert.True(t, ErrorCodeUserInputMissingRole.IsValid()) +} + +func TestErrorCodeUserInputMissingRole_Category(t *testing.T) { + assert.Equal(t, "USER", ErrorCodeUserInputMissingRole.Category()) +} + +func TestErrorCodeUserInputMissingRole_Subcategory(t *testing.T) { + assert.Equal(t, "INPUT", ErrorCodeUserInputMissingRole.Subcategory()) +} + +func TestErrorCodeUserInputMissingRole_Specific(t *testing.T) { + assert.Equal(t, "MISSING_ROLE", ErrorCodeUserInputMissingRole.Specific()) +} + +func TestErrorCodeUserInputMissingRole_ExitCode(t *testing.T) { + assert.Equal(t, 1, ErrorCodeUserInputMissingRole.ExitCode()) +} + +func TestErrorCodeUserInputMissingRole_InUserInputCategory(t *testing.T) { + assert.Equal(t, "USER", ErrorCodeUserInputMissingRole.Category()) + assert.Equal(t, "INPUT", ErrorCodeUserInputMissingRole.Subcategory()) +} diff --git a/internal/domain/ports/agent_role_repository.go b/internal/domain/ports/agent_role_repository.go new file mode 100644 index 00000000..c92fde81 --- /dev/null +++ b/internal/domain/ports/agent_role_repository.go @@ -0,0 +1,12 @@ +package ports + +import ( + "context" + + "github.com/awf-project/cli/internal/domain/workflow" +) + +type AgentRoleRepository interface { + Load(ctx context.Context, name string) (*workflow.AgentRole, error) + LoadFromPath(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) +} diff --git a/internal/domain/workflow/agent_config.go b/internal/domain/workflow/agent_config.go index 9c228dd3..ed7550b1 100644 --- a/internal/domain/workflow/agent_config.go +++ b/internal/domain/workflow/agent_config.go @@ -26,15 +26,16 @@ var validOutputFormats = map[OutputFormat]bool{ // AgentConfig holds configuration for invoking an AI agent. type AgentConfig struct { - Provider string `yaml:"provider"` // agent provider: claude, codex, gemini, opencode, openai_compatible - Prompt string `yaml:"prompt"` // prompt template with {{inputs.*}} and {{states.*}} (single mode) or first user message (conversation mode) - PromptFile string `yaml:"prompt_file"` // path to external prompt template file (mutually exclusive with Prompt) - Options map[string]any `yaml:"options"` // provider-specific options (model, temperature, max_tokens, etc.) - Timeout int `yaml:"timeout"` // seconds, 0 = use DefaultAgentTimeout - Mode string `yaml:"mode"` // execution mode: "single" (default) or "conversation" - SystemPrompt string `yaml:"system_prompt"` // system prompt preserved across conversation (conversation mode only) - Conversation *ConversationConfig `yaml:"conversation"` // conversation-specific configuration (conversation mode only) - OutputFormat OutputFormat `yaml:"output_format"` // output post-processing: json (strip fences + validate), text (strip fences only), or empty (no processing) + Provider string `yaml:"provider"` // agent provider: claude, codex, gemini, opencode, openai_compatible + Prompt string `yaml:"prompt"` // prompt template with {{inputs.*}} and {{states.*}} (single mode) or first user message (conversation mode) + PromptFile string `yaml:"prompt_file"` // path to external prompt template file (mutually exclusive with Prompt) + Options map[string]any `yaml:"options"` // provider-specific options (model, temperature, max_tokens, etc.) + Timeout int `yaml:"timeout"` // seconds, 0 = use DefaultAgentTimeout + Mode string `yaml:"mode"` // execution mode: "single" (default) or "conversation" + SystemPrompt string `yaml:"system_prompt"` // system prompt preserved across conversation (conversation mode only) + Role string `yaml:"role,omitempty"` // role name or path resolved at execution time; may contain {{...}} templates + Conversation *ConversationConfig `yaml:"conversation"` // conversation-specific configuration (conversation mode only) + OutputFormat OutputFormat `yaml:"output_format"` // output post-processing: json (strip fences + validate), text (strip fences only), or empty (no processing) } // Validate checks if the agent configuration is valid. @@ -98,9 +99,22 @@ func (c *AgentConfig) Validate(validator ExpressionCompiler) error { return errors.New("timeout must be non-negative") } + // Reject path traversal in Role; existence is verified at execution time + if c.Role != "" && containsPathTraversal(c.Role) { + return ValidationError{ + Level: ValidationLevelError, + Code: ErrRoleNotFound, + Message: "role path contains path-traversal pattern (..): " + c.Role, + } + } + return nil } +func containsPathTraversal(s string) bool { + return strings.Contains(s, "..") +} + // GetTimeout returns the effective timeout as a time.Duration. // Returns DefaultAgentTimeout seconds if not explicitly set. func (c *AgentConfig) GetTimeout() time.Duration { @@ -115,11 +129,6 @@ func (c *AgentConfig) IsConversationMode() bool { return c.Mode == "conversation" } -// GetEffectivePrompt returns the prompt for this agent step. -func (c *AgentConfig) GetEffectivePrompt() string { - return c.Prompt -} - // AgentResult holds the result of an agent execution. type AgentResult struct { Provider string // provider name used diff --git a/internal/domain/workflow/agent_config_conversation_test.go b/internal/domain/workflow/agent_config_conversation_test.go index 2e5bbf9c..45962b52 100644 --- a/internal/domain/workflow/agent_config_conversation_test.go +++ b/internal/domain/workflow/agent_config_conversation_test.go @@ -283,79 +283,6 @@ func TestAgentConfig_IsConversationMode_AfterValidation(t *testing.T) { } } -// workflow.AgentConfig GetEffectivePrompt Tests - -func TestAgentConfig_GetEffectivePrompt(t *testing.T) { - tests := []struct { - name string - mode string - prompt string - expectedPrompt string - }{ - { - name: "single mode uses prompt", - mode: "single", - prompt: "Main prompt", - expectedPrompt: "Main prompt", - }, - { - name: "conversation mode uses prompt", - mode: "conversation", - prompt: "Conversation prompt", - expectedPrompt: "Conversation prompt", - }, - { - name: "empty mode returns prompt", - mode: "", - prompt: "Main prompt", - expectedPrompt: "Main prompt", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - config := workflow.AgentConfig{ - Provider: "claude", - Mode: tt.mode, - Prompt: tt.prompt, - } - _ = config.Validate(nil) // Normalize mode - assert.Equal(t, tt.expectedPrompt, config.GetEffectivePrompt()) - }) - } -} - -func TestAgentConfig_GetEffectivePrompt_EdgeCases(t *testing.T) { - tests := []struct { - name string - config workflow.AgentConfig - expectedPrompt string - }{ - { - name: "empty prompt returns empty", - config: workflow.AgentConfig{ - Mode: "conversation", - Prompt: "", - }, - expectedPrompt: "", - }, - { - name: "multiline prompt", - config: workflow.AgentConfig{ - Mode: "conversation", - Prompt: "Line 1\nLine 2", - }, - expectedPrompt: "Line 1\nLine 2", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expectedPrompt, tt.config.GetEffectivePrompt()) - }) - } -} - func TestAgentConfig_ConversationMode_Complete(t *testing.T) { config := workflow.AgentConfig{ Provider: "claude", @@ -381,7 +308,7 @@ Say "APPROVED" when done.`, assert.Equal(t, "claude", config.Provider) assert.True(t, config.IsConversationMode()) assert.Contains(t, config.SystemPrompt, "code reviewer") - assert.Contains(t, config.GetEffectivePrompt(), "Review this code") + assert.Contains(t, config.Prompt, "Review this code") require.NotNil(t, config.Conversation) } @@ -395,7 +322,7 @@ func TestAgentConfig_ConversationMode_MinimalConfig(t *testing.T) { err := config.Validate(nil) require.NoError(t, err) assert.True(t, config.IsConversationMode()) - assert.Equal(t, "Hello", config.GetEffectivePrompt()) + assert.Equal(t, "Hello", config.Prompt) assert.Nil(t, config.Conversation) } @@ -413,7 +340,7 @@ func TestAgentConfig_SingleMode_BackwardCompatibility(t *testing.T) { require.NoError(t, err) assert.False(t, config.IsConversationMode()) assert.Equal(t, "single", config.Mode) // Normalized to "single" - assert.Equal(t, "Analyze this code", config.GetEffectivePrompt()) + assert.Equal(t, "Analyze this code", config.Prompt) } func TestAgentConfig_ConversationMode_Errors(t *testing.T) { diff --git a/internal/domain/workflow/agent_config_prompt_file_test.go b/internal/domain/workflow/agent_config_prompt_file_test.go index 299b5f57..46528b32 100644 --- a/internal/domain/workflow/agent_config_prompt_file_test.go +++ b/internal/domain/workflow/agent_config_prompt_file_test.go @@ -269,15 +269,6 @@ func TestAgentConfig_PromptFile_MutualExclusivity_NonEmptyPrompt(t *testing.T) { } } -func TestAgentConfig_PromptFile_GetEffectivePrompt_DoesNotReturnFile(t *testing.T) { - config := AgentConfig{ - Provider: "claude", - PromptFile: "prompts/analyze.md", - } - effectivePrompt := config.GetEffectivePrompt() - assert.Empty(t, effectivePrompt) -} - func TestAgentConfig_PromptFile_ConversationMode_Combinations(t *testing.T) { tests := []struct { name string diff --git a/internal/domain/workflow/agent_config_role_test.go b/internal/domain/workflow/agent_config_role_test.go new file mode 100644 index 00000000..862885d4 --- /dev/null +++ b/internal/domain/workflow/agent_config_role_test.go @@ -0,0 +1,117 @@ +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAgentConfig_Role_EmptyAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "", config.Role) +} + +func TestAgentConfig_Role_TraversalRejected(t *testing.T) { + tests := []struct { + name string + role string + }{ + {"double-dot prefix", "../escape"}, + {"double-dot in middle", "foo/../bar"}, + {"double-dot only", ".."}, + {"double-dot suffix", "roles/.."}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: tt.role, + } + + err := config.Validate(nil) + + require.Error(t, err) + valErr, ok := err.(ValidationError) + require.True(t, ok, "expected ValidationError, got %T", err) + assert.Equal(t, ValidationLevelError, valErr.Level) + assert.Equal(t, ErrRoleNotFound, valErr.Code) + assert.Contains(t, valErr.Message, "..") + }) + } +} + +func TestAgentConfig_Role_PlainNameAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "go-senior", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "go-senior", config.Role) +} + +func TestAgentConfig_Role_RelativePathAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "./roles/backend", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "./roles/backend", config.Role) +} + +func TestAgentConfig_Role_AbsolutePathAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "/abs/path/to/role", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "/abs/path/to/role", config.Role) +} + +func TestAgentConfig_Role_TildePathAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "~/agents/foo", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "~/agents/foo", config.Role) +} + +func TestAgentConfig_Role_TemplateExpressionAccepted(t *testing.T) { + config := AgentConfig{ + Provider: "claude", + Prompt: "hello", + Role: "{{inputs.persona}}", + } + + err := config.Validate(nil) + + require.NoError(t, err) + assert.Equal(t, "{{inputs.persona}}", config.Role) +} diff --git a/internal/domain/workflow/agent_role.go b/internal/domain/workflow/agent_role.go new file mode 100644 index 00000000..bd5c5599 --- /dev/null +++ b/internal/domain/workflow/agent_role.go @@ -0,0 +1,28 @@ +package workflow + +import ( + "fmt" + "strings" +) + +// AgentRole holds the loaded content and metadata for an agent role. +type AgentRole struct { + Name string + SourcePath string + Content string +} + +// AgentRoleNotFoundError is returned when an agent role cannot be resolved by name or path. +type AgentRoleNotFoundError struct { + Name string + SearchPaths []string + Underlying error +} + +func (e *AgentRoleNotFoundError) Error() string { + return fmt.Sprintf("agent role %q not found in search paths: %s", e.Name, strings.Join(e.SearchPaths, ", ")) +} + +func (e *AgentRoleNotFoundError) Unwrap() error { + return e.Underlying +} diff --git a/internal/domain/workflow/agent_role_test.go b/internal/domain/workflow/agent_role_test.go new file mode 100644 index 00000000..58bca7fc --- /dev/null +++ b/internal/domain/workflow/agent_role_test.go @@ -0,0 +1,108 @@ +package workflow + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAgentRole_ZeroValue(t *testing.T) { + var role AgentRole + assert.Equal(t, "", role.Name) + assert.Equal(t, "", role.SourcePath) + assert.Equal(t, "", role.Content) +} + +func TestAgentRole_Fields(t *testing.T) { + role := AgentRole{ + Name: "backend-engineer", + SourcePath: "/project/.agents/backend-engineer.md", + Content: "# Backend Engineer\n\nYou are a backend engineer.", + } + + assert.Equal(t, "backend-engineer", role.Name) + assert.Equal(t, "/project/.agents/backend-engineer.md", role.SourcePath) + assert.Equal(t, "# Backend Engineer\n\nYou are a backend engineer.", role.Content) +} + +func TestAgentRoleNotFoundError_ErrorMessage(t *testing.T) { + tests := []struct { + name string + err *AgentRoleNotFoundError + wantContain []string + }{ + { + name: "single search path", + err: &AgentRoleNotFoundError{ + Name: "frontend", + SearchPaths: []string{"/project/.agents"}, + }, + wantContain: []string{"frontend", "/project/.agents"}, + }, + { + name: "multiple search paths", + err: &AgentRoleNotFoundError{ + Name: "devops", + SearchPaths: []string{"/project/.agents", "/home/user/.awf/agents", "/usr/share/awf/agents"}, + }, + wantContain: []string{"devops", "/project/.agents", "/home/user/.awf/agents", "/usr/share/awf/agents"}, + }, + { + name: "empty search paths", + err: &AgentRoleNotFoundError{ + Name: "missing-role", + SearchPaths: []string{}, + }, + wantContain: []string{"missing-role"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + msg := tt.err.Error() + for _, fragment := range tt.wantContain { + assert.Contains(t, msg, fragment) + } + }) + } +} + +func TestAgentRoleNotFoundError_Unwrap(t *testing.T) { + underlying := errors.New("permission denied") + err := &AgentRoleNotFoundError{ + Name: "ops", + SearchPaths: []string{"/agents"}, + Underlying: underlying, + } + + require.Equal(t, underlying, err.Unwrap()) + require.True(t, errors.Is(err, underlying)) +} + +func TestAgentRoleNotFoundError_UnwrapNil(t *testing.T) { + err := &AgentRoleNotFoundError{ + Name: "ops", + SearchPaths: []string{"/agents"}, + } + + assert.Nil(t, err.Unwrap()) +} + +func TestValidationCode_RoleConstants(t *testing.T) { + constants := []ValidationCode{ + ErrRoleNotFound, + ErrRoleMissingAgentsMD, + } + + for _, c := range constants { + assert.NotEmpty(t, string(c), "constant must have a non-empty string value") + } + + seen := make(map[ValidationCode]bool) + for _, c := range constants { + assert.False(t, seen[c], "constant %q is duplicated", c) + seen[c] = true + } +} diff --git a/internal/domain/workflow/validation_errors.go b/internal/domain/workflow/validation_errors.go index 595cb42e..836094c5 100644 --- a/internal/domain/workflow/validation_errors.go +++ b/internal/domain/workflow/validation_errors.go @@ -44,6 +44,11 @@ const ( ErrSkillNotFound ValidationCode = "skill_not_found" ErrSkillMissingSkillMD ValidationCode = "skill_missing_skillmd" ErrSkillEmptyContent ValidationCode = "skill_empty_content" + + // Agent role validation codes + ErrRoleNotFound ValidationCode = "role_not_found" + ErrRoleMissingAgentsMD ValidationCode = "role_missing_agents_md" + ErrRoleEmptyContent ValidationCode = "role_empty_content" ) // ValidationError represents a single validation issue. diff --git a/internal/infrastructure/repository/yaml_mapper.go b/internal/infrastructure/repository/yaml_mapper.go index 482038b7..c0ba6878 100644 --- a/internal/infrastructure/repository/yaml_mapper.go +++ b/internal/infrastructure/repository/yaml_mapper.go @@ -482,6 +482,7 @@ func mapAgentConfigFlat(y *yamlStep) *workflow.AgentConfig { Options: y.Options, Mode: y.Mode, SystemPrompt: y.SystemPrompt, + Role: y.Role, Conversation: mapConversationConfig(y.Conversation), OutputFormat: workflow.OutputFormat(y.OutputFormat), // Timeout is handled separately via step.Timeout diff --git a/internal/infrastructure/repository/yaml_mapper_role_test.go b/internal/infrastructure/repository/yaml_mapper_role_test.go new file mode 100644 index 00000000..1999e9c3 --- /dev/null +++ b/internal/infrastructure/repository/yaml_mapper_role_test.go @@ -0,0 +1,103 @@ +package repository + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func TestMapAgentConfigFlat_RolePresent(t *testing.T) { + y := &yamlStep{ + Type: "agent", + Provider: "claude", + Prompt: "hello", + Role: "go-senior", + } + + got := mapAgentConfigFlat(y) + + require.NotNil(t, got) + assert.Equal(t, "go-senior", got.Role) +} + +func TestMapAgentConfigFlat_RoleAbsent(t *testing.T) { + y := &yamlStep{ + Type: "agent", + Provider: "claude", + Prompt: "hello", + } + + got := mapAgentConfigFlat(y) + + require.NotNil(t, got) + assert.Equal(t, "", got.Role) +} + +func TestMapAgentConfigFlat_RolePath(t *testing.T) { + y := &yamlStep{ + Type: "agent", + Provider: "claude", + Prompt: "hello", + Role: "./roles/backend", + } + + got := mapAgentConfigFlat(y) + + require.NotNil(t, got) + assert.Equal(t, "./roles/backend", got.Role) +} + +func TestMapAgentConfigFlat_RoleTemplateExpression(t *testing.T) { + y := &yamlStep{ + Type: "agent", + Provider: "claude", + Prompt: "hello", + Role: "{{inputs.persona}}", + } + + got := mapAgentConfigFlat(y) + + require.NotNil(t, got) + assert.Equal(t, "{{inputs.persona}}", got.Role) +} + +func TestMapStep_YAMLRoundtrip_RolePresent(t *testing.T) { + yamlBytes := []byte(` +type: agent +provider: claude +prompt: "hello" +role: go-senior +`) + + var y yamlStep + err := yaml.Unmarshal(yamlBytes, &y) + require.NoError(t, err) + assert.Equal(t, "go-senior", y.Role) + + got, err := mapStep("test.yaml", "agent_step", &y) + + require.NoError(t, err) + require.NotNil(t, got.Agent) + assert.Equal(t, "go-senior", got.Agent.Role) +} + +func TestMapStep_YAMLRoundtrip_RoleAbsent(t *testing.T) { + yamlBytes := []byte(` +type: agent +provider: claude +prompt: "hello" +`) + + var y yamlStep + err := yaml.Unmarshal(yamlBytes, &y) + require.NoError(t, err) + assert.Equal(t, "", y.Role) + + got, err := mapStep("test.yaml", "agent_step", &y) + + require.NoError(t, err) + require.NotNil(t, got.Agent) + assert.Equal(t, "", got.Agent.Role) +} diff --git a/internal/infrastructure/repository/yaml_types.go b/internal/infrastructure/repository/yaml_types.go index 0a44dea8..d3dccb1f 100644 --- a/internal/infrastructure/repository/yaml_types.go +++ b/internal/infrastructure/repository/yaml_types.go @@ -77,9 +77,10 @@ type yamlStep struct { Config map[string]any `yaml:"config"` // plugin-provided step type parameters // Agent conversation mode (F033) - extends agent configuration - Mode string `yaml:"mode"` // execution mode: "single" (default) or "conversation" - SystemPrompt string `yaml:"system_prompt"` // system prompt for conversation mode - Conversation *yamlConversationConfig `yaml:"conversation"` // conversation-specific configuration + Mode string `yaml:"mode"` // execution mode: "single" (default) or "conversation" + SystemPrompt string `yaml:"system_prompt"` // system prompt for conversation mode + Role string `yaml:"role,omitempty"` // agent role name or path (F098) + Conversation *yamlConversationConfig `yaml:"conversation"` // conversation-specific configuration // Skill references (F096) - polymorphic: string (name) or map{"path": "..."} (path-based) Skills []any `yaml:"skills"` diff --git a/internal/infrastructure/roles/filesystem_repository.go b/internal/infrastructure/roles/filesystem_repository.go new file mode 100644 index 00000000..b62b7304 --- /dev/null +++ b/internal/infrastructure/roles/filesystem_repository.go @@ -0,0 +1,116 @@ +package roles + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/awf-project/cli/internal/domain/ports" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/infrastructure/skills" + "github.com/awf-project/cli/internal/infrastructure/xdg" +) + +type FilesystemAgentRoleRepository struct { + searchPaths []string + logger ports.Logger +} + +func NewFilesystemAgentRoleRepository(logger ports.Logger) *FilesystemAgentRoleRepository { + var paths []string + + if envPath := os.Getenv("AWF_AGENTS_PATH"); envPath != "" { + paths = filepath.SplitList(envPath) + } else { + candidates := []string{ + xdg.LocalAgentsDir(), + ".agents", + xdg.AWFAgentsDir(), + crossClientGlobalAgentsDir(), + } + for _, p := range candidates { + if p != "" { + paths = append(paths, p) + } + } + } + + return &FilesystemAgentRoleRepository{ + searchPaths: paths, + logger: logger, + } +} + +func (r *FilesystemAgentRoleRepository) Load(_ context.Context, name string) (*workflow.AgentRole, error) { + if strings.ContainsAny(name, `/\`) || strings.Contains(name, "..") { + return nil, fmt.Errorf("invalid agent role name %q: must not contain path separators or '..'", name) + } + + for _, searchPath := range r.searchPaths { + dirPath := filepath.Join(searchPath, name) + agentsFile := filepath.Join(dirPath, "AGENTS.md") + + if _, err := os.Stat(agentsFile); err != nil { + continue + } + + return r.loadAgentsMD(dirPath) + } + + return nil, &workflow.AgentRoleNotFoundError{Name: name, SearchPaths: r.searchPaths} +} + +func (r *FilesystemAgentRoleRepository) LoadFromPath(_ context.Context, absolutePath string) (*workflow.AgentRole, error) { + cleanPath := filepath.Clean(absolutePath) + + var agentsFile, dirPath string + if strings.HasSuffix(cleanPath, "AGENTS.md") { + agentsFile = cleanPath + dirPath = filepath.Dir(cleanPath) + } else { + agentsFile = filepath.Join(cleanPath, "AGENTS.md") + dirPath = cleanPath + } + + if _, err := os.Stat(agentsFile); err != nil { + return nil, &workflow.AgentRoleNotFoundError{Name: filepath.Base(cleanPath), SearchPaths: []string{cleanPath}} + } + + return r.loadAgentsMD(dirPath) +} + +func (r *FilesystemAgentRoleRepository) loadAgentsMD(dirPath string) (*workflow.AgentRole, error) { + agentsFile := filepath.Join(dirPath, "AGENTS.md") + + data, err := os.ReadFile(agentsFile) + if err != nil { + return nil, fmt.Errorf("reading AGENTS.md in %s: %w", dirPath, err) + } + + if r.logger != nil && len(data) > 500*1024 { + r.logger.Warn("AGENTS.md exceeds 500KB, may impact context window", "path", agentsFile, "size", len(data)) + } + + content := skills.StripFrontmatter(string(data)) + + absPath, err := filepath.Abs(agentsFile) + if err != nil { + return nil, fmt.Errorf("resolving absolute path for %s: %w", agentsFile, err) + } + + return &workflow.AgentRole{ + Name: filepath.Base(dirPath), + Content: content, + SourcePath: absPath, + }, nil +} + +func crossClientGlobalAgentsDir() string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return filepath.Join(home, ".agents") +} diff --git a/internal/infrastructure/roles/filesystem_repository_test.go b/internal/infrastructure/roles/filesystem_repository_test.go new file mode 100644 index 00000000..2b9920aa --- /dev/null +++ b/internal/infrastructure/roles/filesystem_repository_test.go @@ -0,0 +1,432 @@ +package roles_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/awf-project/cli/internal/domain/ports" + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/infrastructure/roles" +) + +// mockLogger captures Warn calls for testing +type mockLogger struct { + warnings []string +} + +func (m *mockLogger) Debug(msg string, fields ...any) {} +func (m *mockLogger) Info(msg string, fields ...any) {} +func (m *mockLogger) Warn(msg string, fields ...any) { + m.warnings = append(m.warnings, msg) +} +func (m *mockLogger) Error(msg string, fields ...any) {} +func (m *mockLogger) WithContext(ctx map[string]any) ports.Logger { + return m +} + +var _ ports.Logger = (*mockLogger)(nil) + +func TestNewFilesystemAgentRoleRepository(t *testing.T) { + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + require.NotNil(t, repo) +} + +func TestLoad_PriorityOrder(t *testing.T) { + tmpDir := t.TempDir() + + // Setup: Create agents in different search paths + localDir := filepath.Join(tmpDir, ".awf", "agents", "go-senior") + require.NoError(t, os.MkdirAll(localDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(localDir, "AGENTS.md"), + []byte("---\ntitle: Local\n---\nLocal agent role"), + 0o644, + )) + + crossClientDir := filepath.Join(tmpDir, ".agents", "go-senior") + require.NoError(t, os.MkdirAll(crossClientDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(crossClientDir, "AGENTS.md"), + []byte("---\ntitle: CrossClient\n---\nCross-client agent role"), + 0o644, + )) + + t.Run("returns first match in priority order", func(t *testing.T) { + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "go-senior") + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Equal(t, "go-senior", role.Name) + assert.Equal(t, localDir+"/AGENTS.md", filepath.Clean(role.SourcePath)) + assert.NotContains(t, role.Content, "---") + assert.Contains(t, role.Content, "Local agent role") + }) + + t.Run("skips missing paths and continues search", func(t *testing.T) { + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + // Remove local agent, should find cross-client + require.NoError(t, os.RemoveAll(filepath.Join(tmpDir, ".awf", "agents", "go-senior"))) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "go-senior") + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Contains(t, role.Content, "Cross-client agent role") + }) +} + +func TestLoad_AWFAgentsPathOverride(t *testing.T) { + tmpDir := t.TempDir() + overridePath := filepath.Join(tmpDir, "override") + + overrideAgentDir := filepath.Join(overridePath, "custom-role") + require.NoError(t, os.MkdirAll(overrideAgentDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(overrideAgentDir, "AGENTS.md"), + []byte("---\ntitle: Override\n---\nOverride agent role"), + 0o644, + )) + + localAgentDir := filepath.Join(tmpDir, ".awf", "agents", "custom-role") + require.NoError(t, os.MkdirAll(localAgentDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(localAgentDir, "AGENTS.md"), + []byte("---\ntitle: Local\n---\nLocal agent role"), + 0o644, + )) + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + t.Run("AWF_AGENTS_PATH exclusive search when set", func(t *testing.T) { + t.Setenv("AWF_AGENTS_PATH", overridePath) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "custom-role") + + require.NoError(t, err) + assert.Contains(t, role.Content, "Override agent role") + }) + + t.Run("default search when AWF_AGENTS_PATH empty", func(t *testing.T) { + t.Setenv("AWF_AGENTS_PATH", "") + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "custom-role") + + require.NoError(t, err) + assert.Contains(t, role.Content, "Local agent role") + }) +} + +func TestLoad_PathTraversalRejection(t *testing.T) { + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + tests := []struct { + name string + input string + wantErr bool + }{ + {"valid name", "go-senior", false}, + {"dot-dot escape", "../escape", true}, + {"nested dot-dot", "foo/../bar", true}, + {"forward slash", "foo/bar", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + role, err := repo.Load(context.Background(), tt.input) + + if tt.wantErr { + assert.Error(t, err) + assert.Nil(t, role) + assert.ErrorContains(t, err, "invalid") + } else if tt.input == "go-senior" { + // Valid name but not found, should return AgentRoleNotFoundError + assert.Error(t, err) + assert.Nil(t, role) + var notFoundErr *workflow.AgentRoleNotFoundError + assert.ErrorAs(t, err, ¬FoundErr) + } + }) + } +} + +func TestLoad_MissingFile(t *testing.T) { + tmpDir := t.TempDir() + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "missing") + + require.Error(t, err) + assert.Nil(t, role) + + var notFoundErr *workflow.AgentRoleNotFoundError + require.ErrorAs(t, err, ¬FoundErr) + assert.Equal(t, "missing", notFoundErr.Name) + assert.NotEmpty(t, notFoundErr.SearchPaths) + assert.Len(t, notFoundErr.SearchPaths, 4) +} + +func TestLoad_FrontmatterStripping(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, ".awf", "agents", "test-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + content := `--- +title: Test Agent +tags: + - testing +--- +# Agent Role + +This is the actual content after frontmatter.` + + require.NoError(t, os.WriteFile( + filepath.Join(agentDir, "AGENTS.md"), + []byte(content), + 0o644, + )) + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "test-role") + + require.NoError(t, err) + assert.NotContains(t, role.Content, "---") + assert.NotContains(t, role.Content, "title: Test Agent") + assert.Contains(t, role.Content, "# Agent Role") + assert.Contains(t, role.Content, "This is the actual content after frontmatter.") +} + +func TestLoad_LargeFileWarning(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, ".awf", "agents", "large-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + // Create a file > 500KB + largeContent := strings.Repeat("x", 501*1024) + require.NoError(t, os.WriteFile( + filepath.Join(agentDir, "AGENTS.md"), + []byte("---\n---\n"+largeContent), + 0o644, + )) + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "large-role") + + require.NoError(t, err) + assert.NotNil(t, role) + assert.NotEmpty(t, logger.warnings) + assert.True(t, strings.Contains(logger.warnings[0], "500KB") || strings.Contains(logger.warnings[0], "exceeds")) +} + +func TestLoad_PerformanceSub100KB(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, ".awf", "agents", "perf-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + // Create a file ~100KB + content := strings.Repeat("y", 100*1024) + require.NoError(t, os.WriteFile( + filepath.Join(agentDir, "AGENTS.md"), + []byte("---\n---\n"+content), + 0o644, + )) + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + start := time.Now() + role, err := repo.Load(context.Background(), "perf-role") + elapsed := time.Since(start) + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Less(t, elapsed, 50*time.Millisecond, "load should complete in < 50ms") +} + +func TestLoadFromPath_HappyPath(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, "agents", "test-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(agentDir, "AGENTS.md"), + []byte("---\n---\nAgent content from path"), + 0o644, + )) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.LoadFromPath(context.Background(), agentDir) + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Contains(t, role.Content, "Agent content from path") + assert.True(t, filepath.IsAbs(role.SourcePath)) +} + +func TestLoadFromPath_DirectFile(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, "agents", "test-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + + agentFilePath := filepath.Join(agentDir, "AGENTS.md") + require.NoError(t, os.WriteFile( + agentFilePath, + []byte("---\n---\nDirect file content"), + 0o644, + )) + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.LoadFromPath(context.Background(), agentDir) + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Contains(t, role.Content, "Direct file content") +} + +func TestLoadFromPath_Missing(t *testing.T) { + tmpDir := t.TempDir() + missingPath := filepath.Join(tmpDir, "nonexistent") + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.LoadFromPath(context.Background(), missingPath) + + require.Error(t, err) + assert.Nil(t, role) + + var notFoundErr *workflow.AgentRoleNotFoundError + require.ErrorAs(t, err, ¬FoundErr) + assert.NotEmpty(t, notFoundErr.SearchPaths) +} + +func TestSearchPaths_LocalToGlobal(t *testing.T) { + tmpDir := t.TempDir() + + // Create agents at different priority levels + paths := map[string]string{ + "local": filepath.Join(tmpDir, ".awf", "agents", "priority-role"), + "cross": filepath.Join(tmpDir, ".agents", "priority-role"), + "global": filepath.Join(tmpDir, ".config", "awf", "agents", "priority-role"), + "crossglobal": filepath.Join(tmpDir, ".agents", "priority-role"), + } + + for _, p := range paths { + require.NoError(t, os.MkdirAll(p, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(p, "AGENTS.md"), + []byte(fmt.Sprintf("---\n---\nContent from %s", p)), + 0o644, + )) + } + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + // Unset AWF_AGENTS_PATH to use default search + t.Setenv("AWF_AGENTS_PATH", "") + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "priority-role") + + require.NoError(t, err) + assert.NotNil(t, role) + // Should find local first (highest priority) + assert.Contains(t, role.SourcePath, ".awf") +} + +func TestLoad_NilLogger(t *testing.T) { + tmpDir := t.TempDir() + + agentDir := filepath.Join(tmpDir, ".awf", "agents", "safe-role") + require.NoError(t, os.MkdirAll(agentDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(agentDir, "AGENTS.md"), + []byte("---\n---\nContent"), + 0o644, + )) + + origDir, err := os.Getwd() + require.NoError(t, err) + defer os.Chdir(origDir) + require.NoError(t, os.Chdir(tmpDir)) + + // Should not panic with nil logger + repo := roles.NewFilesystemAgentRoleRepository(nil) + + role, err := repo.Load(context.Background(), "safe-role") + + require.NoError(t, err) + assert.NotNil(t, role) +} diff --git a/internal/infrastructure/xdg/xdg.go b/internal/infrastructure/xdg/xdg.go index f15a712c..b388be2d 100644 --- a/internal/infrastructure/xdg/xdg.go +++ b/internal/infrastructure/xdg/xdg.go @@ -23,8 +23,12 @@ func DataHome() string { return filepath.Join(home, ".local", "share") } -// AWFConfigDir returns the AWF config directory ($XDG_CONFIG_HOME/awf) +// AWFConfigDir returns the AWF config directory. +// Checks $AWF_CONFIG_HOME first, then falls back to $XDG_CONFIG_HOME/awf. func AWFConfigDir() string { + if dir := os.Getenv("AWF_CONFIG_HOME"); dir != "" { + return dir + } return filepath.Join(ConfigHome(), "awf") } @@ -89,6 +93,16 @@ func LocalSkillsDir() string { return ".awf/skills" } +// AWFAgentsDir returns the global agents directory ($XDG_CONFIG_HOME/awf/agents) +func AWFAgentsDir() string { + return filepath.Join(AWFConfigDir(), "agents") +} + +// LocalAgentsDir returns the local project agents directory +func LocalAgentsDir() string { + return ".awf/agents" +} + // AWFPluginsDir returns the global plugins directory ($XDG_DATA_HOME/awf/plugins) func AWFPluginsDir() string { return filepath.Join(AWFDataDir(), "plugins") diff --git a/internal/interfaces/cli/run.go b/internal/interfaces/cli/run.go index 5c476dec..1cb552f1 100644 --- a/internal/interfaces/cli/run.go +++ b/internal/interfaces/cli/run.go @@ -21,6 +21,7 @@ import ( infraexpression "github.com/awf-project/cli/internal/infrastructure/expression" infraotel "github.com/awf-project/cli/internal/infrastructure/otel" "github.com/awf-project/cli/internal/infrastructure/repository" + "github.com/awf-project/cli/internal/infrastructure/roles" "github.com/awf-project/cli/internal/infrastructure/store" "github.com/awf-project/cli/internal/infrastructure/xdg" "github.com/awf-project/cli/internal/interfaces/cli/ui" @@ -318,6 +319,7 @@ func runWorkflow(cmd *cobra.Command, cfg *Config, workflowName string, inputFlag application.WithHistoryStore(historyStore), application.WithTemplatePaths(templatePaths), application.WithTracer(tracer), + application.WithAgentRoleRepository(roles.NewFilesystemAgentRoleRepository(logger)), application.WithUserInputReader(ui.NewStdinInputReader(os.Stdin, os.Stdout)), application.WithPackContext(packName, packResolver), } @@ -1035,6 +1037,7 @@ func runSingleStep( application.WithTemplatePaths(templatePaths), application.WithUserInputReader(ui.NewStdinInputReader(os.Stdin, os.Stdout)), application.WithPackContext(packName, packResolver), + application.WithAgentRoleRepository(roles.NewFilesystemAgentRoleRepository(logger)), } if !skipPlugins && pluginResult != nil { diff --git a/internal/interfaces/cli/validate.go b/internal/interfaces/cli/validate.go index 3ceada83..a4a3b1dd 100644 --- a/internal/interfaces/cli/validate.go +++ b/internal/interfaces/cli/validate.go @@ -10,10 +10,12 @@ import ( "time" "github.com/awf-project/cli/internal/application" + "github.com/awf-project/cli/internal/domain/ports" "github.com/awf-project/cli/internal/domain/workflow" "github.com/awf-project/cli/internal/infrastructure/analyzer" "github.com/awf-project/cli/internal/infrastructure/expression" "github.com/awf-project/cli/internal/infrastructure/repository" + "github.com/awf-project/cli/internal/infrastructure/roles" "github.com/awf-project/cli/internal/infrastructure/skills" "github.com/awf-project/cli/internal/interfaces/cli/ui" "github.com/spf13/cobra" @@ -150,12 +152,19 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi skillWarnings, validationErr = validateSkillRefs(wf) } + var roleWarnings []string + if validationErr == nil { + roleRepo := roles.NewFilesystemAgentRoleRepository(nil) + roleWarnings, validationErr = validateRoleRefs(wf, roleRepo) + } + // Collect disabled plugin warnings (non-blocking, skipped on quiet format or when --skip-plugins) var pluginWarnings []string if !skipPlugins && validationErr == nil && cfg.OutputFormat != ui.FormatQuiet { pluginWarnings = collectDisabledPluginWarnings(ctx, cfg, wf) } pluginWarnings = append(pluginWarnings, skillWarnings...) + pluginWarnings = append(pluginWarnings, roleWarnings...) // validatorTimeout is reserved for plugin validator invocation (wired in GREEN phase) _ = validatorTimeout @@ -400,6 +409,74 @@ func validateSkillRefs(wf *workflow.Workflow) ([]string, error) { return warnings, nil } +func validateRoleRefs(wf *workflow.Workflow, repo ports.AgentRoleRepository) ([]string, error) { + ctx := context.Background() + var warnings []string + + for _, step := range wf.Steps { + if step.Agent == nil || step.Agent.Role == "" { + continue + } + + role := step.Agent.Role + + // Defense in depth: reject path-traversal even though domain + infra also reject it. + if strings.Contains(role, "..") { + return nil, workflow.ValidationError{ + Level: workflow.ValidationLevelError, + Code: workflow.ErrRoleNotFound, + Message: "role path contains path-traversal pattern (..): " + role, + Path: fmt.Sprintf("states.%s.agent.role", step.Name), + } + } + + agentRole, err := application.ResolveAgentRole(ctx, repo, role, wf.SourceDir) + if err != nil { + var notFound *workflow.AgentRoleNotFoundError + if errors.As(err, ¬Found) { + if roleDirExistsWithoutAgentsMD(notFound) { + return nil, workflow.ValidationError{ + Level: workflow.ValidationLevelError, + Code: workflow.ErrRoleMissingAgentsMD, + Message: fmt.Sprintf("role %q has no AGENTS.md", notFound.Name), + Path: fmt.Sprintf("states.%s.agent.role", step.Name), + } + } + return nil, workflow.ValidationError{ + Level: workflow.ValidationLevelError, + Code: workflow.ErrRoleNotFound, + Message: err.Error(), + Path: fmt.Sprintf("states.%s.agent.role", step.Name), + } + } + return nil, workflow.ValidationError{ + Level: workflow.ValidationLevelError, + Code: workflow.ErrRoleNotFound, + Message: err.Error(), + Path: fmt.Sprintf("states.%s.agent.role", step.Name), + } + } + + if agentRole.Content == "" { + warnings = append(warnings, fmt.Sprintf("[%s] role %q has empty AGENTS.md body", workflow.ErrRoleEmptyContent, agentRole.Name)) + } else { + var rawSize int64 + if agentRole.SourcePath != "" { + if info, statErr := os.Stat(agentRole.SourcePath); statErr == nil { + rawSize = info.Size() + } + } + if rawSize > 500*1024 { + warnings = append(warnings, fmt.Sprintf("role %q: AGENTS.md exceeds 500KB size threshold", agentRole.Name)) + } else if len(agentRole.Content)+len(step.Agent.SystemPrompt) > 10*1024 { + warnings = append(warnings, fmt.Sprintf("role %q: combined role content and system_prompt exceeds 10KB threshold", agentRole.Name)) + } + } + } + + return warnings, nil +} + func skillDirExists(name string, searchPaths []string) bool { for _, searchPath := range searchPaths { if _, err := os.Stat(filepath.Join(searchPath, name)); err == nil { @@ -409,6 +486,21 @@ func skillDirExists(name string, searchPaths []string) bool { return false } +// roleDirExistsWithoutAgentsMD checks whether the role directory exists but +// is missing an AGENTS.md file. For path-based refs (single search path that +// equals the role path itself), it checks the path directly rather than +// joining name into the path (which would double-nest). +func roleDirExistsWithoutAgentsMD(notFound *workflow.AgentRoleNotFoundError) bool { + if len(notFound.SearchPaths) == 1 { + dir := notFound.SearchPaths[0] + if info, err := os.Stat(dir); err == nil && info.IsDir() { + return true + } + return false + } + return skillDirExists(notFound.Name, notFound.SearchPaths) +} + // collectDisabledPluginWarnings checks operation steps for disabled plugin references. // Returns warning strings for each operation whose plugin prefix is explicitly disabled. // Gracefully returns nil if the plugin system cannot be initialized. diff --git a/internal/interfaces/cli/validate_role_test.go b/internal/interfaces/cli/validate_role_test.go new file mode 100644 index 00000000..f11900bc --- /dev/null +++ b/internal/interfaces/cli/validate_role_test.go @@ -0,0 +1,416 @@ +package cli + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/infrastructure/roles" +) + +func TestValidateRoleRefs_NoRolesReturnsNoWarnings(t *testing.T) { + tmpDir := t.TempDir() + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + // No Role field + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} + +func TestValidateRoleRefs_ValidNameBasedRole(t *testing.T) { + tmpDir := t.TempDir() + + // Create role directory with AGENTS.md + roleDir := filepath.Join(tmpDir, ".awf", "agents", "go-senior") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + agentsMD := filepath.Join(roleDir, "AGENTS.md") + require.NoError(t, os.WriteFile(agentsMD, []byte("You are a senior Go engineer."), 0o644)) + + // Override AWF_CONFIG_HOME BEFORE repo construction — the constructor + // reads this eagerly and bakes search paths at init time. + configHome := filepath.Join(tmpDir, ".awf") + t.Setenv("AWF_CONFIG_HOME", configHome) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "go-senior", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} + +func TestValidateRoleRefs_ValidPathBasedRole(t *testing.T) { + tmpDir := t.TempDir() + + // Create role directory with AGENTS.md + roleDir := filepath.Join(tmpDir, "my-role") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + agentsMD := filepath.Join(roleDir, "AGENTS.md") + require.NoError(t, os.WriteFile(agentsMD, []byte("Role content here"), 0o644)) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "./my-role", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} + +func TestValidateRoleRefs_MissingRoleDirectory(t *testing.T) { + tmpDir := t.TempDir() + + t.Setenv("AWF_CONFIG_HOME", filepath.Join(tmpDir, ".awf")) + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "missing-role", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + require.Error(t, err) + assert.Nil(t, warnings) + var validErr workflow.ValidationError + assert.True(t, errors.As(err, &validErr), "error must be ValidationError, got: %T", err) + assert.Equal(t, workflow.ErrRoleNotFound, validErr.Code) + assert.Contains(t, err.Error(), "missing-role") +} + +func TestValidateRoleRefs_MissingAgentsMD(t *testing.T) { + tmpDir := t.TempDir() + + // Create role directory but without AGENTS.md + roleDir := filepath.Join(tmpDir, ".awf", "agents", "incomplete-role") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + + configHome := filepath.Join(tmpDir, ".awf") + t.Setenv("AWF_CONFIG_HOME", configHome) + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "incomplete-role", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + require.Error(t, err) + assert.Nil(t, warnings) + var validErr workflow.ValidationError + assert.True(t, errors.As(err, &validErr), "error must be ValidationError, got: %T", err) + assert.Equal(t, workflow.ErrRoleMissingAgentsMD, validErr.Code) + assert.Contains(t, err.Error(), "incomplete-role") +} + +func TestValidateRoleRefs_EmptyAgentsMDBody(t *testing.T) { + tmpDir := t.TempDir() + + // Create role with empty AGENTS.md + roleDir := filepath.Join(tmpDir, "empty-role") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + agentsMD := filepath.Join(roleDir, "AGENTS.md") + require.NoError(t, os.WriteFile(agentsMD, []byte(""), 0o644)) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "./empty-role", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.NotNil(t, warnings) + require.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "empty-role") + assert.Contains(t, strings.ToLower(warnings[0]), "empty") +} + +func TestValidateRoleRefs_AgentsMDExceedsSizeLimit(t *testing.T) { + tmpDir := t.TempDir() + + // Create role with AGENTS.md > 500KB + roleDir := filepath.Join(tmpDir, "large-role") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + agentsMD := filepath.Join(roleDir, "AGENTS.md") + // Create a file larger than 500KB + content := strings.Repeat("x", 501*1024) + require.NoError(t, os.WriteFile(agentsMD, []byte(content), 0o644)) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "./large-role", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.NotNil(t, warnings) + require.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "large-role") + assert.Contains(t, strings.ToLower(warnings[0]), "500") +} + +func TestValidateRoleRefs_CombinedRoleAndSystemPromptExceeds10KB(t *testing.T) { + tmpDir := t.TempDir() + + // Create role with content that combined with system_prompt exceeds 10KB + roleDir := filepath.Join(tmpDir, "combined-role") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + agentsMD := filepath.Join(roleDir, "AGENTS.md") + roleContent := strings.Repeat("x", 6*1024) + require.NoError(t, os.WriteFile(agentsMD, []byte(roleContent), 0o644)) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "./combined-role", + SystemPrompt: strings.Repeat("y", 5*1024), + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.NotNil(t, warnings) + require.Len(t, warnings, 1) + assert.Contains(t, warnings[0], "combined-role") + assert.Contains(t, strings.ToLower(warnings[0]), "10") +} + +func TestValidateRoleRefs_PathTraversalAttempt(t *testing.T) { + tmpDir := t.TempDir() + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "../../../etc/passwd", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + require.Error(t, err) + assert.Nil(t, warnings) + // Should return hard error (ValidationError) for path traversal + var validErr workflow.ValidationError + assert.True(t, errors.As(err, &validErr), "error must be ValidationError, got: %T", err) +} + +func TestValidateRoleRefs_MultipleStepsMultipleRoles(t *testing.T) { + tmpDir := t.TempDir() + + // Create two role directories + role1Dir := filepath.Join(tmpDir, "role1") + require.NoError(t, os.MkdirAll(role1Dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(role1Dir, "AGENTS.md"), []byte("Role 1"), 0o644)) + + role2Dir := filepath.Join(tmpDir, "role2") + require.NoError(t, os.MkdirAll(role2Dir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(role2Dir, "AGENTS.md"), []byte("Role 2"), 0o644)) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "step1", + Steps: map[string]*workflow.Step{ + "step1": { + Name: "step1", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "./role1", + }, + }, + "step2": { + Name: "step2", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "gemini", + Role: "./role2", + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} + +func TestValidateRoleRefs_StepWithoutAgentConfig(t *testing.T) { + tmpDir := t.TempDir() + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeCommand, + Command: "echo 'hello'", + // No Agent field + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} + +func TestValidateRoleRefs_AgentStepWithEmptyRole(t *testing.T) { + tmpDir := t.TempDir() + repo := roles.NewFilesystemAgentRoleRepository(nil) + + wf := &workflow.Workflow{ + Name: "test-workflow", + Initial: "start", + Steps: map[string]*workflow.Step{ + "start": { + Name: "start", + Type: workflow.StepTypeAgent, + Agent: &workflow.AgentConfig{ + Provider: "claude", + Role: "", // explicitly empty + }, + }, + }, + SourceDir: tmpDir, + } + + warnings, err := validateRoleRefs(wf, repo) + + assert.NoError(t, err) + assert.Empty(t, warnings) +} diff --git a/internal/testutil/mocks/mocks.go b/internal/testutil/mocks/mocks.go index 4a63d90d..e9e9847e 100644 --- a/internal/testutil/mocks/mocks.go +++ b/internal/testutil/mocks/mocks.go @@ -42,6 +42,7 @@ var ( _ ports.Span = (*MockSpan)(nil) _ ports.EventPublisher = (*MockEventPublisher)(nil) _ ports.SkillRepository = (*MockSkillRepository)(nil) + _ ports.AgentRoleRepository = (*MockAgentRoleRepository)(nil) ) // MockWorkflowRepository is a thread-safe mock implementation of ports.WorkflowRepository. @@ -2096,3 +2097,26 @@ func (m *MockSkillRepository) SetLoadError(err error) { defer m.mu.Unlock() m.loadErr = err } + +// MockAgentRoleRepository is a mock implementation of ports.AgentRoleRepository +// with injectable function fields for fine-grained test control. +type MockAgentRoleRepository struct { + LoadFunc func(ctx context.Context, name string) (*workflow.AgentRole, error) + LoadFromPathFunc func(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) +} + +// Load delegates to LoadFunc. +func (m *MockAgentRoleRepository) Load(ctx context.Context, name string) (*workflow.AgentRole, error) { + if m.LoadFunc != nil { + return m.LoadFunc(ctx, name) + } + return nil, nil +} + +// LoadFromPath delegates to LoadFromPathFunc. +func (m *MockAgentRoleRepository) LoadFromPath(ctx context.Context, absolutePath string) (*workflow.AgentRole, error) { + if m.LoadFromPathFunc != nil { + return m.LoadFromPathFunc(ctx, absolutePath) + } + return nil, nil +} diff --git a/tests/integration/api/functional_test.go b/tests/integration/api/functional_test.go index d6b6b767..fc194652 100644 --- a/tests/integration/api/functional_test.go +++ b/tests/integration/api/functional_test.go @@ -153,7 +153,7 @@ func TestAPI_ListExecutions_Integration(t *testing.T) { var result struct { Body struct { Executions []struct { - ExecutionID string `json:"execution_id"` + ExecutionID string `json:"execution_id"` WorkflowName string `json:"workflow_name"` } `json:"executions"` } `json:"body"` @@ -194,7 +194,7 @@ func TestAPI_RunWorkflow_WithInputs_Integration(t *testing.T) { var result struct { Body struct { - ExecutionID string `json:"execution_id"` + ExecutionID string `json:"execution_id"` WorkflowName string `json:"workflow_name"` } `json:"body"` }