diff --git a/.gitignore b/.gitignore index d3a6782b..73e6ec0a 100644 --- a/.gitignore +++ b/.gitignore @@ -57,3 +57,7 @@ site/node_modules/ examples/plugins/*/awf-plugin-* !examples/plugins/*/awf-plugin-*.go !examples/plugins/*/awf-plugin-*_test.go + +# Added by awf project-onboard +.awf/storage/states/ +.awf/storage/logs/ diff --git a/.zpm/kb/default/knowledge.pl b/.zpm/kb/default/knowledge.pl new file mode 100644 index 00000000..5319b9dc --- /dev/null +++ b/.zpm/kb/default/knowledge.pl @@ -0,0 +1,304 @@ +% base-schema.pl — Shared AWF knowledge-base schema. +% Loaded automatically from .zpm/kb/default/. + +% Dynamic declarations — required by Trealla Prolog so that rules referencing +% these predicates compile even before any facts are asserted at runtime. +:- dynamic(task/3). +:- dynamic(depends_on/2). +:- dynamic(task_complete/1). +:- dynamic(task_file/2). +:- dynamic(component/4). +:- dynamic(stub/3). +:- dynamic(test_file/2). +:- dynamic(source_file/1). +:- dynamic(adr/3). +:- dynamic(workflow_run/3). +:- dynamic(impl_note/3). +:- dynamic(feedback_rule/3). +:- dynamic(imports/2). +:- dynamic(layer/2). +:- dynamic(uses_panic/1). +:- dynamic(coverage/2). +:- dynamic(integrity_violation/2). + +% --- Task graph (written by plan, read by implement/fix-errors) --- +% task(Id, Title, Status). Status ∈ {pending, in_progress, completed, blocked}. +% depends_on(TaskA, TaskB). TaskA depends on TaskB completing first. +% task_complete(Id). Written by implement when stubs cleared + tests green. +% incomplete_task(Id). Derived: task that is pending/in_progress with unresolved stubs. + +incomplete_task(Id) :- + task(Id, _, Status), + Status \= completed, + stub(File, _, critical), + task_file(Id, File). + +% --- Component graph (written by plan, read by implement) --- +% component(Id, Name, Path, Type). Type ∈ {module, file, package, pipeline}. +% pipeline_handled(Path). Paths managed by pipelines (CHANGELOG, docs/...). +% forbidden_component(Id) :- Components plan must not include. +% component(Id, _, Path, _), pipeline_handled(Path). + +% forbidden_component/1: Path is bound via component/4, so nonvar-guarded +% pipeline_handled/1 prefix rules fire correctly. Do not call +% pipeline_handled(X) with X unbound — rules will not enumerate. +forbidden_component(Id) :- + component(Id, _, Path, _), + pipeline_handled(Path). + +% Seed the standard pipeline-handled paths (exact matches) +pipeline_handled('CHANGELOG.md'). +pipeline_handled('docs/CHANGELOG.md'). +pipeline_handled('README.md'). +pipeline_handled('README'). +pipeline_handled('CHANGELOG'). +pipeline_handled('CLAUDE.md'). + +% Prefix-based rules (match directories handled by pipelines). +% nonvar/1 guard prevents ZPM 0.1.0 instantiation errors when Path is unbound, +% which would otherwise abort enumeration of the entire predicate (see KNOWN-ISSUES). +% These rules only fire when Path is already bound (the normal use case via forbidden_component/1). +pipeline_handled(Path) :- nonvar(Path), sub_atom(Path, 0, _, _, 'docs/'). +pipeline_handled(Path) :- nonvar(Path), sub_atom(Path, 0, _, _, '.specify/memory/'). +pipeline_handled(Path) :- nonvar(Path), sub_atom(Path, 0, _, _, 'ADR/'). + +% --- Stub tracking (written by code, read by implement) --- +% stub(File, Line, Kind). Kind ∈ {critical, cosmetic, todo, fixme}. +% task_file(TaskId, File). Links stubs back to tasks. + +% --- Test coverage (written by plan/implement, used by audit) --- +% test_file(SourcePath, TestPath). +% covered_by(Source, Test) :- test_file(Source, Test). + +% Decision: wrap test_file/2 in catch/3, same reason as current_decision. +% Why: ZPM (Scryer Prolog) raises existence_error when a constitution rule +% evaluates \+ covered_by(File, _) on a fresh KB with zero test_file facts. +% catch/3 intercepts that error and cleanly fails, so the negation succeeds +% ("no test covers this source") and missing_test violations derive correctly. +covered_by(Source, Test) :- + catch(test_file(Source, Test), _, fail). + +% --- ADR (written by plan/spec-generate, read by architecture reviews) --- +% adr(Id, Title, Decision). +% supersedes(NewId, OldId). +% current_decision(Decision) :- +% adr(Id, _, Decision), \+ supersedes(_, Id). + +% Decision: use catch/3 instead of \+/1 for supersedes check. +% Why: ZPM (Scryer Prolog) raises existence_error when \+ calls an +% undefined predicate. catch/3 handles both the undefined-predicate case +% (no supersedes facts yet → succeed) and the normal case (supersedes +% facts exist → \+ evaluates correctly). +% Trade-off: slightly less idiomatic Prolog; necessary for ZPM compatibility. +current_decision(Id, Decision) :- + adr(Id, _, Decision), + catch((\+ supersedes(_, Id)), _, true). + +% --- Feedback rules (written by feedback workflow, read by review agents) --- +% feedback_rule(Topic, Date, Text). +% Topic: atom matching CLAUDE.md section slug (e.g. architecture_rules) +% derived from section name: lowercase, spaces→underscores +% Date: ISO 8601 atom 'YYYY-MM-DD' — lexicographic comparison works for +% date ordering, e.g. Date @>= '2026-04-15' for "last 7 days" +% Text: quoted atom with the rule body +% Query "recent feedback" example (caller computes cutoff date): +% feedback_rule(Topic, Date, Text), Date @>= '2026-04-15'. + +% --- Workflow history (written by commit) --- +% workflow_run(Name, Date, Status). Status ∈ {success, failure, partial}. + +% --- LLM-emitted rationale (written by plan/code-review agents, read by audit) --- +% Captured from fenced ```zpm blocks emitted by agents; see +% scripts/zpm/parse-zpm-block.sh. +% +% task_rationale(TaskId, Text). +% Why this task exists — captured at generate_tasks time. +% Keyed by TaskId (use upsert: one rationale per task). +% +% task_risk(TaskId, Level, Reason). +% Level ∈ {low, medium, high}. +% LLM's self-assessed risk. Keyed by TaskId (use upsert). +% +% review_finding(Issue, File, Line, Severity, Kind). +% Severity ∈ {info, warning, critical}. +% Kind: short atom (e.g. 'duplication', 'n_plus_1', 'hard_coded_secret'). +% Multi-valued — many findings per issue. Use assert. +% +% impl_note(File, Symbol, Note). +% Local implementation decisions captured during the code TDD cycle. +% Multi-valued — many notes per (File, Symbol). Use assert. + +% Convenience rule: high-risk pending tasks (for audit and dashboards). +high_risk_open_task(Id, Title) :- + task(Id, Title, Status), + Status \= completed, + task_risk(Id, high, _). + +% --- Generic integrity violation scaffolding --- +% Constitutions assert integrity_violation/2 rules. +% Severity ∈ {error, warning, info}. +% Example (from constitution-go.pl): +% integrity_violation(layer_inversion, File) :- +% imports(File, Target), +% layer(File, domain), +% layer(Target, infrastructure). + +% ─── Project Memory ────────────────────────────────────────────────────────── +:- dynamic(observation/4). +:- dynamic(decision/4). +:- dynamic(convention/3). + +% observation(Id, Category, Content, Date) +% Id: unique atom (auto-generated slug) +% Category: pattern | convention | quirk | dependency | performance +% Content: description (single-quoted atom) +% Date: ISO date atom e.g. '2026-05-24' +% +% decision(Id, What, Why, TradeOff) +% Architectural/design decisions recorded during workflows +% +% convention(Id, Pattern, Context) +% Discovered codebase conventions worth preserving + +observations_by_category(Cat, Ids) :- + findall(Id, observation(Id, Cat, _, _), Ids). + +decisions_about(Topic, Ids) :- + findall(Id, (decision(Id, What, _, _), sub_atom(What, _, _, _, Topic)), Ids). +% constitution-go.pl — Go project integrity rules (Prolog). +% Loaded automatically from .zpm/kb/ alongside base-schema.pl. +% +% Encodes a machine-checkable subset of Project Constitution v1.0.0 (go.md). +% Rules produce integrity_violation(Kind, File) facts that audit tooling queries. +% +% --- 7 Principles (verbatim titles + 1-line summaries from go.md) ---------- +% +% P1 — Hexagonal Architecture +% Domain depends on nothing; application on domain; infrastructure +% implements ports; interfaces depend on application. +% +% P2 — Go Idioms +% No panic in library code; interfaces defined by consumer; accept +% interfaces return structs; explicit error handling. +% +% P3 — Test-Driven Development +% RED → GREEN → REFACTOR; 80% minimum code coverage; table-driven tests. +% +% P4 — Error Taxonomy +% Exit codes 1-4 map to user / config / execution / system errors. +% (Not modelled: purely a runtime/CLI convention, not statically checkable) +% +% P5 — Security First +% Inputs untrusted; secrets masked; atomic writes; no plaintext creds. +% (Not modelled: requires data-flow analysis, beyond static ZPM facts) +% +% P6 — Minimal Abstraction +% No premature optimisation; delete unused code; comments only when needed. +% (Not modelled: heuristic too weak without AST; no reliable Prolog rule) +% +% P7 — Documentation Co-location +% README.md, CLAUDE.md, CHANGELOG.md, specs in .specify/. +% (Not modelled: file-presence checks handled by shell audit scripts) +% +% --------------------------------------------------------------------------- +% +% FACT SCHEMA (asserted by callers, e.g. via zpm-assert.sh): +% +% layer(File, Layer) File belongs to Layer. +% Layer ∈ {domain, application, infrastructure, interfaces} +% imports(File, Target) File depends on Target (path or module import). +% source_file(File) File is a Go source file tracked in the project. +% coverage(File, Pct) File has measured test coverage Pct (integer 0-100). +% uses_panic(File) File contains a panic() call. +% +% covered_by/2 is defined in base-schema.pl via test_file/2 — do NOT redefine here. +% +% --------------------------------------------------------------------------- + +% layer_index/2: numeric order of layers, inner (low) → outer (high). +% Used to detect when a lower-indexed layer imports a higher-indexed layer. + +layer_index(domain, 0). +layer_index(application, 1). +layer_index(infrastructure, 2). +layer_index(interfaces, 3). + +% --- Violation kinds modelled in this constitution ------------------------- +% Enumeration helper for audit tooling. Without this, querying +% integrity_violation(K, F) with both vars unbound returns [] in ZPM 0.1.0. +violation_kind(layer_inversion). +violation_kind(missing_test). +violation_kind(framework_in_domain). +violation_kind(panic_in_library). +violation_kind(low_coverage). + +% --- P1: Layer inversion ---------------------------------------------------- +% Violation when a file in an inner layer imports a file in an outer layer. + +integrity_violation(layer_inversion, File) :- + imports(File, Target), + layer(File, LayerA), + layer(Target, LayerB), + layer_index(LayerA, IdxA), + layer_index(LayerB, IdxB), + IdxA < IdxB. + +% --- P3: Missing test ------------------------------------------------------- +% A source_file has no covered_by entry (via base-schema.pl test_file/2). +% Excludes main.go (entry point) and _test.go files (they ARE the tests). + +is_main_file('main.go'). +is_main_file(File) :- atom_concat(_, '/main.go', File). +is_main_file(File) :- atom_concat(_, '_test.go', File). + +integrity_violation(missing_test, File) :- + source_file(File), + \+ covered_by(File, _), + \+ is_main_file(File). + +% --- P2: Framework import in domain layer ----------------------------------- +% Domain code must not import web / ORM frameworks (tight coupling to infra). + +web_framework('github.com/gin-gonic/gin'). +web_framework('github.com/labstack/echo'). +web_framework('github.com/labstack/echo/v4'). +web_framework('gorm.io/gorm'). +web_framework('github.com/gofiber/fiber/v2'). +web_framework('github.com/go-chi/chi/v5'). +web_framework('github.com/gorilla/mux'). + +integrity_violation(framework_in_domain, File) :- + layer(File, domain), + imports(File, Target), + web_framework(Target). + +% --- P2: Panic in library code ---------------------------------------------- +% Library layers (domain, application, infrastructure) must not use panic. +% The interfaces layer (CLI/API entry points) is exempt — it may panic on +% unrecoverable startup errors. + +library_layer(domain). +library_layer(application). +library_layer(infrastructure). + +integrity_violation(panic_in_library, File) :- + uses_panic(File), + layer(File, Layer), + library_layer(Layer). + +% --- P3: Low coverage ------------------------------------------------------- +% Any file with a coverage fact below 80% is a violation. + +integrity_violation(low_coverage, File) :- + coverage(File, Pct), + Pct < 80. + +% --- Skipped rules ---------------------------------------------------------- +% +% P5 — logic_in_interfaces (draft rule from plan §563-630): +% The heuristic "interface file has >N lines of non-declaration code" is +% unreliable without a proper AST. Skipped; catches too many false positives. +% +% P5 — package_name_mismatch: +% Go package naming is enforced by `go vet` and the compiler. Reproducing +% that logic in ZPM provides no additional safety signal. diff --git a/.zpm/kb/feedback/journal.wal b/.zpm/kb/feedback/journal.wal new file mode 100644 index 00000000..e69de29b diff --git a/.zpm/kb/feedback/knowledge.pl b/.zpm/kb/feedback/knowledge.pl new file mode 100644 index 00000000..08144761 --- /dev/null +++ b/.zpm/kb/feedback/knowledge.pl @@ -0,0 +1,62 @@ +:- module(feedback, []). +% ─── Feedback Rules Schema ─────────────────────────────────────────────────── +% Memory segment: feedback +% Lifecycle: created at project setup, accumulates across workflow runs. +% Replaces markdown-based feedback in CLAUDE.md with queryable Prolog facts. +% +% Facts: +% rule(Id, Category, Description, Priority, Source) +% Id: unique atom (e.g. rule_001, rule_avoid_sql_concat) +% Category: architecture | pitfall | test | review | style +% Description: the rule text (single-quoted atom) +% Priority: high | medium | low +% Source: implement | audit | fix_errors | pr_review +% +% example(RuleId, Type, Code, Explanation) +% Type: good | bad +% +% trigger(RuleId, Pattern, Scope) +% Pattern: file/path substring to match +% Scope: file | directory | project +% +% Dynamic declarations (required by Trealla Prolog for runtime assertion). +:- dynamic(rule/5). +:- dynamic(example/4). +:- dynamic(trigger/3). + +% Derived rules: + +% A rule is applicable to a file if its trigger pattern matches. +applicable(RuleId, File) :- + trigger(RuleId, Pattern, file), + sub_atom(File, _, _, _, Pattern). +applicable(RuleId, File) :- + trigger(RuleId, Pattern, directory), + sub_atom(File, _, _, _, Pattern). +applicable(RuleId, _File) :- + trigger(RuleId, _, project). + +% Find all rules effective for a list of files (deduplicated). +effective_rules(Files, RuleIds) :- + findall(Id, (member(F, Files), applicable(Id, F)), All), + sort(All, RuleIds). + +% Get high-priority rules. +priority_rules(Ids) :- + findall(Id, rule(Id, _, _, high, _), Ids). + +% Rules by category. +rules_by_category(Cat, Ids) :- + findall(Id, rule(Id, Cat, _, _, _), Ids). + +% Rules by source workflow. +rules_by_source(Src, Ids) :- + findall(Id, rule(Id, _, _, _, Src), Ids). + +% Full rule detail for display. +rule_detail(Id, Cat, Desc, Prio) :- + rule(Id, Cat, Desc, Prio, _). + +% Count rules. +rule_count(N) :- + findall(Id, rule(Id, _, _, _, _), L), length(L, N). diff --git a/.zpm/mounts.json b/.zpm/mounts.json index 6842d4bc..0430b57f 100644 --- a/.zpm/mounts.json +++ b/.zpm/mounts.json @@ -6,6 +6,12 @@ "path": ".zpm/kb/default", "scope": "project", "mode": "rw" + }, + { + "name": "feedback", + "path": ".zpm/kb/feedback", + "scope": "project", + "mode": "rw" } ] } \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index e0213f32..90f48066 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Breaking Changes + +- **F100**: Dedicated `roles/` namespace for agent roles — the environment variable for overriding role search paths is renamed from `AWF_AGENTS_PATH` to `AWF_ROLES_PATH`. All role discovery now happens exclusively under `roles/`-namespaced directories. Migration guide for users upgrading from F098: + - **Environment variable**: rename `AWF_AGENTS_PATH` → `AWF_ROLES_PATH` in shell profiles, CI scripts, and Docker environments. + - **Local project directory**: move `.awf/agents//` → `.awf/roles//`. The old `.awf/agents/` path is no longer searched. + - **Cross-client local directory**: move `.agents//` (flat, at root) → `.agents/roles//`. Roles stored directly under `.agents/` without the `roles/` sub-namespace are no longer discovered. + - **Cross-client global directory**: move `~/.agents//` → `~/.agents/roles//`. Same sub-namespace requirement as above. + - **XDG global directory**: `$XDG_CONFIG_HOME/awf/roles/` path is unchanged (was not previously a dedicated path; roles were found via the old `AWF_AGENTS_PATH` override only). + - Workflows referencing roles by name (e.g. `role: go-senior`) will fail at validation or execution time with `ErrRoleNotFound` until the role directories are migrated to the new namespace. Path-based refs (e.g. `role: ./path/to/role`) are unaffected as long as the referenced path still exists. + ### 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 +- **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_ROLES_PATH` (exclusive override), `.awf/roles/`, `.agents/roles/`, `$XDG_CONFIG_HOME/awf/roles/`, `~/.agents/roles/`. 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 5576a29b..16693245 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -312,3 +312,69 @@ func TestWorkflowValidation(t *testing.T) { - Never mark implementation complete without confirming `make build`, `make lint`, and `make test` pass with zero violations - Always verify all validation expert reports generate successfully; missing sections (Conformance, Coverage, Cleanup) indicate incomplete validation + +## ZPM Project Memory + +This project uses ZPM memory segments as its knowledge base. **Query ZPM before starting work. Update ZPM as you learn.** + +### Segments + +| Segment | Purpose | Mount | +|---------|---------|-------| +| `default` | Project knowledge: ADRs, decisions, observations, conventions, architecture facts | auto | +| `feedback` | Learned rules from past implementations/reviews — queryable by file pattern | auto | +| `pr_` | PR tracking: TODOs, stubs, mocks, blocking issues, completion gate | per-implementation | + +### Read before acting + +```bash +# What decisions exist about this area? +zpm query-logic --goal "decision(Id, What, Why)" --memory default +# What feedback rules apply to files I'm touching? +zpm query-logic --goal "rule(Id, Cat, Desc, Prio, Src)" --memory feedback +# What rules apply specifically to a file/directory? +zpm query-logic --goal "applicable(RuleId, 'src/path/file.ext')" --memory feedback +# What ADRs are active? +zpm query-logic --goal "current_decision(Id, Decision)" --memory default +# Any architecture violations? +zpm query-logic --goal "integrity_violation(Kind, File)" --memory default +``` + +### Write as you work + +```bash +# Observation: discovered something non-obvious +zpm remember-fact --fact "observation(id, Category, 'Description', 'YYYY-MM-DD')" --memory default +# Categories: pattern | convention | quirk | dependency | performance + +# Decision: made an architectural choice +zpm remember-fact --fact "decision(id, 'What', 'Why', 'Trade-off')" --memory default + +# Feedback rule: a mistake teaches a reusable lesson +zpm remember-fact --fact "rule(rule_id, Category, 'Imperative rule', Priority, Source)" --memory feedback +zpm remember-fact --fact "trigger(rule_id, 'pattern', Scope)" --memory feedback +# Categories: architecture | pitfall | test | review | style +# Priority: high | medium | low — Scope: file | directory | project +``` + +### What NOT to store + +- File contents, git status, directory listings — ephemeral +- Anything already in code comments or documentation +- Duplicate of an existing fact — query first + +### Architecture Rules + +Query: `zpm query-logic --goal "rule(Id, architecture, Desc, Prio, Src)" --memory feedback` + +### Test Conventions + +Query: `zpm query-logic --goal "rule(Id, test, Desc, Prio, Src)" --memory feedback` + +### Common Pitfalls + +Query: `zpm query-logic --goal "rule(Id, pitfall, Desc, Prio, Src)" --memory feedback` + +### Review Standards + +Query: `zpm query-logic --goal "rule(Id, review, Desc, Prio, Src)" --memory feedback` diff --git a/README.md b/README.md index ebeebaba..d9a5cd34 100644 --- a/README.md +++ b/README.md @@ -12,7 +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` +- **Agent Roles** - Inject reusable personas into agent steps via `role:` field referencing AGENTS.md files; filesystem-based multi-directory discovery in a dedicated `roles/` namespace (project `.awf/roles/`, `.agents/roles/`, `$XDG_CONFIG_HOME/awf/roles/`, `~/.agents/roles/`) with priority ordering and `AWF_ROLES_PATH` exclusive override; 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/reference/error-codes.md b/docs/reference/error-codes.md index 475da926..907b75f4 100644 --- a/docs/reference/error-codes.md +++ b/docs/reference/error-codes.md @@ -79,13 +79,13 @@ awf run code-review **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. +**Resolution:** Verify the role name matches a directory in one of the discovery paths (`.awf/roles/`, `.agents/roles/`, `$XDG_CONFIG_HOME/awf/roles/`, `~/.agents/roles/`) and that `/AGENTS.md` is readable. For path-based references, verify the path is correct relative to the workflow file. Set `AWF_ROLES_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/ +# .awf/roles/, .agents/roles/, ~/.config/awf/roles/, ~/.agents/roles/ ``` **Related codes:** `USER.INPUT.MISSING_SKILL`, `USER.INPUT.MISSING_FILE`, `WORKFLOW.VALIDATION.INVALID_REFERENCE` diff --git a/docs/user-guide/agent-steps.md b/docs/user-guide/agent-steps.md index 801c5b15..ccd68cad 100644 --- a/docs/user-guide/agent-steps.md +++ b/docs/user-guide/agent-steps.md @@ -660,7 +660,7 @@ Create a role directory with an `AGENTS.md` file: **Directory structure:** ``` -.awf/agents/ +.awf/roles/ ├── go-senior/ │ └── AGENTS.md # Agent persona (frontmatter optional) ├── security-reviewer/ @@ -669,7 +669,7 @@ Create a role directory with an `AGENTS.md` file: └── AGENTS.md ``` -**File:** `.awf/agents/go-senior/AGENTS.md` +**File:** `.awf/roles/go-senior/AGENTS.md` ```markdown --- name: go-senior @@ -736,11 +736,11 @@ security_review: 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) +1. **`AWF_ROLES_PATH` environment variable** (if set, exclusive — overrides all others) +2. **`.awf/roles/`** (project-level, AWF-specific) +3. **`.agents/roles/`** (project-level, cross-client compatibility) +4. **`$XDG_CONFIG_HOME/awf/roles/`** (global user, AWF-specific — defaults to `~/.config/awf/roles/`) +5. **`~/.agents/roles/`** (global user, cross-client) This enables shared global personas while allowing project-specific overrides. @@ -748,9 +748,40 @@ This enables shared global personas while allowing project-specific overrides. ```bash # Use only roles from custom directory -AWF_AGENTS_PATH=/shared/agents awf run workflow +AWF_ROLES_PATH=/shared/roles awf run workflow ``` +### Role Resolution: By-Name vs Explicit Path + +The `role:` field accepts two forms that behave differently: + +**By-name** (recommended for overrides): + +```yaml +role: go-senior # discovers .awf/roles/go-senior/, then .agents/roles/go-senior/, etc. +``` + +Discovery scans the 4 standard directories in priority order, starting local (`.awf/roles/`) and falling back to global (`~/.agents/roles/`). Resolution is **independent of the workflow file's location** — moving the workflow file does not affect which role is found. + +**Explicit path** (relative or absolute): + +```yaml +role: ./custom-roles/senior # resolved relative to the workflow file's directory +role: /shared/roles/senior # absolute path, resolved as-is +role: ~/shared-roles/senior # home-relative, expanded at runtime +``` + +Path references are resolved **relative to the workflow file's directory**. Moving the workflow file changes where AWF looks. + +**When to use each:** + +| Form | Use when | +|------|----------| +| By-name | Overriding global personas per-project — place role in `.awf/roles/` and reference by name from any workflow in the project | +| Explicit path | Roles that are tightly coupled to a specific workflow or stored outside the standard directories | + +Prefer by-name for shared roles: a role placed in `.awf/roles/` is found by all workflows in the project regardless of their location in the directory tree. + ### Role Content Format **Frontmatter is optional** — AWF strips YAML frontmatter (content between `---` delimiters) if present, preserving only the Markdown body: @@ -800,15 +831,15 @@ Reference roles by explicit path instead of discovery: review: type: agent provider: claude - role: ./custom-agents/senior-go + role: ./custom-roles/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` +- **Relative** to the workflow directory: `role: ./custom-roles/senior-go` +- **Absolute**: `role: /home/user/roles/senior-go` +- **Home-relative**: `role: ~/shared-roles/senior-go` ### Dynamic Role Selection @@ -854,7 +885,7 @@ Validation reports: |-------|-------|----------| | `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 | +| Role not in expected location | Wrong discovery directory | Move role to one of the 4 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 | diff --git a/docs/user-guide/workflow-syntax.md b/docs/user-guide/workflow-syntax.md index 20ef95ab..8d25c2ce 100644 --- a/docs/user-guide/workflow-syntax.md +++ b/docs/user-guide/workflow-syntax.md @@ -460,7 +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) | +| `role` | string | No | Role reference — name-based (e.g., `go-senior`) or path-based (e.g., `./custom-roles/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/loop_executor_core_test.go b/internal/application/loop_executor_core_test.go index 39253c5f..49b0abc2 100644 --- a/internal/application/loop_executor_core_test.go +++ b/internal/application/loop_executor_core_test.go @@ -69,9 +69,9 @@ func TestLoopResult_AllSucceeded(t *testing.T) { func TestStepExecutorFunc_TypeSignature_ReturnsNextStepAndError(t *testing.T) { var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, - stepName string, - intCtx *interpolation.Context, + _ context.Context, + _ string, + _ *interpolation.Context, ) (string, error) { // Return a transition to another step return "target_step", nil @@ -87,9 +87,9 @@ func TestStepExecutorFunc_TypeSignature_ReturnsNextStepAndError(t *testing.T) { func TestStepExecutorFunc_NoTransition_ReturnsEmptyString(t *testing.T) { var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, - stepName string, - intCtx *interpolation.Context, + _ context.Context, + _ string, + _ *interpolation.Context, ) (string, error) { // No transition - return empty nextStep return "", nil @@ -106,9 +106,9 @@ func TestStepExecutorFunc_NoTransition_ReturnsEmptyString(t *testing.T) { func TestStepExecutorFunc_ErrorWithoutTransition(t *testing.T) { expectedErr := errors.New("step execution failed") var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, - stepName string, - intCtx *interpolation.Context, + _ context.Context, + _ string, + _ *interpolation.Context, ) (string, error) { // Error case - return empty nextStep with error return "", expectedErr @@ -126,9 +126,9 @@ func TestStepExecutorFunc_ErrorWithoutTransition(t *testing.T) { func TestStepExecutorFunc_ErrorWithTransition(t *testing.T) { expectedErr := errors.New("step failed but has on_failure transition") var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, - stepName string, - intCtx *interpolation.Context, + _ context.Context, + _ string, + _ *interpolation.Context, ) (string, error) { // On-failure transition case return "error_handler_step", expectedErr @@ -149,8 +149,8 @@ func TestStepExecutorFunc_ContextCancellation(t *testing.T) { var stepExecutor application.StepExecutorFunc = func( ctx context.Context, - stepName string, - intCtx *interpolation.Context, + _ string, + _ *interpolation.Context, ) (string, error) { // Check context if ctx.Err() != nil { @@ -169,8 +169,8 @@ func TestStepExecutorFunc_ContextCancellation(t *testing.T) { func TestStepExecutorFunc_NilInterpolationContext(t *testing.T) { var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, - stepName string, + _ context.Context, + _ string, intCtx *interpolation.Context, ) (string, error) { // Verify nil is handled (executor should validate) @@ -190,9 +190,9 @@ func TestStepExecutorFunc_NilInterpolationContext(t *testing.T) { func TestStepExecutorFunc_EmptyStepName(t *testing.T) { var stepExecutor application.StepExecutorFunc = func( - ctx context.Context, + _ context.Context, stepName string, - intCtx *interpolation.Context, + _ *interpolation.Context, ) (string, error) { if stepName == "" { return "", errors.New("step name cannot be empty") diff --git a/internal/application/plugin_service_interface_test.go b/internal/application/plugin_service_interface_test.go index 02f65e1b..62606498 100644 --- a/internal/application/plugin_service_interface_test.go +++ b/internal/application/plugin_service_interface_test.go @@ -73,8 +73,12 @@ func TestPluginService_OptionA_CompositeInterfaceAcceptsNarrower(t *testing.T) { mockPluginConfig: config, } - // Should satisfy the interface - var _ ports.PluginStateStore = composite + // Should satisfy the interface and delegate to embedded narrower mocks. + // Exercising the embedded methods proves the composite wraps both + // implementations rather than just satisfying the interface at compile time. + var stateStore ports.PluginStateStore = composite + assert.Empty(t, stateStore.ListDisabled(), "delegates to embedded mockPluginStore") + assert.True(t, stateStore.IsEnabled("any-plugin"), "delegates to embedded mockPluginConfig (defaults to true for unknown plugins)") } // Option B Tests: Alternative Implementation (Separate Interfaces) diff --git a/internal/application/role_loader_test.go b/internal/application/role_loader_test.go index e57360cb..5cf4afe5 100644 --- a/internal/application/role_loader_test.go +++ b/internal/application/role_loader_test.go @@ -2,6 +2,7 @@ package application_test import ( "context" + "errors" "os" "path/filepath" "testing" @@ -11,9 +12,15 @@ import ( "github.com/awf-project/cli/internal/application" "github.com/awf-project/cli/internal/domain/workflow" + "github.com/awf-project/cli/internal/infrastructure/roles" + "github.com/awf-project/cli/internal/testutil" "github.com/awf-project/cli/internal/testutil/mocks" + "github.com/awf-project/cli/pkg/interpolation" ) +// errAssert is a sentinel error for resolver-failure test cases. +var errAssert = errors.New("resolver failure") + func TestResolveAgentRole_EmptyRef(t *testing.T) { repo := &mocks.MockAgentRoleRepository{} ctx := context.Background() @@ -228,3 +235,209 @@ func TestResolveAgentRole_ContextError(t *testing.T) { require.Error(t, err) assert.ErrorIs(t, err, context.Canceled) } + +// TestResolveAgentRole_ByNameIgnoresWorkflowDir documents FR-008 / SC-005: +// a bare name ref searches CWD-local paths only; workflowDir is not consulted. +func TestResolveAgentRole_ByNameIgnoresWorkflowDir(t *testing.T) { + projectDir := t.TempDir() + workflowDir := t.TempDir() // deliberately distinct from projectDir + + localRoleDir := filepath.Join(projectDir, ".awf", "roles", "go-senior") + require.NoError(t, os.MkdirAll(localRoleDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(localRoleDir, "AGENTS.md"), + []byte("---\n---\nCWD-local go-senior content"), + 0o644, + )) + + testutil.ChdirIsolated(t, projectDir) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + ctx := context.Background() + + role, err := application.ResolveAgentRole(ctx, repo, "go-senior", workflowDir) + + require.NoError(t, err) + require.NotNil(t, role) + assert.Equal(t, "go-senior", role.Name) + assert.Contains(t, role.Content, "CWD-local go-senior content") +} + +// TestExpandRolePath_RelativeToWorkflowDir documents FR-009: +// a ./... ref is joined onto workflowDir, not CWD. +func TestExpandRolePath_RelativeToWorkflowDir(t *testing.T) { + projectDir := t.TempDir() // CWD — no roles here + workflowDir := t.TempDir() // has the role + + roleDir := filepath.Join(workflowDir, "roles", "go-remote") + require.NoError(t, os.MkdirAll(roleDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(roleDir, "AGENTS.md"), + []byte("---\n---\nWorkflowDir-relative go-remote content"), + 0o644, + )) + + testutil.ChdirIsolated(t, projectDir) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + ctx := context.Background() + + role, err := application.ResolveAgentRole(ctx, repo, "./roles/go-remote", workflowDir) + + require.NoError(t, err) + require.NotNil(t, role) + assert.Equal(t, "go-remote", role.Name) + assert.Contains(t, role.Content, "WorkflowDir-relative go-remote content") +} + +// TestExpandRolePath_RelativeDoesNotUseCWD documents FR-009 negative case: +// even when the role exists at CWD/./..., it must not be resolved because workflowDir +// is authoritative for path refs. +func TestExpandRolePath_RelativeDoesNotUseCWD(t *testing.T) { + projectDir := t.TempDir() // CWD — role exists here but must NOT be used + workflowDir := t.TempDir() // workflow location — no roles here + + cwdRoleDir := filepath.Join(projectDir, "roles", "go-remote") + require.NoError(t, os.MkdirAll(cwdRoleDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(cwdRoleDir, "AGENTS.md"), + []byte("---\n---\nCWD-only content"), + 0o644, + )) + + testutil.ChdirIsolated(t, projectDir) + + repo := roles.NewFilesystemAgentRoleRepository(nil) + ctx := context.Background() + + // resolves to workflowDir/roles/go-remote which does not exist + _, err := application.ResolveAgentRole(ctx, repo, "./roles/go-remote", workflowDir) + + require.Error(t, err) + var notFoundErr *workflow.AgentRoleNotFoundError + assert.ErrorAs(t, err, ¬FoundErr) +} + +// roleErrResolver implements interpolation.Resolver and always fails — used to +// exercise the template-resolution error branch of BuildRoleSystemPrompt. +type roleErrResolver struct{ err error } + +func (r *roleErrResolver) Resolve(string, *interpolation.Context) (string, error) { + return "", r.err +} + +func agentStep(role, systemPrompt string) *workflow.Step { + return &workflow.Step{Agent: &workflow.AgentConfig{Role: role, SystemPrompt: systemPrompt}} +} + +func TestBuildRoleSystemPrompt_NilAgentReturnsEmpty(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{} + resolver := &testResolverWithValues{} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, &workflow.Step{}, "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Empty(t, got) +} + +func TestBuildRoleSystemPrompt_NoRoleReturnsInlineOnly(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{} + resolver := &testResolverWithValues{} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("", "be concise"), "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Equal(t, "be concise", got) +} + +func TestBuildRoleSystemPrompt_ByNameComposesWithInline(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(_ context.Context, name string) (*workflow.AgentRole, error) { + assert.Equal(t, "go-senior", name) + return &workflow.AgentRole{Name: name, Content: "expert in golang"}, nil + }, + } + resolver := &testResolverWithValues{} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("go-senior", "be concise"), "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Equal(t, "expert in golang\n\nbe concise", got) +} + +func TestBuildRoleSystemPrompt_ByNameRoleOnly(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(_ context.Context, name string) (*workflow.AgentRole, error) { + return &workflow.AgentRole{Name: name, Content: "expert in golang"}, nil + }, + } + resolver := &testResolverWithValues{} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("go-senior", ""), "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Equal(t, "expert in golang", got) +} + +func TestBuildRoleSystemPrompt_TemplateInterpolatedBeforeLookup(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(_ context.Context, name string) (*workflow.AgentRole, error) { + assert.Equal(t, "go-senior", name, "role template must be resolved before repo lookup") + return &workflow.AgentRole{Name: name, Content: "expert"}, nil + }, + } + resolver := &testResolverWithValues{values: map[string]string{"{{inputs.persona}}": "go-senior"}} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("{{inputs.persona}}", ""), "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Equal(t, "expert", got) +} + +func TestBuildRoleSystemPrompt_NilRepoWithRoleErrors(t *testing.T) { + resolver := &testResolverWithValues{} + + _, err := application.BuildRoleSystemPrompt(context.Background(), nil, resolver, agentStep("go-senior", ""), "/dir", interpolation.NewContext()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "go-senior") +} + +func TestBuildRoleSystemPrompt_TemplateResolveErrorPropagates(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{} + resolver := &roleErrResolver{err: errAssert} + + _, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("{{bad}}", ""), "/dir", interpolation.NewContext()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "resolve role template") +} + +func TestBuildRoleSystemPrompt_RepoErrorPropagates(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(context.Context, string) (*workflow.AgentRole, error) { + return nil, &workflow.AgentRoleNotFoundError{Name: "missing", SearchPaths: []string{".awf/roles"}} + }, + } + resolver := &testResolverWithValues{} + + _, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("missing", ""), "/dir", interpolation.NewContext()) + + require.Error(t, err) + var notFound *workflow.AgentRoleNotFoundError + assert.ErrorAs(t, err, ¬Found) +} + +func TestBuildRoleSystemPrompt_NilRoleFallsBackToInline(t *testing.T) { + repo := &mocks.MockAgentRoleRepository{ + LoadFunc: func(context.Context, string) (*workflow.AgentRole, error) { + return nil, nil // resolver returned no role + }, + } + resolver := &testResolverWithValues{} + + got, err := application.BuildRoleSystemPrompt(context.Background(), repo, resolver, agentStep("ghost", "inline only"), "/dir", interpolation.NewContext()) + + require.NoError(t, err) + assert.Equal(t, "inline only", got) +} diff --git a/internal/domain/workflow/agent_role.go b/internal/domain/workflow/agent_role.go index bd5c5599..9efe8f97 100644 --- a/internal/domain/workflow/agent_role.go +++ b/internal/domain/workflow/agent_role.go @@ -5,11 +5,20 @@ import ( "strings" ) +// AgentRoleSizeWarnBytes is the AGENTS.md size (in bytes) above which a +// context-window warning is emitted. Single source of truth for both the +// infrastructure loader and the CLI validator. +const AgentRoleSizeWarnBytes = 500 * 1024 + // AgentRole holds the loaded content and metadata for an agent role. type AgentRole struct { Name string SourcePath string Content string + // RawSizeBytes is the size of the AGENTS.md file as read from disk (before + // frontmatter stripping). Captured at load time so consumers can warn on + // oversized files without re-stat'ing the source. + RawSizeBytes int64 } // AgentRoleNotFoundError is returned when an agent role cannot be resolved by name or path. @@ -17,6 +26,12 @@ type AgentRoleNotFoundError struct { Name string SearchPaths []string Underlying error + // IsPathRef indicates whether the lookup was triggered by a path-based reference + // (LoadFromPath) rather than a symbolic name (Load). Consumers such as the CLI + // validator use this to distinguish "role directory exists but lacks AGENTS.md" + // from "role name not found in search paths", without relying on the number of + // search paths as a heuristic. + IsPathRef bool } func (e *AgentRoleNotFoundError) Error() string { diff --git a/internal/infrastructure/roles/doc.go b/internal/infrastructure/roles/doc.go new file mode 100644 index 00000000..bc374827 --- /dev/null +++ b/internal/infrastructure/roles/doc.go @@ -0,0 +1,125 @@ +// Package roles implements the AgentRoleRepository port for the AWF CLI. +// +// # Overview +// +// This package provides a filesystem-backed repository for discovering and loading +// agent role definitions from AGENTS.md files. Agent roles define who an AI agent +// is (its persona, expertise, and behavior) as opposed to skills, which define what +// it knows. Roles are loaded synchronously and deterministically at workflow +// validation or execution time. +// +// # Search Strategy +// +// The repository follows a multi-path search strategy identical in structure to the +// skills repository. When the AWF_ROLES_PATH environment variable is set, it acts as +// an exclusive override: only the colon-separated list of paths in that variable is +// searched, and the default chain is bypassed entirely. +// +// When AWF_ROLES_PATH is not set, four directories are searched in strict priority +// order (first match wins): +// +// 1. .awf/roles/ — AWF-native local directory (project-level, highest priority) +// 2. .agents/roles/ — cross-client local directory (compatible with other agent tooling) +// 3. $XDG_CONFIG_HOME/awf/roles/ — AWF-native global directory (respects XDG Base Dir spec) +// 4. ~/.agents/roles/ — cross-client global directory (lowest priority) +// +// Paths 1 and 2 are relative to the current working directory (the project root when +// executing awf). Paths 3 and 4 are absolute paths derived from the user environment. +// If UserHomeDir returns an error for path 4, the path is silently omitted rather than +// producing a parasitic join such as "/.agents/roles". +// +// # AWF_ROLES_PATH Environment Variable +// +// Setting AWF_ROLES_PATH replaces the entire default search chain: +// +// export AWF_ROLES_PATH=/team/roles:/opt/shared/roles +// +// Multiple paths are separated by the OS path list separator (colon on Unix, +// semicolon on Windows). When this variable is set, the four default paths above +// are not consulted at all. To restore the default chain, unset the variable or +// set it to the empty string. +// +// # AGENTS.md File Format +// +// Each role is a directory containing a single AGENTS.md file. The directory name +// becomes the role name. The AGENTS.md file may optionally begin with a YAML +// frontmatter block delimited by --- lines: +// +// --- +// title: Senior Go Engineer +// tags: [go, backend] +// --- +// You are a senior Go engineer with 10+ years of experience... +// +// The frontmatter block is stripped by skills.StripFrontmatter before the content is +// returned to callers. Only the body (everything after the closing ---) is exposed as +// the role content. If no frontmatter is present, the entire file content is returned. +// +// # Dependency on infra-skills +// +// This package reuses skills.StripFrontmatter to strip YAML frontmatter from +// AGENTS.md files. This is an intentional architectural coupling: the frontmatter +// format is identical for both SKILL.md and AGENTS.md, and duplicating the parsing +// logic would violate the single source of truth principle. The dependency is +// declared in .go-arch-lint.yml under the infra-roles component. +// +// # Error Types +// +// The repository returns typed errors from the domain layer: +// +// - *workflow.AgentRoleNotFoundError: returned when a role cannot be located in any +// of the configured search paths. The error includes the role name and the full list +// of paths that were searched, enabling callers to produce actionable diagnostics. +// +// A plain fmt.Errorf (wrapped with %w) is returned for lower-level I/O failures such +// as permission errors when reading an AGENTS.md file that was located via stat. +// +// # Name Validation +// +// The Load method rejects role names containing path separators (/ or \) or the +// double-dot sequence (..) to prevent path traversal attacks. These checks are +// defense-in-depth: the application layer (ResolveAgentRole) and the CLI validator +// also perform independent path traversal rejection. +// +// # Size Warning +// +// When a loaded AGENTS.md file exceeds workflow.AgentRoleSizeWarnBytes (500 KB), a +// structured warning is emitted via the injected ports.Logger. The warning includes +// the file path and size as structured fields and a human-readable message. This +// threshold is defined in the domain layer (domain/workflow/agent_role.go) so that +// both the infrastructure loader and the CLI validator share a single constant. +// +// RawSizeBytes on the returned AgentRole captures the file size before frontmatter +// stripping, enabling CLI-layer size checks without re-reading the file. +// +// # Concurrency +// +// The repository is stateless after construction: all search paths are resolved and +// frozen in NewFilesystemAgentRoleRepository. Individual Load and LoadFromPath calls +// are safe to invoke from concurrent goroutines because they do not mutate shared +// state — they only perform filesystem reads and construct new AgentRole values. +// +// # Relationship to Skills Repository +// +// The roles repository mirrors the design of the skills repository +// (internal/infrastructure/skills). Both repositories: +// +// - Construct a search path slice at init time from an env var or defaults +// - Look for a well-known file (AGENTS.md vs SKILL.md) inside a named subdirectory +// - Strip YAML frontmatter before returning content +// - Return typed NotFoundError with SearchPaths for diagnostics +// - Emit a structured size warning via an injected logger +// +// The parallel design is intentional to keep both mechanisms consistent for operators +// and contributors. +// +// # Architecture Constraints +// +// See .go-arch-lint.yml for the declared dependency rules of infra-roles: +// +// - Allowed: domain-workflow, domain-ports, infra-skills, infra-xdg +// - Allowed vendors: go-stdlib only +// +// This package must not import application, interfaces, or any external vendor +// libraries. +package roles diff --git a/internal/infrastructure/roles/filesystem_repository.go b/internal/infrastructure/roles/filesystem_repository.go index b62b7304..fe817163 100644 --- a/internal/infrastructure/roles/filesystem_repository.go +++ b/internal/infrastructure/roles/filesystem_repository.go @@ -21,14 +21,19 @@ type FilesystemAgentRoleRepository struct { func NewFilesystemAgentRoleRepository(logger ports.Logger) *FilesystemAgentRoleRepository { var paths []string - if envPath := os.Getenv("AWF_AGENTS_PATH"); envPath != "" { + if envPath := os.Getenv("AWF_ROLES_PATH"); envPath != "" { paths = filepath.SplitList(envPath) } else { + // Two namespaces are searched, mirroring the skills repository: + // - AWF-native: .awf/roles (local), $XDG_CONFIG_HOME/awf/roles (global) + // - cross-client: .agents/roles (local), ~/.agents/roles (global) + // The cross-client paths let roles authored for other agent tooling be + // reused without duplication. candidates := []string{ - xdg.LocalAgentsDir(), - ".agents", - xdg.AWFAgentsDir(), - crossClientGlobalAgentsDir(), + xdg.LocalRolesDir(), + ".agents/roles", + xdg.AWFRolesDir(), + crossClientGlobalRolesDir(), } for _, p := range candidates { if p != "" { @@ -75,7 +80,7 @@ func (r *FilesystemAgentRoleRepository) LoadFromPath(_ context.Context, absolute } if _, err := os.Stat(agentsFile); err != nil { - return nil, &workflow.AgentRoleNotFoundError{Name: filepath.Base(cleanPath), SearchPaths: []string{cleanPath}} + return nil, &workflow.AgentRoleNotFoundError{Name: filepath.Base(cleanPath), SearchPaths: []string{cleanPath}, IsPathRef: true} } return r.loadAgentsMD(dirPath) @@ -89,8 +94,11 @@ func (r *FilesystemAgentRoleRepository) loadAgentsMD(dirPath string) (*workflow. 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)) + if r.logger != nil && len(data) > workflow.AgentRoleSizeWarnBytes { + r.logger.Warn( + fmt.Sprintf("AGENTS.md exceeds %dKB, may impact context window", workflow.AgentRoleSizeWarnBytes/1024), + "path", agentsFile, "size", len(data), + ) } content := skills.StripFrontmatter(string(data)) @@ -101,16 +109,17 @@ func (r *FilesystemAgentRoleRepository) loadAgentsMD(dirPath string) (*workflow. } return &workflow.AgentRole{ - Name: filepath.Base(dirPath), - Content: content, - SourcePath: absPath, + Name: filepath.Base(dirPath), + Content: content, + SourcePath: absPath, + RawSizeBytes: int64(len(data)), }, nil } -func crossClientGlobalAgentsDir() string { +func crossClientGlobalRolesDir() string { home, err := os.UserHomeDir() if err != nil { return "" } - return filepath.Join(home, ".agents") + return filepath.Join(home, ".agents", "roles") } diff --git a/internal/infrastructure/roles/filesystem_repository_test.go b/internal/infrastructure/roles/filesystem_repository_test.go index 2b9920aa..59ea48d7 100644 --- a/internal/infrastructure/roles/filesystem_repository_test.go +++ b/internal/infrastructure/roles/filesystem_repository_test.go @@ -15,6 +15,7 @@ import ( "github.com/awf-project/cli/internal/domain/ports" "github.com/awf-project/cli/internal/domain/workflow" "github.com/awf-project/cli/internal/infrastructure/roles" + "github.com/awf-project/cli/internal/testutil" ) // mockLogger captures Warn calls for testing @@ -43,8 +44,7 @@ func TestNewFilesystemAgentRoleRepository(t *testing.T) { func TestLoad_PriorityOrder(t *testing.T) { tmpDir := t.TempDir() - // Setup: Create agents in different search paths - localDir := filepath.Join(tmpDir, ".awf", "agents", "go-senior") + localDir := filepath.Join(tmpDir, ".awf", "roles", "go-senior") require.NoError(t, os.MkdirAll(localDir, 0o755)) require.NoError(t, os.WriteFile( filepath.Join(localDir, "AGENTS.md"), @@ -52,7 +52,7 @@ func TestLoad_PriorityOrder(t *testing.T) { 0o644, )) - crossClientDir := filepath.Join(tmpDir, ".agents", "go-senior") + crossClientDir := filepath.Join(tmpDir, ".agents", "roles", "go-senior") require.NoError(t, os.MkdirAll(crossClientDir, 0o755)) require.NoError(t, os.WriteFile( filepath.Join(crossClientDir, "AGENTS.md"), @@ -61,10 +61,7 @@ func TestLoad_PriorityOrder(t *testing.T) { )) 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)) + testutil.ChdirIsolated(t, tmpDir) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -80,13 +77,9 @@ func TestLoad_PriorityOrder(t *testing.T) { }) 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)) + testutil.ChdirIsolated(t, tmpDir) - // Remove local agent, should find cross-client - require.NoError(t, os.RemoveAll(filepath.Join(tmpDir, ".awf", "agents", "go-senior"))) + require.NoError(t, os.RemoveAll(filepath.Join(tmpDir, ".awf", "roles", "go-senior"))) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -99,7 +92,7 @@ func TestLoad_PriorityOrder(t *testing.T) { }) } -func TestLoad_AWFAgentsPathOverride(t *testing.T) { +func TestLoad_AWFRolesPathOverride(t *testing.T) { tmpDir := t.TempDir() overridePath := filepath.Join(tmpDir, "override") @@ -111,7 +104,7 @@ func TestLoad_AWFAgentsPathOverride(t *testing.T) { 0o644, )) - localAgentDir := filepath.Join(tmpDir, ".awf", "agents", "custom-role") + localAgentDir := filepath.Join(tmpDir, ".awf", "roles", "custom-role") require.NoError(t, os.MkdirAll(localAgentDir, 0o755)) require.NoError(t, os.WriteFile( filepath.Join(localAgentDir, "AGENTS.md"), @@ -119,13 +112,10 @@ func TestLoad_AWFAgentsPathOverride(t *testing.T) { 0o644, )) - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) - t.Run("AWF_AGENTS_PATH exclusive search when set", func(t *testing.T) { - t.Setenv("AWF_AGENTS_PATH", overridePath) + t.Run("AWF_ROLES_PATH exclusive search when set", func(t *testing.T) { + t.Setenv("AWF_ROLES_PATH", overridePath) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -136,12 +126,26 @@ func TestLoad_AWFAgentsPathOverride(t *testing.T) { 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", "") + t.Run("default search when AWF_ROLES_PATH empty", func(t *testing.T) { + t.Setenv("AWF_ROLES_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") + }) + + t.Run("AWF_AGENTS_PATH ignored when AWF_ROLES_PATH unset", func(t *testing.T) { + t.Setenv("AWF_ROLES_PATH", "") + t.Setenv("AWF_AGENTS_PATH", overridePath) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) + // Should use default roles/ chain, not the agents override role, err := repo.Load(context.Background(), "custom-role") require.NoError(t, err) @@ -149,6 +153,66 @@ func TestLoad_AWFAgentsPathOverride(t *testing.T) { }) } +func TestLoad_OldAgentsPathNotFound(t *testing.T) { + tmpDir := t.TempDir() + + // Role exists at old agents/ location only — must NOT be found + oldAgentDir := filepath.Join(tmpDir, ".agents", "legacy-role") + require.NoError(t, os.MkdirAll(oldAgentDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(oldAgentDir, "AGENTS.md"), + []byte("---\n---\nOld agents path content"), + 0o644, + )) + + testutil.ChdirIsolated(t, tmpDir) + + t.Setenv("AWF_ROLES_PATH", "") + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "legacy-role") + + require.Error(t, err) + assert.Nil(t, role) + + var notFoundErr *workflow.AgentRoleNotFoundError + require.ErrorAs(t, err, ¬FoundErr) + assert.Equal(t, "legacy-role", notFoundErr.Name) +} + +func TestLoad_SkillsRoleNoCollision(t *testing.T) { + tmpDir := t.TempDir() + + // Role named "skills" under the roles/ namespace + skillsRoleDir := filepath.Join(tmpDir, ".agents", "roles", "skills") + require.NoError(t, os.MkdirAll(skillsRoleDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(skillsRoleDir, "AGENTS.md"), + []byte("---\n---\nSkills agent role content"), + 0o644, + )) + + // A skills/ container at the old .agents/ level — must not interfere + skillsContainerDir := filepath.Join(tmpDir, ".agents", "skills") + require.NoError(t, os.MkdirAll(skillsContainerDir, 0o755)) + + testutil.ChdirIsolated(t, tmpDir) + + t.Setenv("AWF_ROLES_PATH", "") + + logger := &mockLogger{} + repo := roles.NewFilesystemAgentRoleRepository(logger) + + role, err := repo.Load(context.Background(), "skills") + + require.NoError(t, err) + assert.NotNil(t, role) + assert.Equal(t, "skills", role.Name) + assert.Contains(t, role.Content, "Skills agent role content") +} + func TestLoad_PathTraversalRejection(t *testing.T) { logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -173,7 +237,6 @@ func TestLoad_PathTraversalRejection(t *testing.T) { 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 @@ -186,10 +249,7 @@ func TestLoad_PathTraversalRejection(t *testing.T) { 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)) + testutil.ChdirIsolated(t, tmpDir) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -209,7 +269,7 @@ func TestLoad_MissingFile(t *testing.T) { func TestLoad_FrontmatterStripping(t *testing.T) { tmpDir := t.TempDir() - agentDir := filepath.Join(tmpDir, ".awf", "agents", "test-role") + agentDir := filepath.Join(tmpDir, ".awf", "roles", "test-role") require.NoError(t, os.MkdirAll(agentDir, 0o755)) content := `--- @@ -227,10 +287,7 @@ This is the actual content after frontmatter.` 0o644, )) - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -247,10 +304,9 @@ This is the actual content after frontmatter.` func TestLoad_LargeFileWarning(t *testing.T) { tmpDir := t.TempDir() - agentDir := filepath.Join(tmpDir, ".awf", "agents", "large-role") + agentDir := filepath.Join(tmpDir, ".awf", "roles", "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"), @@ -258,10 +314,7 @@ func TestLoad_LargeFileWarning(t *testing.T) { 0o644, )) - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -271,16 +324,16 @@ func TestLoad_LargeFileWarning(t *testing.T) { 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")) + wantThreshold := fmt.Sprintf("exceeds %dKB", workflow.AgentRoleSizeWarnBytes/1024) + assert.Contains(t, logger.warnings[0], wantThreshold) } func TestLoad_PerformanceSub100KB(t *testing.T) { tmpDir := t.TempDir() - agentDir := filepath.Join(tmpDir, ".awf", "agents", "perf-role") + agentDir := filepath.Join(tmpDir, ".awf", "roles", "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"), @@ -288,10 +341,7 @@ func TestLoad_PerformanceSub100KB(t *testing.T) { 0o644, )) - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -370,30 +420,30 @@ func TestLoadFromPath_Missing(t *testing.T) { func TestSearchPaths_LocalToGlobal(t *testing.T) { tmpDir := t.TempDir() - // Create agents at different priority levels + // F100 removed the "crossglobal" case ($HOME/.agents/) in favor of the + // roles/-namespaced path $HOME/.agents/roles/. Old flat paths directly + // under ~/.agents/ are no longer searched; users must migrate to the roles/ sub- + // namespace. The four active search positions are: .awf/roles (local, highest + // priority), .agents/roles (cross-client local), $XDG_CONFIG_HOME/awf/roles + // (AWF-native global), and ~/.agents/roles (cross-client global). 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"), + "local": filepath.Join(tmpDir, ".awf", "roles", "priority-role"), + "cross": filepath.Join(tmpDir, ".agents", "roles", "priority-role"), + "global": filepath.Join(tmpDir, ".config", "awf", "roles", "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)), + fmt.Appendf(nil, "---\n---\nContent from %s", p), 0o644, )) } - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) - // Unset AWF_AGENTS_PATH to use default search - t.Setenv("AWF_AGENTS_PATH", "") + t.Setenv("AWF_ROLES_PATH", "") logger := &mockLogger{} repo := roles.NewFilesystemAgentRoleRepository(logger) @@ -402,14 +452,13 @@ func TestSearchPaths_LocalToGlobal(t *testing.T) { 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") + agentDir := filepath.Join(tmpDir, ".awf", "roles", "safe-role") require.NoError(t, os.MkdirAll(agentDir, 0o755)) require.NoError(t, os.WriteFile( filepath.Join(agentDir, "AGENTS.md"), @@ -417,12 +466,8 @@ func TestLoad_NilLogger(t *testing.T) { 0o644, )) - origDir, err := os.Getwd() - require.NoError(t, err) - defer os.Chdir(origDir) - require.NoError(t, os.Chdir(tmpDir)) + testutil.ChdirIsolated(t, tmpDir) - // Should not panic with nil logger repo := roles.NewFilesystemAgentRoleRepository(nil) role, err := repo.Load(context.Background(), "safe-role") diff --git a/internal/infrastructure/skills/filesystem_repository.go b/internal/infrastructure/skills/filesystem_repository.go index c1e92515..15ca30b9 100644 --- a/internal/infrastructure/skills/filesystem_repository.go +++ b/internal/infrastructure/skills/filesystem_repository.go @@ -13,6 +13,12 @@ import ( "github.com/awf-project/cli/internal/infrastructure/xdg" ) +// skillSizeWarnBytes is the SKILL.md size (in bytes) above which a +// context-window warning is emitted. Intentionally separate from +// workflow.AgentRoleSizeWarnBytes to allow independent evolution of the two +// thresholds without coupling the skills and roles domains. +const skillSizeWarnBytes = 500 * 1024 + type FilesystemSkillRepository struct { searchPaths []string logger ports.Logger @@ -24,7 +30,7 @@ func NewFilesystemSkillRepository(logger ports.Logger) *FilesystemSkillRepositor if envPath := os.Getenv("AWF_SKILLS_PATH"); envPath != "" { paths = filepath.SplitList(envPath) } else { - paths = []string{ + candidates := []string{ xdg.LocalSkillsDir(), ".agents/skills", ".claude/skills", @@ -32,6 +38,11 @@ func NewFilesystemSkillRepository(logger ports.Logger) *FilesystemSkillRepositor crossClientGlobalSkillsDir(), claudeGlobalSkillsDir(), } + for _, p := range candidates { + if p != "" { + paths = append(paths, p) + } + } } return &FilesystemSkillRepository{ @@ -41,7 +52,9 @@ func NewFilesystemSkillRepository(logger ports.Logger) *FilesystemSkillRepositor } func (r *FilesystemSkillRepository) Load(_ context.Context, name string) (*workflow.Skill, error) { - if strings.ContainsRune(name, filepath.Separator) || strings.Contains(name, "..") { + // Reject both / and \ regardless of OS, and double-dot sequences, to prevent + // path traversal. Using ContainsAny mirrors the roles repository for consistency. + if strings.ContainsAny(name, `/\`) || strings.Contains(name, "..") { return nil, fmt.Errorf("invalid skill name %q: must not contain path separators or '..'", name) } @@ -78,8 +91,11 @@ func (r *FilesystemSkillRepository) loadSkillFromDir(dirPath string) (*workflow. return nil, fmt.Errorf("reading SKILL.md in %s: %w", dirPath, err) } - if r.logger != nil && len(data) > 500*1024 { - r.logger.Warn("SKILL.md exceeds 500KB, may impact context window", "path", skillFile, "size", len(data)) + if r.logger != nil && len(data) > skillSizeWarnBytes { + r.logger.Warn( + fmt.Sprintf("SKILL.md exceeds %dKB, may impact context window", skillSizeWarnBytes/1024), + "path", skillFile, "size", len(data), + ) } content := StripFrontmatter(string(data)) @@ -139,11 +155,17 @@ func (r *FilesystemSkillRepository) enumerateResources(dirPath string) ([]string } func crossClientGlobalSkillsDir() string { - home, _ := os.UserHomeDir() + home, err := os.UserHomeDir() + if err != nil { + return "" + } return filepath.Join(home, ".agents", "skills") } func claudeGlobalSkillsDir() string { - home, _ := os.UserHomeDir() + home, err := os.UserHomeDir() + if err != nil { + return "" + } return filepath.Join(home, ".claude", "skills") } diff --git a/internal/infrastructure/skills/filesystem_repository_test.go b/internal/infrastructure/skills/filesystem_repository_test.go index 7bc6fc14..b77d9360 100644 --- a/internal/infrastructure/skills/filesystem_repository_test.go +++ b/internal/infrastructure/skills/filesystem_repository_test.go @@ -389,7 +389,7 @@ func TestResourceEnumeration_Exactly4LevelsBoundary(t *testing.T) { for i := 1; i <= 4; i++ { path = filepath.Join(path, fmt.Sprintf("level%d", i)) require.NoError(t, os.MkdirAll(path, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(path, fmt.Sprintf("file%d.txt", i)), []byte(fmt.Sprintf("content%d", i)), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(path, fmt.Sprintf("file%d.txt", i)), fmt.Appendf(nil, "content%d", i), 0o644)) } repo := NewFilesystemSkillRepository(&mockLogger{}) @@ -413,7 +413,7 @@ func TestResourceEnumeration_Exceeds5LevelExcluded(t *testing.T) { for i := 1; i <= 5; i++ { path = filepath.Join(path, fmt.Sprintf("level%d", i)) require.NoError(t, os.MkdirAll(path, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(path, fmt.Sprintf("file%d.txt", i)), []byte(fmt.Sprintf("content%d", i)), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(path, fmt.Sprintf("file%d.txt", i)), fmt.Appendf(nil, "content%d", i), 0o644)) } repo := NewFilesystemSkillRepository(&mockLogger{}) @@ -620,6 +620,32 @@ func TestLoadSkillFromDir_UnreadableSKILLMD(t *testing.T) { assert.False(t, errors.As(err, ¬Found), "read failure should not be misreported as SkillNotFoundError") } +// TestLoad_NameValidation verifies that skill names containing path separators +// or double-dot sequences are rejected to prevent path traversal. The validation +// mirrors the roles repository (infra-roles) which rejects both / and \ explicitly. +func TestLoad_NameValidation(t *testing.T) { + repo := NewFilesystemSkillRepository(&mockLogger{}) + + tests := []struct { + name string + input string + }{ + {"forward slash", "foo/bar"}, + {"backslash", `foo\bar`}, + {"dot-dot", "../escape"}, + {"nested dot-dot", "foo/../bar"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + skill, err := repo.Load(context.Background(), tt.input) + assert.Error(t, err, "name %q should be rejected", tt.input) + assert.Nil(t, skill) + assert.ErrorContains(t, err, "invalid") + }) + } +} + func TestEnumerateResources_InaccessibleSubdir(t *testing.T) { // Covers the if err != nil { return nil } branch in the enumerateResources walk callback. // WalkDir calls the callback a second time with the permission error when it cannot diff --git a/internal/infrastructure/xdg/xdg.go b/internal/infrastructure/xdg/xdg.go index b388be2d..a7ceefbe 100644 --- a/internal/infrastructure/xdg/xdg.go +++ b/internal/infrastructure/xdg/xdg.go @@ -5,21 +5,31 @@ import ( "path/filepath" ) -// ConfigHome returns XDG_CONFIG_HOME or defaults to ~/.config +// ConfigHome returns XDG_CONFIG_HOME or defaults to ~/.config. +// Returns "" if XDG_CONFIG_HOME is not set and the user home directory +// cannot be determined, rather than producing a parasitic path such as "/.config". func ConfigHome() string { if dir := os.Getenv("XDG_CONFIG_HOME"); dir != "" { return dir } - home, _ := os.UserHomeDir() + home, err := os.UserHomeDir() + if err != nil { + return "" + } return filepath.Join(home, ".config") } -// DataHome returns XDG_DATA_HOME or defaults to ~/.local/share +// DataHome returns XDG_DATA_HOME or defaults to ~/.local/share. +// Returns "" if XDG_DATA_HOME is not set and the user home directory +// cannot be determined, rather than producing a parasitic path such as "/.local/share". func DataHome() string { if dir := os.Getenv("XDG_DATA_HOME"); dir != "" { return dir } - home, _ := os.UserHomeDir() + home, err := os.UserHomeDir() + if err != nil { + return "" + } return filepath.Join(home, ".local", "share") } @@ -93,14 +103,14 @@ 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") +// AWFRolesDir returns the global roles directory ($XDG_CONFIG_HOME/awf/roles) +func AWFRolesDir() string { + return filepath.Join(AWFConfigDir(), "roles") } -// LocalAgentsDir returns the local project agents directory -func LocalAgentsDir() string { - return ".awf/agents" +// LocalRolesDir returns the local project roles directory +func LocalRolesDir() string { + return ".awf/roles" } // AWFPluginsDir returns the global plugins directory ($XDG_DATA_HOME/awf/plugins) @@ -124,7 +134,7 @@ func LocalWorkflowPacksDir() string { } // AWFPaths returns all AWF XDG directory paths as a map for template interpolation. -// Keys: prompts_dir, scripts_dir, config_dir, data_dir, workflows_dir, plugins_dir, skills_dir. +// Keys: prompts_dir, scripts_dir, config_dir, data_dir, workflows_dir, plugins_dir, skills_dir, roles_dir. func AWFPaths() map[string]string { return map[string]string{ "prompts_dir": AWFPromptsDir(), @@ -134,6 +144,7 @@ func AWFPaths() map[string]string { "workflows_dir": AWFWorkflowsDir(), "plugins_dir": AWFPluginsDir(), "skills_dir": AWFSkillsDir(), + "roles_dir": AWFRolesDir(), } } diff --git a/internal/infrastructure/xdg/xdg_roles_dir_test.go b/internal/infrastructure/xdg/xdg_roles_dir_test.go new file mode 100644 index 00000000..a1381ba3 --- /dev/null +++ b/internal/infrastructure/xdg/xdg_roles_dir_test.go @@ -0,0 +1,65 @@ +// Package xdg — tests dedicated to the roles namespace directory resolution. +// +// These tests are kept in a separate file from xdg_test.go because they cover +// the AWFRolesDir and LocalRolesDir helpers introduced by F100 (dedicated roles +// namespace). Isolating them makes it easy to locate tests for the +// AWF_CONFIG_HOME / XDG_CONFIG_HOME priority chain and the AWF_ROLES_PATH +// override, as well as the structural parity assertion between LocalRolesDir +// and LocalSkillsDir. +package xdg + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAWFRolesDir(t *testing.T) { + home, err := os.UserHomeDir() + require.NoError(t, err) + + tests := []struct { + name string + awfConfigHome string + xdgConfigHome string + want string + }{ + { + name: "uses XDG_CONFIG_HOME when set", + xdgConfigHome: "/custom/config", + want: "/custom/config/awf/roles", + }, + { + name: "defaults to ~/.config/awf/roles when unset", + want: filepath.Join(home, ".config", "awf", "roles"), + }, + { + name: "AWF_CONFIG_HOME takes priority over XDG_CONFIG_HOME", + awfConfigHome: "/custom/awf-home", + xdgConfigHome: "/some/xdg/config", + want: "/custom/awf-home/roles", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("AWF_CONFIG_HOME", tt.awfConfigHome) + t.Setenv("XDG_CONFIG_HOME", tt.xdgConfigHome) + + got := AWFRolesDir() + assert.Equal(t, tt.want, got) + }) + } +} + +func TestLocalRolesDir(t *testing.T) { + got := LocalRolesDir() + + assert.Equal(t, ".awf/roles", got) + assert.False(t, filepath.IsAbs(got), "LocalRolesDir must be a project-relative path") + // Structural parity with the skills helper it mirrors. + assert.Equal(t, filepath.Dir(LocalSkillsDir()), filepath.Dir(got), "roles and skills local dirs share the same base") +} diff --git a/internal/infrastructure/xdg/xdg_test.go b/internal/infrastructure/xdg/xdg_test.go index bcfd0d7c..10c7592e 100644 --- a/internal/infrastructure/xdg/xdg_test.go +++ b/internal/infrastructure/xdg/xdg_test.go @@ -33,7 +33,6 @@ func TestConfigHome(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Save and restore env if tt.envValue != "" { t.Setenv("XDG_CONFIG_HOME", tt.envValue) } else { @@ -534,6 +533,7 @@ func TestAWFPaths(t *testing.T) { "workflows_dir": AWFWorkflowsDir(), "plugins_dir": AWFPluginsDir(), "skills_dir": AWFSkillsDir(), + "roles_dir": AWFRolesDir(), } for key, want := range expected { got, ok := paths[key] @@ -557,6 +557,7 @@ func TestAWFPaths_HasExactKeys(t *testing.T) { "workflows_dir", "plugins_dir", "skills_dir", + "roles_dir", } assert.Len(t, paths, len(expectedKeys), "AWFPaths() should have exactly %d keys", len(expectedKeys)) for _, key := range expectedKeys { diff --git a/internal/interfaces/cli/validate.go b/internal/interfaces/cli/validate.go index 2033a658..ffde91fd 100644 --- a/internal/interfaces/cli/validate.go +++ b/internal/interfaces/cli/validate.go @@ -22,6 +22,12 @@ import ( "github.com/spf13/cobra" ) +// agentRoleCombinedPromptWarnBytes is the combined size (role content + +// inline system_prompt, in bytes) above which a non-blocking validation warning +// is emitted. This threshold is a CLI-layer concern: it guards against oversized +// system prompt payloads before execution, not a domain invariant. +const agentRoleCombinedPromptWarnBytes = 10 * 1024 + func newValidateCommand(cfg *Config) *cobra.Command { var skipPlugins bool var validatorTimeout time.Duration @@ -73,10 +79,8 @@ Examples: func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugins bool, validatorTimeout time.Duration) error { ctx := context.Background() - // Create output writer writer := ui.NewOutputWriter(cmd.OutOrStdout(), cmd.ErrOrStderr(), cfg.OutputFormat, cfg.NoColor, cfg.NoHints) - // Detect pack/workflow namespace syntax packName, baseName := parseWorkflowNamespace(workflowName) var repo *repository.CompositeRepository if packName != "" { @@ -93,10 +97,7 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi repo = NewWorkflowRepository() } - // Create validator validator := expression.NewExprValidator() - - // Create service svc := application.NewWorkflowService(repo, nil, nil, nil, validator) // Inject an OperationProvider so that mcp_proxy.plugin_tools checks run. @@ -105,26 +106,21 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi // behavior when no plugins are installed in the current environment. svc.SetPluginOperationProvider(infrastructurePlugin.NewCompositeOperationProvider()) - // Load workflow first to check existence wf, err := svc.GetWorkflow(ctx, workflowName) if err != nil { return writeErrorAndExit(writer, err, ExitUser) } - // Validate workflow structure validationErr := svc.ValidateWorkflow(ctx, workflowName) - // If workflow structure is valid, validate template interpolation references if validationErr == nil { templateAnalyzer := analyzer.NewInterpolationAnalyzer() templateValidator := workflow.NewTemplateValidator(wf, templateAnalyzer) result := templateValidator.Validate() if result != nil && result.HasErrors() { - // Format all errors for display if len(result.Errors) == 1 { validationErr = result.Errors[0] } else { - // Create a multi-line error with all validation errors var sb strings.Builder fmt.Fprintf(&sb, "validation failed with %d errors:", len(result.Errors)) for _, err := range result.Errors { @@ -135,7 +131,6 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi } } - // If workflow is valid, also validate template references if validationErr == nil { templatePaths := []string{ ".awf/templates", @@ -162,7 +157,7 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi var roleWarnings []string if validationErr == nil { roleRepo := roles.NewFilesystemAgentRoleRepository(nil) - roleWarnings, validationErr = validateRoleRefs(wf, roleRepo) + roleWarnings, validationErr = validateRoleRefs(ctx, wf, roleRepo) } // Collect disabled plugin warnings (non-blocking, skipped on quiet format or when --skip-plugins) @@ -205,7 +200,6 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi if validationErr != nil { result.Errors = []string{validationErr.Error()} } - // Build inputs info for _, inp := range wf.Inputs { defaultVal := "" if inp.Default != nil { @@ -218,7 +212,6 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi Default: defaultVal, }) } - // Build steps info for _, step := range wf.Steps { next := step.OnSuccess if next == "" { @@ -254,10 +247,8 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi return writeErrorAndExit(writer, validationErr, ExitWorkflow) } - // Show success formatter.Success(fmt.Sprintf("Workflow '%s' is valid", workflowName)) - // Print disabled plugin warnings for _, warning := range pluginWarnings { fmt.Fprintf(cmd.ErrOrStderr(), "\n warning: %s", warning) fmt.Fprintf(cmd.ErrOrStderr(), "\n Hint: Run 'awf plugin enable ' to re-enable") @@ -266,7 +257,6 @@ func runValidate(cmd *cobra.Command, cfg *Config, workflowName string, skipPlugi fmt.Fprintln(cmd.ErrOrStderr()) } - // Verbose output if cfg.Verbose { formatter.Println() formatter.Printf("Name: %s\n", wf.Name) @@ -416,8 +406,7 @@ func validateSkillRefs(wf *workflow.Workflow) ([]string, error) { return warnings, nil } -func validateRoleRefs(wf *workflow.Workflow, repo ports.AgentRoleRepository) ([]string, error) { - ctx := context.Background() +func validateRoleRefs(ctx context.Context, wf *workflow.Workflow, repo ports.AgentRoleRepository) ([]string, error) { var warnings []string for _, step := range wf.Steps { @@ -427,7 +416,15 @@ func validateRoleRefs(wf *workflow.Workflow, repo ports.AgentRoleRepository) ([] role := step.Agent.Role - // Defense in depth: reject path-traversal even though domain + infra also reject it. + // Defense in depth: block path-traversal sequences, mirroring the + // infrastructure Load guard. application.isRolePathRef routes refs + // containing '/' or '\' (or starting with '.', '~') to LoadFromPath; + // pure name-refs are routed to Load, which also rejects '/' and '\'. + // Since path-refs may legitimately contain separators, we only block + // '..' here (which is invalid in both name-refs and path-refs). + // Name-refs containing '/' or '\' are already unreachable: isRolePathRef + // classifies them as path-refs and LoadFromPath handles them safely via + // filepath.Clean — no additional guard is needed at this layer. if strings.Contains(role, "..") { return nil, workflow.ValidationError{ Level: workflow.ValidationLevelError, @@ -440,50 +437,59 @@ func validateRoleRefs(wf *workflow.Workflow, repo ports.AgentRoleRepository) ([] 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), - } - } + if errors.As(err, ¬Found) && roleDirExistsWithoutAgentsMD(notFound) { return nil, workflow.ValidationError{ Level: workflow.ValidationLevelError, - Code: workflow.ErrRoleNotFound, - Message: err.Error(), + Code: workflow.ErrRoleMissingAgentsMD, + Message: fmt.Sprintf("role %q has no AGENTS.md", notFound.Name), Path: fmt.Sprintf("states.%s.agent.role", step.Name), } } + // Use a user-oriented message that identifies the role by name without + // exposing internal filesystem search paths, which are environment-specific + // and would pollute machine-readable output (JSON/quiet format). The full + // path list is available in the underlying AgentRoleNotFoundError for + // callers that need it. + roleName := role + if notFound != nil { + roleName = notFound.Name + } return nil, workflow.ValidationError{ Level: workflow.ValidationLevelError, Code: workflow.ErrRoleNotFound, - Message: err.Error(), + Message: fmt.Sprintf("resolve role %q: not found", roleName), 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)) - } - } + warnings = append(warnings, roleContentWarnings(agentRole, step.Agent.SystemPrompt)...) } return warnings, nil } +// roleContentWarnings returns non-blocking warnings about a resolved role's +// AGENTS.md content: empty body, oversized file, or an oversized combined +// role+system_prompt payload. It returns nil when the content is within limits. +// +// The checks are evaluated in priority order and the first match wins, so at +// most one warning is emitted per role. Order matters: a file whose body is +// empty after frontmatter stripping reports "empty body" even if the raw file +// exceeds the size threshold. RawSizeBytes is captured at load time, so no +// filesystem access is needed here. +func roleContentWarnings(role *workflow.AgentRole, systemPrompt string) []string { + if role.Content == "" { + return []string{fmt.Sprintf("role %q has empty AGENTS.md body", role.Name)} + } + if role.RawSizeBytes > workflow.AgentRoleSizeWarnBytes { + return []string{fmt.Sprintf("role %q: AGENTS.md exceeds %dKB size threshold", role.Name, workflow.AgentRoleSizeWarnBytes/1024)} + } + if len(role.Content)+len(systemPrompt) > agentRoleCombinedPromptWarnBytes { + return []string{fmt.Sprintf("role %q: combined role content and system_prompt exceeds %dKB threshold", role.Name, agentRoleCombinedPromptWarnBytes/1024)} + } + return nil +} + func skillDirExists(name string, searchPaths []string) bool { for _, searchPath := range searchPaths { if _, err := os.Stat(filepath.Join(searchPath, name)); err == nil { @@ -494,11 +500,14 @@ func skillDirExists(name string, searchPaths []string) bool { } // 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). +// is missing an AGENTS.md file. For path-based refs (IsPathRef=true), the +// single search path equals the role directory itself, so it is checked +// directly. For name-based refs, the name is joined into each search path. func roleDirExistsWithoutAgentsMD(notFound *workflow.AgentRoleNotFoundError) bool { - if len(notFound.SearchPaths) == 1 { + if notFound.IsPathRef { + if len(notFound.SearchPaths) == 0 { + return false + } dir := notFound.SearchPaths[0] if info, err := os.Stat(dir); err == nil && info.IsDir() { return true diff --git a/internal/interfaces/cli/validate_helpers_test.go b/internal/interfaces/cli/validate_helpers_test.go new file mode 100644 index 00000000..b8b1dcd9 --- /dev/null +++ b/internal/interfaces/cli/validate_helpers_test.go @@ -0,0 +1,107 @@ +package cli + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/awf-project/cli/internal/domain/workflow" +) + +func TestSkillDirExists(t *testing.T) { + base := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(base, "present"), 0o755)) + + t.Run("directory present", func(t *testing.T) { + assert.True(t, skillDirExists("present", []string{base})) + }) + t.Run("directory absent", func(t *testing.T) { + assert.False(t, skillDirExists("missing", []string{base})) + }) + t.Run("empty search paths", func(t *testing.T) { + assert.False(t, skillDirExists("anything", nil)) + }) + t.Run("found on second path", func(t *testing.T) { + assert.True(t, skillDirExists("present", []string{filepath.Join(base, "nope"), base})) + }) +} + +func TestRoleDirExistsWithoutAgentsMD(t *testing.T) { + base := t.TempDir() + roleDir := filepath.Join(base, "go-senior") + require.NoError(t, os.Mkdir(roleDir, 0o755)) + filePath := filepath.Join(base, "afile") + require.NoError(t, os.WriteFile(filePath, []byte("x"), 0o644)) + + t.Run("single path: existing directory (path-ref)", func(t *testing.T) { + nf := &workflow.AgentRoleNotFoundError{Name: "go-senior", SearchPaths: []string{roleDir}, IsPathRef: true} + assert.True(t, roleDirExistsWithoutAgentsMD(nf)) + }) + t.Run("single path: a file, not a directory (path-ref)", func(t *testing.T) { + nf := &workflow.AgentRoleNotFoundError{Name: "afile", SearchPaths: []string{filePath}, IsPathRef: true} + assert.False(t, roleDirExistsWithoutAgentsMD(nf)) + }) + t.Run("single path: missing (path-ref)", func(t *testing.T) { + nf := &workflow.AgentRoleNotFoundError{Name: "ghost", SearchPaths: []string{filepath.Join(base, "ghost")}, IsPathRef: true} + assert.False(t, roleDirExistsWithoutAgentsMD(nf)) + }) + t.Run("multi path: dir found on second search path", func(t *testing.T) { + nf := &workflow.AgentRoleNotFoundError{ + Name: "go-senior", + SearchPaths: []string{filepath.Join(base, "elsewhere"), base}, + } + assert.True(t, roleDirExistsWithoutAgentsMD(nf)) + }) + t.Run("multi path: dir absent on all search paths", func(t *testing.T) { + nf := &workflow.AgentRoleNotFoundError{ + Name: "go-senior", + SearchPaths: []string{filepath.Join(base, "a"), filepath.Join(base, "b")}, + } + assert.False(t, roleDirExistsWithoutAgentsMD(nf)) + }) +} + +func TestRoleContentWarnings(t *testing.T) { + t.Run("empty content warns", func(t *testing.T) { + got := roleContentWarnings(&workflow.AgentRole{Name: "r"}, "") + require.Len(t, got, 1) + assert.Contains(t, got[0], "empty AGENTS.md body") + }) + t.Run("within limits returns nil", func(t *testing.T) { + assert.Nil(t, roleContentWarnings(&workflow.AgentRole{Name: "r", Content: "small body"}, "short prompt")) + }) + t.Run("oversized AGENTS.md warns from RawSizeBytes (no filesystem access)", func(t *testing.T) { + got := roleContentWarnings(&workflow.AgentRole{ + Name: "r", + Content: "non-empty", + RawSizeBytes: workflow.AgentRoleSizeWarnBytes + 1, + }, "") + require.Len(t, got, 1) + assert.Contains(t, got[0], "exceeds 500KB") + }) + t.Run("combined role + system_prompt over threshold warns", func(t *testing.T) { + content := strings.Repeat("a", 6000) + prompt := strings.Repeat("b", 6000) // 12000 > 10KB, SourcePath empty so file size is 0 + got := roleContentWarnings(&workflow.AgentRole{Name: "r", Content: content}, prompt) + require.Len(t, got, 1) + assert.Contains(t, got[0], "exceeds 10KB") + }) +} + +func TestCollectDisabledPluginWarnings_NoOperationSteps(t *testing.T) { + cfg := &Config{StoragePath: t.TempDir(), PluginsDir: t.TempDir()} + wf := &workflow.Workflow{ + Steps: map[string]*workflow.Step{ + "done": {Name: "done", Type: workflow.StepTypeTerminal}, + }, + } + + got := collectDisabledPluginWarnings(context.Background(), cfg, wf) + + assert.Empty(t, got) +} diff --git a/internal/interfaces/cli/validate_role_test.go b/internal/interfaces/cli/validate_role_test.go index f11900bc..c564fe9d 100644 --- a/internal/interfaces/cli/validate_role_test.go +++ b/internal/interfaces/cli/validate_role_test.go @@ -1,6 +1,7 @@ package cli import ( + "context" "errors" "os" "path/filepath" @@ -12,6 +13,7 @@ import ( "github.com/awf-project/cli/internal/domain/workflow" "github.com/awf-project/cli/internal/infrastructure/roles" + "github.com/awf-project/cli/internal/testutil" ) func TestValidateRoleRefs_NoRolesReturnsNoWarnings(t *testing.T) { @@ -34,7 +36,7 @@ func TestValidateRoleRefs_NoRolesReturnsNoWarnings(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) @@ -43,8 +45,11 @@ func TestValidateRoleRefs_NoRolesReturnsNoWarnings(t *testing.T) { func TestValidateRoleRefs_ValidNameBasedRole(t *testing.T) { tmpDir := t.TempDir() - // Create role directory with AGENTS.md - roleDir := filepath.Join(tmpDir, ".awf", "agents", "go-senior") + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + + // Create role directory with AGENTS.md in roles/ namespace + roleDir := filepath.Join(tmpDir, ".awf", "roles", "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)) @@ -72,7 +77,7 @@ func TestValidateRoleRefs_ValidNameBasedRole(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) @@ -81,6 +86,9 @@ func TestValidateRoleRefs_ValidNameBasedRole(t *testing.T) { func TestValidateRoleRefs_ValidPathBasedRole(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + // Create role directory with AGENTS.md roleDir := filepath.Join(tmpDir, "my-role") require.NoError(t, os.MkdirAll(roleDir, 0o755)) @@ -105,7 +113,7 @@ func TestValidateRoleRefs_ValidPathBasedRole(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) @@ -114,7 +122,12 @@ func TestValidateRoleRefs_ValidPathBasedRole(t *testing.T) { func TestValidateRoleRefs_MissingRoleDirectory(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir so relative paths (.awf/roles, .agents/roles) resolve deterministically + testutil.ChdirIsolated(t, tmpDir) + + // Set both AWF_CONFIG_HOME and HOME for deterministic path resolution t.Setenv("AWF_CONFIG_HOME", filepath.Join(tmpDir, ".awf")) + t.Setenv("HOME", tmpDir) repo := roles.NewFilesystemAgentRoleRepository(nil) wf := &workflow.Workflow{ @@ -133,21 +146,36 @@ func TestValidateRoleRefs_MissingRoleDirectory(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), 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) + + // The user-facing message must contain the role name but NOT expose internal + // filesystem search paths. Search paths are implementation details that change + // across environments and would pollute machine-readable output (JSON/quiet format). assert.Contains(t, err.Error(), "missing-role") + assert.NotContains(t, err.Error(), ".awf/roles/missing-role") + assert.NotContains(t, err.Error(), ".agents/roles/missing-role") + assert.NotContains(t, err.Error(), filepath.Join(tmpDir, ".awf", "roles")) + assert.NotContains(t, err.Error(), filepath.Join(tmpDir, ".agents", "roles")) + + // Verify no old agents/ root paths appear in the message + assert.NotContains(t, err.Error(), ".awf/agents") + assert.NotContains(t, err.Error(), ".agents/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") + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + + // Create role directory in roles/ namespace but without AGENTS.md + roleDir := filepath.Join(tmpDir, ".awf", "roles", "incomplete-role") require.NoError(t, os.MkdirAll(roleDir, 0o755)) configHome := filepath.Join(tmpDir, ".awf") @@ -170,7 +198,7 @@ func TestValidateRoleRefs_MissingAgentsMD(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) require.Error(t, err) assert.Nil(t, warnings) @@ -183,6 +211,9 @@ func TestValidateRoleRefs_MissingAgentsMD(t *testing.T) { func TestValidateRoleRefs_EmptyAgentsMDBody(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + // Create role with empty AGENTS.md roleDir := filepath.Join(tmpDir, "empty-role") require.NoError(t, os.MkdirAll(roleDir, 0o755)) @@ -207,7 +238,7 @@ func TestValidateRoleRefs_EmptyAgentsMDBody(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.NotNil(t, warnings) @@ -219,6 +250,9 @@ func TestValidateRoleRefs_EmptyAgentsMDBody(t *testing.T) { func TestValidateRoleRefs_AgentsMDExceedsSizeLimit(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + // Create role with AGENTS.md > 500KB roleDir := filepath.Join(tmpDir, "large-role") require.NoError(t, os.MkdirAll(roleDir, 0o755)) @@ -245,7 +279,7 @@ func TestValidateRoleRefs_AgentsMDExceedsSizeLimit(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.NotNil(t, warnings) @@ -257,6 +291,9 @@ func TestValidateRoleRefs_AgentsMDExceedsSizeLimit(t *testing.T) { func TestValidateRoleRefs_CombinedRoleAndSystemPromptExceeds10KB(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + // Create role with content that combined with system_prompt exceeds 10KB roleDir := filepath.Join(tmpDir, "combined-role") require.NoError(t, os.MkdirAll(roleDir, 0o755)) @@ -283,7 +320,7 @@ func TestValidateRoleRefs_CombinedRoleAndSystemPromptExceeds10KB(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.NotNil(t, warnings) @@ -312,7 +349,7 @@ func TestValidateRoleRefs_PathTraversalAttempt(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) require.Error(t, err) assert.Nil(t, warnings) @@ -324,6 +361,9 @@ func TestValidateRoleRefs_PathTraversalAttempt(t *testing.T) { func TestValidateRoleRefs_MultipleStepsMultipleRoles(t *testing.T) { tmpDir := t.TempDir() + // Chdir to tmpDir for deterministic relative path resolution + testutil.ChdirIsolated(t, tmpDir) + // Create two role directories role1Dir := filepath.Join(tmpDir, "role1") require.NoError(t, os.MkdirAll(role1Dir, 0o755)) @@ -359,7 +399,7 @@ func TestValidateRoleRefs_MultipleStepsMultipleRoles(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) @@ -383,7 +423,7 @@ func TestValidateRoleRefs_StepWithoutAgentConfig(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) @@ -409,7 +449,7 @@ func TestValidateRoleRefs_AgentStepWithEmptyRole(t *testing.T) { SourceDir: tmpDir, } - warnings, err := validateRoleRefs(wf, repo) + warnings, err := validateRoleRefs(context.Background(), wf, repo) assert.NoError(t, err) assert.Empty(t, warnings) diff --git a/internal/testutil/env.go b/internal/testutil/env.go new file mode 100644 index 00000000..2ccec78e --- /dev/null +++ b/internal/testutil/env.go @@ -0,0 +1,44 @@ +package testutil + +import ( + "os" + "path/filepath" + "testing" +) + +// ChdirIsolated changes the working directory to dir and isolates HOME, +// XDG_CONFIG_HOME, XDG_DATA_HOME, and AWF_ROLES_PATH so that role and skill +// search chains (which derive paths from these variables) resolve under dir +// rather than the real user home directory. +// +// Without isolation, global search-path positions such as ~/.agents/roles and +// $XDG_CONFIG_HOME/awf/roles would resolve against the real home directory, +// making tests non-deterministic across machines. +// +// AWF_ROLES_PATH is cleared explicitly so that an operator-level env override +// does not mask the default search chain under test. +// +// The original working directory is restored via t.Cleanup. +// +// Usage: +// +// func TestSomething(t *testing.T) { +// tmpDir := t.TempDir() +// testutil.ChdirIsolated(t, tmpDir) +// // ... test against deterministic paths +// } +func ChdirIsolated(t *testing.T, dir string) { + t.Helper() + t.Setenv("HOME", dir) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(dir, ".config")) + t.Setenv("XDG_DATA_HOME", filepath.Join(dir, ".local", "share")) + t.Setenv("AWF_ROLES_PATH", "") + orig, err := os.Getwd() + if err != nil { + t.Fatalf("ChdirIsolated: os.Getwd: %v", err) + } + t.Cleanup(func() { os.Chdir(orig) }) //nolint:errcheck,gosec // best-effort restore of original working directory; G104 false positive in cleanup context + if err := os.Chdir(dir); err != nil { + t.Fatalf("ChdirIsolated: os.Chdir(%q): %v", dir, err) + } +}