diff --git a/AGENTS.md b/AGENTS.md index a87efea7..23ebab9b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -96,8 +96,10 @@ step-by-step checklist. - `validator.py` - Cross-reference validation (agent names, routes, parallel groups) - **skills/**: Skill registry and loader (opt-in, bundled skill content) - - `registry.py` - Resolves built-in skill names to on-disk directories (probes editable-install + wheel-install layouts) - - `loader.py` - Reads `SKILL.md` + `references/*.md` for providers that require eager preamble injection; wraps each skill in `` tags inside a `` envelope + - `registry.py` - Resolves `skills:` entries to on-disk directories — built-in names (probing editable-install + wheel-install layouts) and filesystem paths, including expanding a `skills/` root into its children. Also `resolve_skill_plugin`, which maps a directory to the Claude Code plugin that owns it + - `frontmatter.py` - Parses and validates `SKILL.md` YAML frontmatter with **ruamel.yaml**, requiring `name` + `description`. Exists because both downstream CLIs skip an unparseable skill *silently*; called from `resolve_skills` so `conductor run` is covered too, not just `conductor validate` + - `loader.py` - Reads `SKILL.md` + `references/*.md` for providers that require eager preamble injection; wraps each skill in `` tags inside a `` envelope. Bounded by `runtime.skill_injection`. `_read_file` **raises** `SkillManifestError` rather than logging and skipping — and since `lru_cache` never memoizes a raising call, a transient read error is retried instead of frozen for the run + - `errors.py` - `SkillError(ValueError)`, the shared base for `SkillNotFoundError` / `SkillPluginError` / `SkillManifestError`. Its own module so `registry` and `frontmatter` can both use it without a cycle. Resolution and manifest failures originate in different modules but reach the same handlers, so call sites catch the one base rather than enumerating subclasses (`_check_skill_injection_budget` forgot one, and an unreadable `references/*.md` escaped `conductor validate` as a traceback) - Built-in skills live under `plugins/conductor/skills//` (bundled into the wheel via hatchling `force-include`) - **engine/**: Workflow execution orchestration @@ -164,7 +166,7 @@ step-by-step checklist. - **Set step typing**: `output_type` defaults to `auto` (safe YAML parse with `_to_json_safe` normalisation — `datetime`/`date`/`time` → ISO 8601, non-string dict keys and other non-JSON-safe values raise `ExecutionError`). Explicit `string`/`number`/`integer`/`boolean`/`list`/`dict` only valid on single `value:`. `WorkflowContext.store` accepts any JSON-safe value (scalars/lists from `set` steps in addition to the dicts produced by LLM / script / gate / parallel-group outputs); `_add_agent_input` returns the scalar verbatim for `step.output` and raises a clear `KeyError` for `step.output.field` shorthand on non-dict outputs. - **Reasoning effort**: `runtime.default_reasoning_effort` sets a workflow-wide default; per-agent `reasoning.effort` overrides it. Allowed values: `low`, `medium`, `high`, `xhigh`, `max`. Each provider translates the unified value to its native API (Copilot: `reasoning_effort` on the session, validated against the model's `supported_reasoning_efforts`; Claude: extended thinking with budget mapping low=2048, medium=8192, high=16384, xhigh=32768, max=59904 tokens, with `temperature` coerced to 1.0 and `max_tokens` bumped to fit the budget). `max` is Copilot/Claude-only — the Hermes provider advertises only the first four levels in `CAPABILITIES.reasoning_effort` and re-checks the resolved effort against that tuple at execute time (in addition to the static `conductor validate` cross-check), so `max` is rejected on Hermes both statically and at runtime, including when it only resolves to `max` after Jinja template rendering. See `examples/reasoning-effort.yaml`. - **Periodic checkpoints** (`runtime.checkpoint`, issue #244): opt-in `CheckpointConfig` (`every_agent: bool`, `every_seconds: int|None`, `keep_last: int=5`; `is_enabled = every_agent or every_seconds is not None`). Off by default → failure-only behavior preserved. `WorkflowEngine._maybe_save_periodic_checkpoint()` is called once at the **top of `_execute_loop`** (single choke point), where prior outputs are committed and `_current_agent_name` is the step *about to run* — so a periodic checkpoint reuses failure-checkpoint `current_agent` semantics and resume continues forward with no special-casing. Gated via the `_periodic_checkpoints_active` property (**root engine only**, `_subworkflow_depth == 0`, + `is_enabled`) and skips the first iteration (`limits.current_iteration == 0`). The save decision is `_periodic_checkpoint_due(now)` (`every_agent` OR `every_seconds` throttle; first save always fires). `_save_checkpoint_on_failure` and the periodic path share `_write_checkpoint(error, trigger)` (which best-effort-guards provider `get_session_ids()` so it never raises). The periodic save wraps write+emit+rotate; on any failure it calls `_record_periodic_checkpoint_failure()` which emits a **`checkpoint_save_failed`** event (consecutive-failure count; surfaced by `ConsoleEventSubscriber` + JSONL + dashboard) so a recovery-reliant user isn't silently left without checkpoints. After a save the engine calls `rotate_periodic_checkpoints`; at a terminal **non-resumable** outcome (clean completion via `run()`/`resume()`, or an explicit `status: failed` terminate) `_cleanup_run_periodic_checkpoints()` deletes the run's periodic checkpoints (an unexpected failure leaves them in place alongside the failure checkpoint). `conductor checkpoint list` shows a `Trigger` column and `—` for periodic rows' error type. See `examples/periodic-checkpoints.yaml` and `docs/workflow-syntax.md` (Periodic Checkpoints section). -- **Skills**: `runtime.skills: [name, ...]` sets a workflow-wide default list of skills enabled for every provider-backed agent; per-agent `skills: [name, ...]` overrides it (tri-state via list presence: omitted = inherit, `skills: []` = explicit opt-out, `skills: [name, ...]` = explicit set). Skill names must resolve to a registered built-in (currently just `conductor`). The observable contract is the same across providers — *"the agent has access to the named skill"* — but the mechanism differs by provider via `AgentProvider.supports_native_skills`: **Copilot** (`True`) registers the skill directory on the SDK session via `skill_directories`, so the agent discovers and loads skill content natively (progressive disclosure via `SKILL.md` frontmatter); **Claude Agent SDK** (`True`) is also native but goes through the Claude Code *plugin* surface — `providers/claude_agent_sdk.py::_resolve_skill_plugins` maps each resolved skill directory back to the plugin that owns it (`skills/registry.py::resolve_skill_plugin` walks up for `.claude-plugin/plugin.json`), registers that root via `ClaudeAgentOptions.plugins` and enables the skill by its `:` name via `ClaudeAgentOptions.skills`; **Claude** (`False`) eagerly injects every enabled skill's `SKILL.md` plus `references/*.md` into the agent's rendered prompt inside `...` tags. Providers also declare `skills: bool` on their `ProviderCapabilities` descriptor so `conductor validate` can catch skills-against-unsupported-provider mismatches. Built-in skills live under `plugins/conductor/skills//` and are bundled into the wheel via the hatchling `force-include` entries in `pyproject.toml` — both the skill body **and** `plugins/conductor/.claude-plugin/`, because without the manifest no plugin root resolves and every skills-enabled agent on `claude-agent-sdk` fails with a `ProviderError`. Skills are rejected on non-provider-backed step types (script, wait, set, terminate, workflow, human_gate). See `examples/skills-self-improving-workflow.yaml`. +- **Skills**: `runtime.skills: [entry, ...]` sets a workflow-wide default list enabled for every provider-backed agent; per-agent `skills: [entry, ...]` overrides it (tri-state via list presence: omitted = inherit, `skills: []` = explicit opt-out, `skills: [entry, ...]` = explicit set). **Each entry is either a registered built-in name or a filesystem path** (issue #350). Classification is *syntactic* — path when it starts with `~`/`.` or contains `/` or `\`, otherwise a built-in name — so a bare `conductor` can never be shadowed by a same-named local directory and resolution never depends on what happens to exist. A path may be a single skill directory (holds `SKILL.md`) or a root of them, which expands to every immediate child holding one (not recursive); `skills/registry.py::resolve_skills(entries, base_dir)` does the expansion centrally rather than passing roots through, because eager injection needs a name per skill and claude-agent-sdk needs a `:` name. Relative paths resolve against the workflow file's directory (`AgentExecutor(workflow_dir=...)`, threaded from `WorkflowEngine._workflow_dir`), mirroring `_resolve_agent_working_dir` — `normpath`, not `resolve()`, so symlink aliases stay distinct. Paths are **trusted input**: the same YAML can already run arbitrary shell via `type: script`, so no allowlist applies. `AgentDef.validate_skills` only shape-checks path entries (the schema has no base dir) but keeps the eager built-in-name check, so an unknown *name* still fails at load time as before. Every resolved `SKILL.md` must have valid YAML frontmatter declaring `name` and `description` — checked inside `resolve_skills` (via `skills/frontmatter.py`, parsed with **ruamel.yaml**, not PyYAML) rather than only in `conductor validate`, because `conductor run` never calls the static validator; both CLIs skip an unparseable skill *silently*, which is the bug this closes. The observable contract is the same across providers — *"the agent has access to the named skill"* — but the mechanism differs via `AgentProvider.supports_native_skills` (readable without instantiating a provider via `providers/capabilities.py::uses_native_skills`, which returns `None` when it cannot be determined so callers skip rather than guess): **Copilot** (`True`) registers the skill directory on the SDK session via `skill_directories` (progressive disclosure via `SKILL.md` frontmatter); **Claude Agent SDK** (`True`) is also native but goes through the Claude Code *plugin* surface — `providers/claude_agent_sdk.py::_resolve_skill_plugins` maps each resolved directory back to the plugin that owns it (`skills/registry.py::resolve_skill_plugin` walks up for `.claude-plugin/plugin.json`), registers that root via `ClaudeAgentOptions.plugins` and enables the skill by its `:` name via `ClaudeAgentOptions.skills`. Because that SDK has **no bare skill-directory option**, a path skill outside a plugin is unreachable there — `config/validator.py` now refuses it statically (naming both remedies) instead of letting it fail as a runtime `ProviderError`; the identical skill works on `copilot` untouched. **Claude** and **Hermes** (`False`) eagerly inject every enabled skill's `SKILL.md` plus `references/*.md` into the rendered prompt inside `...` tags. That is expensive — the bundled `conductor` skill alone is ~117KB (~29K tokens), paid on every call and every retry — so `runtime.skill_injection` (`SkillInjectionConfig`: `warn_bytes` default 64KB, `max_bytes` default 128KB, either nullable) bounds it, enforced both in `AgentExecutor` and statically in `conductor validate`, measured against the exact string prepended and reported with a per-skill breakdown. The defaults deliberately straddle the bundled skill so enabling it on `claude` warns rather than breaking; a `warn_bytes` above `max_bytes` is rejected as unreachable. Native providers are exempt. Providers also declare `skills: bool` on their `ProviderCapabilities` descriptor so `conductor validate` catches skills-against-unsupported-provider mismatches — `hermes` declares `True` (it reaches skills through the provider-agnostic eager-injection path in `AgentExecutor`; it previously omitted the field, defaulting to `False`, while its own `execute()` docstring described injection working), and `aca` is the one `False` (skill directories are host paths the in-sandbox runner cannot read). `AgentExecutor._reject_unsupported_skills` now enforces a `skills=False` declaration at run time too, because `conductor run` never calls the static validator — otherwise the declaration held only at validate time while the eager-injection path happily injected anyway. Built-in skills live under `plugins/conductor/skills//` and are bundled into the wheel via the hatchling `force-include` entries in `pyproject.toml` — both the skill body **and** `plugins/conductor/.claude-plugin/`, because without the manifest no plugin root resolves and every skills-enabled agent on `claude-agent-sdk` fails with a `ProviderError`. Skills are rejected on non-provider-backed step types (script, wait, set, terminate, workflow, human_gate). See `examples/skills-self-improving-workflow.yaml` and `docs/workflow-syntax.md` (Skills section). - **Terminate steps** (`type: terminate`): explicit terminal step with `status` (`success` | `failed`), Jinja2 `reason`, and optional `output_template` (a `dict[str, str]` that replaces `workflow.output:` when set; each value is rendered then passed through `_maybe_parse_json` so `"true"` becomes `True`, `"42"` becomes `42`, JSON literals are parsed). Reaching a terminate step ends the workflow immediately (no routes evaluated after). `success` → CLI exit 0, dashboard ✅, `workflow_completed { termination_reason, terminated_by, is_explicit: true, status }`; runs `on_complete` hook. `failed` → CLI exit 1 (with rendered output JSON still printed to stdout for downstream tooling), dashboard ❌, raises `WorkflowTerminated` (subclass of `ExecutionError`), emits `workflow_failed { error_type: "WorkflowTerminated", is_explicit: true, status, output }`, runs `on_error` hook, and **does not** save an on-failure checkpoint (explicit terminations are intentionally non-resumable). Terminate steps cannot have `routes`, `tools`, `output`, `prompt`, `model`, etc.; cannot be used as parallel-group members or as a for_each inline agent (route to one from those groups' `routes:` instead). Inside a sub-workflow, a `status: failed` terminate is downgraded at the parent boundary to `SubworkflowTerminatedError` (also a subclass of `ExecutionError`) preserving the child's rendered `terminated_output` / `terminated_reason` / `terminated_by` as structured attributes — the parent treats it as a normal sub-workflow failure (its own `workflow_failed` does NOT inherit `is_explicit: true`). For more detail see `examples/terminate.yaml`, `docs/workflow-syntax.md` (Terminate Steps section), and `plugins/conductor/skills/conductor/references/authoring.md`. - **Structured `runtime.provider` (Copilot custom routing)**: `runtime.provider` accepts either the bare string shorthand (`provider: copilot`) or a structured `ProviderSettings` object that routes the Copilot SDK at OpenAI-compatible / Azure / Anthropic endpoints (Ollama, vLLM, LM Studio, Azure OpenAI, etc.). Object fields: `name` (defaults to `copilot`), `type` (`openai`|`azure`|`anthropic`), `wire_api` (`completions`|`responses`), `base_url`, `api_key`, `bearer_token`, `headers`, `azure.api_version`. `api_key` and `bearer_token` are `SecretStr` (redacted in `model_dump` / dashboard / event logs). The model is frozen after construction. Custom routing activates only when at least one non-`name` field is set in YAML — ambient `OPENAI_*` env vars never divert default routing on their own. Once activated, missing fields fall back from env vars in this order: `base_url` ← `COPILOT_PROVIDER_BASE_URL` → `OPENAI_BASE_URL`; `api_key` ← `COPILOT_PROVIDER_API_KEY` (only — ambient `OPENAI_API_KEY` is intentionally NOT a fallback to avoid credential leaks); `bearer_token` ← `COPILOT_PROVIDER_BEARER_TOKEN`. The schema rejects every non-`name` field when `name != "copilot"` (structured config for other providers is a follow-up). It also rejects anchorless / broken combinations that would silently no-op at the SDK boundary: `wire_api` / `type` / `headers` / `azure` cannot stand alone without `base_url` / `api_key` / `bearer_token`; empty `headers`, empty `SecretStr`, and `azure: {api_version: null}` are rejected. The resolver raises `ProviderError` when custom routing is activated but every resolved field is falsy (e.g. expected env vars all unset). Custom routing applies to both agent execution and dialog turns so all sessions hit the same endpoint. `--provider ` CLI override replaces the whole `ProviderSettings` (logs a notice when YAML had structured fields). See `examples/copilot-local-llm.yaml`. - **Connect to an existing Copilot runtime (Copilot)**: `runtime.provider.runtime_url` (Copilot-only) points the provider at an already-running `copilot --headless` process instead of spawning a nested one. Agents share the authenticated runtime process while retaining separate SDK sessions. Optional `runtime_token` (`SecretStr`, redacted, requires `runtime_url`) is the socket connection secret. Both fields fall back to env vars (`COPILOT_PROVIDER_RUNTIME_URL` / `COPILOT_PROVIDER_RUNTIME_TOKEN`) which activate the connection on their own (zero-YAML path for external orchestrators). `has_external_runtime()` is a separate axis from `has_custom_routing()`; the two can be combined because runtime transport and per-session model routing are independent. `has_structured_config()` keeps either mode from collapsing to bare-string serialization. Schema rejects: `runtime_token` without `runtime_url`; empty or whitespace-only runtime values; either field when `name != "copilot"`. Provider layer: `_resolve_runtime_connection()` (YAML then env) and `_build_client()` (in `copilot.py`). See `examples/copilot-existing-runtime.yaml` and `docs/configuration.md` (Connecting to an Existing Copilot Runtime). @@ -204,7 +206,7 @@ Tests mirror source structure in `tests/`: - `test_providers/` - Provider implementation tests - `test_integration/` - Full workflow execution tests - `test_gates/` - Human gate tests -- `test_skills/` - Skill registry, loader, schema field, and executor-integration tests +- `test_skills/` - Skill registry, frontmatter parsing, path entries, injection budget, loader, schema field, and executor/engine-integration tests. `test_engine_integration.py` is load-bearing: an `AgentExecutor` built directly in a test is handed `workflow_dir` and `skill_injection` by the test itself, so only an engine-level test can catch the engine failing to supply them Use `pytest.mark.performance` for performance tests (exclude with `-m "not performance"`). diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1336ae..0dd26ad9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`skills:` now accepts filesystem paths, not just built-in names** + (issue #350) — an entry is treated as a path when it starts with `.` + or `~`, or contains `/` or `\`; everything else must still be + a registered built-in, so a bare `conductor` can never be shadowed by a + same-named local directory. A path may point at a single skill directory + (one holding `SKILL.md`) or at a root of them, which expands to every + immediate child that holds one. Relative paths resolve against the workflow + file's directory — the same rule `working_dir` uses — so a team can version + a skill alongside the workflow that uses it with no per-developer install + step, and the workflow resolves identically from any working directory. + Conductor expands roots itself rather than handing them to a provider, + because eager injection needs a name per skill and `claude-agent-sdk` needs + a `:` name; doing it centrally keeps every provider seeing + the same set. Skill paths are trusted input by design: the same workflow + file can already declare `type: script` steps running arbitrary shell, so + no additional allowlist applies. +- **`runtime.skill_injection` bounds eagerly injected skill content** + (issue #350) — `warn_bytes` (default 64KB) logs a warning and reports from + `conductor validate`; `max_bytes` (default 128KB) fails the agent. Either + can be set to `null` to disable it. Providers without a native skill + surface (`claude`, `hermes`) have no progressive disclosure: `AgentExecutor` + prepends each enabled skill's `SKILL.md` **plus its entire `references/` + tree** on every call and every retry, and there + was previously no ceiling at all. The bundled `conductor` skill alone is + ~117KB (~29K tokens), so the defaults deliberately straddle it — enabling + it on `claude` now warns instead of breaking, while accumulating several + large skills errors. Both limits are measured against the exact string + being prepended and report a per-skill breakdown naming the offender. + Providers with progressive disclosure (`copilot`, `claude-agent-sdk`) are + unaffected. +- **`hermes` declares `skills=True`** (issue #350) — the provider omitted + `skills` from its `CAPABILITIES`, which defaults to `False`, so + `conductor validate` rejected `skills:` on it while its own `execute()` + docstring described eager injection working. Injection happens in + `AgentExecutor`, upstream of every provider, so the path was always + reachable and the declaration was simply inaccurate. Now bounded by + `runtime.skill_injection` like `claude`. + - **`claude-agent-sdk` provider now honors `working_dir`** — the directory resolved from `agent.working_dir` / `runtime.working_dir` is forwarded to `ClaudeAgentOptions.cwd`, so the `claude` CLI runs there and every stdio MCP @@ -81,6 +119,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A malformed `SKILL.md` no longer fails silently** (issue #350) — both the + Copilot CLI and Claude Code skip a skill whose YAML frontmatter cannot be + parsed, with no warning and no error, leaving an agent running without the + knowledge its author asked for. The trap is ordinary: a `description` + containing `Triggers: ...` as an unquoted plain scalar is invalid YAML. + Conductor now parses the frontmatter itself, requires a non-empty `name` + and `description`, and reports the underlying YAML error along with the + `description: |` block-scalar fix. Enforced during resolution rather than + only in `conductor validate`, because `conductor run` never invokes the + static validator. +- **`conductor validate` rejects a `claude-agent-sdk` skill outside a plugin** + (issue #350) — that SDK exposes no bare skill-directory option, only plugin + roots plus skill names, so such a skill is unreachable there even though + `copilot` loads it fine. It previously surfaced as a runtime + `ProviderError` on first execution; it is now reported before the run + starts, naming the directory and offering both remedies (package it as a + plugin, or run the agent on `copilot`). + - **`skills: []` is now a real opt-out on `claude-agent-sdk`, and agents no longer inherit ambient skills from the machine.** The provider left the SDK's `setting_sources` unset, so the `claude` CLI discovered and enabled skills @@ -141,8 +197,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 every prompt.** The provider previously took the eager preamble path on the grounds that the SDK had no skill surface — out of date, and expensive: the full `SKILL.md` plus the entire `references/` tree was prepended to every - call, every retry, and every validator pass (~27K tokens for the bundled - `conductor` skill). The owning Claude Code plugin is now registered on the + call and every retry (~29K tokens for the bundled `conductor` skill). The owning Claude Code plugin is now registered on the session and the skill enabled by its `:` name, so the CLI reads only the frontmatter up front and loads the body on demand. An agent with an explicit `tools: []` is granted back the single `Skill` tool when it has diff --git a/docs/providers/comparison.md b/docs/providers/comparison.md index 6d25b830..79e978e5 100644 --- a/docs/providers/comparison.md +++ b/docs/providers/comparison.md @@ -157,7 +157,18 @@ the environment instead. Note the SDK treats the enabled-skill list as a context filter rather than a sandbox: undeclared skills are hidden from the model's listing and rejected by the `Skill` tool, but their files remain readable on disk through `Read`/`Bash`. -- `temperature` and `max_tokens` are **rejected at the factory** — sampling behavior is controlled by the CLI. + +Because the SDK exposes **no bare skill-directory option** — a skill is enabled by +name through the plugin that ships it — a skill directory that is not inside a +Claude Code plugin cannot be loaded here at all. `conductor validate` reports such +a `skills:` entry before the run starts, naming the directory and both remedies: +package it as a plugin (a `.claude-plugin/plugin.json` with the skill under +`/skills/`), or run that agent on `copilot`, which registers skill +directories directly and accepts the identical skill untouched. See the +[Skills section of the workflow syntax guide](../workflow-syntax.md#skills). + +Separately, `temperature` and `max_tokens` are **rejected at the factory** — +sampling behavior is controlled by the CLI. ### Example Claude Agent SDK Workflow diff --git a/docs/workflow-syntax.md b/docs/workflow-syntax.md index 0b256c34..7d81e051 100644 --- a/docs/workflow-syntax.md +++ b/docs/workflow-syntax.md @@ -11,6 +11,7 @@ This document provides a comprehensive reference for the Conductor workflow YAML - [Inputs and Outputs](#inputs-and-outputs) - [Limits and Safety](#limits-and-safety) - [Tools](#tools) +- [Skills](#skills) - [External File References](#external-file-references) - [Hooks](#hooks) @@ -1419,6 +1420,156 @@ agents: For full MCP configuration details, see the [MCP Tools guide](mcp-tools.md). +## Skills + +A **skill** is a directory of reusable knowledge an agent can opt into: a +`SKILL.md` describing what the skill covers, plus an optional `references/` +tree of supporting docs. Conductor consumes the same format the GitHub +Copilot CLI and Anthropic Claude Code use, so a skill written for either +generally works here unchanged. Conductor adds two constraints neither CLI +enforces: the frontmatter must actually parse (that is the point of this — +see below), and on `claude-agent-sdk` the frontmatter `name` must match the +directory basename, since the skill is enabled by name. + +### Enabling skills + +Set a workflow-wide default and override it per agent: + +```yaml +workflow: + runtime: + skills: + - conductor # built-in, ships in the wheel + - ./team-skills/acme-widgets # versioned alongside the workflow + - ~/scratch/skills # a skills root — every skill directly inside + +agents: + - name: reviewer + prompt: "Review this workflow." + # inherits runtime.skills + + - name: summarizer + prompt: "Summarize the review." + skills: [] # explicit opt-out — no skills + + - name: widget_expert + prompt: "Check the widget conventions." + skills: [./team-skills/acme-widgets] # overrides the workflow default +``` + +The per-agent field is tri-state: + +| Value | Meaning | +|---|---| +| omitted | inherit `runtime.skills` | +| `[]` | explicit opt-out — no skills, ignores the workflow default | +| `[...]` | explicit set — replaces the workflow default | + +Skills apply only to provider-backed agents. They are rejected on `script`, +`wait`, `set`, `terminate`, `workflow`, and `human_gate` steps. + +### Names and paths + +Each entry is either a **registered built-in name** or a **filesystem path**. +The distinction is syntactic — an entry is a path when it starts with `.` or +`~`, or contains `/` or `\`. Everything else must be a +built-in name, so a bare `conductor` can never be shadowed by a directory +that happens to share its name. + +Conductor ships one built-in skill: + +| Name | Contents | +|---|---| +| `conductor` | Conductor's YAML schema, execution model, authoring patterns, and CLI commands | + +A path may point at either granularity: + +* a **skill directory** — one containing `SKILL.md` +* a **skills root** — a directory of skill directories, which expands to + every immediate child containing a `SKILL.md` (not recursive) + +Relative paths resolve against the **workflow file's directory**, the same +rule `working_dir` uses, so a workflow validates and runs identically from +any working directory. This is what lets a team version a skill next to the +workflow that uses it, with no per-developer install step. + +> **Trust:** a `SKILL.md` is injected into the agent's context, so treat +> skill paths as trusted input. Conductor applies no additional allowlist — +> the same workflow file can already declare `type: script` steps that run +> arbitrary shell, so a skill path grants strictly less. + +### `SKILL.md` frontmatter + +Every resolved skill must declare a `name` and a `description` in valid YAML +frontmatter: + +```yaml +--- +name: acme-widgets +description: | + Internal ACME widget conventions. Triggers: widget, acme widget. +--- +``` + +Use a block scalar (`description: |`) whenever the text contains a colon +followed by a space — without one the value is invalid YAML, and this is the +single most common mistake: + +```yaml +# Wrong — 'Triggers:' makes this unparseable +description: Internal ACME widget conventions. Triggers: widget, acme widget. +``` + +Both the Copilot CLI and Claude Code **silently skip** a skill whose +frontmatter fails to parse — no warning, no error, the skill is simply +absent. Conductor parses it itself and fails loudly instead, both at +`conductor validate` and at run time. + +### How skills reach the model + +The contract is the same everywhere — *the agent has access to the named +skill* — but the mechanism and its cost differ: + +| Provider | Mechanism | Cost | +|---|---|---| +| `copilot` | `skill_directories` on the SDK session | progressive — frontmatter only, body loaded on demand | +| `claude-agent-sdk` | owning plugin registered, skill enabled by `:` | progressive | +| `claude` | eager injection into the rendered prompt | **full body on every call** | +| `hermes` | eager injection into the rendered prompt | **full body on every call** | +| `aca` | not supported (`skills=False`) — rejected by `conductor validate` and by the executor at run time | n/a | + +Two consequences worth knowing: + +* **`claude-agent-sdk` requires a plugin.** The SDK has no bare + skill-directory option, so a skill must live inside a Claude Code plugin + (a `.claude-plugin/plugin.json` with the skill under `/skills/`). + `conductor validate` reports a path skill that is not, rather than letting + it fail mid-run. The same skill works on `copilot` untouched. +* **Eager injection is expensive.** The bundled `conductor` skill alone is + ~117KB (~29K tokens), prepended to *every* call and every retry. + +### Limiting eager injection + +`runtime.skill_injection` bounds what eager-injection providers prepend. It +has no effect on providers with progressive disclosure. + +```yaml +workflow: + runtime: + skill_injection: + warn_bytes: 65536 # Default: 65536 (64KB). null disables the warning. + max_bytes: 131072 # Default: 131072 (128KB). null disables the limit. +``` + +Exceeding `warn_bytes` logs a warning and reports it from `conductor +validate`; exceeding `max_bytes` fails the agent. Both are measured against +the exact string being prepended and report a per-skill breakdown, so the +offender is named. The defaults sit either side of the bundled `conductor` +skill: enabling it on `claude` warns rather than breaking, while +accumulating several large skills errors. + +See `examples/skills-self-improving-workflow.yaml` for a complete example. + ## External File References The `!file` YAML tag lets you reference external files from any YAML field value. The file content is transparently inlined during loading, keeping workflow files concise and enabling reuse of prompts, schemas, and configuration across workflows. diff --git a/examples/skills-self-improving-workflow.yaml b/examples/skills-self-improving-workflow.yaml index 25fc9a25..2ca5f5a9 100644 --- a/examples/skills-self-improving-workflow.yaml +++ b/examples/skills-self-improving-workflow.yaml @@ -16,7 +16,9 @@ # by its `:` name. # * Claude: SKILL.md + references/*.md are eagerly prepended to the # agent's rendered prompt inside -# ... tags. +# ... tags. That is ~117KB (~29K tokens) for this +# skill, paid on every call and retry — `runtime.skill_injection` +# bounds it. # # Tri-state opt-in via list presence: # * Omit `skills:` → inherit `runtime.skills` @@ -24,6 +26,16 @@ # below, which doesn't need workflow knowledge) # * `skills: [name]` → explicit set, replaces the workflow default # +# Entries may also be filesystem paths, not just built-in names — see +# `docs/workflow-syntax.md` (Skills). A path starts with . or ~, or +# contains / or \; it resolves against THIS FILE's directory, and may +# point at a single skill directory or a root of them: +# +# runtime: +# skills: +# - conductor # built-in, ships in the wheel +# - ./team-skills/acme-widgets # versioned next to the workflow +# # Usage: # conductor run examples/skills-self-improving-workflow.yaml \ # --input task="Write a workflow that summarises a GitHub issue." diff --git a/plugins/conductor/skills/conductor/references/authoring.md b/plugins/conductor/skills/conductor/references/authoring.md index 7edd57b4..ec032cb9 100644 --- a/plugins/conductor/skills/conductor/references/authoring.md +++ b/plugins/conductor/skills/conductor/references/authoring.md @@ -220,6 +220,18 @@ See `examples/validator.yaml` for a complete example. `skills` enables reusable knowledge or capability bundles for provider-backed agents. The Conductor distribution ships one built-in skill — `conductor` — which packages the YAML schema, execution model, and authoring patterns (the same content this reference doc covers) so an agent can evaluate, improve, debug, or generate Conductor workflows. +**Names and paths.** Each entry is either a registered built-in name or a filesystem path. The distinction is syntactic: an entry is a path when it starts with `.` or `~`, or contains `/` or `\` — everything else must be a built-in name, so a bare `conductor` is never shadowed by a same-named local directory. A path points at either a single skill directory (one holding `SKILL.md`) or a root of them, which expands to every immediate child holding one (not recursive). Relative paths resolve against the **workflow file's directory**, the same rule `working_dir` uses, so a skill can be versioned alongside the workflow with no per-developer install step and the workflow behaves identically from any working directory. Skill paths are trusted input — no allowlist applies, since the same file can already run arbitrary shell via `type: script`. + +**`SKILL.md` frontmatter.** Every resolved skill must declare a non-empty `name` and `description` in valid YAML frontmatter. Use a block scalar whenever the text contains a colon followed by a space — `description: Does things. Triggers: a, b` is invalid YAML, and both the Copilot CLI and Claude Code skip such a skill *silently*. Conductor parses it and fails loudly instead, at validate time and at run time: + +```yaml +--- +name: acme-widgets +description: | + Internal ACME widget conventions. Triggers: widget, acme widget. +--- +``` + **Tri-state per-agent field (resolved via list presence):** - Omit `skills:` — inherit from `runtime.skills` - `skills: []` — explicit opt-out (no skills for this agent, regardless of workflow default) @@ -230,18 +242,27 @@ See `examples/validator.yaml` for a complete example. **Provider mechanism (same observable contract — "the agent has access to the named skill"):** - **Copilot** — the resolved skill directory is registered on the SDK session via `skill_directories`, so the agent discovers and loads skill content natively (progressive disclosure via `SKILL.md` frontmatter). This is more token-efficient than eager injection. - **Claude Agent SDK** — also native, through the Claude Code plugin surface: the plugin owning the skill is registered on the session and the skill enabled by its `:` name. Skills the workflow did not declare are suppressed, so `skills: []` really is an opt-out and ambient skills from the machine never load. -- **Claude** — the loader reads `SKILL.md` plus every `references/*.md` file in the skill directory and prepends them to the agent's rendered prompt inside `...` tags. Inserted between workspace instructions and the user prompt. +- **Claude** and **hermes** — the loader reads `SKILL.md` plus every `references/*.md` file in the skill directory and prepends them to the agent's rendered prompt inside `...` tags. Inserted between workspace instructions and the user prompt. + +**Two provider-specific limits worth knowing:** +- `claude-agent-sdk` has **no bare skill-directory option** — a skill is enabled by name through the plugin that ships it. A path skill outside a Claude Code plugin is therefore rejected at validation time, with both remedies named (package it as a plugin, or run that agent on `copilot`, which accepts the identical skill untouched). +- Eager injection has no progressive disclosure: the whole body is prepended on every call and every retry. The bundled `conductor` skill alone is ~117KB (~29K tokens). `runtime.skill_injection` bounds it — `warn_bytes` (default 64KB) warns, `max_bytes` (default 128KB) fails the agent, either can be `null` to disable. Providers with progressive disclosure are unaffected. -Not allowed on `script`, `human_gate`, `workflow`, `wait`, `set`, or `terminate` agent types. Unknown skill names fail at workflow validation time. +Not allowed on `script`, `human_gate`, `workflow`, `wait`, `set`, or `terminate` agent types. Unknown built-in names fail when the config loads; unresolvable paths and malformed `SKILL.md` files fail at workflow validation time and again at run time. ```yaml workflow: runtime: - skills: [conductor] # all provider-backed agents get the conductor skill + skills: + - conductor # built-in: ships in the wheel + - ./team-skills/acme-widgets # path: versioned next to this workflow + skill_injection: # only bounds eager-injection providers + warn_bytes: 65536 + max_bytes: 131072 agents: - name: workflow_reviewer - skills: [conductor] # per-agent opt-in (redundant here, kept for clarity) + skills: [conductor] # explicit set, replaces the workflow default prompt: "Review this workflow for correctness..." - name: simple_agent diff --git a/plugins/conductor/skills/conductor/references/yaml-schema.md b/plugins/conductor/skills/conductor/references/yaml-schema.md index 076e3b4e..3d886ea6 100644 --- a/plugins/conductor/skills/conductor/references/yaml-schema.md +++ b/plugins/conductor/skills/conductor/references/yaml-schema.md @@ -37,11 +37,24 @@ workflow: max_session_seconds: float # Wall-clock timeout per agent session in seconds (optional) default_reasoning_effort: string # Workflow-wide reasoning/thinking effort: low, medium, high, xhigh, max (optional) skills: [string] # Skills enabled for every provider-backed agent (default: []) - # Currently registered built-ins: "conductor" + # Each entry is a built-in NAME or a filesystem PATH. + # name: "conductor" (the only registered built-in) + # path: starts with . or ~, or contains / or \ + # -> a skill dir (holds SKILL.md), or a root of them + # which expands to every immediate child holding one + # Relative paths resolve against the workflow FILE's directory. + # Every resolved SKILL.md must declare `name` and `description` + # in valid YAML frontmatter (use `description: |` if it contains + # a colon followed by a space) -- both CLIs skip + # an unparseable skill silently. # Copilot loads natively via `skill_directories`; - # claude-agent-sdk loads natively via its plugin surface; - # Claude eagerly injects SKILL.md + references/*.md into the prompt. - mcp_servers: # MCP server configurations (ignored by claude-agent-sdk — uses CLI config) + # claude-agent-sdk loads natively via its plugin surface, so a + # path skill OUTSIDE a Claude Code plugin is rejected there; + # Claude and hermes eagerly inject SKILL.md + references/*.md. + skill_injection: # Bounds EAGERLY injected skill content (claude, hermes only). + warn_bytes: integer # Warn above this many bytes (default: 65536; null disables) + max_bytes: integer # Fail above this many bytes (default: 131072; null disables) + mcp_servers: # MCP server configurations : type: string # "stdio" (default), "http", or "sse" command: string # Command to run (required for stdio) @@ -164,7 +177,8 @@ agents: # - omit: inherit runtime.skills # - skills: [] explicit opt-out (no skills for this agent) # - skills: [a, b] explicit set, replaces the workflow default - skills: [string] # Skill names enabled for this agent (built-ins: "conductor") + skills: [string] # Built-in names and/or paths (see runtime.skills above) + # e.g. [conductor, ./team-skills/acme-widgets] # Per-agent retry policy (optional, not allowed for script, human_gate, workflow, or wait agents) retry: diff --git a/src/conductor/cli/run.py b/src/conductor/cli/run.py index a286821a..792e9728 100644 --- a/src/conductor/cli/run.py +++ b/src/conductor/cli/run.py @@ -1061,6 +1061,21 @@ def on_event(self, event: WorkflowEvent) -> None: for issue in issues: verbose_log(f" - {issue}", style="dim") + elif t == "skill_injection_warning": + # Only reaches the console through this branch: the executor's + # logger.warning has no handler behind it (see the comment at the + # emit site in executor/agent.py). + verbose_log( + f" WARNING: agent '{d.get('agent_name')}' injects " + f"{d.get('bytes', 0):,} bytes (~{d.get('approx_tokens', 0):,} tokens) " + f"of skill content on every call — provider " + f"'{d.get('provider')}' has no progressive disclosure " + f"(runtime.skill_injection.warn_bytes={d.get('warn_bytes', 0):,})", + style="yellow", + ) + if breakdown := d.get("breakdown"): + verbose_log(f" {breakdown}", style="dim") + elif t == "checkpoint_save_failed": n = d.get("consecutive_failures", 1) # Avoid spamming when every boundary fails (e.g. disk full): warn on diff --git a/src/conductor/config/schema.py b/src/conductor/config/schema.py index 40f2bc1c..6ef1685e 100644 --- a/src/conductor/config/schema.py +++ b/src/conductor/config/schema.py @@ -650,6 +650,42 @@ class SandboxConfig(BaseModel): """ +def _validate_skill_entries(entries: list[str]) -> list[str]: + """Validate the shape of ``skills:`` entries at config-load time. + + Bare **names** are checked eagerly against the built-in registry — + they need no base directory, so an unknown name still surfaces at + load time exactly as it did before path entries existed. + + **Path** entries are only shape-checked here. Resolving them needs + the workflow file's directory, which the schema does not have, so + that happens in :func:`conductor.config.validator.validate_workflow_config` + (statically) and in ``AgentExecutor`` (at run time). + + Args: + entries: The raw ``skills:`` list. + + Returns: + The list unchanged. + + Raises: + ValueError: If an entry is not a non-empty string, or is a bare + name that no built-in skill matches. + """ + from conductor.skills import SkillNotFoundError, get_skill_directory, is_path_entry + + for entry in entries: + if not isinstance(entry, str) or not entry.strip(): + raise ValueError(f"skills entries must be non-empty strings, got {entry!r}") + if is_path_entry(entry): + continue + try: + get_skill_directory(entry) + except SkillNotFoundError as exc: + raise ValueError(str(exc)) from exc + return entries + + class AgentDef(BaseModel): """Definition for a single agent in the workflow. @@ -1083,9 +1119,29 @@ class AgentDef(BaseModel): """ skills: list[str] | None = None - """Opt this agent into a list of named built-in skills. + r"""Opt this agent into a list of skills. + + Each entry is either a **registered built-in name** (e.g. + ``conductor``) or a **filesystem path**. An entry is treated as a + path when it starts with ``.`` or ``~``, or contains ``/`` or ``\``; + everything else must be a built-in name, so a bare name can never be + shadowed by a same-named local directory. + + A path may point at either granularity: + + * a **skill directory** — one containing ``SKILL.md`` + * a **skills root** — a directory of skill directories, which + expands to every immediate child containing a ``SKILL.md`` + + Relative paths resolve against the workflow file's directory + (consistent with ``working_dir``), so a skill can be versioned + alongside the workflow with no per-developer install step. + + Skill paths are trusted input: a ``SKILL.md`` is injected into the + agent's context, but the same workflow file can already declare + ``type: script`` steps running arbitrary shell, so no additional + allowlist applies. - Each entry is a skill name registered in :mod:`conductor.skills`. The agent receives that skill's content via whichever mechanism the provider supports natively: @@ -1097,12 +1153,13 @@ class AgentDef(BaseModel): ``:`` name, so the CLI loads only the ``SKILL.md`` frontmatter up front. Skills the workflow did not declare are filtered out of the model's listing instead of being inherited - from the machine. + from the machine. The SDK has no bare skill-directory surface, so + a path skill that is not inside a Claude Code plugin is rejected. * **Claude** — ``SKILL.md`` plus ``references/*.md`` is eagerly injected into the agent's rendered prompt, wrapped in ```` tags. There is no native skill surface on the Anthropic API without adopting the container/code-execution - beta. + beta. Injected size is bounded by ``runtime.skill_injection``. Tri-state semantics via list presence: @@ -1118,13 +1175,20 @@ class AgentDef(BaseModel): Enables agents to evaluate, improve, debug, or generate Conductor workflows. + Every resolved skill's ``SKILL.md`` must have valid YAML frontmatter + declaring ``name`` and ``description``; both Copilot and Claude Code + skip an unparseable skill in silence, so Conductor fails loudly + instead. + Only applies to provider-backed agents (type='agent' or None). Example YAML:: agents: - name: workflow_reviewer - skills: [conductor] + skills: + - conductor # built-in + - ./team-skills/acme-widgets # versioned with the workflow prompt: "Review this workflow for correctness..." """ @@ -1197,24 +1261,16 @@ def validate_timeout(cls, v: int | None) -> int | None: @field_validator("skills") @classmethod def validate_skills(cls, v: list[str] | None) -> list[str] | None: - """Ensure every skill name resolves to a known built-in. + """Validate ``skills:`` entry shape and built-in names. - Validates at load time so unknown skill names surface in - ``conductor validate`` and ``conductor run`` startup rather than - at execute time. Empty lists are allowed (explicit opt-out). + Unknown built-in names surface at load time as before. Path + entries need the workflow file's directory to resolve, so they + are only shape-checked here — see :func:`_validate_skill_entries`. + Empty lists are allowed (explicit opt-out). """ if v is None: return v - from conductor.skills import SkillNotFoundError, get_skill_directory - - for name in v: - if not isinstance(name, str) or not name.strip(): - raise ValueError(f"skills entries must be non-empty strings, got {name!r}") - try: - get_skill_directory(name) - except SkillNotFoundError as exc: - raise ValueError(str(exc)) from exc - return v + return _validate_skill_entries(v) @field_validator("duration", mode="before") @classmethod @@ -2362,6 +2418,70 @@ def _normalize_spill_dir(cls, v: str | None) -> str | None: return v.strip() or None +class SkillInjectionConfig(BaseModel): + """Size limits for eagerly injected skill content. + + Providers without a native skill surface (``claude``, ``hermes``) + have no progressive disclosure: :class:`~conductor.executor.agent.AgentExecutor` + prepends every enabled skill's ``SKILL.md`` **plus its entire + ``references/`` tree** to the rendered prompt, on every agent call and + every retry. The bundled ``conductor`` skill alone is ~117KB (~29K + tokens), so an unbounded list is easy to turn into most of a context + window by accident. + + Both limits are measured against the exact string that gets + prepended. Setting either to ``null`` disables that limit. + + Example YAML:: + + runtime: + skill_injection: + warn_bytes: 65536 # warn above 64KB + max_bytes: 131072 # fail above 128KB + """ + + # Frozen for the reason ``ProviderSettings`` documents: this model carries a + # cross-field invariant in a ``model_validator(mode="after")``, and that does + # not re-fire on per-attribute assignment even under the enclosing + # ``RuntimeConfig``'s ``validate_assignment=True``. + model_config = ConfigDict(extra="forbid", frozen=True) + + warn_bytes: int | None = Field(default=64 * 1024, ge=0) + """Log a warning when injected skill content exceeds this many bytes. + + ``None`` disables the warning. The 64KB default is below the bundled + ``conductor`` skill's ~117KB so that combination is surfaced rather + than passing silently. + """ + + max_bytes: int | None = Field(default=128 * 1024, ge=0) + """Fail the agent when injected skill content exceeds this many bytes. + + ``None`` disables the limit. The 128KB default is above the bundled + ``conductor`` skill's ~117KB, so enabling it does not break an + existing single-skill workflow — it catches accumulation. + """ + + @model_validator(mode="after") + def validate_thresholds(self) -> SkillInjectionConfig: + """Reject a warning threshold above the hard limit. + + Such a config can never warn: the error fires first, so the + warning is unreachable and the author's intent is ambiguous. + """ + if ( + self.warn_bytes is not None + and self.max_bytes is not None + and self.warn_bytes > self.max_bytes + ): + raise ValueError( + f"skill_injection.warn_bytes ({self.warn_bytes}) must not exceed " + f"max_bytes ({self.max_bytes}); the error would fire before the " + "warning could ever be emitted." + ) + return self + + class RuntimeConfig(BaseModel): """Provider and runtime configuration.""" @@ -2509,10 +2629,11 @@ def _coerce_provider(cls, value: Any) -> Any: skills: list[str] = Field(default_factory=list) """Workflow-wide default skills for every provider-backed agent. - Each entry is a skill name registered in :mod:`conductor.skills` (e.g. - ``conductor``). Every provider-backed agent inherits this list as its - default; individual agents override by setting their own ``skills:`` - field (use ``skills: []`` for explicit opt-out). + Each entry is either a registered built-in name (e.g. ``conductor``) + or a filesystem path — see :attr:`AgentDef.skills` for the full + resolution rules. Every provider-backed agent inherits this list as + its default; individual agents override by setting their own + ``skills:`` field (use ``skills: []`` for explicit opt-out). Skill content reaches the model differently per provider: @@ -2521,32 +2642,35 @@ def _coerce_provider(cls, value: Any) -> Any: ``--plugin-dir`` and the skill enabled by its ``:`` name, so the CLI loads it on demand * **Claude** — eagerly injected into the rendered prompt inside - ``...`` tags + ``...`` tags, bounded by + :attr:`skill_injection` - Defaults to an empty list (no skills). Phase 1 ships one built-in - skill (``conductor``); user-defined skill directories will be added - in a follow-up. + Defaults to an empty list (no skills). Conductor ships one built-in + skill (``conductor``); anything else is referenced by path. Example YAML:: runtime: - skills: [conductor] + skills: + - conductor + - ./team-skills/acme-widgets + """ + + skill_injection: SkillInjectionConfig = Field(default_factory=SkillInjectionConfig) + """Size limits for *eagerly injected* skill content. + + Only affects providers without a native skill surface (``claude``, + ``hermes``), where the full skill body is prepended to every agent + call. Providers with progressive disclosure (``copilot``, + ``claude-agent-sdk``) send only frontmatter up front and are + unaffected. """ @field_validator("skills") @classmethod def validate_skills(cls, v: list[str]) -> list[str]: - """Ensure every workflow-default skill name resolves to a known built-in.""" - from conductor.skills import SkillNotFoundError, get_skill_directory - - for name in v: - if not isinstance(name, str) or not name.strip(): - raise ValueError(f"skills entries must be non-empty strings, got {name!r}") - try: - get_skill_directory(name) - except SkillNotFoundError as exc: - raise ValueError(str(exc)) from exc - return v + """Validate workflow-default ``skills:`` entry shape and built-in names.""" + return _validate_skill_entries(v) class WorkflowDef(BaseModel): diff --git a/src/conductor/config/validator.py b/src/conductor/config/validator.py index 1b9d6f7b..b173f15f 100644 --- a/src/conductor/config/validator.py +++ b/src/conductor/config/validator.py @@ -15,11 +15,25 @@ from jinja2 import Environment, meta, nodes from conductor.exceptions import ConfigurationError -from conductor.providers.capabilities import ProviderCapabilities, get_capabilities +from conductor.providers.capabilities import ( + ProviderCapabilities, + get_capabilities, + uses_native_skills, +) +from conductor.skills import ( + BYTES_PER_TOKEN_ESTIMATE, + SkillError, + SkillPluginError, + is_path_entry, + load_skill_content, + resolve_skill_plugin, + resolve_skills, +) from conductor.templating import is_jinja_template if TYPE_CHECKING: from conductor.config.schema import AgentDef, WorkflowConfig + from conductor.skills import ResolvedSkill # Shared Jinja2 environment used purely for AST parsing of template strings. @@ -256,7 +270,7 @@ def validate_workflow_config( # Cross-check workflow features against each provider's declared # ProviderCapabilities (issue #241). Surfaces silent capability # mismatches at validate time rather than at runtime. - cap_errors, cap_warnings = _validate_provider_capabilities(config) + cap_errors, cap_warnings = _validate_provider_capabilities(config, workflow_path) errors.extend(cap_errors) warnings.extend(cap_warnings) @@ -1550,6 +1564,7 @@ def _resolved_provider_name(agent: AgentDef, default: str) -> str: def _validate_provider_capabilities( config: WorkflowConfig, + workflow_path: Path | None = None, ) -> tuple[list[str], list[str]]: """Cross-check workflow features against each provider's declared capabilities. @@ -1569,6 +1584,15 @@ def _validate_provider_capabilities( Capabilities are resolved lazily without instantiating providers so this runs cleanly in environments without API keys / network. + + Args: + config: The workflow configuration to check. + workflow_path: Path of the workflow file, used as the base directory + for relative skill paths. When ``None``, checks needing the + filesystem are skipped with a warning rather than falling back to + ``Path.cwd()`` as ``_validate_subworkflow_refs`` does — resolving + a skill path against an arbitrary working directory would report + failures that say nothing about the workflow. """ errors: list[str] = [] warnings: list[str] = [] @@ -1584,6 +1608,11 @@ def _validate_provider_capabilities( runtime_max_session_seconds = config.workflow.runtime.max_session_seconds runtime_working_dir = config.workflow.runtime.working_dir runtime_skills = config.workflow.runtime.skills + skill_limits = config.workflow.runtime.skill_injection + skill_base_dir = workflow_path.resolve().parent if workflow_path is not None else None + # Keyed by the entry tuple, so agents sharing a skill list resolve once. + # A ``str`` value is a cached resolution failure. + skill_cache: dict[tuple[str, ...], list[ResolvedSkill] | str] = {} # Cache per provider name so we don't re-resolve for every agent. cache: dict[str, ProviderCapabilities] = {} @@ -1657,6 +1686,134 @@ def _check_agent_tools(agent: AgentDef, provider_name: str, caps: ProviderCapabi f"'tools: []' to disable the built-in tools." ) + def _check_agent_skills( + agent: AgentDef, provider_name: str, caps: ProviderCapabilities + ) -> None: + """Resolve an agent's effective skills and check them against its provider. + + Three failure classes, all of which otherwise leave the agent running + without the knowledge its author asked for: + + * The entry does not resolve — unknown built-in name, missing path, or + a directory holding no ``SKILL.md``. + * The resolved ``SKILL.md`` has broken or incomplete frontmatter. Both + Copilot and Claude Code skip such a skill *silently*, so this is the + only place a user finds out. + * The provider cannot deliver it: ``claude-agent-sdk`` has no bare + skill-directory surface, so a skill outside a Claude Code plugin is + unreachable there even though Copilot loads it fine. + + Eager-injection providers additionally get the ``runtime.skill_injection`` + budget applied here, so an oversized preamble is reported before a run + starts rather than on first execution. + """ + entries = agent.skills if agent.skills is not None else runtime_skills + # A provider that declares skills=False already produced an error + # above; re-reporting resolution failures for it would be noise. + if not entries or not caps.skills: + return + # Only *relative* entries need a base directory. ``~/skills`` becomes + # absolute under expanduser(), so it is checkable too — narrowing here + # rather than on is_path_entry() keeps absolute entries validated + # instead of silently waved through. + unresolvable = [ + entry + for entry in entries + if is_path_entry(entry) and not Path(entry).expanduser().is_absolute() + ] + if skill_base_dir is None and unresolvable: + # Every other skip in this function has a second reporting path; + # this one has none, so say so rather than returning mute. Same + # don't-return-mute pattern as ``_caps_for``, at warning severity + # rather than error because the skill is still resolved at run time. + warnings.append( + f"Agent '{agent.name}': relative skill path(s) {sorted(unresolvable)!r} " + "were not checked because no workflow file path was supplied, so there " + "is no base directory to resolve them against. They are still resolved " + "at run time." + ) + return + + key = tuple(entries) + if key not in skill_cache: + try: + skill_cache[key] = resolve_skills( + list(entries), base_dir=skill_base_dir, on_warning=warnings.append + ) + except SkillError as exc: + skill_cache[key] = str(exc) + resolved = skill_cache[key] + if isinstance(resolved, str): + errors.append(f"Agent '{agent.name}': {resolved}") + return + + if provider_name == "claude-agent-sdk": + for item in resolved: + try: + plugin = resolve_skill_plugin(item.directory) + except SkillPluginError as exc: + errors.append( + f"Agent '{agent.name}': skill {item.source!r} resolves to " + f"{item.directory}, whose Claude Code plugin cannot be " + f"loaded: {exc}" + ) + continue + if plugin is None: + errors.append( + f"Agent '{agent.name}': skill {item.source!r} resolves to " + f"{item.directory}, which is not inside a Claude Code " + f"plugin. Provider 'claude-agent-sdk' can only enable a " + f"skill through the plugin that owns it — it has no " + f"skill-directory option. Package the skill as a plugin " + f"(add ../.claude-plugin/plugin.json and move the skill " + f"under /skills/), or run this agent on 'copilot', " + f"which loads skill directories directly." + ) + + if uses_native_skills(provider_name) is False: + _check_skill_injection_budget(agent, provider_name, resolved) + + def _check_skill_injection_budget( + agent: AgentDef, provider_name: str, resolved: list[ResolvedSkill] + ) -> None: + """Apply ``runtime.skill_injection`` limits statically. + + Measures the exact string ``AgentExecutor`` would prepend, so the + numbers reported here match the ones enforced at run time. + """ + # Reading the content can fail on an unreadable ``references/*.md``, + # which ``read_skill_frontmatter`` never opens and so cannot have + # caught upstream. Collect it like any other validation failure — + # letting it escape prints a traceback out of ``conductor validate``. + try: + content = load_skill_content([(item.name, item.directory) for item in resolved]) + except SkillError as exc: + errors.append(f"Agent '{agent.name}': {exc}") + return + if not content: + return + size = len(content.encode("utf-8")) + approx_tokens = size // BYTES_PER_TOKEN_ESTIMATE + detail = ( + f"Agent '{agent.name}' eagerly injects {size:,} bytes " + f"(~{approx_tokens:,} tokens) of skill content on every call: provider " + f"'{provider_name}' has no progressive disclosure, so this is paid " + f"again on every retry." + ) + if skill_limits.max_bytes is not None and size > skill_limits.max_bytes: + errors.append( + f"{detail} That is over the runtime.skill_injection.max_bytes " + f"limit of {skill_limits.max_bytes:,}. Enable fewer skills, trim " + f"their references/ trees, run the agent on a provider with " + f"progressive disclosure (copilot, claude-agent-sdk), or raise " + f"the limit." + ) + elif skill_limits.warn_bytes is not None and size > skill_limits.warn_bytes: + warnings.append( + f"{detail} That is over the runtime.skill_injection.warn_bytes " + f"threshold of {skill_limits.warn_bytes:,}." + ) + def _check_agent_capabilities( agent: AgentDef, provider_name: str, caps: ProviderCapabilities ) -> None: @@ -1777,6 +1934,8 @@ def _check_agent_capabilities( f"'skills: []', or override the agent to a skill-aware provider." ) + _check_agent_skills(agent, provider_name, caps) + # All provider-backed agents that run at workflow scope: top-level agents # PLUS for_each inline agents (``ForEachDef.agent``), which inherit the # workflow-level ``mcp_servers`` / ``max_session_seconds`` and run with diff --git a/src/conductor/engine/workflow.py b/src/conductor/engine/workflow.py index 4af15aad..dbcfba76 100644 --- a/src/conductor/engine/workflow.py +++ b/src/conductor/engine/workflow.py @@ -418,6 +418,8 @@ def __init__( workflow_tools=config.tools, instructions_preamble=self._instructions_preamble, workflow_skills=self._workflow_skills, + workflow_dir=self._workflow_dir, + skill_injection=config.workflow.runtime.skill_injection, ) self.provider = provider # Keep for backward compatibility else: @@ -968,6 +970,8 @@ async def _get_executor_for_agent(self, agent: AgentDef) -> AgentExecutor: workflow_tools=self.config.tools, instructions_preamble=self._instructions_preamble, workflow_skills=self._workflow_skills, + workflow_dir=self._workflow_dir, + skill_injection=self.config.workflow.runtime.skill_injection, ) elif self.executor is not None: # Single provider mode (backward compatibility) diff --git a/src/conductor/executor/agent.py b/src/conductor/executor/agent.py index caef1185..824c9c45 100644 --- a/src/conductor/executor/agent.py +++ b/src/conductor/executor/agent.py @@ -8,16 +8,20 @@ import asyncio import contextlib +import logging from typing import TYPE_CHECKING, Any, get_args -from conductor.exceptions import ValidationError +from conductor.exceptions import ExecutionError, ValidationError from conductor.executor.output import parse_json_output, validate_output from conductor.executor.template import TemplateRenderer from conductor.providers.base import AgentOutput, EventCallback from conductor.providers.context_tier import ContextTier from conductor.providers.reasoning import ReasoningEffort +from conductor.skills import BYTES_PER_TOKEN_ESTIMATE from conductor.templating import is_jinja_template +logger = logging.getLogger(__name__) + def _verbose_log(message: str, style: str = "dim") -> None: """Lazy import wrapper for verbose_log to avoid circular imports.""" @@ -34,8 +38,11 @@ def _verbose_log_section(title: str, content: str) -> None: if TYPE_CHECKING: - from conductor.config.schema import AgentDef + from pathlib import Path + + from conductor.config.schema import AgentDef, SkillInjectionConfig from conductor.providers.base import AgentProvider + from conductor.skills import ResolvedSkill def resolve_agent_tools( @@ -102,6 +109,8 @@ def __init__( workflow_tools: list[str] | None = None, instructions_preamble: str | None = None, workflow_skills: list[str] | None = None, + workflow_dir: Path | None = None, + skill_injection: SkillInjectionConfig | None = None, ) -> None: """Initialize the AgentExecutor. @@ -114,11 +123,23 @@ def __init__( ``runtime.skills``). Agents inherit this list unless they set their own ``skills:`` field — ``[]`` opts out explicitly, ``[name, ...]`` overrides the default. + workflow_dir: Directory of the workflow file, used as the base + for relative skill paths (consistent with ``working_dir``). + Falls back to the process working directory. + skill_injection: Size limits for eager skill-content injection + (from ``runtime.skill_injection``). Defaults apply when + omitted. """ self.provider = provider self.workflow_tools = workflow_tools or [] self.instructions_preamble = instructions_preamble self._workflow_skills: list[str] = list(workflow_skills or []) + self._workflow_dir = workflow_dir + if skill_injection is None: + from conductor.config.schema import SkillInjectionConfig + + skill_injection = SkillInjectionConfig() + self._skill_injection = skill_injection self.renderer = TemplateRenderer() def _render_enum_field( @@ -248,7 +269,7 @@ async def execute( rendered_prompt = self.renderer.render(agent.prompt, context) # Prepend prompt prefix (workspace instructions + optional skills) - prefix = self._build_prompt_prefix(agent) + prefix = self._build_prompt_prefix(agent, event_callback) if prefix: rendered_prompt = prefix + rendered_prompt @@ -298,12 +319,11 @@ async def execute( # rendered_prompt above and ignore this). skill_dirs: list[str] | None = None if getattr(self.provider, "supports_native_skills", False): - skill_names = self._resolve_skills_for_agent(agent) - if skill_names: - from conductor.skills import resolve_skill_directories - - skill_dirs = [str(p) for p in resolve_skill_directories(skill_names)] - _verbose_log(f" Skills: {skill_names}") + skill_entries = self._resolve_skills_for_agent(agent) + if skill_entries: + resolved = self._resolve_skills(skill_entries) + skill_dirs = [str(item.directory) for item in resolved] + _verbose_log(f" Skills: {[item.name for item in resolved]}") # Execute via provider output = await self.provider.execute( @@ -346,6 +366,12 @@ def render_prompt(self, agent: AgentDef, context: dict[str, Any]) -> str: This is useful for debugging or dry-run mode. + No ``event_callback``: the only caller is the ``validator:`` block's + re-render of the primary prompt, and the agent's own ``execute`` has + already emitted any ``skill_injection_warning`` for the same content. + The warning still reaches the log from here; only the duplicate event + is suppressed. + Args: agent: Agent definition from workflow config. context: Context for prompt rendering. @@ -356,6 +382,11 @@ def render_prompt(self, agent: AgentDef, context: dict[str, Any]) -> str: Raises: TemplateError: If prompt rendering fails. + SkillNotFoundError: If an enabled skill entry cannot be resolved. + SkillManifestError: If a resolved skill's ``SKILL.md`` is missing, + unparseable, or incomplete. + ExecutionError: If the provider does not support skills, or the + eagerly injected content exceeds ``runtime.skill_injection``. """ rendered = self.renderer.render(agent.prompt, context) prefix = self._build_prompt_prefix(agent) @@ -376,14 +407,142 @@ def _resolve_skills_for_agent(self, agent: AgentDef) -> list[str]: agent is not a provider-backed type (script / wait / set / terminate / human_gate / workflow — schema rejects ``skills`` on these so this is defensive only). + + The ``capabilities.skills`` check lives here rather than in + :meth:`_build_prompt_prefix` so it covers **both** delivery paths. + Native providers never reach the eager-injection branch, so a + provider that declared ``skills=False`` while supporting native + loading would otherwise skip the check entirely. + + Raises: + ExecutionError: If skills are enabled for an agent whose + provider declares ``capabilities.skills=False``. """ if agent.type not in (None, "agent"): return [] - if agent.skills is not None: - return list(agent.skills) - return list(self._workflow_skills) + entries = list(agent.skills) if agent.skills is not None else list(self._workflow_skills) + if entries: + self._reject_unsupported_skills(agent, entries) + return entries + + def _resolve_skills(self, entries: list[str]) -> list[ResolvedSkill]: + """Resolve ``skills:`` entries against the workflow file's directory. + + Both delivery paths — native ``skill_directories`` and eager + preamble injection — go through here so names, paths, and + ``skills/`` roots behave identically regardless of provider. + """ + from conductor.skills import resolve_skills + + return resolve_skills( + entries, + base_dir=self._workflow_dir, + on_warning=lambda message: _verbose_log(f" Skills: {message}", style="yellow"), + ) + + def _enforce_injection_budget( + self, + agent: AgentDef, + resolved: list[ResolvedSkill], + content: str, + event_callback: EventCallback | None = None, + ) -> None: + """Apply ``runtime.skill_injection`` limits to eager skill content. + + Measured against the exact string being prepended, so the number + reported is the number actually paid — on every call to this + agent and on every retry. (A ``validator:`` block's own grading + call bypasses prompt rendering and embeds only a truncated + excerpt, so it does not re-pay this.) - def _build_prompt_prefix(self, agent: AgentDef) -> str: + Args: + agent: The agent the content is being injected for. + resolved: The skills that produced the content, used to give + a per-skill breakdown when a limit is hit. + content: The rendered skill preamble. + event_callback: Optional sink for ``skill_injection_warning`` + when the content exceeds ``warn_bytes``. The warning is + also logged, but Conductor installs no logging handlers, + so this is the channel that actually reaches the user. + + Raises: + ExecutionError: If the content exceeds ``max_bytes``. + """ + limits = self._skill_injection + size = len(content.encode("utf-8")) + approx_tokens = size // BYTES_PER_TOKEN_ESTIMATE + provider = type(self.provider).__name__ + if limits.max_bytes is not None and size > limits.max_bytes: + raise ExecutionError( + f"Agent '{agent.name}': eagerly injected skill content is " + f"{size:,} bytes (~{approx_tokens:,} tokens), over the " + f"runtime.skill_injection.max_bytes limit of {limits.max_bytes:,}. " + f"Provider '{provider}' has no native skill surface, so " + f"this is prepended to every call and every retry.\n" + f"{self._skill_size_breakdown(resolved)}", + agent_name=agent.name, + suggestion=( + "Enable fewer skills on this agent, trim the skills' " + "references/ trees, run it on a provider with progressive " + "disclosure (copilot, claude-agent-sdk), or raise " + "runtime.skill_injection.max_bytes." + ), + ) + if limits.warn_bytes is not None and size > limits.warn_bytes: + breakdown = self._skill_size_breakdown(resolved) + logger.warning( + "Agent %r: eagerly injecting %s bytes (~%s tokens) of skill content " + "on every call — provider %r has no progressive disclosure. %s", + agent.name, + f"{size:,}", + f"{approx_tokens:,}", + provider, + breakdown, + ) + # The log alone reaches nobody: Conductor installs no logging + # handlers, so this surfaces via logging.lastResort as an + # unattributed line on stderr, interleaved with console output and + # absent from the JSONL log and the dashboard. Since the defaults + # trip this for the bundled skill on every eager-provider call, it + # has to travel the event channel too — same both-halves pattern as + # ``checkpoint_save_failed`` in engine/workflow.py. + if event_callback is not None: + event_callback( + "skill_injection_warning", + { + "agent_name": agent.name, + "bytes": size, + "approx_tokens": approx_tokens, + "warn_bytes": limits.warn_bytes, + "provider": provider, + "breakdown": breakdown, + }, + ) + + @staticmethod + def _skill_size_breakdown(resolved: list[ResolvedSkill]) -> str: + """Summarise each skill's on-disk injected size, largest first. + + Sizes are raw file bytes, so they total slightly below the measured + rendered size, which also carries the ```` envelope and one + ```` tag per entry. + """ + sizes: list[tuple[str, int]] = [] + for item in resolved: + total = 0 + for path in ( + item.directory / "SKILL.md", + *sorted((item.directory / "references").glob("*.md")), + ): + with contextlib.suppress(OSError): + total += path.stat().st_size + sizes.append((item.name, total)) + sizes.sort(key=lambda pair: pair[1], reverse=True) + return "Per skill: " + ", ".join(f"{name} {size:,}B" for name, size in sizes) + + def _build_prompt_prefix( + self, agent: AgentDef, event_callback: EventCallback | None = None + ) -> str: """Build the prefix to prepend before an agent's rendered prompt. Combines workspace instructions and (on providers that lack @@ -396,17 +555,55 @@ def _build_prompt_prefix(self, agent: AgentDef) -> str: (:attr:`AgentProvider.supports_native_skills`), the skill directories are passed to the SDK on the provider side and we skip preamble injection to avoid double-loading. + + Raises: + ExecutionError: If the injected content exceeds + ``runtime.skill_injection``, or (via + :meth:`_resolve_skills_for_agent`) if the provider + declares it does not support skills. """ parts: list[str] = [] if self.instructions_preamble: parts.append(self.instructions_preamble) if not getattr(self.provider, "supports_native_skills", False): - skill_names = self._resolve_skills_for_agent(agent) - if skill_names: - from conductor.skills import load_skill_content, resolve_skill_directories + skill_entries = self._resolve_skills_for_agent(agent) + if skill_entries: + from conductor.skills import load_skill_content - dirs = resolve_skill_directories(skill_names) - content = load_skill_content(list(zip(skill_names, dirs, strict=True))) + resolved = self._resolve_skills(skill_entries) + content = load_skill_content([(item.name, item.directory) for item in resolved]) if content: + self._enforce_injection_budget(agent, resolved, content, event_callback) parts.append(content) return "".join(parts) + + def _reject_unsupported_skills(self, agent: AgentDef, skill_entries: list[str]) -> None: + """Refuse skills on a provider that declares it does not support them. + + Mirrors the ``capabilities.skills`` check in + :func:`conductor.config.validator.validate_workflow_config`. + ``conductor validate`` already rejects the combination, but + ``conductor run`` never invokes the static validator — so without + this the declaration holds in one command and is silently + contradicted in the other. + + A provider with no ``CAPABILITIES`` is left alone. That set is + exactly the abstract ones: ``AgentProvider.__init_subclass__`` + raises at import time unless a subclass either declares + ``CAPABILITIES`` or opts out with ``abstract=True``, so a real + provider cannot reach this branch by forgetting to declare one. + """ + capabilities = getattr(type(self.provider), "CAPABILITIES", None) + if capabilities is None or capabilities.skills: + return + raise ExecutionError( + f"Agent '{agent.name}' declares skills={skill_entries!r} but provider " + f"'{type(self.provider).__name__}' does not support skills " + f"(capabilities.skills=False).", + agent_name=agent.name, + suggestion=( + "Remove the skills, opt out with 'skills: []', or override the " + "agent to a skill-aware provider. 'conductor validate' reports " + "this before a run starts." + ), + ) diff --git a/src/conductor/providers/capabilities.py b/src/conductor/providers/capabilities.py index 26cd1b4a..7230921c 100644 --- a/src/conductor/providers/capabilities.py +++ b/src/conductor/providers/capabilities.py @@ -20,6 +20,8 @@ from __future__ import annotations +import inspect +import logging from typing import TYPE_CHECKING, Final, Literal from pydantic import BaseModel, ConfigDict, field_validator @@ -29,6 +31,8 @@ if TYPE_CHECKING: from conductor.providers.base import AgentProvider +logger = logging.getLogger(__name__) + # Stable / experimental are the only tiers for v1. Promotion criteria for # experimental → stable are documented in docs/providers/experimental.md. @@ -353,6 +357,65 @@ def known_provider_names() -> tuple[str, ...]: return tuple(_PROVIDER_CLASS_PATHS) + tuple(_NOT_YET_IMPLEMENTED_PROVIDERS) +def uses_native_skills(provider_type: str) -> bool | None: + """Whether a provider loads skills natively, resolved without instantiating. + + Mirrors :func:`get_capabilities`' lazy-import approach so it is safe to + call from ``conductor validate``. Providers declare + ``supports_native_skills`` as an instance ``property``, so this reads the + descriptor and evaluates it with no instance. + + Args: + provider_type: Provider name as it appears in workflow YAML. + + Returns: + ``True`` when the provider forwards skill directories to its SDK, + ``False`` when :class:`~conductor.executor.agent.AgentExecutor` + eagerly injects skill content instead, or ``None`` when the answer + cannot be determined without constructing the provider — the + property consults instance state, the provider is unknown or not + yet implemented, or the declaration cannot be read without side + effects. + Callers should skip mechanism-specific static checks on ``None`` + rather than assume either branch. + """ + if provider_type in _NOT_YET_IMPLEMENTED_PROVIDERS: + return None + dotted_path = _PROVIDER_CLASS_PATHS.get(provider_type) + if dotted_path is None: + return None + + module_path, _, class_name = dotted_path.partition(":") + import importlib + + try: + provider_cls = getattr(importlib.import_module(module_path), class_name) + except (ImportError, AttributeError): + return None + + declared = inspect.getattr_static(provider_cls, "supports_native_skills", None) + if isinstance(declared, bool): + return declared + if isinstance(declared, property) and declared.fget is not None: + try: + return bool(declared.fget(None)) + except (AttributeError, TypeError): + # The property dereferences ``self``, so the only honest answer is + # "ask a real instance". Split from the broader handler below so + # this expected case stays silent while a genuine bug inside the + # property is still logged rather than vanishing into "undetermined". + return None + except Exception: + logger.warning( + "Provider %r raised while resolving supports_native_skills " + "statically; treating the mechanism as undetermined.", + provider_type, + exc_info=True, + ) + return None + return None + + __all__ = [ "ProviderCapabilities", "ProviderTier", @@ -360,4 +423,5 @@ def known_provider_names() -> tuple[str, ...]: "StructuredOutputMode", "get_capabilities", "known_provider_names", + "uses_native_skills", ] diff --git a/src/conductor/providers/hermes.py b/src/conductor/providers/hermes.py index 94fa5a3d..4e3baeb4 100644 --- a/src/conductor/providers/hermes.py +++ b/src/conductor/providers/hermes.py @@ -99,6 +99,16 @@ class HermesProvider(AgentProvider): checkpoint_resume=True, usage_tracking=True, concurrent_safe=True, + # Hermes has no native skill surface, but skills are not an allowed + # experimental carve-out: ``AgentExecutor`` eagerly injects SKILL.md + # plus references/*.md into the rendered prompt for any provider whose + # ``supports_native_skills`` is False, entirely upstream of this class. + # That path is provider-agnostic and already works here, so declaring + # False would be inaccurate — and would make the ``skill_directories`` + # docstring on ``execute()`` describe an unreachable branch, since + # config/validator.py rejects ``skills:`` on a skills=False provider. + # Injected size is bounded by ``runtime.skill_injection``. + skills=True, # Hermes runs its own internal toolsets (mcp_tools=False); a # per-agent working directory has no meaning for the session. working_dir=False, diff --git a/src/conductor/skills/__init__.py b/src/conductor/skills/__init__.py index 0d5258ee..e055c75e 100644 --- a/src/conductor/skills/__init__.py +++ b/src/conductor/skills/__init__.py @@ -23,35 +23,58 @@ by its ``:`` name, also progressive. Skills the workflow did not declare are filtered out of the model's listing rather than inherited from the machine. - * **Claude** — eager preamble injection of ``SKILL.md`` plus - ``references/*.md`` into the agent's rendered prompt. The - Anthropic API has no server-side skill surface without adopting - the container/code-execution beta. + * **Claude** and **Hermes** — eager preamble injection of + ``SKILL.md`` plus ``references/*.md`` into the agent's rendered + prompt. Neither has a native skill surface: the Anthropic API + offers none without adopting the container/code-execution beta, + and hermes runs its own internal toolsets. Injected size is + bounded by ``runtime.skill_injection``, since the whole body is + re-sent on every call and every retry. -Phase 1 ships one built-in skill: ``conductor``, sourced from -``plugins/conductor/skills/conductor/``. Future phases will add -user-defined skill directories, executable skill resources, and -progressive disclosure via MCP. +A ``skills:`` entry is either a **built-in name** or a **filesystem +path** — see :func:`conductor.skills.registry.resolve_skills`. Conductor +ships one built-in skill, ``conductor``, sourced from +``plugins/conductor/skills/conductor/``. Discovering skills already +installed in the user's environment is tracked separately in issue #362; +discovery locations differ per provider, so a single switch would hand +different skill sets to different agents inside one run. """ -from conductor.skills.loader import load_skill_content +from conductor.skills.errors import SkillError +from conductor.skills.frontmatter import ( + SkillFrontmatter, + SkillManifestError, + read_skill_frontmatter, +) +from conductor.skills.loader import BYTES_PER_TOKEN_ESTIMATE, load_skill_content from conductor.skills.registry import ( + ResolvedSkill, SkillNotFoundError, SkillPlugin, SkillPluginError, + WarningSink, get_skill_directory, + is_path_entry, list_builtin_skills, - resolve_skill_directories, resolve_skill_plugin, + resolve_skills, ) __all__ = [ + "BYTES_PER_TOKEN_ESTIMATE", + "ResolvedSkill", + "SkillError", + "SkillFrontmatter", + "SkillManifestError", "SkillNotFoundError", "SkillPlugin", "SkillPluginError", + "WarningSink", "get_skill_directory", + "is_path_entry", "list_builtin_skills", "load_skill_content", - "resolve_skill_directories", + "read_skill_frontmatter", "resolve_skill_plugin", + "resolve_skills", ] diff --git a/src/conductor/skills/errors.py b/src/conductor/skills/errors.py new file mode 100644 index 00000000..984e1183 --- /dev/null +++ b/src/conductor/skills/errors.py @@ -0,0 +1,25 @@ +"""Common base for skill resolution and manifest failures. + +Lives in its own module so :mod:`conductor.skills.registry` and +:mod:`conductor.skills.frontmatter` can share it without importing each +other — registry already depends on frontmatter, and the reverse edge +would close a cycle. + +Every skill failure descends from this, so a call site that can trigger +more than one kind has a single correct thing to catch. That matters +here: resolution and manifest errors originate in different modules but +reach the same handlers, and enumerating both by name is a step that is +easy to forget (``_check_skill_injection_budget`` did, and an unreadable +``references/*.md`` escaped ``conductor validate`` as a traceback). +""" + +from __future__ import annotations + + +class SkillError(ValueError): + """Base for every skill resolution or manifest failure. + + A ``ValueError`` subclass so these nest cleanly inside Pydantic field + validation — ``AgentDef.validate_skills`` surfaces an unknown built-in + name as an ordinary schema error rather than an opaque crash. + """ diff --git a/src/conductor/skills/frontmatter.py b/src/conductor/skills/frontmatter.py new file mode 100644 index 00000000..1ee3a9d9 --- /dev/null +++ b/src/conductor/skills/frontmatter.py @@ -0,0 +1,142 @@ +"""Parse and validate ``SKILL.md`` YAML frontmatter. + +Every skill directory carries a ``SKILL.md`` whose leading ``---`` +block declares at minimum a ``name`` and a ``description``. Both the +Copilot CLI and Claude Code resolve skills through those two fields: +``name`` is how an enabled skill is referenced, ``description`` is the +progressive-disclosure summary the model reads before deciding to load +the body. + +The reason this module exists is that both CLIs **silently skip** a +skill whose frontmatter fails to parse — no warning, no error, the +skill is simply absent. The trap is ordinary: + +.. code-block:: yaml + + --- + name: acme-widgets + description: Internal ACME conventions. Triggers: widget, acme widget. + --- + +``Triggers:`` inside an unquoted plain scalar makes that invalid YAML. +Conductor parses the block itself so the failure surfaces at +``conductor validate`` time with the fix spelled out, instead of as an +agent that quietly never received its skill. + +Parsing uses ``ruamel.yaml`` — the project's YAML library. PyYAML is +not a dependency. +""" + +from __future__ import annotations + +import io +import re +from dataclasses import dataclass +from pathlib import Path + +from ruamel.yaml import YAML +from ruamel.yaml.error import YAMLError + +from conductor.skills.errors import SkillError + +# The leading ``---`` fenced block. Anchored at the start of the file: +# a ``---`` further down is a thematic break in the body, not metadata. +_FRONTMATTER = re.compile(r"\A---[ \t]*\r?\n(.*?)\r?\n---[ \t]*(?:\r?\n|\Z)", re.DOTALL) + +# Appended to every parse failure: the block-scalar form sidesteps the +# colon-in-a-plain-scalar trap entirely. +_BLOCK_SCALAR_HINT = ( + "A ':' followed by a space inside an unquoted value (e.g. " + "'description: Does things. Triggers: a, b') is invalid YAML. Use a " + "block scalar instead:\n" + " description: |\n" + " Does things. Triggers: a, b" +) + + +class SkillManifestError(SkillError): + """Raised when a skill's ``SKILL.md`` is missing, unparseable, or incomplete. + + A sibling of :class:`~conductor.skills.registry.SkillNotFoundError` + rather than a subclass — a broken manifest is not a species of "no + such skill". Catch :class:`~conductor.skills.errors.SkillError` to + handle both. + """ + + +@dataclass(frozen=True) +class SkillFrontmatter: + """The ``name`` and ``description`` a ``SKILL.md`` declares.""" + + name: str + """Skill name. Both CLIs resolve an enabled skill by this value, not + by its directory name — :func:`~conductor.skills.registry.resolve_skill_plugin` + checks the two agree.""" + + description: str + """One-paragraph summary used for progressive disclosure.""" + + +def read_skill_frontmatter(skill_dir: Path) -> SkillFrontmatter: + """Read and validate the frontmatter of ``skill_dir/SKILL.md``. + + Args: + skill_dir: Directory expected to contain ``SKILL.md``. + + Returns: + The parsed :class:`SkillFrontmatter`. + + Raises: + SkillManifestError: If ``SKILL.md`` is missing or unreadable, has + no frontmatter block, contains invalid YAML, does not parse + to a mapping, or omits a non-empty string ``name`` or + ``description``. + """ + skill_md = skill_dir / "SKILL.md" + if not skill_md.is_file(): + raise SkillManifestError( + f"Skill directory {skill_dir} has no SKILL.md. A skill directory must " + "contain a SKILL.md whose YAML frontmatter declares 'name' and " + "'description'." + ) + try: + text = skill_md.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + raise SkillManifestError(f"Skill manifest at {skill_md} could not be read: {exc}") from exc + + match = _FRONTMATTER.match(text) + if match is None: + raise SkillManifestError( + f"Skill manifest at {skill_md} has no YAML frontmatter. It must begin " + "with a '---' line, followed by 'name' and 'description', followed by " + "a closing '---' line." + ) + + try: + parsed = YAML(typ="safe").load(io.StringIO(match.group(1))) + except YAMLError as exc: + raise SkillManifestError( + f"Skill manifest at {skill_md} has invalid YAML frontmatter: {exc}\n\n" + f"{_BLOCK_SCALAR_HINT}" + ) from exc + + if not isinstance(parsed, dict): + raise SkillManifestError( + f"Skill manifest at {skill_md} has frontmatter that is not a YAML " + f"mapping (parsed as {type(parsed).__name__}). It must declare 'name' " + "and 'description' as keys." + ) + + values: dict[str, str] = {} + for field in ("name", "description"): + value = parsed.get(field) + if not isinstance(value, str) or not value.strip(): + raise SkillManifestError( + f"Skill manifest at {skill_md} declares no usable {field!r} in its " + f"YAML frontmatter (got {value!r}). Both CLIs skip a skill whose " + f"frontmatter is incomplete, so the agent would run without it.\n\n" + f"{_BLOCK_SCALAR_HINT}" + ) + values[field] = value.strip() + + return SkillFrontmatter(name=values["name"], description=values["description"]) diff --git a/src/conductor/skills/loader.py b/src/conductor/skills/loader.py index d8455b0d..f8c63a4a 100644 --- a/src/conductor/skills/loader.py +++ b/src/conductor/skills/loader.py @@ -11,8 +11,17 @@ The loader is the *content* side of the skill abstraction. The :mod:`conductor.skills.registry` module is the *resolution* side. -Results are cached per-directory for the lifetime of the process — skill -content is bundled and immutable. + +Results are cached per-directory for the lifetime of the process. The +cache key is the directory and name only — no mtime — so a skill edited +mid-process keeps serving its first-read content. That is intentional +for a single ``conductor run`` (one process, one workflow, consistent +prompts across retries) but it is *not* the "bundled and immutable" +guarantee this once relied on: since issue #350 a skill may be any +directory the user points at. Tests that rewrite a skill at a path they +have already loaded must call ``_cached_skill_payload.cache_clear()`` — +``tests/test_skills/conftest.py`` does this automatically for tests under +that package. """ from __future__ import annotations @@ -23,6 +32,13 @@ logger = logging.getLogger(__name__) +# Rough bytes-per-token ratio, used only to annotate size messages with an +# approximate token count. English prose sits near 4; the exact figure is +# model- and tokenizer-specific, so every message derived from it marks the +# value as approximate — a leading ``~``, or the ``approx_tokens`` key. Lives +# here because this module produces the string those messages measure. +BYTES_PER_TOKEN_ESTIMATE = 4 + _HEADER = ( "The following content describes skills available to this agent. " "Each skill provides reusable knowledge or capabilities — consult " @@ -30,33 +46,66 @@ ) +def _read_file(path: Path, label: str) -> str: + """Read one file of a skill's declared content, or fail loudly. + + A read failure is **not** skipped. Every file under a skill directory is + content the workflow asked the agent to have, and dropping one silently + is the precise defect this package exists to prevent — the upstream CLIs + skip an unreadable skill without a word, which is what issue #350 was + filed about. Doing the same one directory deeper would be no better, and + is easy to hit now that a skill may be any path the user points at + (a `chmod`, a non-UTF-8 byte, a flaky network mount). + + Raising also keeps the failure out of :func:`_cached_skill_payload`'s + cache: ``lru_cache`` never memoizes a call that raised, so a transient + error is retried rather than frozen in for the rest of the run. + + Args: + path: File to read. + label: How to name it in an error message. + + Returns: + The file's text, stripped. + + Raises: + SkillManifestError: If the file cannot be read or decoded. + """ + from conductor.skills.frontmatter import SkillManifestError + + try: + return path.read_text(encoding="utf-8").strip() + except (OSError, UnicodeDecodeError) as exc: + raise SkillManifestError( + f"Skill {label} at {path} could not be read: {exc}. It is part of the " + "skill's declared content, so loading the skill without it would hand " + "the agent less than the workflow asked for. Fix the file's permissions " + "or encoding, or remove it from the skill directory." + ) from exc + + def _read_skill_dir(skill_dir: Path) -> str: """Read ``SKILL.md`` plus all ``references/*.md`` files in order. Returns the concatenated text, with each file preceded by a heading - divider. Returns an empty string if the directory has no readable - content. + divider. Returns an empty string only when the directory genuinely has + no content — an unreadable file raises rather than being skipped. + + Raises: + SkillManifestError: If any file in the skill cannot be read. """ sections: list[str] = [] skill_md = skill_dir / "SKILL.md" if skill_md.is_file(): - try: - text = skill_md.read_text(encoding="utf-8").strip() - except (OSError, UnicodeDecodeError) as exc: - logger.warning("Failed to read SKILL.md at %s: %s", skill_md, exc) - text = "" + text = _read_file(skill_md, "manifest") if text: sections.append(f"# SKILL.md\n\n{text}") references_dir = skill_dir / "references" if references_dir.is_dir(): for ref in sorted(references_dir.glob("*.md")): - try: - text = ref.read_text(encoding="utf-8").strip() - except (OSError, UnicodeDecodeError) as exc: - logger.warning("Failed to read %s: %s", ref, exc) - continue + text = _read_file(ref, "reference") if text: sections.append(f"# references/{ref.name}\n\n{text}") diff --git a/src/conductor/skills/registry.py b/src/conductor/skills/registry.py index 3301c624..98b2727e 100644 --- a/src/conductor/skills/registry.py +++ b/src/conductor/skills/registry.py @@ -1,10 +1,9 @@ """Built-in skill registry for Conductor. -Phase 1 ships a single built-in skill — ``conductor`` — that points at -the existing ``plugins/conductor/skills/conductor/`` directory inside the -wheel. The skill directory follows the Copilot/Claude-Code skill format: -``SKILL.md`` plus an optional ``references/`` subdirectory of supporting -docs. +Conductor ships one built-in skill — ``conductor`` — pointing at the +``plugins/conductor/skills/conductor/`` directory inside the wheel. The skill +directory follows the Copilot/Claude-Code skill format: ``SKILL.md`` plus an +optional ``references/`` subdirectory of supporting docs. The plugins directory is bundled as wheel package data via the ``[tool.hatch.build.targets.wheel.force-include]`` entries in @@ -12,23 +11,36 @@ installed wheels work; it falls back to a source-checkout location for editable installs and tests. -Follow-up issues will add user-defined skill directories with a trust / -allowlist model; for now the registry only accepts built-in skill names. +Entries written in ``skills:`` are resolved here: a bare name must be a +registered built-in, while anything path-shaped is resolved against the +workflow file's directory. Discovering skills already installed in the +user's environment (``~/.copilot/skills``, ``.github/skills``, plugin +roots) is deliberately out of scope — discovery locations differ per +provider, so it is tracked separately in issue #362. """ from __future__ import annotations import json import logging +import os import re +from collections.abc import Callable, Sequence from dataclasses import dataclass from functools import lru_cache from pathlib import Path +from conductor.skills.errors import SkillError + logger = logging.getLogger(__name__) +# Sink for non-fatal diagnostics raised during resolution. Callers decide +# where these land, because the right destination differs: warnings from +# ``conductor validate`` are printed, warnings at run time are verbose-only. +WarningSink = Callable[[str], None] + -class SkillNotFoundError(ValueError): +class SkillNotFoundError(SkillError): """Raised when a skill name is not found in the registry.""" @@ -65,7 +77,7 @@ def _repo_or_wheel_root() -> Path: repository root, three directories above this file (``src/conductor/skills/registry.py``). * **Wheel install** — ``plugins/`` is bundled as package data via - hatchling's ``artifacts`` entry and lands alongside the + hatchling's ``force-include`` entries and lands alongside the ``conductor/`` package directory inside ``site-packages``. We probe both. The first hit wins. @@ -77,7 +89,7 @@ def _repo_or_wheel_root() -> Path: if (repo_root / "plugins" / "conductor" / "skills").is_dir(): return repo_root - # Wheel install: artifacts land alongside the package itself + # Wheel install: force-included files land alongside the package itself # (site-packages/plugins next to site-packages/conductor). wheel_root = here.parents[2] if (wheel_root / "plugins" / "conductor" / "skills").is_dir(): @@ -94,7 +106,10 @@ def list_builtin_skills() -> list[str]: def get_skill_directory(skill: str) -> Path: - """Resolve a built-in skill name to its on-disk directory. + """Resolve a built-in skill *name* to its on-disk directory. + + Only handles registered built-in names. Path entries are handled by + :func:`resolve_skills`. Args: skill: The skill name as it appears in ``skills: [...]`` (e.g. @@ -112,7 +127,9 @@ def get_skill_directory(skill: str) -> Path: available = ", ".join(list_builtin_skills()) or "(none)" raise SkillNotFoundError( f"Unknown skill {skill!r}. Available built-in skills: {available}. " - "User-defined skill directories are not yet supported." + "To use a skill of your own, give a path instead (e.g. " + "'./team-skills/my-skill'); an entry counts as a path when it " + "starts with '.' or '~', or contains a path separator." ) path = (_repo_or_wheel_root() / rel).resolve() if not path.is_dir(): @@ -125,27 +142,231 @@ def get_skill_directory(skill: str) -> Path: return path -def resolve_skill_directories(skills: list[str]) -> list[Path]: - """Resolve a list of skill names to their on-disk directories. +@dataclass(frozen=True) +class ResolvedSkill: + """A single skill resolved from one ``skills:`` entry.""" + + name: str + """Skill name — always the directory's basename. + + Deliberately *not* the ``SKILL.md`` frontmatter name: the built-in + registry keys, the eager-injection ```` tag, and + claude-agent-sdk's ``:`` qualified name are all + basename-derived. :func:`resolve_skill_plugin` enforces that the + frontmatter agrees for the provider that needs it. + """ + + directory: Path + """Absolute path to the directory holding ``SKILL.md``.""" + + source: str + """The ``skills:`` entry this was resolved from, verbatim. + + A ``skills/`` root expands to several :class:`ResolvedSkill` objects + that share one ``source``, so error messages can name what the user + actually wrote. + """ + + def __post_init__(self) -> None: + """Assert the two invariants the docstrings above claim. + + Both are established by :func:`resolve_skills` today, but this is + an exported public constructor and ``name`` is interpolated + unescaped into ```` by the loader. Same + reasoning as :class:`SkillPlugin`, which checks its own names for + the same reason. + + Deliberately no ``is_dir()`` probe: filesystem state in a + constructor is a TOCTOU illusion, and establishing it is + correctly the producer's job. + + Raises: + SkillNotFoundError: If ``name`` is not ``directory``'s + basename, or ``directory`` is not absolute. + """ + if self.name != self.directory.name: + raise SkillNotFoundError( + f"ResolvedSkill.name {self.name!r} must equal its directory " + f"basename {self.directory.name!r}" + ) + if not self.directory.is_absolute(): + raise SkillNotFoundError( + f"ResolvedSkill.directory must be absolute, got {self.directory}" + ) + + +def is_path_entry(entry: str) -> bool: + """Whether a ``skills:`` entry denotes a filesystem path. + + The test is purely syntactic — no disk access — so classification + never depends on what happens to exist locally, and a bare name like + ``conductor`` can never be shadowed by a same-named directory. + + No ``os.path.isabs`` check is needed: every absolute form on either + POSIX or Windows (``/x``, ``C:\\x``, ``C:/x``, ``\\\\server\\share``) + already contains a separator. + """ + return entry.startswith(("~", ".")) or "/" in entry or "\\" in entry + + +def _resolve_path_entry( + entry: str, base_dir: Path | None, on_warning: WarningSink | None = None +) -> list[Path]: + """Expand a path entry to the skill directories it denotes. + + Accepts either granularity: a single skill directory (one holding + ``SKILL.md``) or a root containing several such directories. Args: - skills: List of built-in skill names. + entry: The raw path as written in ``skills:``. + base_dir: Directory a relative entry resolves against. Falls back + to the process working directory. + on_warning: Optional sink for non-fatal diagnostics — currently a + skills root that skipped subdirectories lacking a ``SKILL.md``. Returns: - List of absolute paths in the same order, with duplicates removed - (preserving first occurrence). + The entry itself for a single skill directory, or every child + holding a ``SKILL.md`` (sorted by name) for a root. Raises: - SkillNotFoundError: If any skill name is unknown. + SkillNotFoundError: If the path does not exist, is not a + directory, cannot be read, or is a directory holding neither + a ``SKILL.md`` nor any child that does. """ - seen: set[Path] = set() - out: list[Path] = [] - for name in skills: - path = get_skill_directory(name) - if path in seen: - continue - seen.add(path) - out.append(path) + path = Path(entry).expanduser() + if not path.is_absolute(): + path = (base_dir if base_dir is not None else Path.cwd()) / path + # normpath, not resolve(): symlink aliases stay distinct, matching + # WorkflowEngine._resolve_agent_working_dir. + resolved = Path(os.path.normpath(path)) + + try: + if not resolved.exists(): + raise SkillNotFoundError( + f"Skill path {entry!r} resolved to {resolved!s}, which does not exist. " + "Relative skill paths resolve against the workflow file's directory." + ) + if not resolved.is_dir(): + raise SkillNotFoundError( + f"Skill path {entry!r} resolved to {resolved!s}, which is not a " + "directory. Point it at a skill directory (one containing SKILL.md) " + "or at a directory of them." + ) + if (resolved / "SKILL.md").is_file(): + return [resolved] + entries = list(resolved.iterdir()) + children = sorted( + (child for child in entries if (child / "SKILL.md").is_file()), + key=lambda child: child.name, + ) + # Subdirectories that look like skills but have no SKILL.md. Files + # (a README, a LICENSE) are not reported — only a directory can + # plausibly have been *meant* as a skill. Without this, pointing at + # a root turns the loud "no SKILL.md" error you would get from + # naming the directory directly into silence: a mis-cased + # ``Skill.md`` or a file someone forgot to commit simply yields one + # fewer skill. + skipped = sorted( + child.name for child in entries if child.is_dir() and not (child / "SKILL.md").is_file() + ) + except OSError as exc: + # A path Conductor can name but not inspect — an unreadable directory, + # or one whose parent is unreadable. Python re-raises EACCES from + # ``exists``, ``is_dir``, ``is_file`` and ``iterdir`` alike, so without + # this the caller sees a bare PermissionError traceback instead of a + # message naming the entry. + raise SkillNotFoundError( + f"Skill path {entry!r} resolved to {resolved!s}, which could not be read: {exc}" + ) from exc + + if children: + if skipped and on_warning is not None: + on_warning( + f"Skills root {entry!r} expanded to {len(children)} skill(s) but " + f"skipped {len(skipped)} subdirector{'y' if len(skipped) == 1 else 'ies'} " + f"with no SKILL.md: {skipped!r}. Add a SKILL.md to each, or remove them." + ) + return children + + raise SkillNotFoundError( + f"Skill path {entry!r} resolved to {resolved!s}, which contains neither a " + "SKILL.md nor any subdirectory containing one. A skill directory holds " + "SKILL.md directly; a skills root holds one subdirectory per skill." + ) + + +def resolve_skills( + skills: Sequence[str], + base_dir: Path | None = None, + on_warning: WarningSink | None = None, +) -> list[ResolvedSkill]: + """Resolve ``skills:`` entries to named, on-disk skill directories. + + Each entry is either a **registered built-in name** (``conductor``) + or a **filesystem path**. Classification is syntactic — see + :func:`is_path_entry` — so a bare name is never shadowed by a + same-named local directory. + + Every resolved directory's ``SKILL.md`` frontmatter is parsed and + checked here rather than only at ``conductor validate`` time, + because ``conductor run`` does not run the static validator and both + downstream CLIs skip an unparseable skill in silence. + + Args: + skills: The entries as written in ``skills:``. + base_dir: Directory relative path entries resolve against + (normally the workflow file's directory). Falls back to the + process working directory. + on_warning: Optional sink for non-fatal diagnostics. ``conductor + validate`` routes these into its warning list; the executor + routes them to verbose logging. + + Returns: + One :class:`ResolvedSkill` per skill directory, in entry order, + with duplicate directories removed (first occurrence wins). + + Raises: + SkillNotFoundError: If an entry is an unknown built-in name, a + path that does not resolve to any skill directory, or two + different directories that would claim the same skill name. + SkillManifestError: If a resolved skill's ``SKILL.md`` is + missing, unparseable, or omits ``name`` / ``description``. + """ + from conductor.skills.frontmatter import read_skill_frontmatter + + # A skill's name is its directory basename, so this doubles as the + # seen-directories index: one name maps to exactly one directory. + by_name: dict[str, ResolvedSkill] = {} + out: list[ResolvedSkill] = [] + for entry in skills: + if is_path_entry(entry): + directories = _resolve_path_entry(entry, base_dir, on_warning) + else: + directories = [get_skill_directory(entry)] + for directory in directories: + name = directory.name + seen = by_name.get(name) + if seen is not None: + if seen.directory == directory: + # The same directory reached twice — a name and its + # equivalent path, or two overlapping roots. First wins. + continue + # Two different directories, one name. Every downstream + # consumer is name-keyed — the eager preamble emits + # ```` per skill and the native CLIs + # resolve by name — so one of the two would be shadowed + # with no indication which. Refusing is the same call + # ``resolve_skill_plugin`` makes for a qualified-name clash. + raise SkillNotFoundError( + f"Skills {seen.source!r} and {entry!r} both resolve to a skill " + f"named {name!r} ({seen.directory} and {directory}). Skill names " + "must be unique — one would silently shadow the other. Rename one " + "of the directories, or enable only one of them." + ) + read_skill_frontmatter(directory) + resolved = ResolvedSkill(name=name, directory=directory, source=entry) + by_name[name] = resolved + out.append(resolved) return out @@ -270,6 +491,12 @@ def _read_plugin_name(manifest: Path) -> str: def _declared_skill_name(skill_dir: Path) -> str: """Read the ``name`` a skill's ``SKILL.md`` frontmatter declares. + Delegates to :func:`~conductor.skills.frontmatter.read_skill_frontmatter` + so a broken manifest is reported the same way here as at + ``conductor validate`` time, and re-raises as + :class:`SkillPluginError` because that is the class + :func:`resolve_skill_plugin`'s callers catch. + Args: skill_dir: Directory expected to contain ``SKILL.md``. @@ -277,27 +504,15 @@ def _declared_skill_name(skill_dir: Path) -> str: The declared skill name. Raises: - SkillPluginError: If ``SKILL.md`` is missing, unreadable, or - declares no ``name`` in its frontmatter. + SkillPluginError: If ``SKILL.md`` is missing, unreadable, has + unparseable frontmatter, or omits ``name`` / ``description``. """ - skill_md = skill_dir / "SKILL.md" - if not skill_md.is_file(): - raise SkillPluginError( - f"Skill directory {skill_dir} has no SKILL.md, so the claude CLI will " - "not expose it under any name." - ) + from conductor.skills.frontmatter import SkillManifestError, read_skill_frontmatter + try: - text = skill_md.read_text(encoding="utf-8") - except OSError as exc: - raise SkillPluginError(f"Skill manifest at {skill_md} could not be read: {exc}") from exc - match = re.search(r"\A---\r?\n(.*?)\r?\n---", text, re.DOTALL) - name = re.search(r"^name:\s*(\S+)\s*$", match.group(1), re.MULTILINE) if match else None - if name is None: - raise SkillPluginError( - f"Skill manifest at {skill_md} declares no 'name' in its YAML frontmatter. " - "The claude CLI resolves enabled skills by that name." - ) - return name.group(1) + return read_skill_frontmatter(skill_dir).name + except SkillManifestError as exc: + raise SkillPluginError(str(exc)) from exc def resolve_skill_plugin(skill_dir: Path) -> SkillPlugin | None: diff --git a/tests/test_config/test_validator_skills.py b/tests/test_config/test_validator_skills.py new file mode 100644 index 00000000..5060662d --- /dev/null +++ b/tests/test_config/test_validator_skills.py @@ -0,0 +1,393 @@ +"""Static skill validation at ``conductor validate`` time (issue #350). + +``conductor run`` does **not** call :func:`validate_workflow_config`, so +resolution failures are also enforced inside ``resolve_skills`` (covered in +``tests/test_skills/test_path_entries.py``). What these tests cover is the +part that only exists statically: reporting every problem before a run +starts, and the two provider-specific checks that need the resolved +provider — ``claude-agent-sdk``'s plugin requirement and the eager-injection +budget. +""" + +from __future__ import annotations + +import os +import re +from pathlib import Path +from typing import Any + +import pytest +from pydantic import ValidationError as PydanticValidationError + +from conductor.config.schema import ( + AgentDef, + ForEachDef, + OutputField, + RuntimeConfig, + SkillInjectionConfig, + WorkflowConfig, + WorkflowDef, +) +from conductor.config.validator import validate_workflow_config +from conductor.exceptions import ConfigurationError, ExecutionError +from conductor.executor.agent import AgentExecutor +from conductor.skills import load_skill_content +from tests.test_skills.test_injection_budget import _EagerProvider + +_FRONTMATTER = "---\nname: {name}\ndescription: A test skill.\n---\nBody\n" + + +def _make_skill(directory: Path, *, frontmatter: str | None = None, filler: int = 0) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text( + frontmatter if frontmatter is not None else _FRONTMATTER.format(name=directory.name) + ) + if filler: + (directory / "references").mkdir(exist_ok=True) + (directory / "references" / "big.md").write_text("x" * filler) + return directory + + +def _make_plugin(root: Path, plugin_name: str, skill_name: str) -> Path: + """Build a Claude Code plugin tree and return its skill directory.""" + (root / ".claude-plugin").mkdir(parents=True, exist_ok=True) + (root / ".claude-plugin" / "plugin.json").write_text(f'{{"name": "{plugin_name}"}}') + return _make_skill(root / "skills" / skill_name) + + +def _workflow( + *, + provider: str = "copilot", + agent_skills: list[str] | None = None, + runtime_skills: list[str] | None = None, + skill_injection: SkillInjectionConfig | None = None, + for_each_skills: list[str] | None = None, +) -> WorkflowConfig: + runtime_kwargs: dict[str, Any] = {"provider": provider} + if runtime_skills is not None: + runtime_kwargs["skills"] = runtime_skills + if skill_injection is not None: + runtime_kwargs["skill_injection"] = skill_injection + + agents = [ + AgentDef( + name="worker", + prompt="Do the thing.", + skills=agent_skills, + output={"result": OutputField(type="string")}, + ) + ] + for_each = ( + [ + ForEachDef( + name="fan_out", + type="for_each", + source="worker.output.result", + **{"as": "item"}, + agent=AgentDef(name="inner", prompt="Handle {{ item }}", skills=for_each_skills), + ) + ] + if for_each_skills is not None + else [] + ) + return WorkflowConfig( + workflow=WorkflowDef( + name="wf", entry_point="worker", runtime=RuntimeConfig(**runtime_kwargs) + ), + agents=agents, + for_each=for_each, + output={"result": "{{ worker.output.result }}"}, + ) + + +def _validate(config: WorkflowConfig, workflow_path: Path | None) -> list[str]: + """Run validation, returning warnings. Errors raise.""" + return validate_workflow_config(config, workflow_path=workflow_path) + + +def _wf_path(tmp_path: Path) -> Path: + path = tmp_path / "wf.yaml" + path.write_text("# placeholder; validation reads the parsed config, not this file\n") + return path + + +class TestPathResolution: + def test_valid_path_skill_passes(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "team-skills" / "acme") + _validate(_workflow(agent_skills=["./team-skills/acme"]), _wf_path(tmp_path)) + + def test_missing_path_is_an_error(self, tmp_path: Path) -> None: + with pytest.raises(ConfigurationError, match="does not exist"): + _validate(_workflow(agent_skills=["./nope"]), _wf_path(tmp_path)) + + def test_error_names_the_agent(self, tmp_path: Path) -> None: + with pytest.raises(ConfigurationError, match="Agent 'worker'"): + _validate(_workflow(agent_skills=["./nope"]), _wf_path(tmp_path)) + + def test_relative_paths_resolve_against_the_workflow_file(self, tmp_path: Path) -> None: + """Not the process cwd — a workflow must validate the same from anywhere.""" + nested = tmp_path / "flows" + nested.mkdir() + _make_skill(nested / "acme") + path = nested / "wf.yaml" + path.write_text("# placeholder\n") + _validate(_workflow(agent_skills=["./acme"]), path) + + def test_relative_paths_are_skipped_without_a_workflow_path(self, tmp_path: Path) -> None: + """Mirrors ``_validate_subworkflow_refs``: with no base directory a + relative path cannot be resolved, so it is not reported as missing. + Absolute entries are unaffected — see the test below. + + Paired with a positive control — the same config *with* a workflow path + must raise — so deleting the skill check outright cannot pass this. + """ + config = _workflow(agent_skills=["./nope"]) + _validate(config, None) + with pytest.raises(ConfigurationError, match="does not exist"): + _validate(config, _wf_path(tmp_path)) + + def test_absolute_paths_still_validate_without_a_workflow_path(self, tmp_path: Path) -> None: + """An absolute entry needs no base directory, so the skip must not + swallow it either. + + ``~``-prefixed entries take the same branch, since ``expanduser()`` + makes them absolute. + """ + _make_skill(tmp_path / "acme") + _validate(_workflow(agent_skills=[str(tmp_path / "acme")]), None) + with pytest.raises(ConfigurationError, match="does not exist"): + _validate(_workflow(agent_skills=[str(tmp_path / "nope")]), None) + + def test_builtin_names_still_validate_without_a_workflow_path(self) -> None: + """Built-in names need no base directory, so the skip must not swallow + them. + + The control is that an unknown *name* is caught earlier still — at + config construction, by ``AgentDef.validate_skills`` — which is the + pre-#350 error timing this change deliberately preserves. + """ + _validate(_workflow(agent_skills=["conductor"]), None) + with pytest.raises(PydanticValidationError, match="Unknown skill"): + _workflow(agent_skills=["not-a-real-skill"]) + + def test_runtime_skills_are_validated(self, tmp_path: Path) -> None: + with pytest.raises(ConfigurationError, match="does not exist"): + _validate(_workflow(runtime_skills=["./nope"]), _wf_path(tmp_path)) + + def test_agent_opt_out_skips_inherited_runtime_skills(self, tmp_path: Path) -> None: + """``skills: []`` overrides the workflow default, so a broken default + must not be charged to an agent that opted out.""" + _validate(_workflow(runtime_skills=["./nope"], agent_skills=[]), _wf_path(tmp_path)) + + def test_for_each_inline_agents_are_validated(self, tmp_path: Path) -> None: + with pytest.raises(ConfigurationError, match="Agent 'inner'"): + _validate(_workflow(for_each_skills=["./nope"]), _wf_path(tmp_path)) + + +class TestFrontmatterValidation: + def test_unparseable_frontmatter_is_an_error(self, tmp_path: Path) -> None: + """The exact trap from issue #350, which both CLIs skip in silence.""" + _make_skill( + tmp_path / "acme", + frontmatter="---\nname: acme\ndescription: Does things. Triggers: a, b\n---\n", + ) + with pytest.raises(ConfigurationError) as exc_info: + _validate(_workflow(agent_skills=["./acme"]), _wf_path(tmp_path)) + assert "invalid YAML frontmatter" in str(exc_info.value) + assert "description: |" in str(exc_info.value), "the fix must be shown" + + def test_missing_description_is_an_error(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "acme", frontmatter="---\nname: acme\n---\n") + with pytest.raises(ConfigurationError, match="no usable 'description'"): + _validate(_workflow(agent_skills=["./acme"]), _wf_path(tmp_path)) + + def test_directory_without_skill_md_is_an_error(self, tmp_path: Path) -> None: + (tmp_path / "acme").mkdir() + with pytest.raises(ConfigurationError, match="neither a SKILL.md nor"): + _validate(_workflow(agent_skills=["./acme"]), _wf_path(tmp_path)) + + +class TestClaudeAgentSdkPluginRequirement: + """The SDK has no bare skill-directory option — only ``plugins`` + + ``skills``. A skill outside a plugin is unreachable there, so it is + refused statically instead of failing mid-run.""" + + def test_skill_outside_a_plugin_is_refused(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "acme") + with pytest.raises(ConfigurationError) as exc_info: + _validate( + _workflow(provider="claude-agent-sdk", agent_skills=["./acme"]), _wf_path(tmp_path) + ) + message = str(exc_info.value) + assert "not inside a Claude Code plugin" in message + assert "copilot" in message, "the message must offer a provider that works" + + def test_skill_inside_a_plugin_is_accepted(self, tmp_path: Path) -> None: + skill = _make_plugin(tmp_path / "plug", "acme", "widgets") + _validate( + _workflow(provider="claude-agent-sdk", agent_skills=[str(skill)]), _wf_path(tmp_path) + ) + + def test_broken_plugin_manifest_reports_the_reason(self, tmp_path: Path) -> None: + skill = _make_plugin(tmp_path / "plug", "acme", "widgets") + (tmp_path / "plug" / ".claude-plugin" / "plugin.json").write_text("{ not json") + with pytest.raises(ConfigurationError, match="could not be read"): + _validate( + _workflow(provider="claude-agent-sdk", agent_skills=[str(skill)]), + _wf_path(tmp_path), + ) + + def test_builtin_skill_is_accepted(self) -> None: + """The bundled skill ships inside a plugin, which is how it loads there.""" + _validate(_workflow(provider="claude-agent-sdk", agent_skills=["conductor"]), None) + + def test_copilot_accepts_the_same_non_plugin_skill(self, tmp_path: Path) -> None: + """The restriction is provider-specific, not a property of the skill.""" + _make_skill(tmp_path / "acme") + _validate(_workflow(provider="copilot", agent_skills=["./acme"]), _wf_path(tmp_path)) + + +class TestInjectionBudget: + def test_oversized_injection_is_an_error(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "acme", filler=5000) + with pytest.raises(ConfigurationError, match="max_bytes"): + _validate( + _workflow( + provider="claude", + agent_skills=["./acme"], + skill_injection=SkillInjectionConfig(warn_bytes=100, max_bytes=1000), + ), + _wf_path(tmp_path), + ) + + def test_between_thresholds_is_a_warning(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "acme", filler=5000) + warnings = _validate( + _workflow( + provider="claude", + agent_skills=["./acme"], + skill_injection=SkillInjectionConfig(warn_bytes=1000, max_bytes=100_000), + ), + _wf_path(tmp_path), + ) + assert any("warn_bytes" in warning for warning in warnings) + + def test_bundled_skill_on_claude_warns_but_validates(self) -> None: + """``skills: [conductor]`` on ``claude`` works today; defaults must not + turn an existing workflow into a validation failure.""" + warnings = _validate(_workflow(provider="claude", agent_skills=["conductor"]), None) + assert any("progressive disclosure" in warning for warning in warnings) + + def test_native_providers_are_not_budgeted(self, tmp_path: Path) -> None: + """Copilot loads on demand, so a large skill costs nothing up front.""" + _make_skill(tmp_path / "acme", filler=5000) + warnings = _validate( + _workflow( + provider="copilot", + agent_skills=["./acme"], + skill_injection=SkillInjectionConfig(warn_bytes=10, max_bytes=100), + ), + _wf_path(tmp_path), + ) + assert not any("skill content" in warning for warning in warnings) + + def test_hermes_is_budgeted(self, tmp_path: Path) -> None: + """Hermes gained ``skills=True`` in this change; it injects eagerly, so + it must be bounded like ``claude``.""" + _make_skill(tmp_path / "acme", filler=5000) + with pytest.raises(ConfigurationError, match="max_bytes"): + _validate( + _workflow( + provider="hermes", + agent_skills=["./acme"], + skill_injection=SkillInjectionConfig(warn_bytes=100, max_bytes=1000), + ), + _wf_path(tmp_path), + ) + + def test_unreadable_reference_is_reported_not_raised(self, tmp_path: Path) -> None: + """``read_skill_frontmatter`` only opens ``SKILL.md``, so a broken + ``references/*.md`` first surfaces here, inside ``load_skill_content``. + + Without a guard it escaped ``validate_workflow_config`` as a bare + traceback — the one file class in a skill directory whose failure was + reported differently from every other. + """ + skill = _make_skill(tmp_path / "acme", filler=10) + unreadable = skill / "references" / "big.md" + unreadable.chmod(0o000) + try: + if os.access(unreadable, os.R_OK): + pytest.skip("running as a user that bypasses file permissions") + with pytest.raises(ConfigurationError, match="could not be read"): + _validate( + _workflow(provider="claude", agent_skills=["./acme"]), + _wf_path(tmp_path), + ) + finally: + unreadable.chmod(0o644) + + +class TestStaticAndRuntimeBudgetAgree: + """`conductor validate` and `conductor run` compute the injected size + independently, in `_check_skill_injection_budget` and + `AgentExecutor._enforce_injection_budget`. If they drift, a workflow + passes one command and fails the other — which is the exact class of + validate/run disagreement `_reject_unsupported_skills` exists to close. + + Both tests deliberately exercise the two paths together so neither can be + changed in isolation. + """ + + @staticmethod + def _bytes_reported(message: str) -> str: + match = re.search(r"([\d,]+) bytes", message) + assert match is not None, f"no byte count in: {message}" + return match.group(1) + + @staticmethod + def _runtime_executor(tmp_path: Path, limits: SkillInjectionConfig) -> AgentExecutor: + return AgentExecutor(_EagerProvider(), workflow_dir=tmp_path, skill_injection=limits) + + def test_both_paths_report_the_same_byte_count(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "acme", filler=5000) + limits = SkillInjectionConfig(warn_bytes=100, max_bytes=1000) + + with pytest.raises(ConfigurationError) as static_exc: + _validate( + _workflow(provider="claude", agent_skills=["./acme"], skill_injection=limits), + _wf_path(tmp_path), + ) + with pytest.raises(ExecutionError) as runtime_exc: + self._runtime_executor(tmp_path, limits)._build_prompt_prefix( + AgentDef(name="worker", prompt="p", skills=["./acme"]) + ) + + assert self._bytes_reported(str(static_exc.value)) == self._bytes_reported( + str(runtime_exc.value) + ) + + @pytest.mark.parametrize( + ("delta", "should_reject"), + [(0, False), (-1, True)], + ids=["limit-exactly-at-size", "limit-one-byte-under"], + ) + def test_limit_at_the_exact_rendered_size( + self, tmp_path: Path, delta: int, should_reject: bool + ) -> None: + """Catches both envelope drift (measuring raw file bytes instead of the + rendered string) and a `>` / `>=` comparison flip, on both paths.""" + skill = _make_skill(tmp_path / "acme", filler=5000) + exact = len(load_skill_content([("acme", skill)]).encode("utf-8")) + limits = SkillInjectionConfig(warn_bytes=None, max_bytes=exact + delta) + config = _workflow(provider="claude", agent_skills=["./acme"], skill_injection=limits) + agent = AgentDef(name="worker", prompt="p", skills=["./acme"]) + + if should_reject: + with pytest.raises(ConfigurationError, match="max_bytes"): + _validate(config, _wf_path(tmp_path)) + with pytest.raises(ExecutionError, match="max_bytes"): + self._runtime_executor(tmp_path, limits)._build_prompt_prefix(agent) + else: + _validate(config, _wf_path(tmp_path)) + assert self._runtime_executor(tmp_path, limits)._build_prompt_prefix(agent) diff --git a/tests/test_integration/test_existing_workflows_integration.py b/tests/test_integration/test_existing_workflows_integration.py index 54fe35ec..dddacc61 100644 --- a/tests/test_integration/test_existing_workflows_integration.py +++ b/tests/test_integration/test_existing_workflows_integration.py @@ -194,6 +194,9 @@ async def test_schema_changes_dont_affect_copilot_provider(): # None and excluded by exclude_none. "checkpoint": {"every_agent": False, "keep_last": 5}, "skills": [], + # Eager skill-injection budget (issue #350). Bounds only providers + # without progressive disclosure; copilot is unaffected. + "skill_injection": {"warn_bytes": 65536, "max_bytes": 131072}, } # Verify provider can be instantiated diff --git a/tests/test_integration/test_mixed_providers.py b/tests/test_integration/test_mixed_providers.py index fea825ef..c110b00c 100644 --- a/tests/test_integration/test_mixed_providers.py +++ b/tests/test_integration/test_mixed_providers.py @@ -102,6 +102,9 @@ def test_claude_fields_ignored_by_copilot_provider(self, tmp_path): # is None and excluded by exclude_none. "checkpoint": {"every_agent": False, "keep_last": 5}, "skills": [], + # Eager skill-injection budget (issue #350). Bounds only providers + # without progressive disclosure; copilot is unaffected. + "skill_injection": {"warn_bytes": 65536, "max_bytes": 131072}, } def test_provider_parameter_isolation(self, tmp_path): diff --git a/tests/test_providers/test_capabilities.py b/tests/test_providers/test_capabilities.py index 2a584517..039c338c 100644 --- a/tests/test_providers/test_capabilities.py +++ b/tests/test_providers/test_capabilities.py @@ -5,9 +5,11 @@ import pytest from conductor.providers.capabilities import ( + _NOT_YET_IMPLEMENTED_PROVIDERS, ProviderCapabilities, get_capabilities, known_provider_names, + uses_native_skills, ) @@ -302,3 +304,68 @@ async def close(self) -> None: # No exception — abstract=True bypasses the check. assert _Fake.CAPABILITIES is None + + +class TestDeclaredSkillsSupport: + """``skills`` is not an allowed experimental carve-out: a provider gets it + natively, or via ``AgentExecutor``'s provider-agnostic eager injection. + ``False`` is only accurate when neither path can work. + """ + + @pytest.mark.parametrize( + ("provider", "expected"), + [ + ("copilot", True), + ("claude", True), + ("claude-agent-sdk", True), + # Issue #350: hermes previously omitted ``skills``, defaulting to + # False, so the validator rejected ``skills:`` on it -- while its + # own execute() docstring described eager injection working. + ("hermes", True), + # aca is the one honest False: skill directories are host paths + # the in-sandbox runner cannot read. + ("aca", False), + ], + ) + def test_declared_skills_support(self, provider: str, expected: bool) -> None: + assert get_capabilities(provider).skills is expected + + @pytest.mark.parametrize( + ("provider", "expected"), + [ + ("copilot", True), + ("claude-agent-sdk", True), + ("claude", False), + ("hermes", False), + ], + ) + def test_native_skill_mechanism_resolves_without_instantiating( + self, provider: str, expected: bool + ) -> None: + """``conductor validate`` reads this to decide whether the eager + injection budget applies, and must not construct a provider to do it.""" + assert uses_native_skills(provider) is expected + + def test_unknown_provider_is_undetermined_not_a_guess(self) -> None: + """``None`` makes callers skip the mechanism-specific check rather than + assume a branch.""" + assert uses_native_skills("no-such-provider") is None + + def test_every_implemented_provider_resolves_its_mechanism(self) -> None: + """A ``None`` here means ``conductor validate`` silently stops applying + the skill-injection budget to that provider while ``AgentExecutor`` + keeps enforcing it — a validate/run disagreement with no other signal. + + Pinned as a completeness check rather than a name list so a newly added + provider, or a ``supports_native_skills`` property refactored to read + ``self``, fails here instead of degrading quietly. + """ + undetermined = [ + name + for name in known_provider_names() + if name not in _NOT_YET_IMPLEMENTED_PROVIDERS and uses_native_skills(name) is None + ] + assert not undetermined, ( + f"providers {undetermined} escape static skill-budget checks; " + "uses_native_skills must resolve without instantiating them" + ) diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 3494db18..1e3d77f7 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -2348,9 +2348,9 @@ def _argv(options) -> list[str]: @staticmethod def _skill_dirs() -> list[str]: - from conductor.skills import resolve_skill_directories + from conductor.skills import resolve_skills - return [str(p) for p in resolve_skill_directories(["conductor"])] + return [str(item.directory) for item in resolve_skills(["conductor"])] @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) async def test_declared_skill_is_enabled_via_plugin(self) -> None: @@ -2435,14 +2435,18 @@ def _make_plugin(root: Path, plugin_name: str, *skills: str) -> Path: (root / ".claude-plugin" / "plugin.json").write_text(f'{{"name": "{plugin_name}"}}') for skill in skills: (root / "skills" / skill).mkdir(parents=True) - (root / "skills" / skill / "SKILL.md").write_text(f"---\nname: {skill}\n---\n") + (root / "skills" / skill / "SKILL.md").write_text( + # ``description`` is required frontmatter; without it the + # manifest parser rejects the skill before plugin resolution. + f"---\nname: {skill}\ndescription: A test skill.\n---\n" + ) return root @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) async def test_skill_outside_a_plugin_is_refused(self, tmp_path: Path) -> None: orphan = tmp_path / "skills" / "lonely" orphan.mkdir(parents=True) - (orphan / "SKILL.md").write_text("---\nname: lonely\n---\n") + (orphan / "SKILL.md").write_text("---\nname: lonely\ndescription: A test skill.\n---\n") with pytest.raises(ProviderError, match="not part of a Claude Code plugin"): await self._capture_options( @@ -2472,7 +2476,7 @@ async def test_missing_plugin_error_is_not_retryable(self, tmp_path: Path) -> No heuristic, which sniffs the message text.""" orphan = tmp_path / "connection-hub" / "skills" / "lonely" orphan.mkdir(parents=True) - (orphan / "SKILL.md").write_text("---\nname: lonely\n---\n") + (orphan / "SKILL.md").write_text("---\nname: lonely\ndescription: A test skill.\n---\n") with pytest.raises(ProviderError) as exc: await self._capture_options( diff --git a/tests/test_skills/conftest.py b/tests/test_skills/conftest.py new file mode 100644 index 00000000..e605867c --- /dev/null +++ b/tests/test_skills/conftest.py @@ -0,0 +1,19 @@ +"""Shared fixtures for skill tests.""" + +from __future__ import annotations + +import pytest + +from conductor.skills.loader import _cached_skill_payload + + +@pytest.fixture(autouse=True) +def _clear_skill_content_cache() -> None: + """Drop the loader's per-directory content cache between tests. + + ``_cached_skill_payload`` is keyed on ``(directory, name)`` with no + mtime component, so a test that rewrites a skill at a path an earlier + test already loaded would silently receive the stale body and pass for + the wrong reason. ``tmp_path`` makes that rare rather than impossible. + """ + _cached_skill_payload.cache_clear() diff --git a/tests/test_skills/test_engine_integration.py b/tests/test_skills/test_engine_integration.py new file mode 100644 index 00000000..c2fc74c8 --- /dev/null +++ b/tests/test_skills/test_engine_integration.py @@ -0,0 +1,183 @@ +"""Skill resolution through a real :class:`WorkflowEngine` (issue #350). + +``AgentExecutor`` cannot resolve a relative skill path on its own — it needs +``workflow_dir``, which only the engine knows. Tests that build an executor +directly pass that argument themselves, so they cannot detect the engine +failing to supply it: the whole feature silently degrades to "relative paths +never resolve" with every other skill test still green. + +These tests drive the engine end to end for that reason. The same applies to +``runtime.skill_injection`` — an executor built by hand gets whatever limits +the test hands it, not the ones the workflow declared. +""" + +from __future__ import annotations + +import asyncio +from pathlib import Path +from typing import Any + +import pytest + +from conductor.config.schema import ( + AgentDef, + OutputField, + RuntimeConfig, + SkillInjectionConfig, + WorkflowConfig, + WorkflowDef, +) +from conductor.engine.workflow import WorkflowEngine +from conductor.exceptions import ExecutionError +from conductor.providers.base import AgentOutput, AgentProvider, EventCallback +from conductor.skills import SkillNotFoundError + + +class _CapturingProvider(AgentProvider, abstract=True): + """Records what the executor forwarded on the last ``execute`` call.""" + + native = True + + def __init__(self) -> None: + self.skill_directories: list[str] | None = None + self.rendered_prompt: str = "" + + @property + def supports_native_skills(self) -> bool: + return self.native + + async def execute( + self, + agent: AgentDef, + context: dict[str, Any], + rendered_prompt: str, + tools: list[str] | None = None, + interrupt_signal: asyncio.Event | None = None, + event_callback: EventCallback | None = None, + skill_directories: list[str] | None = None, + ) -> AgentOutput: + self.skill_directories = skill_directories + self.rendered_prompt = rendered_prompt + return AgentOutput(content={"result": "done"}, raw_response="{}") + + async def validate_connection(self) -> bool: + return True + + async def close(self) -> None: + return None + + +class _EagerProvider(_CapturingProvider, abstract=True): + native = False + + +def _write_skill(directory: Path, filler: int = 0) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text( + f"---\nname: {directory.name}\ndescription: A test skill.\n---\nSkill body text\n" + ) + if filler: + (directory / "references").mkdir(exist_ok=True) + (directory / "references" / "big.md").write_text("x" * filler) + return directory + + +def _config( + skills: list[str], skill_injection: SkillInjectionConfig | None = None +) -> WorkflowConfig: + runtime_kwargs: dict[str, Any] = {"provider": "copilot", "skills": skills} + if skill_injection is not None: + runtime_kwargs["skill_injection"] = skill_injection + return WorkflowConfig( + workflow=WorkflowDef( + name="wf", entry_point="worker", runtime=RuntimeConfig(**runtime_kwargs) + ), + agents=[ + AgentDef( + name="worker", + prompt="Do the thing.", + output={"result": OutputField(type="string")}, + ) + ], + output={"result": "{{ worker.output.result }}"}, + ) + + +def _run(config: WorkflowConfig, provider: AgentProvider, workflow_path: Path | None) -> None: + engine = WorkflowEngine(config, provider, workflow_path=workflow_path) + asyncio.run(engine.run({})) + + +class TestRelativeSkillPathsThroughTheEngine: + def test_relative_path_resolves_against_the_workflow_file(self, tmp_path: Path) -> None: + """Fails if the engine stops passing ``workflow_dir`` to the executor.""" + skill = _write_skill(tmp_path / "team-skills" / "acme") + path = tmp_path / "wf.yaml" + path.write_text("# placeholder\n") + provider = _CapturingProvider() + _run(_config(["./team-skills/acme"]), provider, path) + assert provider.skill_directories == [str(skill)] + + def test_resolution_does_not_depend_on_the_process_cwd( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A workflow must resolve its own skills identically from any cwd.""" + skill = _write_skill(tmp_path / "flows" / "team-skills" / "acme") + path = tmp_path / "flows" / "wf.yaml" + path.write_text("# placeholder\n") + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + monkeypatch.chdir(elsewhere) + provider = _CapturingProvider() + _run(_config(["./team-skills/acme"]), provider, path) + assert provider.skill_directories == [str(skill)] + + def test_relative_path_reaches_eager_injection_too(self, tmp_path: Path) -> None: + skill = _write_skill(tmp_path / "team-skills" / "acme") + path = tmp_path / "wf.yaml" + path.write_text("# placeholder\n") + provider = _EagerProvider() + _run(_config(["./team-skills/acme"]), provider, path) + assert provider.skill_directories is None + assert '' in provider.rendered_prompt + assert "Skill body text" in provider.rendered_prompt + assert skill.exists() + + def test_builtin_names_work_without_a_workflow_path(self) -> None: + provider = _CapturingProvider() + _run(_config(["conductor"]), provider, None) + assert provider.skill_directories is not None + assert provider.skill_directories[0].endswith("conductor") + + def test_unresolvable_relative_path_fails_the_run(self, tmp_path: Path) -> None: + """Surfaces as ``SkillNotFoundError``, which the CLI renders as a titled + error panel rather than a traceback — verified manually against + ``conductor run``.""" + path = tmp_path / "wf.yaml" + path.write_text("# placeholder\n") + with pytest.raises(SkillNotFoundError, match="does not exist"): + _run(_config(["./nope"]), _CapturingProvider(), path) + + +class TestInjectionBudgetThroughTheEngine: + def test_workflow_limits_reach_the_executor(self, tmp_path: Path) -> None: + """Fails if the engine stops forwarding ``runtime.skill_injection``.""" + _write_skill(tmp_path / "acme", filler=5000) + path = tmp_path / "wf.yaml" + path.write_text("# placeholder\n") + config = _config( + ["./acme"], skill_injection=SkillInjectionConfig(warn_bytes=100, max_bytes=1000) + ) + with pytest.raises(ExecutionError, match="max_bytes"): + _run(config, _EagerProvider(), path) + + def test_generous_limits_allow_the_same_workflow(self, tmp_path: Path) -> None: + _write_skill(tmp_path / "acme", filler=5000) + path = tmp_path / "wf.yaml" + path.write_text("# placeholder\n") + config = _config( + ["./acme"], skill_injection=SkillInjectionConfig(warn_bytes=None, max_bytes=None) + ) + provider = _EagerProvider() + _run(config, provider, path) + assert '' in provider.rendered_prompt diff --git a/tests/test_skills/test_executor_integration.py b/tests/test_skills/test_executor_integration.py index 60c07ca6..e2b74031 100644 --- a/tests/test_skills/test_executor_integration.py +++ b/tests/test_skills/test_executor_integration.py @@ -11,6 +11,7 @@ from __future__ import annotations import asyncio +from pathlib import Path from typing import Any import pytest @@ -20,7 +21,6 @@ from conductor.providers.base import AgentOutput, AgentProvider, EventCallback from conductor.providers.copilot import CopilotProvider from conductor.skills import get_skill_directory -from conductor.skills.loader import _cached_skill_payload class _StubNonNativeProvider(AgentProvider, abstract=True): @@ -61,9 +61,6 @@ async def close(self) -> None: class TestCopilotProviderNativeSkills: """Copilot owns native ``skill_directories``; preamble is NOT injected.""" - def setup_method(self) -> None: - _cached_skill_payload.cache_clear() - def test_no_skill_content_in_rendered_prompt(self) -> None: provider = CopilotProvider() executor = AgentExecutor(provider, workflow_skills=["conductor"]) @@ -120,9 +117,6 @@ class TestSkillDirectoriesReachTheProvider: suite stays green -- the exact silent-drop failure #352 was about. """ - def setup_method(self) -> None: - _cached_skill_payload.cache_clear() - @staticmethod def _run(provider: _CapturingNativeProvider, agent: AgentDef) -> None: executor = AgentExecutor(provider, workflow_skills=["conductor"]) @@ -154,6 +148,58 @@ def test_non_native_provider_gets_no_directories(self) -> None: assert provider.captured is None +class TestPathSkillsReachTheProvider: + """Path entries ride the same executor -> provider seam as built-in names + (issue #350). ``workflow_dir`` is the only thing that makes a relative + entry resolvable, so a dropped constructor argument would silently turn + every team-local skill into a resolution error.""" + + @staticmethod + def _make_skill(directory: Path) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text( + f"---\nname: {directory.name}\ndescription: A test skill.\n---\nBody text\n" + ) + return directory + + def test_absolute_path_reaches_provider(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + provider = _CapturingNativeProvider() + executor = AgentExecutor(provider, workflow_skills=[str(skill)]) + asyncio.run(executor.execute(AgentDef(name="a", model="m", prompt="p"), {})) + assert provider.captured == [str(skill)] + + def test_relative_path_resolves_against_workflow_dir(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "team-skills" / "acme") + provider = _CapturingNativeProvider() + executor = AgentExecutor( + provider, workflow_skills=["./team-skills/acme"], workflow_dir=tmp_path + ) + asyncio.run(executor.execute(AgentDef(name="a", model="m", prompt="p"), {})) + assert provider.captured == [str(skill)] + + def test_skills_root_expands_before_reaching_provider(self, tmp_path: Path) -> None: + """Conductor expands a root itself so every provider — including the + eager-injection ones, which need a name per skill — sees the same set.""" + root = tmp_path / "skills" + for name in ("beta", "alpha"): + self._make_skill(root / name) + provider = _CapturingNativeProvider() + executor = AgentExecutor(provider, workflow_skills=[str(root)]) + asyncio.run(executor.execute(AgentDef(name="a", model="m", prompt="p"), {})) + assert provider.captured == [str(root / "alpha"), str(root / "beta")] + + def test_path_skill_content_is_eagerly_injected(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + provider = _StubNonNativeProvider() + executor = AgentExecutor(provider, workflow_skills=["./acme"], workflow_dir=tmp_path) + output = asyncio.run(executor.execute(AgentDef(name="a", model="m", prompt="p"), {})) + prompt = output.content["echo"] + assert '' in prompt + assert "Body text" in prompt + assert skill.name in prompt + + class TestClaudeAgentSdkNativeSkills: """claude-agent-sdk loads skills through the SDK, not the prompt.""" @@ -179,9 +225,6 @@ def test_provider_advertises_native_support(self) -> None: class TestNonNativeProviderEagerInjection: """Non-native providers receive skill content via the rendered prompt.""" - def setup_method(self) -> None: - _cached_skill_payload.cache_clear() - def test_not_injected_when_no_skills(self) -> None: executor = AgentExecutor(_StubNonNativeProvider()) agent = AgentDef(name="a", model="gpt-4", prompt="Hello world") diff --git a/tests/test_skills/test_frontmatter.py b/tests/test_skills/test_frontmatter.py new file mode 100644 index 00000000..207ae295 --- /dev/null +++ b/tests/test_skills/test_frontmatter.py @@ -0,0 +1,146 @@ +"""Tests for ``SKILL.md`` frontmatter parsing (issue #350). + +Both the Copilot CLI and Claude Code **silently skip** a skill whose +frontmatter fails to parse. Conductor parses it itself so the failure is +loud, which is the entire point of this module — every test here stands +in for a skill that would otherwise have gone missing without a word. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from conductor.skills import ( + SkillFrontmatter, + SkillManifestError, + get_skill_directory, + read_skill_frontmatter, +) + + +def _write_skill(directory: Path, body: str) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text(body, encoding="utf-8") + return directory + + +class TestValidFrontmatter: + def test_bundled_conductor_skill_parses(self) -> None: + """The skill Conductor ships must satisfy its own parser.""" + parsed = read_skill_frontmatter(get_skill_directory("conductor")) + assert parsed.name == "conductor" + assert parsed.description + + def test_returns_name_and_description(self, tmp_path: Path) -> None: + skill = _write_skill( + tmp_path / "s", "---\nname: acme\ndescription: Does acme things.\n---\nBody\n" + ) + assert read_skill_frontmatter(skill) == SkillFrontmatter( + name="acme", description="Does acme things." + ) + + def test_block_scalar_description_parses(self, tmp_path: Path) -> None: + """The documented workaround for the ``Triggers:`` trap must work.""" + skill = _write_skill( + tmp_path / "s", + "---\nname: acme\ndescription: |\n Does things. Triggers: widget, acme.\n---\n", + ) + assert "Triggers: widget, acme." in read_skill_frontmatter(skill).description + + def test_extra_keys_are_ignored(self, tmp_path: Path) -> None: + skill = _write_skill( + tmp_path / "s", + "---\nname: acme\ndescription: D\nlicense: MIT\nallowed-tools: [Bash]\n---\n", + ) + assert read_skill_frontmatter(skill).name == "acme" + + def test_crlf_line_endings_parse(self, tmp_path: Path) -> None: + skill = _write_skill( + tmp_path / "s", "---\r\nname: acme\r\ndescription: D\r\n---\r\nBody\r\n" + ) + assert read_skill_frontmatter(skill).name == "acme" + + def test_surrounding_whitespace_is_stripped(self, tmp_path: Path) -> None: + skill = _write_skill(tmp_path / "s", "---\nname: ' acme '\ndescription: ' D '\n---\n") + assert read_skill_frontmatter(skill) == SkillFrontmatter(name="acme", description="D") + + def test_thematic_break_in_body_is_not_frontmatter(self, tmp_path: Path) -> None: + """A ``---`` further down the file is Markdown, not a second manifest.""" + skill = _write_skill( + tmp_path / "s", "---\nname: acme\ndescription: D\n---\n\nIntro\n\n---\n\nMore\n" + ) + assert read_skill_frontmatter(skill).description == "D" + + +class TestMalformedFrontmatter: + def test_unquoted_colon_is_reported_with_the_fix(self, tmp_path: Path) -> None: + """The exact trap from issue #350. + + ``Triggers:`` inside an unquoted plain scalar is invalid YAML. This + is the case that cost the issue author several debugging rounds + because both CLIs skipped the skill without saying anything. + """ + skill = _write_skill( + tmp_path / "s", + "---\nname: acme-widgets\n" + "description: Internal ACME conventions. Triggers: widget, acme widget.\n---\n", + ) + with pytest.raises(SkillManifestError) as exc_info: + read_skill_frontmatter(skill) + message = str(exc_info.value) + assert "invalid YAML frontmatter" in message + assert "description: |" in message, "the error must show the block-scalar fix" + + def test_missing_skill_md(self, tmp_path: Path) -> None: + (tmp_path / "s").mkdir() + with pytest.raises(SkillManifestError, match="has no SKILL.md"): + read_skill_frontmatter(tmp_path / "s") + + def test_no_frontmatter_block(self, tmp_path: Path) -> None: + skill = _write_skill(tmp_path / "s", "# Just a heading\n\nNo frontmatter here.\n") + with pytest.raises(SkillManifestError, match="no YAML frontmatter"): + read_skill_frontmatter(skill) + + def test_unterminated_frontmatter_block(self, tmp_path: Path) -> None: + skill = _write_skill(tmp_path / "s", "---\nname: acme\ndescription: D\n\nBody\n") + with pytest.raises(SkillManifestError, match="no YAML frontmatter"): + read_skill_frontmatter(skill) + + @pytest.mark.parametrize( + ("body", "missing"), + [ + ("---\ndescription: D\n---\n", "name"), + ("---\nname: acme\n---\n", "description"), + ("---\nname: ''\ndescription: D\n---\n", "name"), + ("---\nname: acme\ndescription: ' '\n---\n", "description"), + ("---\nname: 42\ndescription: D\n---\n", "name"), + ("---\nname: acme\ndescription: [a, b]\n---\n", "description"), + ], + ) + def test_missing_or_unusable_fields(self, tmp_path: Path, body: str, missing: str) -> None: + skill = _write_skill(tmp_path / "s", body) + with pytest.raises(SkillManifestError, match=f"no usable '{missing}'"): + read_skill_frontmatter(skill) + + @pytest.mark.parametrize( + "body", + [ + "---\n- a\n- b\n---\n", + "---\njust a string\n---\n", + ], + ids=["sequence", "scalar"], + ) + def test_frontmatter_that_is_not_a_mapping(self, tmp_path: Path, body: str) -> None: + skill = _write_skill(tmp_path / "s", body) + with pytest.raises(SkillManifestError, match="not a YAML mapping"): + read_skill_frontmatter(skill) + + def test_unreadable_skill_md_is_reported(self, tmp_path: Path) -> None: + """Invalid UTF-8 must not escape as a bare UnicodeDecodeError.""" + skill = tmp_path / "s" + skill.mkdir() + (skill / "SKILL.md").write_bytes(b"---\nname: \xff\xfe\ndescription: D\n---\n") + with pytest.raises(SkillManifestError, match="could not be read"): + read_skill_frontmatter(skill) diff --git a/tests/test_skills/test_injection_budget.py b/tests/test_skills/test_injection_budget.py new file mode 100644 index 00000000..aa085ab4 --- /dev/null +++ b/tests/test_skills/test_injection_budget.py @@ -0,0 +1,332 @@ +"""Tests for the eager skill-injection budget (issue #350). + +Providers without a native skill surface have no progressive disclosure: +``AgentExecutor`` prepends every enabled skill's ``SKILL.md`` *plus its +whole ``references/`` tree* to the prompt on every call and every retry. The +bundled ``conductor`` skill alone is ~117KB (~29K tokens), and before this +there was no ceiling of any kind. (A ``validator:`` block's +own grading call bypasses prompt rendering, so it does not re-pay this.) + +Defaults are deliberately chosen so that the existing ~117KB case *warns* +rather than breaking — ``test_bundled_skill_warns_but_does_not_error`` +pins that, since a stricter default would be a silent breaking change. +""" + +from __future__ import annotations + +import asyncio +import logging +from pathlib import Path +from typing import Any + +import pytest +from pydantic import ValidationError + +from conductor.config.schema import AgentDef, RuntimeConfig, SkillInjectionConfig +from conductor.exceptions import ExecutionError +from conductor.executor.agent import AgentExecutor +from conductor.providers.base import AgentOutput, AgentProvider, EventCallback +from conductor.skills import get_skill_directory, load_skill_content + + +class _EagerProvider(AgentProvider, abstract=True): + """Stub with ``supports_native_skills = False`` (the injecting path).""" + + @property + def supports_native_skills(self) -> bool: + return False + + async def execute( + self, + agent: AgentDef, + context: dict[str, Any], + rendered_prompt: str, + tools: list[str] | None = None, + interrupt_signal: asyncio.Event | None = None, + event_callback: EventCallback | None = None, + skill_directories: list[str] | None = None, + ) -> AgentOutput: + return AgentOutput(content={"ok": True}, raw_response="") + + async def validate_connection(self) -> bool: + return True + + async def close(self) -> None: + return None + + +class _NativeProvider(_EagerProvider, abstract=True): + @property + def supports_native_skills(self) -> bool: + return True + + +def _make_skill(directory: Path, filler_bytes: int = 0) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text( + f"---\nname: {directory.name}\ndescription: A test skill.\n---\nBody\n" + ) + if filler_bytes: + references = directory / "references" + references.mkdir(exist_ok=True) + (references / "big.md").write_text("x" * filler_bytes) + return directory + + +def _executor(provider: AgentProvider, **limits: int | None) -> AgentExecutor: + return AgentExecutor(provider, skill_injection=SkillInjectionConfig(**limits)) + + +class TestBudgetEnforcement: + def test_over_max_bytes_raises(self, tmp_path: Path) -> None: + skill = _make_skill(tmp_path / "big", filler_bytes=5000) + agent = AgentDef(name="a", prompt="hi", skills=[str(skill)]) + executor = _executor(_EagerProvider(), warn_bytes=100, max_bytes=1000) + with pytest.raises(ExecutionError) as exc_info: + executor._build_prompt_prefix(agent) + message = str(exc_info.value) + assert "max_bytes" in message + assert "big" in message, "the per-skill breakdown names the offender" + assert exc_info.value.agent_name == "a" + assert exc_info.value.suggestion is not None + + def test_under_warn_bytes_is_silent( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + skill = _make_skill(tmp_path / "small") + agent = AgentDef(name="a", prompt="hi", skills=[str(skill)]) + with caplog.at_level(logging.WARNING): + prefix = _executor(_EagerProvider())._build_prompt_prefix(agent) + assert prefix, "content is still injected" + assert not caplog.records + + def test_between_thresholds_warns_without_raising( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + skill = _make_skill(tmp_path / "mid", filler_bytes=5000) + agent = AgentDef(name="a", prompt="hi", skills=[str(skill)]) + executor = _executor(_EagerProvider(), warn_bytes=1000, max_bytes=100_000) + with caplog.at_level(logging.WARNING): + prefix = executor._build_prompt_prefix(agent) + assert prefix + assert any("skill content" in record.message for record in caplog.records) + + @pytest.mark.parametrize( + ("limits", "label"), + [ + ({"warn_bytes": None, "max_bytes": None}, "both disabled"), + ({"warn_bytes": None, "max_bytes": 100_000}, "warning disabled"), + ], + ) + def test_null_limits_disable_checks( + self, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, + limits: dict[str, int | None], + label: str, + ) -> None: + skill = _make_skill(tmp_path / "mid", filler_bytes=5000) + agent = AgentDef(name="a", prompt="hi", skills=[str(skill)]) + with caplog.at_level(logging.WARNING): + assert _executor(_EagerProvider(), **limits)._build_prompt_prefix(agent) + assert not caplog.records, label + + def test_bundled_skill_warns_but_does_not_error(self, caplog: pytest.LogCaptureFixture) -> None: + """``skills: [conductor]`` on an eager provider already ships today. + + The defaults must surface its ~117KB payload without breaking it — + a lower ``max_bytes`` default would be a silent breaking change. + """ + agent = AgentDef(name="a", prompt="hi", skills=["conductor"]) + with caplog.at_level(logging.WARNING): + prefix = _executor(_EagerProvider())._build_prompt_prefix(agent) + assert prefix + assert any("skill content" in record.message for record in caplog.records) + + def test_bundled_skill_size_sits_between_the_defaults(self) -> None: + """Pins the assumption the defaults were chosen against, so a skill + that grows past 128KB fails here rather than in a user's workflow. + + Measures the *rendered* string, which is what both enforcement paths + compare against — summing raw file sizes would understate it by the + ````/```` envelope and drift further as references grow. + """ + directory = get_skill_directory("conductor") + size = len(load_skill_content([("conductor", directory)]).encode("utf-8")) + defaults = SkillInjectionConfig() + assert defaults.warn_bytes is not None and defaults.max_bytes is not None + assert defaults.warn_bytes < size < defaults.max_bytes + + +class TestBudgetScope: + def test_native_providers_are_unaffected( + self, tmp_path: Path, caplog: pytest.LogCaptureFixture + ) -> None: + """Progressive disclosure means nothing is prepended, so no limit applies + even at a threshold the same skill would blow past when injected.""" + skill = _make_skill(tmp_path / "big", filler_bytes=5000) + agent = AgentDef(name="a", prompt="hi", skills=[str(skill)]) + executor = _executor(_NativeProvider(), warn_bytes=10, max_bytes=100) + with caplog.at_level(logging.WARNING): + assert executor._build_prompt_prefix(agent) == "" + assert not caplog.records + + def test_agent_opting_out_is_unaffected(self, caplog: pytest.LogCaptureFixture) -> None: + agent = AgentDef(name="a", prompt="hi", skills=[]) + executor = _executor(_EagerProvider(), warn_bytes=10, max_bytes=100) + with caplog.at_level(logging.WARNING): + assert executor._build_prompt_prefix(agent) == "" + assert not caplog.records + + def test_workflow_default_skills_are_budgeted(self, tmp_path: Path) -> None: + """An inherited ``runtime.skills`` list costs the same as a per-agent one.""" + skill = _make_skill(tmp_path / "big", filler_bytes=5000) + agent = AgentDef(name="a", prompt="hi") + executor = AgentExecutor( + _EagerProvider(), + workflow_skills=[str(skill)], + skill_injection=SkillInjectionConfig(warn_bytes=100, max_bytes=1000), + ) + with pytest.raises(ExecutionError, match="max_bytes"): + executor._build_prompt_prefix(agent) + + def test_combined_skills_are_measured_together(self, tmp_path: Path) -> None: + """Two skills that each fit can still exceed the limit together — the + accumulation case the budget exists for.""" + first = _make_skill(tmp_path / "one", filler_bytes=3000) + second = _make_skill(tmp_path / "two", filler_bytes=3000) + executor = _executor(_EagerProvider(), warn_bytes=100, max_bytes=5000) + for skill in (first, second): + alone = executor._build_prompt_prefix( + AgentDef(name="a", prompt="hi", skills=[str(skill)]) + ) + assert len(alone.encode("utf-8")) <= 5000, "each skill must fit on its own" + with pytest.raises(ExecutionError, match="max_bytes"): + executor._build_prompt_prefix( + AgentDef(name="a", prompt="hi", skills=[str(first), str(second)]) + ) + + +class TestSkillInjectionConfigSchema: + def test_defaults(self) -> None: + config = SkillInjectionConfig() + assert config.warn_bytes == 64 * 1024 + assert config.max_bytes == 128 * 1024 + + def test_warn_above_max_is_rejected(self) -> None: + """Such a config can never warn — the error fires first.""" + with pytest.raises(ValueError, match="must not exceed"): + SkillInjectionConfig(warn_bytes=200_000, max_bytes=1_000) + + def test_equal_thresholds_are_allowed(self) -> None: + """Equality makes the warning unreachable for the same reason + ``warn > max`` does, but it is a coherent "hard limit only" request + and ``warn_bytes: null`` is not the only way to spell it. Allowed + deliberately rather than by oversight. + """ + assert SkillInjectionConfig(warn_bytes=1000, max_bytes=1000).max_bytes == 1000 + + def test_frozen_after_construction(self) -> None: + """``validate_assignment`` on the enclosing ``RuntimeConfig`` does not + re-fire this model's cross-field validator on attribute assignment, so + without ``frozen=True`` both invariants are bypassable post-construction. + + Same reasoning ``ProviderSettings`` records for the same Pydantic gotcha. + """ + config = SkillInjectionConfig(warn_bytes=100, max_bytes=1000) + with pytest.raises(ValidationError): + config.warn_bytes = 999_999 + assert RuntimeConfig().skill_injection is not None + with pytest.raises(ValidationError): + RuntimeConfig().skill_injection.max_bytes = -5 + + def test_negative_values_rejected(self) -> None: + with pytest.raises(ValueError): + SkillInjectionConfig(max_bytes=-1) + + def test_unknown_field_rejected(self) -> None: + with pytest.raises(ValueError): + SkillInjectionConfig(max_byte=1) # ty: ignore[unknown-argument] + + +class TestUnsupportedProviderRejection: + """``capabilities.skills=False`` must hold at run time, not only at + ``conductor validate`` time. + + ``conductor run`` never calls the static validator, so without this the + declaration was enforced in one place and quietly contradicted in the + other: the eager-injection path keys off ``supports_native_skills``, so a + provider declaring ``skills=False`` still had the full skill body + prepended to its prompt. + """ + + def test_provider_declaring_no_skill_support_is_refused(self) -> None: + from conductor.providers.aca import AcaRuntimeProvider + + assert AcaRuntimeProvider.CAPABILITIES.skills is False + + class _Unsupported(_EagerProvider, abstract=True): + CAPABILITIES = AcaRuntimeProvider.CAPABILITIES + + agent = AgentDef(name="a", prompt="hi", skills=["conductor"]) + with pytest.raises(ExecutionError) as exc_info: + AgentExecutor(_Unsupported())._build_prompt_prefix(agent) + assert "does not support skills" in str(exc_info.value) + assert exc_info.value.agent_name == "a" + + def test_opting_out_on_such_a_provider_is_fine(self) -> None: + from conductor.providers.aca import AcaRuntimeProvider + + class _Unsupported(_EagerProvider, abstract=True): + CAPABILITIES = AcaRuntimeProvider.CAPABILITIES + + agent = AgentDef(name="a", prompt="hi", skills=[]) + assert AgentExecutor(_Unsupported())._build_prompt_prefix(agent) == "" + + def test_provider_without_capabilities_is_left_alone(self) -> None: + """Test fakes declare ``abstract=True`` and have no CAPABILITIES; + they must not be swept up by the check.""" + agent = AgentDef(name="a", prompt="hi", skills=["conductor"]) + assert AgentExecutor(_EagerProvider())._build_prompt_prefix(agent) + + +class TestWarningReachesTheUser: + """`logger.warning` alone does not reach a user running `conductor run`: + Conductor installs no logging handlers, so it surfaces through + `logging.lastResort` as an unattributed stderr line, absent from the JSONL + log and the dashboard. Since the defaults trip this for the bundled skill + on every eager-provider call, it has to travel the event channel too — + the same both-halves pattern as `checkpoint_save_failed`. + """ + + @staticmethod + def _capture(tmp_path: Path, **limits: int | None) -> list[tuple[str, dict[str, object]]]: + skill = _make_skill(tmp_path / "mid", filler_bytes=5000) + events: list[tuple[str, dict[str, object]]] = [] + executor = _executor(_EagerProvider(), **limits) + executor._build_prompt_prefix( + AgentDef(name="a", prompt="hi", skills=[str(skill)]), + lambda name, data: events.append((name, data)), + ) + return events + + def test_breach_emits_an_event(self, tmp_path: Path) -> None: + events = self._capture(tmp_path, warn_bytes=1000, max_bytes=100_000) + assert [name for name, _ in events] == ["skill_injection_warning"] + payload = events[0][1] + assert payload["agent_name"] == "a" + assert isinstance(payload["bytes"], int) and payload["bytes"] > 1000 + assert payload["warn_bytes"] == 1000 + assert "mid" in str(payload["breakdown"]) + + def test_no_event_below_the_threshold(self, tmp_path: Path) -> None: + assert self._capture(tmp_path, warn_bytes=100_000, max_bytes=200_000) == [] + + def test_no_event_when_warning_disabled(self, tmp_path: Path) -> None: + assert self._capture(tmp_path, warn_bytes=None, max_bytes=200_000) == [] + + def test_omitting_the_callback_still_works(self, tmp_path: Path) -> None: + """The callback is optional — `render_prompt` is called without one.""" + skill = _make_skill(tmp_path / "mid", filler_bytes=5000) + executor = _executor(_EagerProvider(), warn_bytes=1000, max_bytes=100_000) + assert executor._build_prompt_prefix(AgentDef(name="a", prompt="hi", skills=[str(skill)])) diff --git a/tests/test_skills/test_loader.py b/tests/test_skills/test_loader.py index e9cdfd6f..8c77044d 100644 --- a/tests/test_skills/test_loader.py +++ b/tests/test_skills/test_loader.py @@ -2,9 +2,12 @@ from __future__ import annotations +import os from pathlib import Path -from conductor.skills import get_skill_directory, load_skill_content +import pytest + +from conductor.skills import SkillManifestError, get_skill_directory, load_skill_content from conductor.skills.loader import _cached_skill_payload @@ -53,3 +56,70 @@ def test_caches_per_dir(self) -> None: def test_empty_dir_returns_empty(self, tmp_path: Path) -> None: # No SKILL.md, no references/. assert load_skill_content([("empty", tmp_path)]) == "" + + +class TestUnreadableContentFailsLoudly: + """A skill file that cannot be read must raise, not be skipped. + + This package exists because the upstream CLIs drop an unloadable skill in + silence. Doing the same for a `references/*.md` file would be the same + defect one directory deeper — and by volume it is the worse half: for the + bundled `conductor` skill the references are ~93% of the payload, so a + single unreadable file could cut the agent's knowledge to a fraction while + the run reported success. + """ + + @staticmethod + def _make_skill(directory: Path) -> Path: + (directory / "references").mkdir(parents=True) + (directory / "SKILL.md").write_text( + f"---\nname: {directory.name}\ndescription: A test skill.\n---\nBody\n" + ) + (directory / "references" / "a.md").write_text("Reference A") + return directory + + def test_undecodable_reference_raises(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + (skill / "references" / "bad.md").write_bytes(b"\xff\xfe not utf-8") + with pytest.raises(SkillManifestError, match="reference.*could not be read"): + load_skill_content([("acme", skill)]) + + def test_undecodable_manifest_raises(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + (skill / "SKILL.md").write_bytes(b"\xff\xfe not utf-8") + with pytest.raises(SkillManifestError, match="manifest.*could not be read"): + load_skill_content([("acme", skill)]) + + @pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="root bypasses file permissions", + ) + def test_unreadable_reference_raises(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + blocked = skill / "references" / "blocked.md" + blocked.write_text("secret") + blocked.chmod(0o000) + try: + with pytest.raises(SkillManifestError, match="could not be read"): + load_skill_content([("acme", skill)]) + finally: + blocked.chmod(0o644) + + def test_failure_is_not_cached(self, tmp_path: Path) -> None: + """``lru_cache`` never memoizes a raising call, so a transient error is + retried rather than frozen in as an empty payload for the whole run.""" + skill = self._make_skill(tmp_path / "acme") + bad = skill / "references" / "bad.md" + bad.write_bytes(b"\xff\xfe") + with pytest.raises(SkillManifestError): + load_skill_content([("acme", skill)]) + + bad.write_text("Now readable") + content = load_skill_content([("acme", skill)]) + assert "Now readable" in content + assert "Reference A" in content + + def test_a_readable_skill_is_unaffected(self, tmp_path: Path) -> None: + skill = self._make_skill(tmp_path / "acme") + content = load_skill_content([("acme", skill)]) + assert "Reference A" in content and "Body" in content diff --git a/tests/test_skills/test_path_entries.py b/tests/test_skills/test_path_entries.py new file mode 100644 index 00000000..35a2088c --- /dev/null +++ b/tests/test_skills/test_path_entries.py @@ -0,0 +1,338 @@ +"""Tests for path entries in ``skills:`` (issue #350). + +Before this, ``skills:`` accepted exactly one hardcoded name and +``get_skill_directory`` raised for anything else, so a team could not +version a skill alongside its workflow. These tests cover the two things +this made possible — classifying an entry as a name or a path, and +expanding a path at either granularity. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +import pytest + +from conductor.skills import ( + SkillManifestError, + SkillNotFoundError, + get_skill_directory, + is_path_entry, + resolve_skills, +) + +_FRONTMATTER = "---\nname: {name}\ndescription: A test skill.\n---\nBody\n" + + +def _make_skill(directory: Path, name: str | None = None) -> Path: + directory.mkdir(parents=True, exist_ok=True) + (directory / "SKILL.md").write_text(_FRONTMATTER.format(name=name or directory.name)) + return directory + + +class TestPathClassification: + """Classification is syntactic so it never depends on what happens to + exist locally — a bare name cannot be shadowed by a same-named directory.""" + + @pytest.mark.parametrize( + "entry", + ["./skills/a", "../a", "~/skills", "/abs/a", "team/a", r"team\a", "~"], + ) + def test_path_shaped_entries(self, entry: str) -> None: + assert is_path_entry(entry) is True + + @pytest.mark.parametrize("entry", ["conductor", "acme-widgets", "a_b.c"]) + def test_name_shaped_entries(self, entry: str) -> None: + assert is_path_entry(entry) is False + + def test_bare_name_is_not_shadowed_by_a_local_directory( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A directory named ``conductor`` in the cwd must not hijack the + built-in — otherwise resolution would silently depend on cwd.""" + _make_skill(tmp_path / "conductor") + monkeypatch.chdir(tmp_path) + resolved = resolve_skills(["conductor"]) + assert resolved[0].directory == get_skill_directory("conductor") + + +class TestResolveNames: + def test_builtin_name_resolves(self) -> None: + resolved = resolve_skills(["conductor"]) + assert [item.name for item in resolved] == ["conductor"] + assert resolved[0].directory == get_skill_directory("conductor") + assert resolved[0].source == "conductor" + + def test_unknown_name_points_at_the_path_form(self) -> None: + with pytest.raises(SkillNotFoundError, match="Unknown skill 'nope'") as exc_info: + resolve_skills(["nope"]) + assert "./team-skills/my-skill" in str(exc_info.value) + + +class TestResolveSingleSkillDirectory: + def test_absolute_path(self, tmp_path: Path) -> None: + skill = _make_skill(tmp_path / "acme-widgets") + resolved = resolve_skills([str(skill)]) + assert [(item.name, item.directory) for item in resolved] == [("acme-widgets", skill)] + + def test_relative_path_resolves_against_base_dir(self, tmp_path: Path) -> None: + skill = _make_skill(tmp_path / "team-skills" / "acme-widgets") + resolved = resolve_skills(["./team-skills/acme-widgets"], base_dir=tmp_path) + assert resolved[0].directory == skill + assert resolved[0].source == "./team-skills/acme-widgets" + + def test_relative_path_falls_back_to_cwd( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + skill = _make_skill(tmp_path / "acme") + monkeypatch.chdir(tmp_path) + assert resolve_skills(["./acme"])[0].directory == skill + + def test_user_home_is_expanded(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + skill = _make_skill(tmp_path / "scratch" / "acme") + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + assert resolve_skills(["~/scratch/acme"])[0].directory == skill + + def test_dot_segments_are_normalised(self, tmp_path: Path) -> None: + skill = _make_skill(tmp_path / "a" / "acme") + resolved = resolve_skills(["./a/../a/acme"], base_dir=tmp_path) + assert resolved[0].directory == skill + + def test_symlink_alias_is_not_collapsed(self, tmp_path: Path) -> None: + """``normpath`` rather than ``resolve()``, matching how the engine + treats ``working_dir`` — a symlinked path stays the path the user wrote.""" + skill = _make_skill(tmp_path / "real" / "acme") + link = tmp_path / "link" + link.symlink_to(tmp_path / "real", target_is_directory=True) + assert resolve_skills([str(link / "acme")])[0].directory == link / "acme" + assert skill.exists() + + +class TestResolveSkillsRoot: + def test_root_expands_to_every_child(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + for name in ("gamma", "alpha", "beta"): + _make_skill(root / name) + resolved = resolve_skills([str(root)]) + assert [item.name for item in resolved] == ["alpha", "beta", "gamma"] + + def test_expanded_children_share_the_written_source(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + for name in ("alpha", "beta"): + _make_skill(root / name) + resolved = resolve_skills(["./skills"], base_dir=tmp_path) + assert {item.source for item in resolved} == {"./skills"} + + def test_children_without_skill_md_are_not_skills(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + _make_skill(root / "alpha") + (root / "not-a-skill").mkdir() + assert [item.name for item in resolve_skills([str(root)])] == ["alpha"] + + def test_a_directory_holding_skill_md_wins_over_child_scan(self, tmp_path: Path) -> None: + """A skill directory that happens to contain a nested skill directory + resolves as itself, not as a root.""" + skill = _make_skill(tmp_path / "acme") + _make_skill(skill / "nested") + assert [item.name for item in resolve_skills([str(skill)])] == ["acme"] + + def test_expansion_is_not_recursive(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + _make_skill(root / "group" / "deep") + with pytest.raises(SkillNotFoundError, match="neither a SKILL.md nor"): + resolve_skills([str(root)]) + + +class TestResolutionErrors: + def test_missing_path(self, tmp_path: Path) -> None: + with pytest.raises(SkillNotFoundError, match="does not exist"): + resolve_skills(["./nope"], base_dir=tmp_path) + + def test_path_pointing_at_a_file(self, tmp_path: Path) -> None: + (tmp_path / "a-file").write_text("hi") + with pytest.raises(SkillNotFoundError, match="is not a directory"): + resolve_skills(["./a-file"], base_dir=tmp_path) + + def test_empty_directory(self, tmp_path: Path) -> None: + (tmp_path / "empty").mkdir() + with pytest.raises(SkillNotFoundError, match="neither a SKILL.md nor"): + resolve_skills(["./empty"], base_dir=tmp_path) + + def test_malformed_frontmatter_fails_resolution(self, tmp_path: Path) -> None: + """Resolution — not just ``conductor validate`` — rejects a broken + manifest, because ``conductor run`` never calls the static validator.""" + skill = tmp_path / "acme" + skill.mkdir() + (skill / "SKILL.md").write_text("---\nname: acme\ndescription: A. Triggers: b, c\n---\n") + with pytest.raises(SkillManifestError, match="invalid YAML frontmatter"): + resolve_skills([str(skill)]) + + def test_one_bad_skill_in_a_root_fails_the_whole_entry(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + _make_skill(root / "good") + bad = root / "bad" + bad.mkdir(parents=True) + (bad / "SKILL.md").write_text("---\nname: bad\n---\n") + with pytest.raises(SkillManifestError, match="no usable 'description'"): + resolve_skills([str(root)]) + + +class TestOrderingAndDeduplication: + def test_entry_order_is_preserved(self, tmp_path: Path) -> None: + zulu = _make_skill(tmp_path / "zulu") + alpha = _make_skill(tmp_path / "alpha") + resolved = resolve_skills([str(zulu), str(alpha), "conductor"]) + assert [item.name for item in resolved] == ["zulu", "alpha", "conductor"] + + def test_duplicate_directories_collapse_to_first_occurrence(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + _make_skill(root / "alpha") + _make_skill(root / "beta") + resolved = resolve_skills([str(root), str(root / "alpha")]) + assert [item.name for item in resolved] == ["alpha", "beta"] + assert resolved[0].source == str(root), "first occurrence wins" + + def test_name_and_equivalent_path_deduplicate(self) -> None: + builtin = get_skill_directory("conductor") + resolved = resolve_skills(["conductor", str(builtin)]) + assert [item.name for item in resolved] == ["conductor"] + + +class TestUnreadableDirectory: + @pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="root bypasses directory permissions", + ) + def test_unreadable_directory_is_reported_not_raised_raw(self, tmp_path: Path) -> None: + """A stat-able but unreadable directory must name the entry rather than + surfacing a bare PermissionError traceback from ``iterdir``.""" + blocked = tmp_path / "blocked" + blocked.mkdir() + blocked.chmod(0o000) + try: + with pytest.raises(SkillNotFoundError, match="could not be read"): + resolve_skills([str(blocked)]) + finally: + blocked.chmod(0o755) + + +class TestNameCollisions: + """Every consumer is name-keyed — the eager preamble emits one + ```` per skill and the native CLIs resolve by name — so + two directories claiming one name would leave one silently shadowed. That + is the failure mode #350 exists to remove, so it is refused.""" + + def test_two_directories_with_the_same_basename_are_refused(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "a" / "review") + _make_skill(tmp_path / "b" / "review") + with pytest.raises(SkillNotFoundError, match="both resolve to a skill named 'review'"): + resolve_skills([str(tmp_path / "a" / "review"), str(tmp_path / "b" / "review")]) + + def test_error_names_both_sources_as_written(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "a" / "review") + _make_skill(tmp_path / "b" / "review") + with pytest.raises(SkillNotFoundError) as exc_info: + resolve_skills(["./a/review", "./b/review"], base_dir=tmp_path) + message = str(exc_info.value) + assert "'./a/review'" in message and "'./b/review'" in message + + def test_a_root_colliding_with_an_explicit_path_is_refused(self, tmp_path: Path) -> None: + """The likely real-world shape: a skills root plus a same-named skill + from somewhere else.""" + _make_skill(tmp_path / "root" / "review") + _make_skill(tmp_path / "other" / "review") + with pytest.raises(SkillNotFoundError, match="must be unique"): + resolve_skills([str(tmp_path / "root"), str(tmp_path / "other" / "review")]) + + def test_the_same_directory_twice_is_still_deduplicated(self, tmp_path: Path) -> None: + """Collision refusal must not break dedupe — the same directory named + twice is one skill, not a clash.""" + skill = _make_skill(tmp_path / "review") + resolved = resolve_skills([str(skill), str(skill)]) + assert [item.name for item in resolved] == ["review"] + + def test_builtin_and_a_same_named_path_collide(self, tmp_path: Path) -> None: + _make_skill(tmp_path / "conductor") + with pytest.raises(SkillNotFoundError, match="must be unique"): + resolve_skills(["conductor", str(tmp_path / "conductor")]) + + +class TestUnreadableParent: + @pytest.mark.skipif( + hasattr(os, "geteuid") and os.geteuid() == 0, + reason="root bypasses directory permissions", + ) + def test_unreadable_parent_is_reported_not_raised_raw(self, tmp_path: Path) -> None: + """``exists`` and ``is_dir`` sit inside the OSError guard for this case; + without them there it escapes as a bare PermissionError.""" + parent = tmp_path / "locked" + (parent / "acme").mkdir(parents=True) + parent.chmod(0o000) + try: + with pytest.raises(SkillNotFoundError, match="could not be read"): + resolve_skills([str(parent / "acme")]) + finally: + parent.chmod(0o755) + + +class TestWindowsSeparatorOnPosix: + @pytest.mark.skipif(os.name == "nt", reason="backslash is a real separator on Windows") + def test_backslash_relative_path_classifies_but_does_not_resolve(self, tmp_path: Path) -> None: + """Pins current behaviour: a workflow authored on Windows with + ``skills: ["team\\acme"]`` classifies as a path everywhere, but on POSIX + the backslash is an ordinary filename character, so it fails to resolve + rather than finding ``team/acme``.""" + _make_skill(tmp_path / "team" / "acme") + assert is_path_entry("team\\acme") is True + with pytest.raises(SkillNotFoundError, match="does not exist"): + resolve_skills(["team\\acme"], base_dir=tmp_path) + + +class TestSkillsRootDiagnostics: + """A skills root that skips a subdirectory reports it. + + Naming a directory with no `SKILL.md` directly raises; naming its *parent* + used to turn that same mistake into silence — one fewer skill, no message. + """ + + def test_subdirectory_without_skill_md_is_reported(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + _make_skill(root / "alpha") + (root / "oops").mkdir() + (root / "oops" / "Skill.md").write_text("mis-cased filename") + + warnings: list[str] = [] + resolved = resolve_skills([str(root)], on_warning=warnings.append) + + assert [item.name for item in resolved] == ["alpha"] + assert len(warnings) == 1 + assert "oops" in warnings[0] + assert "SKILL.md" in warnings[0] + + def test_loose_files_are_not_reported(self, tmp_path: Path) -> None: + """A README beside skill directories is normal, not a mistake.""" + root = tmp_path / "skills" + _make_skill(root / "alpha") + (root / "README.md").write_text("about these skills") + (root / "LICENSE").write_text("MIT") + + warnings: list[str] = [] + resolve_skills([str(root)], on_warning=warnings.append) + assert warnings == [] + + def test_no_warning_when_every_child_is_a_skill(self, tmp_path: Path) -> None: + root = tmp_path / "skills" + for name in ("alpha", "beta"): + _make_skill(root / name) + warnings: list[str] = [] + resolve_skills([str(root)], on_warning=warnings.append) + assert warnings == [] + + def test_resolution_works_without_a_sink(self, tmp_path: Path) -> None: + """The sink is optional — omitting it must not break resolution.""" + root = tmp_path / "skills" + _make_skill(root / "alpha") + (root / "oops").mkdir() + assert [item.name for item in resolve_skills([str(root)])] == ["alpha"] diff --git a/tests/test_skills/test_registry.py b/tests/test_skills/test_registry.py index 0a14eca0..067dcb6a 100644 --- a/tests/test_skills/test_registry.py +++ b/tests/test_skills/test_registry.py @@ -9,13 +9,16 @@ import pytest from conductor.skills import ( + ResolvedSkill, + SkillError, + SkillManifestError, SkillNotFoundError, SkillPlugin, SkillPluginError, get_skill_directory, list_builtin_skills, - resolve_skill_directories, resolve_skill_plugin, + resolve_skills, ) from conductor.skills.registry import _BUILTIN_SKILLS @@ -54,22 +57,22 @@ def test_unknown_skill_lists_available(self) -> None: get_skill_directory("does-not-exist") -class TestResolveSkillDirectories: +class TestResolveSkills: def test_empty_input_returns_empty(self) -> None: - assert resolve_skill_directories([]) == [] + assert resolve_skills([]) == [] def test_single_skill(self) -> None: - dirs = resolve_skill_directories(["conductor"]) - assert len(dirs) == 1 - assert dirs[0].is_dir() + resolved = resolve_skills(["conductor"]) + assert len(resolved) == 1 + assert resolved[0].name == "conductor" + assert resolved[0].directory.is_dir() def test_deduplicates(self) -> None: - dirs = resolve_skill_directories(["conductor", "conductor"]) - assert len(dirs) == 1 + assert len(resolve_skills(["conductor", "conductor"])) == 1 def test_unknown_raises(self) -> None: with pytest.raises(SkillNotFoundError): - resolve_skill_directories(["conductor", "nope"]) + resolve_skills(["conductor", "nope"]) def _make_plugin( @@ -87,7 +90,12 @@ def _make_plugin( skill_dir = root / "skills" / nest / skill if nest else root / "skills" / skill skill_dir.mkdir(parents=True) if frontmatter_name is not None: - (skill_dir / "SKILL.md").write_text(f"---\nname: {frontmatter_name}\n---\n") + # ``description`` is required frontmatter — without it the manifest + # parser rejects the skill before any plugin resolution happens, and + # these tests would stop covering what they name. + (skill_dir / "SKILL.md").write_text( + f"---\nname: {frontmatter_name}\ndescription: A test skill.\n---\n" + ) return skill_dir @@ -162,7 +170,7 @@ def test_plugin_that_does_not_ship_the_skill_is_skipped(self, tmp_path: Path) -> (root / ".claude-plugin" / "plugin.json").write_text('{"name": "unrelated"}') stray = root / "elsewhere" / "mySkill" stray.mkdir(parents=True) - (stray / "SKILL.md").write_text("---\nname: mySkill\n---\n") + (stray / "SKILL.md").write_text("---\nname: mySkill\ndescription: Stray.\n---\n") assert resolve_skill_plugin(stray) is None @pytest.mark.parametrize( @@ -201,7 +209,7 @@ def test_missing_skill_md_raises(self, tmp_path: Path) -> None: def test_frontmatter_without_name_raises(self, tmp_path: Path) -> None: skill = _make_plugin(tmp_path, frontmatter_name=None) (skill / "SKILL.md").write_text("---\ndescription: no name here\n---\n") - with pytest.raises(SkillPluginError, match="no 'name'"): + with pytest.raises(SkillPluginError, match="no usable 'name'"): resolve_skill_plugin(skill) def test_frontmatter_name_disagreeing_with_directory_raises(self, tmp_path: Path) -> None: @@ -250,3 +258,33 @@ def test_unsafe_directory_name_surfaces_as_skill_plugin_error(self, tmp_path: Pa def test_valid_instance_builds_qualified_name(self) -> None: plugin = SkillPlugin(skill_name="s", plugin_name="p", plugin_root=Path("/plug")) assert plugin.qualified_name == "p:s" + + +class TestResolvedSkillInvariants: + """Also exported, and ``name`` is interpolated unescaped into the + ```` tag the loader emits — so it guards itself for the + same reason :class:`SkillPlugin` does.""" + + def test_name_must_be_the_directory_basename(self) -> None: + with pytest.raises(SkillNotFoundError, match="must equal its directory"): + ResolvedSkill(name="other", directory=Path("/skills/acme"), source="./acme") + + def test_directory_must_be_absolute(self) -> None: + with pytest.raises(SkillNotFoundError, match="must be absolute"): + ResolvedSkill(name="acme", directory=Path("skills/acme"), source="./acme") + + def test_valid_construction_is_unaffected(self) -> None: + item = ResolvedSkill(name="acme", directory=Path("/skills/acme"), source="./acme") + assert item.name == "acme" + + +class TestSkillErrorHierarchy: + """Resolution and manifest failures originate in different modules but + reach the same handlers, so a call site that can trigger both needs one + correct thing to catch.""" + + @pytest.mark.parametrize("exc_type", [SkillNotFoundError, SkillPluginError, SkillManifestError]) + def test_every_skill_failure_shares_a_base(self, exc_type: type[Exception]) -> None: + assert issubclass(exc_type, SkillError) + # ValueError so these still nest inside Pydantic field validation. + assert issubclass(exc_type, ValueError) diff --git a/tests/test_skills/test_schema.py b/tests/test_skills/test_schema.py index 11184ecb..0f1dba1f 100644 --- a/tests/test_skills/test_schema.py +++ b/tests/test_skills/test_schema.py @@ -91,3 +91,43 @@ def test_unknown_skill_rejected(self) -> None: def test_empty_string_rejected(self) -> None: with pytest.raises(ValidationError, match="non-empty strings"): RuntimeConfig(skills=[""]) + + +class TestPathEntriesAtSchemaLevel: + """Path entries (issue #350) need the workflow file's directory to + resolve, which the schema does not have — so they are shape-checked + here and resolved in ``conductor validate`` / ``AgentExecutor``. + + Bare *names* keep their eager check, so an unknown built-in still fails + at load time exactly as it did before paths existed. + """ + + @pytest.mark.parametrize( + "entry", + ["./team-skills/acme", "../shared/acme", "~/scratch/skills", "/abs/acme", "team/acme"], + ) + def test_path_entries_are_accepted_unresolved(self, entry: str) -> None: + assert AgentDef(name="r", prompt="p", skills=[entry]).skills == [entry] + assert RuntimeConfig(skills=[entry]).skills == [entry] + + def test_unknown_bare_name_still_fails_at_load_time(self) -> None: + """The pre-existing error timing must not regress: a typo'd built-in + name needs no base directory to detect.""" + with pytest.raises(ValidationError, match="Unknown skill"): + AgentDef(name="r", prompt="p", skills=["conductorr"]) + + def test_unknown_name_error_mentions_the_path_form(self) -> None: + with pytest.raises(ValidationError, match=r"\./team-skills/my-skill"): + AgentDef(name="r", prompt="p", skills=["nope"]) + + def test_names_and_paths_can_be_mixed(self) -> None: + entries = ["conductor", "./team-skills/acme"] + assert AgentDef(name="r", prompt="p", skills=entries).skills == entries + + def test_whitespace_only_entry_still_rejected(self) -> None: + with pytest.raises(ValidationError, match="non-empty strings"): + AgentDef(name="r", prompt="p", skills=[" "]) + + def test_path_entries_still_forbidden_on_non_provider_steps(self) -> None: + with pytest.raises(ValidationError, match="cannot have 'skills'"): + AgentDef(name="s", type="script", command="echo hi", skills=["./a/b"])