diff --git a/.gitignore b/.gitignore index 6ed73f9..2075975 100644 --- a/.gitignore +++ b/.gitignore @@ -46,6 +46,10 @@ Thumbs.db *.local .claude/ +# Generated upskill output (per default skills_dir / runs_dir) +runs/ +skills/ + # Temporary files *.tmp *.temp diff --git a/README.md b/README.md index 2f34ed3..fc11792 100644 --- a/README.md +++ b/README.md @@ -617,3 +617,105 @@ upskill supports local models through any OpenAI-compatible endpoint (llama.cpp, # Configure endpoint via fast-agent config/env, then evaluate upskill eval ./skills/my-skill/ --model generic.my-model ``` + +## CLI Providers (Claude Code, Copilot CLI, Kiro CLI) + +upskill can drive evaluations through CLI tools you already pay for, instead of +sending API requests to Anthropic/OpenAI directly. This lets you reuse existing +subscriptions (Claude Code/Pro, GitHub Copilot, Kiro) without setting any +`ANTHROPIC_API_KEY` / `OPENAI_API_KEY` for evaluation. + +### Supported providers (v1) + +| Alias | Underlying CLI | Subscription | +|--------------------|-------------------------------|-------------------------| +| `cli.claude-code` | `claude` (Claude Code) | Claude Code / Pro | +| `cli.copilot` | `copilot` (GitHub Copilot CLI) | GitHub + Copilot | +| `cli.kiro` | `kiro-cli` | Kiro | + +### Quick start + +Make sure the CLI is installed and authenticated, then point `--model` at it: + +```bash +# Authenticate the CLIs once +claude login # Claude Code +gh auth login # GitHub Copilot CLI (with Copilot scope) +kiro-cli login # Kiro CLI + +# Evaluate a skill against your subscription-backed CLIs (no API keys needed!) +upskill eval ./skills/my-skill/ -m cli.claude-code + +# Mixed benchmark across CLI agents and a fast-agent model +upskill eval ./skills/my-skill/ -m cli.claude-code -m cli.copilot -m cli.kiro -m sonnet + +# Cross-model eval pass during generate (skill is generated with sonnet, +# evaluated with Claude Code via the CLI) +upskill generate "write good git commit messages" --model sonnet --eval-model cli.claude-code + +# Fully CLI-backed generation + evaluation (no API keys at all) +upskill generate "write good git commit messages" \ + --model cli.claude-code \ + --test-gen-model cli.claude-code +``` + +### Per-provider configuration + +Override CLI commands, extra args, timeouts, and env vars in `upskill.config.yaml`: + +```yaml +cli_providers: + claude-code: + command: claude + args: ["--model", "sonnet"] # Pass model selection to the CLI + timeout_seconds: 600 + # By default, claude-code unsets ANTHROPIC_API_KEY, ANTHROPIC_AUTH_TOKEN, + # CLAUDE_CODE_USE_BEDROCK, and CLAUDE_CODE_USE_VERTEX so the `claude` CLI + # uses your Pro subscription instead of falling back to API-key billing. + # Override unset_env: [] to keep these vars (e.g. for Bedrock/Vertex routing). + copilot: + command: copilot + kiro: + command: kiro-cli + args: ["--trust-all-tools"] # Opt in to unattended tool execution +``` + +Defaults are sane: with no config, `cli.claude-code` runs `claude`, `cli.copilot` +runs `copilot`, and `cli.kiro` runs `kiro-cli`. + +### Subscription billing + +upskill never wants to silently spend your paid API quota when you've +configured a CLI provider. By default each runner strips the env vars that +would otherwise route the CLI onto API-key billing. For Claude Code that means +`ANTHROPIC_API_KEY`, `ANTHROPIC_AUTH_TOKEN`, `CLAUDE_CODE_USE_BEDROCK`, and +`CLAUDE_CODE_USE_VERTEX` are removed from the subprocess environment. When any +of these were set, upskill prints a one-shot stderr line so you can see what +was stripped. + +If you actually want API-key billing for a CLI run (e.g. routing Claude Code +through your own Bedrock account), override the list: + +```yaml +cli_providers: + claude-code: + command: claude + unset_env: [] # keep ANTHROPIC_API_KEY etc. +``` + +### Caveats + +- **Token usage.** Claude Code reports input/output tokens via its JSON output; + Copilot and Kiro do not. When a CLI does not expose token counts, + `metadata.tokens_unavailable` is set on the result and tokens are recorded as + zero in `upskill runs` plots. +- **Authentication.** All three runners assume the binary is on `PATH` and + already signed in. If a runner cannot find the binary, you'll get an + actionable error pointing you at the right `login` command. +- **Structured outputs.** Skill and test generation through a CLI provider goes + through the CLI's plain-text response (instructed to emit JSON matching the + expected schema). Output is best-effort: if a CLI returns malformed JSON, + upskill surfaces it as a parsing error rather than crashing. +- **HF Jobs.** `--executor jobs --no-wait` is incompatible with `cli.*` models + (CLI providers always run locally). If you mix them, upskill rejects the + command with a clear error. diff --git a/src/upskill/cli.py b/src/upskill/cli.py index 9788ee8..9993411 100644 --- a/src/upskill/cli.py +++ b/src/upskill/cli.py @@ -7,6 +7,7 @@ import sys from collections.abc import Callable, Mapping from contextlib import asynccontextmanager +from dataclasses import dataclass from importlib import resources from pathlib import Path from typing import TYPE_CHECKING, Literal, Protocol, TypedDict, TypeVar, cast @@ -19,10 +20,12 @@ from rich.table import Table from rich.tree import Tree +from upskill.cli_agents import CliFastAgentAdapter from upskill.config import Config, resolve_upskill_config_path from upskill.evaluate import build_eval_requests, evaluate_skill, get_failure_descriptions from upskill.executors.local_fast_agent import LocalFastAgentExecutor from upskill.executors.remote_fast_agent import RemoteFastAgentExecutor +from upskill.executors.router import build_cli_executor, is_cli_model, select_executor_for_model from upskill.generate import generate_skill, generate_tests, improve_skill, refine_skill from upskill.hf_jobs import JobsConfig, verify_artifact_repo_access from upskill.logging import ( @@ -58,6 +61,7 @@ from fast_agent.agents.llm_agent import LlmAgent from fast_agent.interfaces import AgentProtocol + from upskill.cli_agents import SkillGenerator from upskill.executors.base import Executor load_dotenv() @@ -220,6 +224,97 @@ def _install_fast_agent_model_references( fast_config.model_references = merged_references +@dataclass(slots=True) +class _AgentSession: + """Session shape consumed by ``generate``/``eval`` orchestration helpers. + + Mirrors the relevant attributes of fast-agent's session so callers don't + need to care whether they're talking to a fast-agent agent or a + ``CliFastAgentAdapter`` for a given phase. + """ + + skill_gen: SkillGenerator + test_gen: SkillGenerator + + +@asynccontextmanager +async def _agent_session( + config: Config, + *, + skill_gen_model: str, + test_gen_model: str, + cards_source_dir: Path, + artifact_root: Path, + model_references: Mapping[str, Mapping[str, str]] | None = None, +) -> AsyncIterator[_AgentSession]: + """Yield a session whose skill_gen / test_gen agents fit the resolved models. + + - If both phases use ``cli.`` models, fast-agent is skipped + entirely (no API keys required). + - If both phases use fast-agent compatible models, behavior is identical + to the legacy ``_fast_agent_context`` path. + - Hybrid runs (one CLI, one fast-agent) load fast-agent for the non-CLI + phase and override the CLI phase with a ``CliFastAgentAdapter``. + """ + skill_gen_is_cli = is_cli_model(skill_gen_model) + test_gen_is_cli = is_cli_model(test_gen_model) + cli_executor = build_cli_executor(config) if (skill_gen_is_cli or test_gen_is_cli) else None + + def _build_adapter(model: str, agent_name: str) -> CliFastAgentAdapter: + assert cli_executor is not None # narrow for ty + return CliFastAgentAdapter( + executor=cli_executor, + model=model, + fastagent_config_path=config.effective_fastagent_config, + cards_source_dir=cards_source_dir, + artifact_root=artifact_root / agent_name, + agent_name=agent_name, + ) + + if skill_gen_is_cli and test_gen_is_cli: + # Pure CLI path: no fast-agent, no API keys. + yield _AgentSession( + skill_gen=_build_adapter(skill_gen_model, "skill_gen"), + test_gen=_build_adapter(test_gen_model, "test_gen"), + ) + return + + if not skill_gen_is_cli and not test_gen_is_cli: + async with _fast_agent_context(config, model_references=model_references) as fa_session: + yield _AgentSession( + skill_gen=fa_session.skill_gen, + test_gen=fa_session.test_gen, + ) + return + + # Hybrid: load fast-agent for the non-CLI side, override the CLI side. + sanitized_references = _strip_cli_model_references(model_references) + async with _fast_agent_context(config, model_references=sanitized_references) as fa_session: + skill_gen: object = ( + _build_adapter(skill_gen_model, "skill_gen") + if skill_gen_is_cli + else fa_session.skill_gen + ) + test_gen: object = ( + _build_adapter(test_gen_model, "test_gen") if test_gen_is_cli else fa_session.test_gen + ) + yield _AgentSession(skill_gen=skill_gen, test_gen=test_gen) + + +def _strip_cli_model_references( + references: Mapping[str, Mapping[str, str]] | None, +) -> Mapping[str, Mapping[str, str]] | None: + """Drop ``cli.*`` entries so fast-agent doesn't try to resolve them.""" + if not references: + return references + sanitized: dict[str, dict[str, str]] = {} + for namespace, entries in references.items(): + sanitized[namespace] = { + slot: model for slot, model in entries.items() if not is_cli_model(model) + } + return sanitized + + def _require_resolved_model(value: str | None, *, field: str, command: str) -> str: """Require a non-null resolved model value for a command.""" if value is None: @@ -263,11 +358,82 @@ def _build_executor( ) +ExecutorFactory = Callable[[str], "Executor"] + + +def _make_executor_factory( + *, + config: Config, + executor_name: ExecutorName, + jobs_config: JobsConfig | None, + jobs_progress_callback: Callable[[str], None] | None = None, +) -> ExecutorFactory: + """Return a per-model executor factory bound to the resolved config. + + The returned callable picks ``CliAgentExecutor`` for ``cli.`` + models and the configured fast-agent backend for everything else, mirroring + ``upskill.executors.router.select_executor_for_model`` semantics. + + The factory caches the fast-agent and CLI executor instances after first + use so multi-model benchmarks don't re-run jobs preflight checks per model. + """ + cached_fast_agent: list[Executor] = [] + cached_cli: list[Executor] = [] + + def _factory(model: str) -> Executor: + if is_cli_model(model): + if not cached_cli: + cached_cli.append(select_executor_for_model(model, config=config)) + return cached_cli[0] + if not cached_fast_agent: + cached_fast_agent.append( + _build_executor( + executor_name, + jobs_config=jobs_config, + progress_callback=jobs_progress_callback, + ) + ) + return cached_fast_agent[0] + + return _factory + + +def _reject_cli_models_with_jobs_no_wait(models: list[str]) -> None: + """Reject ``cli.*`` models in the `--executor jobs --no-wait` submission path.""" + cli_models = [model for model in models if is_cli_model(model)] + if not cli_models: + return + raise click.ClickException( + "`--executor jobs --no-wait` is incompatible with CLI agent models " + f"({', '.join(cli_models)}). CLI providers always run locally; either " + "remove `--no-wait` or omit the cli. models from this run." + ) + + def _resolve_executor_name(config: Config, cli_executor_name: ExecutorName | None) -> ExecutorName: """Resolve the effective execution backend from CLI override or config.""" return cli_executor_name or config.executor +def _maybe_downgrade_to_local( + executor_name: ExecutorName, + *, + models: list[str], +) -> ExecutorName: + """Downgrade ``jobs`` to ``local`` when every model in ``models`` is cli.*. + + CLI providers always run locally; forcing the user to supply jobs flags + (``--artifact-repo`` etc.) when their entire run is CLI-backed adds friction + without any benefit. The fast-agent ``local``/``jobs`` distinction only + matters once at least one phase routes through fast-agent. + """ + if executor_name != "jobs": + return executor_name + if models and all(is_cli_model(model) for model in models): + return "local" + return executor_name + + def _resolve_num_runs( config: Config, cli_num_runs: int | None, @@ -527,6 +693,8 @@ async def _load_test_cases( tests_path: str | None, test_gen_model: str, model_references: Mapping[str, Mapping[str, str]], + artifact_root: Path, + cards_source_dir: Path, ) -> tuple[list[TestCase], str]: """Load explicit, persisted, or generated test cases for a skill.""" if tests_path: @@ -537,7 +705,15 @@ async def _load_test_cases( if skill_record.state.tests: return skill_record.state.tests, "skill_meta.json" - async with _fast_agent_context(config, model_references=model_references) as agent: + # Use the dispatcher so cli. test-gen models work with no API keys. + async with _agent_session( + config, + skill_gen_model=test_gen_model, + test_gen_model=test_gen_model, + cards_source_dir=cards_source_dir, + artifact_root=artifact_root / "test_gen_session", + model_references=model_references, + ) as agent: console.print(f"Generating test cases from skill with {test_gen_model}...", style="dim") test_cases = await generate_tests( skill_record.skill.description, @@ -598,7 +774,7 @@ async def _create_generate_skill_record( examples: list[str] | None, from_skill: str | None, from_trace: str | None, - agent: FastAgentSession, + agent: _AgentSession, skill_gen_model: str, ) -> tuple[SkillRecord, str]: """Create or improve the skill record used by ``generate``.""" @@ -711,7 +887,7 @@ async def _run_generate_refinement_loop( skill_record: SkillRecord, task: str, test_cases: list[TestCase], - executor: Executor, + executor_factory: ExecutorFactory, config: Config, cards_path: Path, batch_id: str, @@ -719,13 +895,14 @@ async def _run_generate_refinement_loop( skill_gen_model: str, log_runs: bool, max_parallel: int, - agent: FastAgentSession, + agent: _AgentSession, ) -> tuple[SkillRecord, EvalResults | None, list[RunResult]]: """Run generate-time eval/refinement attempts on the main model.""" run_results: list[RunResult] = [] prev_success_rate = 0.0 results: EvalResults | None = None attempts = max(1, config.max_refine_attempts) + executor = executor_factory(skill_gen_model) for attempt in range(attempts): attempt_number = attempt + 1 @@ -802,7 +979,7 @@ async def _run_generate_extra_eval( skill_record: SkillRecord, task: str, test_cases: list[TestCase], - executor: Executor, + executor_factory: ExecutorFactory, config: Config, cards_path: Path, batch_id: str, @@ -814,6 +991,7 @@ async def _run_generate_extra_eval( ) -> tuple[EvalResults, list[RunResult]]: """Run the optional cross-model eval pass for ``generate``.""" console.print(f"Evaluating on {model}...", style="dim") + executor = executor_factory(model) results = await evaluate_skill( skill_record.skill, test_cases, @@ -858,7 +1036,7 @@ async def _run_with_skill_benchmark( evaluation_models: list[str], num_runs: int, test_cases: list[TestCase], - executor: Executor, + executor_factory: ExecutorFactory, config: Config, cards_path: Path, batch_id: str, @@ -874,6 +1052,7 @@ async def _run_with_skill_benchmark( for model in evaluation_models: console.print(f"[bold]{model}[/bold]") + executor = executor_factory(model) for run_num in range(1, num_runs + 1): run_folder = create_run_folder(batch_folder, len(all_run_results) + 1) @@ -1049,15 +1228,30 @@ def main(): @click.option( "-m", "--model", - help="Skill generation model for skill creation/refinement", + help=( + "Skill generation model for skill creation/refinement. Accepts both " + "fast-agent aliases (e.g. 'sonnet', 'haiku', 'openai.gpt-4.1', " + "'generic.') and CLI agents (cli.claude-code, cli.copilot, " + "cli.kiro)." + ), ) @click.option( "--test-gen-model", - help="Override test generation model for this run", + help=( + "Override test generation model for this run. Accepts both fast-agent " + "and cli. aliases." + ), ) @click.option("-o", "--output", type=click.Path(), help="Output directory for skill") @click.option("--no-eval", is_flag=True, help="Skip eval and refinement") -@click.option("--eval-model", help="Optional extra cross-model eval pass after generation") +@click.option( + "--eval-model", + help=( + "Optional extra cross-model eval pass after generation. Accepts both " + "fast-agent aliases and cli. aliases (cli.claude-code, " + "cli.copilot, cli.kiro)." + ), +) @_jobs_execution_options( executor_help="Execution backend for evaluation/refinement runs", runs_dir_help="Directory for run logs (default: ./runs)", @@ -1177,16 +1371,6 @@ async def _generate_async( executor_name = _resolve_executor_name(config, executor_name) max_parallel = _resolve_max_parallel(config, max_parallel) jobs_secrets = _resolve_jobs_secrets(config, jobs_secrets) - jobs_config = _require_jobs_config( - executor_name=executor_name, - artifact_repo=artifact_repo, - wait=wait, - jobs_timeout=jobs_timeout, - jobs_flavor=jobs_flavor, - jobs_secrets=jobs_secrets, - jobs_namespace=jobs_namespace, - jobs_image=config.jobs_image, - ) resolved = resolve_models( "generate", config=config, @@ -1205,6 +1389,22 @@ async def _generate_async( command="generate", ) extra_eval_model = resolved.extra_eval_model + + relevant_models = [skill_gen_model, test_gen_model] + if extra_eval_model: + relevant_models.append(extra_eval_model) + executor_name = _maybe_downgrade_to_local(executor_name, models=relevant_models) + jobs_config = _require_jobs_config( + executor_name=executor_name, + artifact_repo=artifact_repo, + wait=wait, + jobs_timeout=jobs_timeout, + jobs_flavor=jobs_flavor, + jobs_secrets=jobs_secrets, + jobs_namespace=jobs_namespace, + jobs_image=config.jobs_image, + ) + model_references = build_fastagent_model_references(config=config, resolved=resolved) _print_model_plan("generate", resolved) @@ -1217,9 +1417,16 @@ async def _generate_async( if log_runs: console.print(f"Logging runs to: {batch_folder}", style="dim") - async with _fast_agent_context(config, model_references=model_references) as agent: - cards = resources.files("upskill").joinpath("agent_cards") - with resources.as_file(cards) as cards_path: + cards = resources.files("upskill").joinpath("agent_cards") + with resources.as_file(cards) as cards_path: + async with _agent_session( + config, + skill_gen_model=skill_gen_model, + test_gen_model=test_gen_model, + cards_source_dir=cards_path, + artifact_root=batch_folder, + model_references=model_references, + ) as agent: skill_record, eval_task = await _create_generate_skill_record( task=task, examples=examples, @@ -1260,17 +1467,18 @@ async def _generate_async( _save_and_display(skill_record, output, config, artifact_path=batch_folder) return - executor = _build_executor( - executor_name, + executor_factory = _make_executor_factory( + config=config, + executor_name=executor_name, jobs_config=jobs_config, - progress_callback=_print_eval_progress, + jobs_progress_callback=_print_eval_progress, ) skill_record, results, run_results = await _run_generate_refinement_loop( skill_record=skill_record, task=eval_task, test_cases=test_cases, - executor=executor, + executor_factory=executor_factory, config=config, cards_path=cards_path, batch_id=batch_id, @@ -1288,7 +1496,7 @@ async def _generate_async( skill_record=skill_record, task=eval_task, test_cases=test_cases, - executor=executor, + executor_factory=executor_factory, config=config, cards_path=cards_path, batch_id=batch_id, @@ -1425,11 +1633,18 @@ def _save_and_display( "--model", "models", multiple=True, - help="Evaluation model(s) to run tests on (repeatable)", + help=( + "Evaluation model(s) to run tests on (repeatable). Accepts fast-agent " + "aliases (e.g. 'sonnet') and CLI agents (cli.claude-code, cli.copilot, " + "cli.kiro). Mix freely: `-m sonnet -m cli.claude-code` is valid." + ), ) @click.option( "--test-gen-model", - help="Override test generation model when tests must be generated", + help=( + "Override test generation model when tests must be generated. Accepts " + "both fast-agent aliases and cli. aliases." + ), ) @click.option( "--runs", @@ -1515,7 +1730,182 @@ def eval_cmd( ) -async def _eval_async( # noqa: C901 +async def _submit_eval_jobs_no_wait( + *, + skill: Skill, + test_cases: list[TestCase], + evaluation_models: list[str], + resolved: ResolvedModels, + jobs_config: JobsConfig, + config: Config, + cards_path: Path, + batch_folder: Path, + num_runs: int, +) -> None: + """Submit remote eval jobs without waiting (the `--executor jobs --no-wait` path).""" + if resolved.is_benchmark_mode: + submitted_job_refs: list[str] = [] + for model in evaluation_models: + console.print(f"[bold]{model}[/bold]") + for run_num in range(1, num_runs + 1): + job_refs = await _submit_remote_eval_jobs( + skill=skill, + test_cases=test_cases, + model=model, + jobs_config=jobs_config, + fastagent_config_path=config.effective_fastagent_config, + cards_path=cards_path, + artifact_root=batch_folder / "remote_downloads" / model / f"run_{run_num}", + run_baseline=False, + operation="benchmark", + ) + submitted_job_refs.extend(job_refs) + console.print(f"Remote fast-agent job id(s): {', '.join(job_refs)}") + console.print(f"Submitted remote fast-agent job id(s): {', '.join(submitted_job_refs)}") + return + + job_refs = await _submit_remote_eval_jobs( + skill=skill, + test_cases=test_cases, + model=evaluation_models[0], + jobs_config=jobs_config, + fastagent_config_path=config.effective_fastagent_config, + cards_path=cards_path, + artifact_root=batch_folder / "remote_downloads", + run_baseline=resolved.run_baseline, + operation="eval", + ) + console.print(f"Remote fast-agent job id(s): {', '.join(job_refs)}") + + +async def _run_simple_eval( + *, + skill: Skill, + test_cases: list[TestCase], + evaluation_model: str, + resolved: ResolvedModels, + executor_factory: ExecutorFactory, + config: Config, + cards_path: Path, + batch_id: str, + batch_folder: Path, + verbose: bool, + max_parallel: int, + log_runs: bool, +) -> None: + """Run the single-model, single-run eval flow with baseline + summary.""" + console.print(f"Running {len(test_cases)} test cases...", style="dim") + executor = executor_factory(evaluation_model) + + results = await evaluate_skill( + skill, + test_cases, + executor=executor, + model=evaluation_model, + fastagent_config_path=config.effective_fastagent_config, + cards_source_dir=cards_path, + artifact_root=batch_folder / "eval", + run_baseline=resolved.run_baseline, + max_parallel=max_parallel, + progress_callback=_print_eval_progress, + operation="eval", + ) + _raise_on_execution_errors(results, context=f"Evaluation on {evaluation_model}") + + run_results: list[RunResult] = [] + if log_runs: + run_results = _persist_comparison_run_results( + batch_folder=batch_folder, + model=evaluation_model, + task=skill.description, + batch_id=batch_id, + first_run_number=1, + results=results, + assertions_total=len(test_cases), + run_baseline=resolved.run_baseline, + with_skill_passed=( + results.is_beneficial + if resolved.run_baseline + else results.with_skill_success_rate > 0.5 + ), + skill_name=skill.name, + ) + summary = BatchSummary( + batch_id=batch_id, + model=evaluation_model, + task=skill.description, + total_runs=len(run_results), + passed_runs=sum(1 for r in run_results if r.passed), + results=run_results, + ) + write_batch_summary(batch_folder, summary) + + if verbose and resolved.run_baseline: + console.print() + for i, (with_r, base_r) in enumerate( + zip(results.with_skill_results, results.baseline_results, strict=True), + 1, + ): + base_icon = "[green]OK[/green]" if base_r.success else "[red]FAIL[/red]" + skill_icon = "[green]OK[/green]" if with_r.success else "[red]FAIL[/red]" + input_preview = with_r.test_case.input[:40] + console.print(f" {i}. {input_preview} {base_icon} base {skill_icon} skill") + console.print() + + _print_simple_eval_results(results, resolved=resolved, batch_folder=batch_folder) + + +def _print_simple_eval_results( + results: EvalResults, + *, + resolved: ResolvedModels, + batch_folder: Path, +) -> None: + """Render the simple-eval bar chart + recommendation.""" + console.print() + if resolved.run_baseline: + baseline_rate = results.baseline_success_rate + with_skill_rate = results.with_skill_success_rate + lift = results.skill_lift + + baseline_bar = _render_bar(baseline_rate) + with_skill_bar = _render_bar(with_skill_rate) + + lift_str = f"+{lift:.0%}" if lift >= 0 else f"{lift:.0%}" + lift_style = "green" if lift > 0 else "red" if lift < 0 else "dim" + + console.print(f" baseline {baseline_bar} {baseline_rate:>5.0%}") + console.print( + f" with skill {with_skill_bar} {with_skill_rate:>5.0%} " + f"[{lift_style}]({lift_str})[/{lift_style}]" + ) + + if results.baseline_total_tokens > 0: + savings = results.token_savings + savings_str = f"-{savings:.0%}" if savings >= 0 else f"+{-savings:.0%}" + savings_style = "green" if savings > 0 else "red" if savings < 0 else "dim" + console.print() + token_line = ( + f" tokens: {results.baseline_total_tokens} → " + f"{results.with_skill_total_tokens} " + f"[{savings_style}]({savings_str})[/{savings_style}]" + ) + console.print(token_line) + else: + with_skill_rate = results.with_skill_success_rate + with_skill_bar = _render_bar(with_skill_rate) + console.print(f" with skill {with_skill_bar} {with_skill_rate:>5.0%}") + console.print(f" tokens: {results.with_skill_total_tokens}") + + console.print(f"\nArtifacts saved to: {batch_folder}") + if resolved.run_baseline: + if results.is_beneficial: + console.print("\n[green]Recommendation: keep skill[/green]") + else: + console.print("\n[yellow]Recommendation: skill may not be beneficial[/yellow]") + + +async def _eval_async( skill_path: str, tests: str | None, models: list[str] | None, @@ -1540,23 +1930,7 @@ async def _eval_async( # noqa: C901 num_runs = _resolve_num_runs(config, num_runs, command="eval") max_parallel = _resolve_max_parallel(config, max_parallel) jobs_secrets = _resolve_jobs_secrets(config, jobs_secrets) - jobs_config = _require_jobs_config( - executor_name=executor_name, - artifact_repo=artifact_repo, - wait=wait, - jobs_timeout=jobs_timeout, - jobs_flavor=jobs_flavor, - jobs_secrets=jobs_secrets, - jobs_namespace=jobs_namespace, - jobs_image=config.jobs_image, - ) - executor = None - if executor_name == "local" or wait: - executor = _build_executor( - executor_name, - jobs_config=jobs_config, - progress_callback=_print_eval_progress, - ) + resolved = resolve_models( "eval", config=config, @@ -1575,6 +1949,31 @@ async def _eval_async( # noqa: C901 field="test_generation_model", command="eval", ) + # If every model in this run is cli.*, the jobs executor is irrelevant — + # downgrade silently so users with `executor: jobs` configs don't have to + # supply --artifact-repo for a CLI-only run. + executor_name = _maybe_downgrade_to_local( + executor_name, + models=[*evaluation_models, test_gen_model], + ) + jobs_config = _require_jobs_config( + executor_name=executor_name, + artifact_repo=artifact_repo, + wait=wait, + jobs_timeout=jobs_timeout, + jobs_flavor=jobs_flavor, + jobs_secrets=jobs_secrets, + jobs_namespace=jobs_namespace, + jobs_image=config.jobs_image, + ) + executor_factory = _make_executor_factory( + config=config, + executor_name=executor_name, + jobs_config=jobs_config, + jobs_progress_callback=_print_eval_progress, + ) + if executor_name == "jobs" and not wait: + _reject_cli_models_with_jobs_no_wait(evaluation_models) model_references = build_fastagent_model_references(config=config, resolved=resolved) _print_model_plan("eval", resolved, runs=num_runs) @@ -1592,80 +1991,49 @@ async def _eval_async( # noqa: C901 sys.exit(1) skill = skill_record.skill - test_cases, test_source = await _load_test_cases( - config=config, - skill_record=skill_record, - tests_path=tests, - test_gen_model=test_gen_model, - model_references=model_references, - ) - - invalid_expected = _count_invalid_expected_cases(test_cases) - console.print(f"[dim]Loaded {len(test_cases)} test case(s) from {test_source}[/dim]") - if invalid_expected: - console.print( - "[yellow]" - f"{invalid_expected} legacy expected-string test case(s) missing expected strings" - "[/yellow]" - ) - runs_path = Path(runs_dir) if runs_dir else config.runs_dir batch_id, batch_folder = create_batch_folder(runs_path) console.print(f"Artifacts saved to: {batch_folder}", style="dim") if log_runs: console.print(f"Logging to: {batch_folder}", style="dim") - if executor_name == "jobs" and not wait: - if jobs_config is None: - raise RuntimeError("Jobs config was not initialized.") - cards = resources.files("upskill").joinpath("agent_cards") - with resources.as_file(cards) as cards_path: - if resolved.is_benchmark_mode: - submitted_job_refs: list[str] = [] - - for model in evaluation_models: - console.print(f"[bold]{model}[/bold]") - for run_num in range(1, num_runs + 1): - job_refs = await _submit_remote_eval_jobs( - skill=skill, - test_cases=test_cases, - model=model, - jobs_config=jobs_config, - fastagent_config_path=config.effective_fastagent_config, - cards_path=cards_path, - artifact_root=batch_folder - / "remote_downloads" - / model - / f"run_{run_num}", - run_baseline=False, - operation="benchmark", - ) - submitted_job_refs.extend(job_refs) - console.print(f"Remote fast-agent job id(s): {', '.join(job_refs)}") - console.print( - f"Submitted remote fast-agent job id(s): {', '.join(submitted_job_refs)}" - ) - return + cards = resources.files("upskill").joinpath("agent_cards") + with resources.as_file(cards) as cards_path: + test_cases, test_source = await _load_test_cases( + config=config, + skill_record=skill_record, + tests_path=tests, + test_gen_model=test_gen_model, + model_references=model_references, + artifact_root=batch_folder, + cards_source_dir=cards_path, + ) - job_refs = await _submit_remote_eval_jobs( + invalid_expected = _count_invalid_expected_cases(test_cases) + console.print(f"[dim]Loaded {len(test_cases)} test case(s) from {test_source}[/dim]") + if invalid_expected: + console.print( + "[yellow]" + f"{invalid_expected} legacy expected-string test case(s) missing expected strings" + "[/yellow]" + ) + + if executor_name == "jobs" and not wait: + if jobs_config is None: + raise RuntimeError("Jobs config was not initialized.") + await _submit_eval_jobs_no_wait( skill=skill, test_cases=test_cases, - model=evaluation_models[0], + evaluation_models=evaluation_models, + resolved=resolved, jobs_config=jobs_config, - fastagent_config_path=config.effective_fastagent_config, + config=config, cards_path=cards_path, - artifact_root=batch_folder / "remote_downloads", - run_baseline=resolved.run_baseline, - operation="eval", + batch_folder=batch_folder, + num_runs=num_runs, ) - console.print(f"Remote fast-agent job id(s): {', '.join(job_refs)}") - return + return - if executor is None: - raise RuntimeError("Local executor was not initialized.") - - cards = resources.files("upskill").joinpath("agent_cards") - with resources.as_file(cards) as cards_path: if resolved.is_benchmark_mode: console.print( f"\nEvaluating [bold]{skill.name}[/bold] across {len(evaluation_models)} model(s)" @@ -1676,7 +2044,7 @@ async def _eval_async( # noqa: C901 evaluation_models=evaluation_models, num_runs=num_runs, test_cases=test_cases, - executor=executor, + executor_factory=executor_factory, config=config, cards_path=cards_path, batch_id=batch_id, @@ -1694,113 +2062,23 @@ async def _eval_async( # noqa: C901 task=skill.description, all_run_results=all_run_results, ) + return - else: - # Simple eval mode: single model, single run - model = evaluation_models[0] - console.print(f"Running {len(test_cases)} test cases...", style="dim") - - results = await evaluate_skill( - skill, - test_cases, - executor=executor, - model=model, - fastagent_config_path=config.effective_fastagent_config, - cards_source_dir=cards_path, - artifact_root=batch_folder / "eval", - run_baseline=resolved.run_baseline, - max_parallel=max_parallel, - progress_callback=_print_eval_progress, - operation="eval", - ) - _raise_on_execution_errors(results, context=f"Evaluation on {model}") - - # Log results (both baseline and with-skill) - run_results: list[RunResult] = [] - if log_runs: - run_results = _persist_comparison_run_results( - batch_folder=batch_folder, - model=model, - task=skill.description, - batch_id=batch_id, - first_run_number=1, - results=results, - assertions_total=len(test_cases), - run_baseline=resolved.run_baseline, - with_skill_passed=( - results.is_beneficial - if resolved.run_baseline - else results.with_skill_success_rate > 0.5 - ), - skill_name=skill.name, - ) - - # Write batch summary - summary = BatchSummary( - batch_id=batch_id, - model=model, - task=skill.description, - total_runs=len(run_results), - passed_runs=sum(1 for r in run_results if r.passed), - results=run_results, - ) - write_batch_summary(batch_folder, summary) - - if verbose and resolved.run_baseline: - console.print() - for i, (with_r, base_r) in enumerate( - zip(results.with_skill_results, results.baseline_results, strict=True), - 1, - ): - base_icon = "[green]OK[/green]" if base_r.success else "[red]FAIL[/red]" - skill_icon = "[green]OK[/green]" if with_r.success else "[red]FAIL[/red]" - input_preview = with_r.test_case.input[:40] - console.print(f" {i}. {input_preview} {base_icon} base {skill_icon} skill") - console.print() - - # Display results with horizontal bars - console.print() - if resolved.run_baseline: - baseline_rate = results.baseline_success_rate - with_skill_rate = results.with_skill_success_rate - lift = results.skill_lift - - baseline_bar = _render_bar(baseline_rate) - with_skill_bar = _render_bar(with_skill_rate) - - lift_str = f"+{lift:.0%}" if lift >= 0 else f"{lift:.0%}" - lift_style = "green" if lift > 0 else "red" if lift < 0 else "dim" - - console.print(f" baseline {baseline_bar} {baseline_rate:>5.0%}") - console.print( - f" with skill {with_skill_bar} {with_skill_rate:>5.0%} " - f"[{lift_style}]({lift_str})[/{lift_style}]" - ) - - # Token comparison - if results.baseline_total_tokens > 0: - savings = results.token_savings - savings_str = f"-{savings:.0%}" if savings >= 0 else f"+{-savings:.0%}" - savings_style = "green" if savings > 0 else "red" if savings < 0 else "dim" - console.print() - token_line = ( - f" tokens: {results.baseline_total_tokens} → " - f"{results.with_skill_total_tokens} " - f"[{savings_style}]({savings_str})[/{savings_style}]" - ) - console.print(token_line) - else: - with_skill_rate = results.with_skill_success_rate - with_skill_bar = _render_bar(with_skill_rate) - console.print(f" with skill {with_skill_bar} {with_skill_rate:>5.0%}") - console.print(f" tokens: {results.with_skill_total_tokens}") - - console.print(f"\nArtifacts saved to: {batch_folder}") - if resolved.run_baseline: - if results.is_beneficial: - console.print("\n[green]Recommendation: keep skill[/green]") - else: - console.print("\n[yellow]Recommendation: skill may not be beneficial[/yellow]") + await _run_simple_eval( + skill=skill, + test_cases=test_cases, + evaluation_model=evaluation_models[0], + resolved=resolved, + executor_factory=executor_factory, + config=config, + cards_path=cards_path, + batch_id=batch_id, + batch_folder=batch_folder, + verbose=verbose, + max_parallel=max_parallel, + log_runs=log_runs, + ) + return @main.command("list") @@ -1886,7 +2164,11 @@ def list_cmd(skills_dir: str | None, verbose: bool): "models", multiple=True, required=True, - help="Evaluation model(s) to benchmark (repeatable)", + help=( + "Evaluation model(s) to benchmark (repeatable). Accepts fast-agent " + "aliases (e.g. 'sonnet') and CLI agents (cli.claude-code, cli.copilot, " + "cli.kiro)." + ), ) @click.option( "--runs", @@ -2019,26 +2301,7 @@ async def _benchmark_async( num_runs = _resolve_num_runs(config, num_runs, command="benchmark") max_parallel = _resolve_max_parallel(config, max_parallel) jobs_secrets = _resolve_jobs_secrets(config, jobs_secrets) - jobs_config = _require_jobs_config( - executor_name=executor_name, - artifact_repo=artifact_repo, - wait=wait, - jobs_timeout=jobs_timeout, - jobs_flavor=jobs_flavor, - jobs_secrets=jobs_secrets, - jobs_namespace=jobs_namespace, - jobs_image=config.jobs_image, - ) - if executor_name == "jobs" and not wait: - raise click.ClickException( - "`benchmark --executor jobs` currently requires `--wait` to assemble results from " - "downloaded fast-agent artifacts." - ) - executor = _build_executor( - executor_name, - jobs_config=jobs_config, - progress_callback=_print_eval_progress, - ) + resolved = resolve_models( "benchmark", config=config, @@ -2056,6 +2319,31 @@ async def _benchmark_async( field="test_generation_model", command="benchmark", ) + executor_name = _maybe_downgrade_to_local( + executor_name, + models=[*evaluation_models, test_gen_model], + ) + jobs_config = _require_jobs_config( + executor_name=executor_name, + artifact_repo=artifact_repo, + wait=wait, + jobs_timeout=jobs_timeout, + jobs_flavor=jobs_flavor, + jobs_secrets=jobs_secrets, + jobs_namespace=jobs_namespace, + jobs_image=config.jobs_image, + ) + if executor_name == "jobs" and not wait: + raise click.ClickException( + "`benchmark --executor jobs` currently requires `--wait` to assemble results from " + "downloaded fast-agent artifacts." + ) + executor_factory = _make_executor_factory( + config=config, + executor_name=executor_name, + jobs_config=jobs_config, + jobs_progress_callback=_print_eval_progress, + ) model_references = build_fastagent_model_references(config=config, resolved=resolved) _print_model_plan("benchmark", resolved, runs=num_runs) @@ -2063,6 +2351,10 @@ async def _benchmark_async( skill_record = SkillRecord.load(Path(skill_path)) skill = skill_record.skill + out_path = Path(output_dir) if output_dir else config.runs_dir + batch_id, batch_folder = create_batch_folder(out_path) + console.print(f"Results will be saved to: {batch_folder}", style="dim") + cards = resources.files("upskill").joinpath("agent_cards") with resources.as_file(cards) as cards_path: test_cases, _ = await _load_test_cases( @@ -2071,14 +2363,10 @@ async def _benchmark_async( tests_path=tests_path, test_gen_model=test_gen_model, model_references=model_references, + artifact_root=batch_folder, + cards_source_dir=cards_path, ) - # Setup output directory - out_path = Path(output_dir) if output_dir else config.runs_dir - - batch_id, batch_folder = create_batch_folder(out_path) - console.print(f"Results will be saved to: {batch_folder}", style="dim") - console.print( f"\nBenchmarking [bold]{skill.name}[/bold] across {len(evaluation_models)} model(s)" ) @@ -2088,7 +2376,7 @@ async def _benchmark_async( evaluation_models=evaluation_models, num_runs=num_runs, test_cases=test_cases, - executor=executor, + executor_factory=executor_factory, config=config, cards_path=cards_path, batch_id=batch_id, diff --git a/src/upskill/cli_agents/__init__.py b/src/upskill/cli_agents/__init__.py new file mode 100644 index 0000000..2efc8f4 --- /dev/null +++ b/src/upskill/cli_agents/__init__.py @@ -0,0 +1,32 @@ +"""CLI-backed agent runners (Claude Code, GitHub Copilot CLI, Kiro CLI). + +These runners shell out to user-installed CLI binaries that are themselves agents, +bypassing fast-agent. They let users run upskill against their existing +subscription-backed CLIs instead of raw Anthropic/OpenAI API keys. +""" + +from upskill.cli_agents.base import ( + CLI_PROVIDER_NAMES, + CliAgentError, + CliAgentRunner, + CliProcessOutcome, + CliProviderConfig, + CliRunResult, + default_cli_providers, + run_cli_subprocess, +) +from upskill.cli_agents.fast_agent_adapter import CliFastAgentAdapter +from upskill.cli_agents.protocols import SkillGenerator + +__all__ = [ + "CLI_PROVIDER_NAMES", + "CliAgentError", + "CliAgentRunner", + "CliFastAgentAdapter", + "CliProcessOutcome", + "CliProviderConfig", + "CliRunResult", + "SkillGenerator", + "default_cli_providers", + "run_cli_subprocess", +] diff --git a/src/upskill/cli_agents/base.py b/src/upskill/cli_agents/base.py new file mode 100644 index 0000000..739a467 --- /dev/null +++ b/src/upskill/cli_agents/base.py @@ -0,0 +1,383 @@ +"""Contract types shared by all CLI-backed agent runners.""" + +from __future__ import annotations + +import asyncio +import json +import os +import sys +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, Protocol, runtime_checkable + +from pydantic import BaseModel, ConfigDict, Field + +from upskill.models import ConversationStats + +if TYPE_CHECKING: + from collections.abc import Mapping, Sequence + from pathlib import Path + +# Stable identifiers for the v1 CLI providers. The CLI surface and router both +# rely on this set to resolve `cli.` model strings. +CLI_PROVIDER_NAMES = ("claude-code", "copilot", "kiro") + +CliMetadataValue = str | int | float | bool | None + + +class CliAgentError(RuntimeError): + """Raised when a CLI runner cannot be invoked at all (missing/unauth binary).""" + + +@dataclass(slots=True, frozen=True) +class CliRunResult: + """Result of invoking a CLI agent for a single prompt.""" + + output_text: str | None + stats: ConversationStats = field(default_factory=ConversationStats) + return_code: int = 0 + stdout: str = "" + stderr: str = "" + error: str | None = None + metadata: dict[str, CliMetadataValue] = field(default_factory=dict) + + +class CliProviderConfig(BaseModel): + """Per-provider configuration loaded from ``upskill.config.yaml``.""" + + model_config = ConfigDict(extra="forbid") + + command: str = Field( + ..., + min_length=1, + description="Executable to invoke (e.g. 'claude', 'copilot', 'kiro-cli').", + ) + args: list[str] = Field( + default_factory=list, + description="Extra arguments appended to the runner-built argv.", + ) + timeout_seconds: int | None = Field( + default=600, + ge=1, + description="Per-invocation timeout in seconds (None disables the timeout).", + ) + env: dict[str, str] = Field( + default_factory=dict, + description="Extra environment variables merged into the subprocess environment.", + ) + unset_env: list[str] = Field( + default_factory=list, + description=( + "Environment variables to remove from the subprocess environment before " + "launching the CLI. Used to force CLIs onto subscription auth instead " + "of API-key auth (e.g. ANTHROPIC_API_KEY for `claude`)." + ), + ) + + +@runtime_checkable +class CliAgentRunner(Protocol): + """Strategy interface implemented by each CLI-specific runner.""" + + name: str + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + """Invoke the underlying CLI and return a structured result.""" + + +# Env vars that, if present, route the `claude` CLI to per-token API billing +# instead of the user's Claude Code / Pro subscription. Stripping them by +# default means upskill never silently spends a paid API quota when a user +# explicitly configured cli.claude-code. +_CLAUDE_API_FALLBACK_ENV = ( + "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "CLAUDE_CODE_USE_BEDROCK", + "CLAUDE_CODE_USE_VERTEX", +) + + +def default_cli_providers() -> dict[str, CliProviderConfig]: + """Return the v1 default ``cli_providers`` map. + + Defaults are conservative so users can run ``--model cli.claude-code`` (and + siblings) with zero config, assuming the binary is on ``PATH`` and already + authenticated. + + Each provider's ``unset_env`` defaults to the env vars known to override + the CLI's subscription-based auth, so subscription billing is preserved + without the user having to think about it. + """ + return { + "claude-code": CliProviderConfig( + command="claude", + unset_env=list(_CLAUDE_API_FALLBACK_ENV), + ), + "copilot": CliProviderConfig(command="copilot"), + "kiro": CliProviderConfig(command="kiro-cli"), + } + + +@dataclass(slots=True, frozen=True) +class CliProcessOutcome: + """Captured stdout/stderr and exit code from a CLI subprocess.""" + + return_code: int + stdout: str + stderr: str + timed_out: bool = False + + +def _resolve_subprocess_env( + extra_env: Mapping[str, str] | None, + unset_env: Sequence[str] | None = None, +) -> dict[str, str]: + """Merge optional provider env additions on top of ``os.environ``. + + Variables listed in ``unset_env`` are removed from the resulting + environment regardless of whether they came from ``os.environ`` or + ``extra_env``. This is what keeps subscription-backed CLI runners from + silently falling back to API-key billing. + """ + merged = dict(os.environ) + if extra_env: + merged.update(extra_env) + if unset_env: + for name in unset_env: + merged.pop(name, None) + return merged + + +_warned_stripped_env: set[tuple[str, str]] = set() + + +def _warn_about_stripped_env( + *, + runner_label: str, + unset_env: Sequence[str] | None, +) -> None: + """Emit a one-shot stderr note when a runner is stripping a set env var. + + Helps users discover that upskill is preserving subscription billing on + their behalf (otherwise the disappearance is invisible). + """ + if not unset_env: + return + for name in unset_env: + if name not in os.environ: + continue + signature = (runner_label, name) + if signature in _warned_stripped_env: + continue + _warned_stripped_env.add(signature) + sys.stderr.write( + f"upskill: {runner_label}: unsetting {name} for this subprocess so the " + f"CLI uses your subscription instead of API-key billing. " + f"Override via cli_providers..unset_env in upskill.config.yaml.\n" + ) + + +async def run_cli_subprocess( + *, + argv: Sequence[str], + cwd: Path, + env: Mapping[str, str] | None = None, + unset_env: Sequence[str] | None = None, + timeout_seconds: float | None = None, + stdin_input: str | None = None, + artifact_dir: Path | None = None, + runner_label: str | None = None, +) -> CliProcessOutcome: + """Run a CLI subprocess, capture stdout/stderr, and persist debug artifacts. + + Raises ``CliAgentError`` when the binary cannot be found or executed. + """ + if not argv: + raise CliAgentError("Cannot run CLI subprocess with empty argv.") + + if runner_label is not None: + _warn_about_stripped_env(runner_label=runner_label, unset_env=unset_env) + + if artifact_dir is not None: + artifact_dir.mkdir(parents=True, exist_ok=True) + (artifact_dir / "command.json").write_text( + json.dumps( + { + "argv": list(argv), + "cwd": str(cwd), + "unset_env": list(unset_env or []), + }, + indent=2, + ), + encoding="utf-8", + ) + + try: + process = await asyncio.create_subprocess_exec( + *argv, + cwd=str(cwd), + stdin=asyncio.subprocess.PIPE + if stdin_input is not None + else asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=_resolve_subprocess_env(env, unset_env=unset_env), + ) + except FileNotFoundError as exc: + raise CliAgentError( + f"CLI binary {argv[0]!r} was not found on PATH. " + f"Install it or override the `command` in `cli_providers` config." + ) from exc + + stdin_bytes = stdin_input.encode("utf-8") if stdin_input is not None else None + timed_out = False + try: + stdout_bytes, stderr_bytes = await asyncio.wait_for( + process.communicate(input=stdin_bytes), + timeout=timeout_seconds, + ) + except TimeoutError: + timed_out = True + process.kill() + stdout_bytes, stderr_bytes = await process.communicate() + + stdout_text = stdout_bytes.decode("utf-8", errors="replace") if stdout_bytes else "" + stderr_text = stderr_bytes.decode("utf-8", errors="replace") if stderr_bytes else "" + return_code = process.returncode if process.returncode is not None else -1 + + if artifact_dir is not None: + (artifact_dir / "stdout.txt").write_text(stdout_text, encoding="utf-8") + (artifact_dir / "stderr.txt").write_text(stderr_text, encoding="utf-8") + + return CliProcessOutcome( + return_code=return_code, + stdout=stdout_text, + stderr=stderr_text, + timed_out=timed_out, + ) + + +def _resolve_effective_timeout( + *, + provider: CliProviderConfig, + explicit_timeout: float | None, +) -> float | None: + """Return the effective subprocess timeout, preferring explicit > provider config.""" + if explicit_timeout is not None: + return explicit_timeout + if provider.timeout_seconds is None: + return None + return float(provider.timeout_seconds) + + +def _build_text_metadata( + *, + runner_name: str, + argv: Sequence[str], + timed_out: bool, +) -> dict[str, CliMetadataValue]: + return { + "runner": runner_name, + "argv": " ".join(argv), + "timed_out": timed_out, + "tokens_unavailable": True, + } + + +async def execute_text_cli( + *, + runner_name: str, + display_name: str, + argv: Sequence[str], + provider: CliProviderConfig, + workspace_dir: Path, + artifact_dir: Path, + timeout: float | None, + auth_hint: str, +) -> CliRunResult: + """Run a CLI that emits a plain-text reply, with no usage/token reporting. + + Used by runners that drive CLIs (Copilot, Kiro) which do not currently + expose structured tokens or JSON envelopes. Tokens are recorded as zero + and ``metadata['tokens_unavailable']`` is set so the rest of upskill can + flag the gap when plotting/aggregating. + """ + effective_timeout = _resolve_effective_timeout( + provider=provider, + explicit_timeout=timeout, + ) + + try: + outcome = await run_cli_subprocess( + argv=argv, + cwd=workspace_dir, + env=provider.env or None, + unset_env=provider.unset_env or None, + timeout_seconds=effective_timeout, + artifact_dir=artifact_dir, + runner_label=runner_name, + ) + except CliAgentError as exc: + return CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=-1, + stdout="", + stderr="", + error=f"{exc} {auth_hint}", + metadata=_build_text_metadata( + runner_name=runner_name, + argv=argv, + timed_out=False, + ), + ) + + metadata = _build_text_metadata( + runner_name=runner_name, + argv=argv, + timed_out=outcome.timed_out, + ) + + if outcome.timed_out: + return CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=outcome.return_code, + stdout=outcome.stdout, + stderr=outcome.stderr, + error=f"{display_name} timed out before producing a response.", + metadata=metadata, + ) + + if outcome.return_code != 0: + error_detail = outcome.stderr.strip() or outcome.stdout.strip() or "no output" + error = ( + f"{display_name} exited with code {outcome.return_code}: {error_detail}. {auth_hint}" + ) + return CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=outcome.return_code, + stdout=outcome.stdout, + stderr=outcome.stderr, + error=error, + metadata=metadata, + ) + + output_text = outcome.stdout.strip() or None + return CliRunResult( + output_text=output_text, + stats=ConversationStats(), + return_code=outcome.return_code, + stdout=outcome.stdout, + stderr=outcome.stderr, + error=None if output_text else f"{display_name} produced no stdout.", + metadata=metadata, + ) diff --git a/src/upskill/cli_agents/claude_code.py b/src/upskill/cli_agents/claude_code.py new file mode 100644 index 0000000..bd4d067 --- /dev/null +++ b/src/upskill/cli_agents/claude_code.py @@ -0,0 +1,173 @@ +"""Runner that drives the Claude Code CLI (``claude``) in non-interactive mode.""" + +from __future__ import annotations + +import json +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from upskill.cli_agents.base import ( + CliAgentError, + CliMetadataValue, + CliProviderConfig, + CliRunResult, + run_cli_subprocess, +) +from upskill.models import ConversationStats + +if TYPE_CHECKING: + from pathlib import Path + + +_AUTH_HINT = ( + "Verify Claude Code is installed and authenticated: run `claude login`, " + "then retry. See README -> CLI providers for details." +) + + +def _coerce_int(value: object) -> int: + """Best-effort int coercion that tolerates float/string usage values.""" + if isinstance(value, bool): # bool is an int subclass; reject explicitly. + return 0 + if isinstance(value, int): + return value + if isinstance(value, float): + return int(value) + if isinstance(value, str): + try: + return int(value) + except ValueError: + return 0 + return 0 + + +def _parse_claude_envelope(stdout: str) -> tuple[str | None, ConversationStats, dict[str, object]]: + """Parse Claude Code's ``--output-format json`` payload.""" + try: + envelope = json.loads(stdout) + except json.JSONDecodeError as exc: + raise CliAgentError(f"Claude CLI did not return valid JSON: {exc}") from exc + + if not isinstance(envelope, dict): + raise CliAgentError("Claude CLI JSON envelope must be an object.") + + output_text = envelope.get("result") + if output_text is not None and not isinstance(output_text, str): + raise CliAgentError("Claude CLI 'result' field must be a string when present.") + + usage_raw = envelope.get("usage") + stats = ConversationStats() + if isinstance(usage_raw, dict): + input_tokens = _coerce_int(usage_raw.get("input_tokens", 0)) + output_tokens = _coerce_int(usage_raw.get("output_tokens", 0)) + stats = ConversationStats( + input_tokens=input_tokens, + output_tokens=output_tokens, + total_tokens=input_tokens + output_tokens, + ) + + return output_text, stats, envelope + + +def _build_claude_argv( + *, + provider: CliProviderConfig, + prompt: str, +) -> list[str]: + """Build the canonical Claude Code CLI argv for a single prompt.""" + return [ + provider.command, + "-p", + prompt, + "--output-format", + "json", + *provider.args, + ] + + +@dataclass(slots=True) +class ClaudeCodeRunner: + """Invoke Claude Code (``claude``) for a single prompt and capture its reply.""" + + name: str = "claude-code" + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + argv = _build_claude_argv(provider=provider, prompt=prompt) + effective_timeout = timeout + if effective_timeout is None and provider.timeout_seconds is not None: + effective_timeout = float(provider.timeout_seconds) + + try: + outcome = await run_cli_subprocess( + argv=argv, + cwd=workspace_dir, + env=provider.env or None, + unset_env=provider.unset_env or None, + timeout_seconds=effective_timeout, + artifact_dir=artifact_dir, + runner_label=self.name, + ) + except CliAgentError as exc: + return CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=-1, + stdout="", + stderr="", + error=f"{exc} {_AUTH_HINT}", + metadata={"runner": self.name, "argv": " ".join(argv)}, + ) + + metadata: dict[str, CliMetadataValue] = { + "runner": self.name, + "argv": " ".join(argv), + "timed_out": outcome.timed_out, + } + + if outcome.timed_out: + return CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=outcome.return_code, + stdout=outcome.stdout, + stderr=outcome.stderr, + error="Claude CLI timed out before producing a response.", + metadata=metadata, + ) + + output_text: str | None = None + stats = ConversationStats() + envelope_error: str | None = None + try: + output_text, stats, envelope = _parse_claude_envelope(outcome.stdout) + metadata["envelope_keys"] = ",".join(sorted(envelope.keys())) + except CliAgentError as exc: + envelope_error = str(exc) + + if outcome.return_code != 0: + error_detail = outcome.stderr.strip() or outcome.stdout.strip() or "no output" + error = ( + f"Claude CLI exited with code {outcome.return_code}: {error_detail}. {_AUTH_HINT}" + ) + elif envelope_error: + error = envelope_error + else: + error = None + + return CliRunResult( + output_text=output_text, + stats=stats, + return_code=outcome.return_code, + stdout=outcome.stdout, + stderr=outcome.stderr, + error=error, + metadata=metadata, + ) diff --git a/src/upskill/cli_agents/copilot.py b/src/upskill/cli_agents/copilot.py new file mode 100644 index 0000000..a25c405 --- /dev/null +++ b/src/upskill/cli_agents/copilot.py @@ -0,0 +1,54 @@ +"""Runner that drives the GitHub Copilot CLI (``copilot``) in non-interactive mode.""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from upskill.cli_agents.base import ( + CliProviderConfig, + CliRunResult, + execute_text_cli, +) + +if TYPE_CHECKING: + from pathlib import Path + + +_AUTH_HINT = ( + "Verify the GitHub Copilot CLI is installed and signed in: run " + "`gh auth login` (with Copilot scope) and `copilot --version`, then retry. " + "See README -> CLI providers for details." +) + + +def _build_copilot_argv(*, provider: CliProviderConfig, prompt: str) -> list[str]: + """Build the canonical Copilot CLI argv for a single non-interactive prompt.""" + return [provider.command, "-p", prompt, *provider.args] + + +@dataclass(slots=True) +class CopilotCliRunner: + """Invoke ``copilot`` for a single prompt and capture the textual reply.""" + + name: str = "copilot" + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + return await execute_text_cli( + runner_name=self.name, + display_name="Copilot CLI", + argv=_build_copilot_argv(provider=provider, prompt=prompt), + provider=provider, + workspace_dir=workspace_dir, + artifact_dir=artifact_dir, + timeout=timeout, + auth_hint=_AUTH_HINT, + ) diff --git a/src/upskill/cli_agents/fast_agent_adapter.py b/src/upskill/cli_agents/fast_agent_adapter.py new file mode 100644 index 0000000..6c82061 --- /dev/null +++ b/src/upskill/cli_agents/fast_agent_adapter.py @@ -0,0 +1,136 @@ +"""Adapter that lets a ``CliAgentExecutor`` stand in for a fast-agent agent. + +Implements the narrow surface upskill's ``generate`` / ``test`` flows actually +use (``send`` for free-form text, ``structured`` for JSON-schema-shaped output). +This is what makes ``upskill generate "..." --model cli.claude-code`` work +end-to-end without an Anthropic/OpenAI API key. +""" + +from __future__ import annotations + +import json +import re +from dataclasses import dataclass, field +from typing import TYPE_CHECKING, TypeVar + +from pydantic import BaseModel, ValidationError + +from upskill.executors.contracts import ExecutionRequest, ExecutionResult + +if TYPE_CHECKING: + from pathlib import Path + + from upskill.executors.cli_agent import CliAgentExecutor + +ModelT = TypeVar("ModelT", bound=BaseModel) + + +# Strip a single leading and trailing markdown code fence (```json ... ```). +_CODE_FENCE_OPEN = re.compile(r"^\s*```(?:json|JSON)?\s*\n?") +_CODE_FENCE_CLOSE = re.compile(r"\n?\s*```\s*$") + + +def _strip_code_fence(text: str) -> str: + """Remove a single wrapping markdown code fence from JSON-ish output.""" + stripped = _CODE_FENCE_OPEN.sub("", text) + stripped = _CODE_FENCE_CLOSE.sub("", stripped) + return stripped.strip() + + +def _extract_json_payload(text: str) -> str | None: + """Best-effort extraction of the first JSON object/array from free-form text.""" + cleaned = _strip_code_fence(text) + if not cleaned: + return None + if cleaned[0] in "{[": + return cleaned + for opener, closer in (("{", "}"), ("[", "]")): + start = cleaned.find(opener) + end = cleaned.rfind(closer) + if start != -1 and end != -1 and end > start: + return cleaned[start : end + 1] + return None + + +def _build_structured_prompt(message: str, schema: object) -> str: + """Append a JSON-schema instruction so CLIs know to emit pure JSON.""" + schema_text = json.dumps(schema, indent=2) + return ( + f"{message}\n\n" + "## Output format\n\n" + "Respond with ONLY a single JSON value matching this JSON schema. " + "Do not wrap the response in markdown code fences. Do not include any " + "explanatory prose before or after the JSON.\n\n" + f"```\n{schema_text}\n```" + ) + + +@dataclass(slots=True) +class CliFastAgentAdapter: + """Drive a ``CliAgentExecutor`` as if it were a fast-agent generator agent. + + Only the methods upskill's generation flows touch (``send`` and + ``structured``) are implemented. Each call goes through the executor as a + one-shot ``ExecutionRequest`` (no skill bundle, no workspace files), so + artifacts land alongside the rest of the run for traceability. + """ + + executor: CliAgentExecutor + model: str + fastagent_config_path: Path + cards_source_dir: Path + artifact_root: Path + agent_name: str = "skill_gen" + _call_index: list[int] = field(default_factory=lambda: [0]) + + async def send( + self, + message: str, + request_params: object | None = None, + ) -> str: + """Run a free-form prompt; return the assistant's text reply.""" + del request_params + result = await self._invoke(message) + return result.output_text or "" + + async def structured( + self, + message: str, + model: type[ModelT], + request_params: object | None = None, + ) -> tuple[ModelT | None, str]: + """Ask the CLI for a JSON-schema-shaped reply and validate it.""" + del request_params + prompt = _build_structured_prompt(message, model.model_json_schema()) + result = await self._invoke(prompt) + raw = result.output_text or "" + candidate = _extract_json_payload(raw) + if candidate is None: + return None, raw + try: + payload = json.loads(candidate) + except json.JSONDecodeError: + return None, raw + try: + return model.model_validate(payload), raw + except ValidationError: + return None, raw + + async def _invoke(self, prompt: str) -> ExecutionResult: + """Execute one CLI call against ``self.executor`` and return its result.""" + self._call_index[0] += 1 + index = self._call_index[0] + request = ExecutionRequest( + prompt=prompt, + model=self.model, + agent=self.agent_name, + fastagent_config_path=self.fastagent_config_path, + artifact_dir=self.artifact_root / f"{self.agent_name}_call_{index:03d}", + cards_source_dir=self.cards_source_dir, + label=f"{self.agent_name}-{index}", + skill=None, + workspace_files={}, + metadata={"phase": self.agent_name, "call_index": index}, + ) + handle = await self.executor.execute(request) + return await self.executor.collect(handle) diff --git a/src/upskill/cli_agents/kiro.py b/src/upskill/cli_agents/kiro.py new file mode 100644 index 0000000..31baac6 --- /dev/null +++ b/src/upskill/cli_agents/kiro.py @@ -0,0 +1,65 @@ +"""Runner that drives the Kiro CLI (``kiro-cli``) in non-interactive mode.""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +from upskill.cli_agents.base import ( + CliProviderConfig, + CliRunResult, + execute_text_cli, +) + +if TYPE_CHECKING: + from pathlib import Path + + +_AUTH_HINT = ( + "Verify the Kiro CLI is installed and signed in: run `kiro-cli login` " + "(or follow the Kiro CLI auth flow) and `kiro-cli --version`, then retry. " + "See README -> CLI providers for details." +) + + +def _build_kiro_argv(*, provider: CliProviderConfig, prompt: str) -> list[str]: + """Build the canonical Kiro CLI argv for a single non-interactive prompt. + + The argv shape is ``kiro-cli chat --no-interactive [provider.args...] ``. + Users that need ``--trust-all-tools`` or other flags should set them via + ``cli_providers.kiro.args`` in ``upskill.config.yaml``. + """ + return [ + provider.command, + "chat", + "--no-interactive", + *provider.args, + prompt, + ] + + +@dataclass(slots=True) +class KiroCliRunner: + """Invoke ``kiro-cli chat`` for a single prompt and capture the textual reply.""" + + name: str = "kiro" + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + return await execute_text_cli( + runner_name=self.name, + display_name="Kiro CLI", + argv=_build_kiro_argv(provider=provider, prompt=prompt), + provider=provider, + workspace_dir=workspace_dir, + artifact_dir=artifact_dir, + timeout=timeout, + auth_hint=_AUTH_HINT, + ) diff --git a/src/upskill/cli_agents/protocols.py b/src/upskill/cli_agents/protocols.py new file mode 100644 index 0000000..a1e00ae --- /dev/null +++ b/src/upskill/cli_agents/protocols.py @@ -0,0 +1,34 @@ +"""Narrow generator protocol used by upskill's skill/test-generation flows. + +Both fast-agent's ``AgentProtocol`` and ``CliFastAgentAdapter`` structurally +satisfy this. Using a narrower protocol keeps ``generate.py`` decoupled from +the concrete agent implementation (fast-agent or CLI-backed). + +The signatures here intentionally describe *only* the shape upskill calls, not +fast-agent's full API. Both implementations may accept additional optional +parameters; structural compatibility just requires they accept the calls +upskill actually makes. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Protocol, TypeVar, runtime_checkable + +if TYPE_CHECKING: + from pydantic import BaseModel + + ModelT = TypeVar("ModelT", bound=BaseModel) + + +@runtime_checkable +class SkillGenerator(Protocol): + """Surface upskill calls on its skill_gen / test_gen agents.""" + + async def send(self, message: str, /) -> str: ... + + async def structured( + self, + message: str, + model: type[ModelT], + /, + ) -> tuple[ModelT | None, object]: ... diff --git a/src/upskill/config.py b/src/upskill/config.py index bbf545e..3272ce4 100644 --- a/src/upskill/config.py +++ b/src/upskill/config.py @@ -10,6 +10,8 @@ import yaml from pydantic import AliasChoices, BaseModel, ConfigDict, Field +from upskill.cli_agents import CliProviderConfig, default_cli_providers + UPSKILL_CONFIG_FILE = "upskill.config.yaml" LEGACY_CONFIG_FILE = "config.yaml" UPSKILL_CONFIG_ENV = "UPSKILL_CONFIG" @@ -90,6 +92,36 @@ def get_default_runs_dir() -> Path: return Path.cwd() / "runs" +def _merge_cli_providers( + raw: object, +) -> dict[str, CliProviderConfig]: + """Merge user-supplied ``cli_providers`` mapping with the v1 defaults. + + Users can override a single provider (e.g. `claude-code`) without losing + the other built-in entries. Unknown keys are accepted so new CLIs can be + declared in config without code changes (the router still has to know how + to wire them — see ``executors/router.py``). + """ + if raw is None: + return default_cli_providers() + if not isinstance(raw, dict): + raise TypeError("`cli_providers` must be a mapping of provider name to config.") + + merged = default_cli_providers() + for key, value in raw.items(): + if not isinstance(key, str): + raise TypeError("`cli_providers` keys must be strings.") + if isinstance(value, CliProviderConfig): + merged[key] = value + continue + if not isinstance(value, dict): + raise TypeError( + f"`cli_providers.{key}` must be a mapping of fields, got {type(value).__name__}." + ) + merged[key] = CliProviderConfig.model_validate(value) + return merged + + def find_config_path() -> Path: """Find the fastagent config file, checking cwd first then package root.""" cwd_config = Path.cwd() / "fastagent.config.yaml" @@ -160,6 +192,15 @@ class Config(BaseModel): # FastAgent settings fastagent_config: Path | None = Field(default=None, description="Path to fastagent.config.yaml") + # CLI-backed agent providers (Claude Code, Copilot CLI, Kiro CLI, ...) + cli_providers: dict[str, CliProviderConfig] = Field( + default_factory=default_cli_providers, + description=( + "Per-provider configuration for cli. model aliases. " + "Keys map to the suffix after `cli.`." + ), + ) + @classmethod def load(cls) -> Config: """Load config from file, or return defaults.""" @@ -177,6 +218,8 @@ def load(cls) -> Config: data["runs_dir"] = Path(data["runs_dir"]) if "fastagent_config" in data and isinstance(data["fastagent_config"], str): data["fastagent_config"] = Path(data["fastagent_config"]) + if "cli_providers" in data: + data["cli_providers"] = _merge_cli_providers(data["cli_providers"]) return cls(**data) return cls() @@ -192,6 +235,9 @@ def save(self) -> None: data["runs_dir"] = str(self.runs_dir) if self.fastagent_config: data["fastagent_config"] = str(self.fastagent_config) + data["cli_providers"] = { + key: provider.model_dump(mode="json") for key, provider in self.cli_providers.items() + } with open(config_path, "w", encoding="utf-8") as f: yaml.dump(data, f, default_flow_style=False) diff --git a/src/upskill/executors/cli_agent.py b/src/upskill/executors/cli_agent.py new file mode 100644 index 0000000..0138c12 --- /dev/null +++ b/src/upskill/executors/cli_agent.py @@ -0,0 +1,214 @@ +"""Executor that drives CLI-backed agent runners (Claude Code, Copilot, Kiro).""" + +from __future__ import annotations + +import asyncio +import json +from typing import TYPE_CHECKING + +from upskill.artifacts import ( + ensure_directory, + materialize_skill_bundle, + materialize_workspace, + write_request_file, +) +from upskill.cli_agents import ( + CliAgentRunner, + CliProviderConfig, + CliRunResult, +) +from upskill.executors.contracts import ExecutionHandle, ExecutionRequest, ExecutionResult +from upskill.models import ConversationStats + +if TYPE_CHECKING: + from collections.abc import Mapping + from pathlib import Path + +CLI_MODEL_PREFIX = "cli." + +ExecutionMetadataValue = str | int | float | bool | None + + +def parse_cli_model(model: str) -> str: + """Extract the provider name from a ``cli.`` model string. + + Raises ``ValueError`` if ``model`` does not use the ``cli.`` prefix. + """ + if not model.startswith(CLI_MODEL_PREFIX): + raise ValueError(f"Model {model!r} is not a CLI agent model (expected `cli.`).") + suffix = model[len(CLI_MODEL_PREFIX) :] + if not suffix: + raise ValueError(f"Model {model!r} is missing the provider suffix after `cli.`.") + return suffix + + +def compose_cli_prompt(request: ExecutionRequest) -> str: + """Build the prompt sent to a CLI runner. + + When the request carries a skill, the rendered SKILL.md is prepended as a + system-style block; otherwise the request prompt is returned untouched. + CLI tools don't share a uniform ``system prompt`` flag, so we inline the + skill content here for parity across providers. + """ + if request.skill is None: + return request.prompt + + skill_block = request.skill.render().strip() + return ( + "You have access to the following skill document. Use it to inform your answer.\n\n" + f"{skill_block}\n\n" + "---\n\n" + f"{request.prompt}" + ) + + +def _coerce_cli_metadata( + metadata: Mapping[str, object], +) -> dict[str, ExecutionMetadataValue]: + """Filter CLI runner metadata down to ``ExecutionResult`` compatible types.""" + coerced: dict[str, ExecutionMetadataValue] = {} + for key, value in metadata.items(): + # Use a tuple of types instead of the PEP 604 union form for the + # broadest compatibility with isinstance() across runtimes and + # static analyzers. + if value is None or isinstance(value, (str, int, float, bool)): + coerced[key] = value + return coerced + + +class CliAgentExecutor: + """Execute evaluation requests by shelling out to a CLI agent runner. + + A single executor instance can dispatch any registered ``cli.`` + model to its matching runner. The router constructed in + ``upskill.executors.router`` is responsible for wiring the registry. + """ + + def __init__( + self, + *, + runners: Mapping[str, CliAgentRunner], + providers: Mapping[str, CliProviderConfig], + ) -> None: + if not runners: + raise ValueError("CliAgentExecutor requires at least one runner registration.") + self._runners = dict(runners) + self._providers = dict(providers) + + async def execute(self, request: ExecutionRequest) -> ExecutionHandle: + """Start a CLI-backed execution for a single request.""" + task = asyncio.create_task(self._run_request(request)) + return ExecutionHandle(request=request, task=task) + + async def collect(self, handle: ExecutionHandle) -> ExecutionResult: + """Wait for and collect the result of a CLI-backed execution.""" + return await handle.task + + async def cancel(self, handle: ExecutionHandle) -> None: + """Cancel an in-flight CLI-backed execution.""" + handle.task.cancel() + try: + await handle.task + except asyncio.CancelledError: + return + + def _resolve_runner( + self, + request: ExecutionRequest, + ) -> tuple[CliAgentRunner, CliProviderConfig] | None: + """Look up the runner + provider config for a request's model string.""" + try: + provider_name = parse_cli_model(request.model) + except ValueError: + return None + runner = self._runners.get(provider_name) + provider = self._providers.get(provider_name) + if runner is None or provider is None: + return None + return runner, provider + + def _materialize_artifacts( + self, + request: ExecutionRequest, + ) -> tuple[Path, Path, str]: + """Set up the canonical artifact layout and composed prompt for a request.""" + artifact_dir = ensure_directory(request.artifact_dir.resolve()) + workspace_dir = ensure_directory(artifact_dir / "workspace") + materialize_workspace(workspace_dir, request.workspace_files) + materialize_skill_bundle(artifact_dir / "skills", request) + write_request_file(artifact_dir / "request.json", request) + composed_prompt = compose_cli_prompt(request) + (artifact_dir / "prompt.txt").write_text(composed_prompt, encoding="utf-8") + return artifact_dir, workspace_dir, composed_prompt + + def _build_result( + self, + *, + request: ExecutionRequest, + artifact_dir: Path, + workspace_dir: Path, + cli_run: CliRunResult, + ) -> ExecutionResult: + """Convert a runner outcome into the canonical ``ExecutionResult``.""" + merged_metadata: dict[str, ExecutionMetadataValue] = {**request.metadata} + merged_metadata.update(_coerce_cli_metadata(cli_run.metadata)) + merged_metadata["return_code"] = cli_run.return_code + + results_path = artifact_dir / "results.json" + if cli_run.output_text is not None: + results_path.write_text( + json.dumps({"result": cli_run.output_text}, indent=2), + encoding="utf-8", + ) + + return ExecutionResult( + output_text=cli_run.output_text, + raw_results_path=results_path if results_path.exists() else None, + stdout_path=artifact_dir / "stdout.txt", + stderr_path=artifact_dir / "stderr.txt", + artifact_dir=artifact_dir, + workspace_dir=workspace_dir, + stats=cli_run.stats, + error=cli_run.error, + metadata=merged_metadata, + ) + + async def _run_request(self, request: ExecutionRequest) -> ExecutionResult: + artifact_dir, workspace_dir, composed_prompt = self._materialize_artifacts(request) + + resolved = self._resolve_runner(request) + if resolved is None: + error = ( + f"No CLI runner registered for model {request.model!r}. " + f"Known providers: {sorted(self._runners.keys()) or 'none'}." + ) + empty_result = CliRunResult( + output_text=None, + stats=ConversationStats(), + return_code=-1, + error=error, + metadata={"runner": "missing"}, + ) + # Persist empty stdout/stderr so the canonical artifact layout is preserved. + (artifact_dir / "stdout.txt").write_text("", encoding="utf-8") + (artifact_dir / "stderr.txt").write_text("", encoding="utf-8") + return self._build_result( + request=request, + artifact_dir=artifact_dir, + workspace_dir=workspace_dir, + cli_run=empty_result, + ) + + runner, provider = resolved + cli_run = await runner.run( + prompt=composed_prompt, + workspace_dir=workspace_dir, + artifact_dir=artifact_dir, + provider=provider, + ) + return self._build_result( + request=request, + artifact_dir=artifact_dir, + workspace_dir=workspace_dir, + cli_run=cli_run, + ) diff --git a/src/upskill/executors/router.py b/src/upskill/executors/router.py new file mode 100644 index 0000000..bf2cd29 --- /dev/null +++ b/src/upskill/executors/router.py @@ -0,0 +1,83 @@ +"""Per-model executor routing. + +Decides which executor handles a given model string at runtime so a single +``upskill eval`` invocation can mix CLI-backed agents (``cli.claude-code``, +``cli.copilot``, ``cli.kiro``) with regular fast-agent models in the same +benchmark. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from upskill.cli_agents import CLI_PROVIDER_NAMES, CliAgentRunner, CliProviderConfig +from upskill.cli_agents.claude_code import ClaudeCodeRunner +from upskill.cli_agents.copilot import CopilotCliRunner +from upskill.cli_agents.kiro import KiroCliRunner +from upskill.executors.cli_agent import CLI_MODEL_PREFIX, CliAgentExecutor +from upskill.executors.local_fast_agent import LocalFastAgentExecutor +from upskill.executors.remote_fast_agent import RemoteFastAgentExecutor + +if TYPE_CHECKING: + from collections.abc import Callable + + from upskill.config import Config + from upskill.executors.base import Executor + from upskill.hf_jobs import JobsConfig + + +def is_cli_model(model: str) -> bool: + """Return True when the model string targets a CLI agent provider.""" + return model.startswith(CLI_MODEL_PREFIX) + + +def build_default_cli_runners() -> dict[str, CliAgentRunner]: + """Return the v1 set of registered CLI runners keyed by provider name.""" + runners: dict[str, CliAgentRunner] = { + "claude-code": ClaudeCodeRunner(), + "copilot": CopilotCliRunner(), + "kiro": KiroCliRunner(), + } + # Sanity check: the runner registry must cover every provider name we + # advertise as part of v1 so router lookups never silently miss. + missing = set(CLI_PROVIDER_NAMES) - runners.keys() + if missing: + raise RuntimeError(f"build_default_cli_runners is missing runners for: {sorted(missing)}") + return runners + + +def build_cli_executor(config: Config) -> CliAgentExecutor: + """Build a ``CliAgentExecutor`` wired with the v1 runners and config providers.""" + providers: dict[str, CliProviderConfig] = dict(config.cli_providers) + return CliAgentExecutor(runners=build_default_cli_runners(), providers=providers) + + +def select_executor_for_model( + model: str, + *, + config: Config, + jobs_config: JobsConfig | None = None, + jobs_progress_callback: Callable[[str], None] | None = None, +) -> Executor: + """Resolve the executor that should handle ``model`` for one evaluation pass. + + - ``cli.`` -> ``CliAgentExecutor`` + - otherwise, fall back to the user's configured fast-agent backend + (``local`` -> ``LocalFastAgentExecutor`` or ``jobs`` -> + ``RemoteFastAgentExecutor``). + """ + if is_cli_model(model): + return build_cli_executor(config) + + if config.executor == "jobs": + if jobs_config is None: + raise ValueError( + "config.executor='jobs' but no JobsConfig was supplied to the router; " + "build a JobsConfig from the CLI flags before routing." + ) + return RemoteFastAgentExecutor( + jobs_config=jobs_config, + progress_callback=jobs_progress_callback, + ) + + return LocalFastAgentExecutor() diff --git a/src/upskill/generate.py b/src/upskill/generate.py index 83b6dad..7c38972 100644 --- a/src/upskill/generate.py +++ b/src/upskill/generate.py @@ -9,9 +9,10 @@ from upskill.models import Skill, SkillMetadata, SkillRecord, SkillState, TestCase, TestCaseSuite if TYPE_CHECKING: - from fast_agent.interfaces import AgentProtocol from fast_agent.skills.registry import SkillManifest + from upskill.cli_agents import SkillGenerator + # Few-shot examples for test generation TEST_EXAMPLES = """ ## Example Test Cases @@ -110,7 +111,7 @@ def _build_skill_from_manifest( async def generate_skill( task: str, - generator: AgentProtocol, + generator: SkillGenerator, examples: list[str] | None = None, model: str | None = None, ) -> SkillRecord: @@ -136,7 +137,7 @@ async def generate_skill( async def generate_tests( task: str, - generator: AgentProtocol, + generator: SkillGenerator, ) -> list[TestCase]: """Generate synthetic test cases from a task description using fast-agent.""" @@ -171,7 +172,7 @@ async def generate_tests( async def refine_skill( skill: SkillRecord, failures: list[str], - generator: AgentProtocol, + generator: SkillGenerator, model: str | None = None, ) -> SkillRecord: """Refine a skill based on evaluation failures using fast-agent.""" @@ -234,7 +235,7 @@ async def refine_skill( async def improve_skill( skill: SkillRecord, instructions: str, - generator: AgentProtocol, + generator: SkillGenerator, model: str | None = None, ) -> SkillRecord: """Improve an existing skill based on instructions. diff --git a/src/upskill/model_resolution.py b/src/upskill/model_resolution.py index 6501d03..23c4c38 100644 --- a/src/upskill/model_resolution.py +++ b/src/upskill/model_resolution.py @@ -46,6 +46,47 @@ def build_fastagent_model_references( } +CLI_MODEL_PREFIX = "cli." + + +def _is_cli(model: str) -> bool: + return model.startswith(CLI_MODEL_PREFIX) + + +def _resolve_eval_test_gen_model( + *, + cli_test_gen_model: str | None, + config: Config, + evaluation_models: list[str], +) -> str: + """Pick the test-generation model for ``eval``/``benchmark`` runs. + + Resolution order (first match wins): + + 1. Explicit CLI override (``--test-gen-model``), if provided. + 2. When every evaluation model is ``cli.`` and the configured + ``test_gen_model`` is unset (or itself a non-CLI alias), use the first + CLI evaluation model so the run stays key-free end-to-end. + 3. Config ``test_gen_model``, when set (covers both non-CLI and CLI + values, including the all-CLI case where the user explicitly opted + into a specific CLI test-gen model). + 4. Config ``skill_generation_model`` as the final fallback. + """ + if cli_test_gen_model is not None: + return cli_test_gen_model + + all_eval_cli = bool(evaluation_models) and all(_is_cli(m) for m in evaluation_models) + config_test_gen = config.test_gen_model + + if all_eval_cli and (config_test_gen is None or not _is_cli(config_test_gen)): + return evaluation_models[0] + + if config_test_gen is not None: + return config_test_gen + + return config.skill_generation_model + + def resolve_models( command: CommandName, *, @@ -92,8 +133,10 @@ def resolve_models( is_benchmark_mode = len(evaluation_models) > 1 or num_runs > 1 run_baseline = (not no_baseline) if not is_benchmark_mode else False return ResolvedModels( - test_generation_model=( - cli_test_gen_model or config.test_gen_model or config.skill_generation_model + test_generation_model=_resolve_eval_test_gen_model( + cli_test_gen_model=cli_test_gen_model, + config=config, + evaluation_models=evaluation_models, ), evaluation_models=evaluation_models, is_benchmark_mode=is_benchmark_mode, @@ -105,8 +148,10 @@ def resolve_models( if not evaluation_models: raise ValueError("benchmark requires at least one model") return ResolvedModels( - test_generation_model=( - cli_test_gen_model or config.test_gen_model or config.skill_generation_model + test_generation_model=_resolve_eval_test_gen_model( + cli_test_gen_model=cli_test_gen_model, + config=config, + evaluation_models=evaluation_models, ), evaluation_models=evaluation_models, is_benchmark_mode=True, diff --git a/tests/fixtures/fake_cli_agents/fake_claude.py b/tests/fixtures/fake_cli_agents/fake_claude.py new file mode 100755 index 0000000..13db2d1 --- /dev/null +++ b/tests/fixtures/fake_cli_agents/fake_claude.py @@ -0,0 +1,36 @@ +#!/usr/bin/env python3 +"""Fake Claude Code CLI for upskill end-to-end tests. + +Mimics ``claude -p --output-format json`` by emitting a JSON envelope +with a deterministic ``result`` and synthetic ``usage`` token counts. +""" + +from __future__ import annotations + +import json +import sys + + +def _extract_prompt(argv: list[str]) -> str: + for index, value in enumerate(argv): + if value == "-p" and index + 1 < len(argv): + return argv[index + 1] + return "" + + +def main() -> int: + prompt = _extract_prompt(sys.argv[1:]) + payload = { + "result": f"FAKE-CLAUDE: {prompt[:80]}", + "usage": { + "input_tokens": 13, + "output_tokens": 7, + }, + } + sys.stdout.write(json.dumps(payload)) + sys.stdout.flush() + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/fixtures/fake_cli_agents/fake_copilot.py b/tests/fixtures/fake_cli_agents/fake_copilot.py new file mode 100755 index 0000000..c8e8781 --- /dev/null +++ b/tests/fixtures/fake_cli_agents/fake_copilot.py @@ -0,0 +1,28 @@ +#!/usr/bin/env python3 +"""Fake GitHub Copilot CLI for upskill end-to-end tests. + +Mimics ``copilot -p `` by emitting a deterministic plain-text reply. +No usage/token fields, mirroring the real CLI's behavior in v1. +""" + +from __future__ import annotations + +import sys + + +def _extract_prompt(argv: list[str]) -> str: + for index, value in enumerate(argv): + if value == "-p" and index + 1 < len(argv): + return argv[index + 1] + return "" + + +def main() -> int: + prompt = _extract_prompt(sys.argv[1:]) + sys.stdout.write(f"FAKE-COPILOT: {prompt[:80]}\n") + sys.stdout.flush() + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/fixtures/fake_cli_agents/fake_kiro.py b/tests/fixtures/fake_cli_agents/fake_kiro.py new file mode 100755 index 0000000..cf28f6f --- /dev/null +++ b/tests/fixtures/fake_cli_agents/fake_kiro.py @@ -0,0 +1,30 @@ +#!/usr/bin/env python3 +"""Fake Kiro CLI for upskill end-to-end tests. + +Mimics ``kiro-cli chat --no-interactive [...] `` by emitting a +deterministic plain-text reply that includes the trailing positional prompt. +""" + +from __future__ import annotations + +import sys + + +def _extract_prompt(argv: list[str]) -> str: + # The Kiro CLI invocation we build is: + # kiro-cli chat --no-interactive [provider.args...] + # So the trailing positional argument is always the prompt. + if not argv: + return "" + return argv[-1] + + +def main() -> int: + prompt = _extract_prompt(sys.argv[1:]) + sys.stdout.write(f"FAKE-KIRO: {prompt[:80]}\n") + sys.stdout.flush() + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_claude_code_runner.py b/tests/test_claude_code_runner.py new file mode 100644 index 0000000..882d83f --- /dev/null +++ b/tests/test_claude_code_runner.py @@ -0,0 +1,174 @@ +"""Unit tests for the Claude Code CLI runner.""" + +from __future__ import annotations + +import asyncio +import json +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli_agents import CliProviderConfig, CliRunResult +from upskill.cli_agents.claude_code import ClaudeCodeRunner + +if TYPE_CHECKING: + from pathlib import Path + + +def _make_provider(args: list[str] | None = None) -> CliProviderConfig: + return CliProviderConfig(command="claude", args=args or [], timeout_seconds=60) + + +class _FakeProcess: + """Stand-in for ``asyncio.subprocess.Process``.""" + + def __init__(self, *, stdout: bytes, stderr: bytes, returncode: int) -> None: + self._stdout = stdout + self._stderr = stderr + self.returncode = returncode + + async def communicate(self, input: bytes | None = None) -> tuple[bytes, bytes]: + del input + return self._stdout, self._stderr + + def kill(self) -> None: + return None + + +def _patch_subprocess( + monkeypatch: pytest.MonkeyPatch, + *, + stdout: bytes = b"", + stderr: bytes = b"", + returncode: int = 0, + raise_file_not_found: bool = False, + captured_argv: list[list[str]] | None = None, +) -> None: + async def fake_create_subprocess_exec(*args: str, **_: object) -> _FakeProcess: + if captured_argv is not None: + captured_argv.append(list(args)) + if raise_file_not_found: + raise FileNotFoundError(args[0]) + return _FakeProcess(stdout=stdout, stderr=stderr, returncode=returncode) + + monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec) + + +def _make_dirs(tmp_path: Path) -> tuple[Path, Path]: + workspace = tmp_path / "workspace" + workspace.mkdir() + artifact = tmp_path / "artifact" + artifact.mkdir() + return workspace, artifact + + +@pytest.mark.asyncio +async def test_claude_runner_parses_success_envelope( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + captured: list[list[str]] = [] + payload = json.dumps( + { + "result": "the answer", + "usage": {"input_tokens": 7, "output_tokens": 11}, + } + ).encode("utf-8") + _patch_subprocess(monkeypatch, stdout=payload, captured_argv=captured) + + runner = ClaudeCodeRunner() + result = await runner.run( + prompt="hello", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(args=["--model", "sonnet"]), + timeout=10, + ) + + assert isinstance(result, CliRunResult) + assert result.error is None + assert result.output_text == "the answer" + assert result.stats.input_tokens == 7 + assert result.stats.output_tokens == 11 + assert result.stats.total_tokens == 18 + assert result.return_code == 0 + + # argv shape + assert captured == [["claude", "-p", "hello", "--output-format", "json", "--model", "sonnet"]] + # debug artifacts + assert (artifact / "command.json").exists() + assert (artifact / "stdout.txt").exists() + assert (artifact / "stderr.txt").exists() + + +@pytest.mark.asyncio +async def test_claude_runner_handles_malformed_json( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess(monkeypatch, stdout=b"not-json") + + runner = ClaudeCodeRunner() + result = await runner.run( + prompt="hello", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.output_text is None + assert result.error is not None + assert "valid JSON" in result.error + assert result.return_code == 0 + + +@pytest.mark.asyncio +async def test_claude_runner_surfaces_auth_error( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess( + monkeypatch, + stdout=b"", + stderr=b"Error: not authenticated. Run `claude login`.\n", + returncode=1, + ) + + runner = ClaudeCodeRunner() + result = await runner.run( + prompt="hello", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.error is not None + assert "exited with code 1" in result.error + assert "claude login" in result.error + assert result.return_code == 1 + + +@pytest.mark.asyncio +async def test_claude_runner_handles_missing_binary( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess(monkeypatch, raise_file_not_found=True) + + runner = ClaudeCodeRunner() + result = await runner.run( + prompt="hello", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.output_text is None + assert result.error is not None + assert "not found on PATH" in result.error + assert "claude login" in result.error # auth hint appended + assert result.return_code == -1 diff --git a/tests/test_cli_agent_executor.py b/tests/test_cli_agent_executor.py new file mode 100644 index 0000000..bbdbd85 --- /dev/null +++ b/tests/test_cli_agent_executor.py @@ -0,0 +1,237 @@ +"""Tests for ``CliAgentExecutor``.""" + +from __future__ import annotations + +import asyncio +from dataclasses import dataclass, field +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli_agents import ( + CliProviderConfig, + CliRunResult, +) +from upskill.executors.cli_agent import ( + CliAgentExecutor, + compose_cli_prompt, + parse_cli_model, +) +from upskill.executors.contracts import ExecutionRequest +from upskill.models import ConversationStats, Skill + +if TYPE_CHECKING: + from pathlib import Path + + +@dataclass(slots=True) +class _FakeRunner: + name: str = "claude-code" + canned_output: str = "fake-output" + return_code: int = 0 + error: str | None = None + sleep_seconds: float = 0.0 + invocations: list[dict[str, object]] = field(default_factory=list) + raise_inside: bool = False + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + if self.sleep_seconds: + await asyncio.sleep(self.sleep_seconds) + if self.raise_inside: + raise RuntimeError("runner blew up") + self.invocations.append( + { + "prompt": prompt, + "workspace_dir": workspace_dir, + "artifact_dir": artifact_dir, + "provider_command": provider.command, + "timeout": timeout, + } + ) + # Simulate the runner persisting stdout/stderr/command.json itself. + (artifact_dir / "stdout.txt").write_text(self.canned_output, encoding="utf-8") + (artifact_dir / "stderr.txt").write_text("", encoding="utf-8") + (artifact_dir / "command.json").write_text('{"argv": []}', encoding="utf-8") + return CliRunResult( + output_text=self.canned_output if self.return_code == 0 else None, + stats=ConversationStats(input_tokens=2, output_tokens=3, total_tokens=5), + return_code=self.return_code, + stdout=self.canned_output, + stderr="", + error=self.error, + metadata={"runner": self.name, "tokens_unavailable": False}, + ) + + +def _build_request(tmp_path: Path, *, model: str = "cli.claude-code") -> ExecutionRequest: + config_path = tmp_path / "fastagent.config.yaml" + config_path.write_text("default_model: sonnet\n", encoding="utf-8") + cards_dir = tmp_path / "cards" + cards_dir.mkdir() + (cards_dir / "evaluator.md").write_text("---\nrole: evaluator\n---\nhi\n", encoding="utf-8") + skill = Skill( + name="example-skill", + description="An example skill.", + body="Always be concise.", + ) + return ExecutionRequest( + prompt="Solve the puzzle.", + model=model, + agent="evaluator", + fastagent_config_path=config_path, + artifact_dir=tmp_path / "artifacts" / "test_1", + cards_source_dir=cards_dir, + label="test", + skill=skill, + workspace_files={"context.txt": "hello world"}, + metadata={"phase": "with-skill"}, + ) + + +def _make_executor(runner_name: str = "claude-code") -> tuple[CliAgentExecutor, _FakeRunner]: + runner = _FakeRunner(name=runner_name) + executor = CliAgentExecutor( + runners={runner_name: runner}, + providers={runner_name: CliProviderConfig(command="claude")}, + ) + return executor, runner + + +def test_parse_cli_model_extracts_provider_name() -> None: + assert parse_cli_model("cli.claude-code") == "claude-code" + assert parse_cli_model("cli.copilot") == "copilot" + assert parse_cli_model("cli.kiro") == "kiro" + + +def test_parse_cli_model_rejects_non_cli_prefix() -> None: + with pytest.raises(ValueError): + parse_cli_model("sonnet") + + +def test_parse_cli_model_rejects_empty_suffix() -> None: + with pytest.raises(ValueError): + parse_cli_model("cli.") + + +def test_compose_cli_prompt_passes_through_when_no_skill(tmp_path: Path) -> None: + request = _build_request(tmp_path) + request_no_skill = ExecutionRequest( + prompt="hi", + model=request.model, + agent=request.agent, + fastagent_config_path=request.fastagent_config_path, + artifact_dir=request.artifact_dir, + cards_source_dir=request.cards_source_dir, + label=request.label, + skill=None, + ) + assert compose_cli_prompt(request_no_skill) == "hi" + + +def test_compose_cli_prompt_prepends_skill_block(tmp_path: Path) -> None: + request = _build_request(tmp_path) + composed = compose_cli_prompt(request) + assert composed.startswith("You have access to the following skill") + assert "example-skill" in composed + assert "Always be concise." in composed + assert composed.endswith("Solve the puzzle.") + + +@pytest.mark.asyncio +async def test_executor_runs_request_and_persists_artifacts(tmp_path: Path) -> None: + executor, runner = _make_executor() + request = _build_request(tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.error is None + assert result.output_text == "fake-output" + assert result.stats.total_tokens == 5 + assert result.metadata["return_code"] == 0 + assert result.metadata["runner"] == "claude-code" + assert result.metadata["phase"] == "with-skill" # request metadata preserved + + artifact_dir = request.artifact_dir.resolve() + assert (artifact_dir / "request.json").exists() + assert (artifact_dir / "prompt.txt").exists() + assert (artifact_dir / "stdout.txt").exists() + assert (artifact_dir / "stderr.txt").exists() + assert (artifact_dir / "results.json").exists() + assert (artifact_dir / "workspace" / "context.txt").read_text(encoding="utf-8") == "hello world" + assert (artifact_dir / "skills" / "example-skill" / "SKILL.md").exists() + + assert len(runner.invocations) == 1 + invoked = runner.invocations[0] + assert invoked["workspace_dir"] == artifact_dir / "workspace" + assert invoked["artifact_dir"] == artifact_dir + assert invoked["provider_command"] == "claude" + # The composed prompt was passed in + assert "Solve the puzzle." in invoked["prompt"] # type: ignore[operator] + + +@pytest.mark.asyncio +async def test_executor_propagates_runner_error(tmp_path: Path) -> None: + runner = _FakeRunner( + name="claude-code", + canned_output="", + return_code=2, + error="Claude CLI exited with code 2: not authenticated", + ) + executor = CliAgentExecutor( + runners={"claude-code": runner}, + providers={"claude-code": CliProviderConfig(command="claude")}, + ) + request = _build_request(tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.error == "Claude CLI exited with code 2: not authenticated" + assert result.output_text is None + assert result.metadata["return_code"] == 2 + + +@pytest.mark.asyncio +async def test_executor_returns_clear_error_for_unknown_provider(tmp_path: Path) -> None: + executor, _ = _make_executor() + request = _build_request(tmp_path, model="cli.kiro") + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.output_text is None + assert result.error is not None + assert "No CLI runner registered" in result.error + assert "claude-code" in result.error + # Even with an unknown provider, the canonical artifact layout exists. + assert (request.artifact_dir.resolve() / "stdout.txt").exists() + + +@pytest.mark.asyncio +async def test_executor_supports_cancellation(tmp_path: Path) -> None: + runner = _FakeRunner(name="claude-code", sleep_seconds=5) + executor = CliAgentExecutor( + runners={"claude-code": runner}, + providers={"claude-code": CliProviderConfig(command="claude")}, + ) + request = _build_request(tmp_path) + + handle = await executor.execute(request) + await asyncio.sleep(0) + await executor.cancel(handle) + + assert handle.task.cancelled() or handle.task.done() + + +def test_executor_requires_at_least_one_runner() -> None: + with pytest.raises(ValueError): + CliAgentExecutor(runners={}, providers={}) diff --git a/tests/test_cli_agents_contract.py b/tests/test_cli_agents_contract.py new file mode 100644 index 0000000..0b48dcb --- /dev/null +++ b/tests/test_cli_agents_contract.py @@ -0,0 +1,194 @@ +"""Contract tests for the CLI agent runner protocol and config schema.""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import TYPE_CHECKING + +import pytest +import yaml +from pydantic import ValidationError + +from upskill.cli_agents import ( + CLI_PROVIDER_NAMES, + CliAgentRunner, + CliProviderConfig, + CliRunResult, + default_cli_providers, +) +from upskill.config import Config +from upskill.models import ConversationStats + +if TYPE_CHECKING: + from pathlib import Path + + +@dataclass(slots=True) +class FakeCliRunner: + """Minimal in-memory implementation of the ``CliAgentRunner`` protocol.""" + + name: str = "fake" + canned_output: str = "ok" + invocations: list[dict[str, object]] = field(default_factory=list) + + async def run( + self, + *, + prompt: str, + workspace_dir: Path, + artifact_dir: Path, + provider: CliProviderConfig, + timeout: float | None = None, + ) -> CliRunResult: + self.invocations.append( + { + "prompt": prompt, + "workspace_dir": workspace_dir, + "artifact_dir": artifact_dir, + "provider": provider, + "timeout": timeout, + } + ) + return CliRunResult( + output_text=self.canned_output, + stats=ConversationStats(input_tokens=1, output_tokens=2, total_tokens=3), + return_code=0, + stdout=self.canned_output, + stderr="", + metadata={"runner": self.name}, + ) + + +def test_default_cli_providers_covers_v1_set() -> None: + providers = default_cli_providers() + assert set(providers.keys()) == set(CLI_PROVIDER_NAMES) + assert providers["claude-code"].command == "claude" + assert providers["copilot"].command == "copilot" + assert providers["kiro"].command == "kiro-cli" + for provider in providers.values(): + assert provider.timeout_seconds is not None and provider.timeout_seconds > 0 + assert provider.args == [] + assert provider.env == {} + + +def test_cli_provider_config_rejects_unknown_fields() -> None: + with pytest.raises(ValidationError): + CliProviderConfig.model_validate({"command": "claude", "unknown_key": True}) + + +def test_cli_provider_config_rejects_blank_command() -> None: + with pytest.raises(ValidationError): + CliProviderConfig.model_validate({"command": ""}) + + +def test_cli_provider_config_rejects_non_positive_timeout() -> None: + with pytest.raises(ValidationError): + CliProviderConfig.model_validate({"command": "claude", "timeout_seconds": 0}) + + +def test_config_defaults_include_cli_providers() -> None: + config = Config() + assert set(config.cli_providers.keys()) == set(CLI_PROVIDER_NAMES) + assert config.cli_providers["claude-code"].command == "claude" + + +def test_config_load_merges_user_overrides_with_defaults( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + config_path = tmp_path / "upskill.config.yaml" + config_path.write_text( + yaml.safe_dump( + { + "skill_generation_model": "sonnet", + "cli_providers": { + "claude-code": { + "command": "claude", + "args": ["--model", "opus"], + "timeout_seconds": 1200, + }, + "custom-tool": { + "command": "/usr/local/bin/custom", + }, + }, + } + ), + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + config = Config.load() + + assert config.cli_providers["claude-code"].args == ["--model", "opus"] + assert config.cli_providers["claude-code"].timeout_seconds == 1200 + # Defaults that were not overridden survive. + assert config.cli_providers["copilot"].command == "copilot" + assert config.cli_providers["kiro"].command == "kiro-cli" + # User-defined extra entry is preserved. + assert config.cli_providers["custom-tool"].command == "/usr/local/bin/custom" + + +def test_config_load_rejects_malformed_cli_providers( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + config_path = tmp_path / "upskill.config.yaml" + config_path.write_text( + yaml.safe_dump( + { + "cli_providers": { + "claude-code": "not-a-mapping", + } + } + ), + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + with pytest.raises(TypeError): + Config.load() + + +def test_config_save_round_trips_cli_providers( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.chdir(tmp_path) + config = Config() + config.cli_providers["claude-code"] = CliProviderConfig( + command="claude", + args=["--model", "haiku"], + timeout_seconds=300, + ) + config.save() + + saved_path = tmp_path / "upskill.config.yaml" + saved = yaml.safe_load(saved_path.read_text(encoding="utf-8")) + assert saved["cli_providers"]["claude-code"]["args"] == ["--model", "haiku"] + + reloaded = Config.load() + assert reloaded.cli_providers["claude-code"].args == ["--model", "haiku"] + assert reloaded.cli_providers["claude-code"].timeout_seconds == 300 + + +@pytest.mark.asyncio +async def test_fake_runner_satisfies_protocol(tmp_path: Path) -> None: + runner: CliAgentRunner = FakeCliRunner(name="fake") + provider = CliProviderConfig(command="echo", args=[], timeout_seconds=10) + workspace = tmp_path / "workspace" + workspace.mkdir() + artifact = tmp_path / "artifact" + artifact.mkdir() + + result = await runner.run( + prompt="hi", + workspace_dir=workspace, + artifact_dir=artifact, + provider=provider, + timeout=10, + ) + + assert isinstance(result, CliRunResult) + assert result.output_text == "ok" + assert result.stats.total_tokens == 3 + assert result.metadata["runner"] == "fake" diff --git a/tests/test_cli_agents_e2e.py b/tests/test_cli_agents_e2e.py new file mode 100644 index 0000000..908d0a0 --- /dev/null +++ b/tests/test_cli_agents_e2e.py @@ -0,0 +1,160 @@ +"""End-to-end tests that drive the CLI executor stack via fake CLI binaries. + +These tests prove the full path -- router -> CliAgentExecutor -> runner -> +real subprocess -> result parsing -- works without depending on the real +``claude``/``copilot``/``kiro-cli`` binaries being present in CI. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli_agents import CliProviderConfig +from upskill.config import Config +from upskill.executors.contracts import ExecutionRequest +from upskill.executors.router import build_cli_executor +from upskill.models import Skill + +if TYPE_CHECKING: + from upskill.executors.cli_agent import CliAgentExecutor + + +FIXTURE_DIR = Path(__file__).resolve().parent / "fixtures" / "fake_cli_agents" + + +def _ensure_executable(*names: str) -> None: + """Make sure fixture scripts have the executable bit set on this checkout.""" + for name in names: + path = FIXTURE_DIR / name + mode = path.stat().st_mode + path.chmod(mode | 0o111) + + +def _build_config_with_fakes() -> Config: + """Build a Config whose cli_providers point at the fake fixture scripts.""" + _ensure_executable("fake_claude.py", "fake_copilot.py", "fake_kiro.py") + config = Config() + config.cli_providers = { + "claude-code": CliProviderConfig( + command=str(FIXTURE_DIR / "fake_claude.py"), + timeout_seconds=30, + ), + "copilot": CliProviderConfig( + command=str(FIXTURE_DIR / "fake_copilot.py"), + timeout_seconds=30, + ), + "kiro": CliProviderConfig( + command=str(FIXTURE_DIR / "fake_kiro.py"), + timeout_seconds=30, + ), + } + return config + + +def _make_request(*, model: str, tmp_path: Path) -> ExecutionRequest: + config_path = tmp_path / "fastagent.config.yaml" + config_path.write_text("default_model: sonnet\n", encoding="utf-8") + cards_dir = tmp_path / "cards" + cards_dir.mkdir(exist_ok=True) + (cards_dir / "evaluator.md").write_text("---\nrole: evaluator\n---\nstub\n", encoding="utf-8") + return ExecutionRequest( + prompt="Translate 'hello' into a friendly message.", + model=model, + agent="evaluator", + fastagent_config_path=config_path, + artifact_dir=tmp_path / "artifacts" / model, + cards_source_dir=cards_dir, + label=f"e2e-{model}", + skill=Skill( + name="example-skill", + description="Greet users politely.", + body="Always start with 'Hello,'.", + ), + workspace_files={}, + metadata={"phase": "e2e"}, + ) + + +def _skip_if_no_fixture_python() -> None: + if not os.access("/usr/bin/env", os.X_OK): # pragma: no cover - macOS/linux only + pytest.skip("Fixture shebangs require a POSIX env interpreter.") + + +@pytest.mark.asyncio +async def test_e2e_claude_code_runner_against_fake_binary(tmp_path: Path) -> None: + _skip_if_no_fixture_python() + config = _build_config_with_fakes() + executor: CliAgentExecutor = build_cli_executor(config) + request = _make_request(model="cli.claude-code", tmp_path=tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.error is None + assert result.output_text is not None + assert result.output_text.startswith("FAKE-CLAUDE:") + assert result.stats.input_tokens == 13 + assert result.stats.output_tokens == 7 + assert result.stats.total_tokens == 20 + assert (request.artifact_dir / "stdout.txt").exists() + assert (request.artifact_dir / "command.json").exists() + + +@pytest.mark.asyncio +async def test_e2e_copilot_runner_against_fake_binary(tmp_path: Path) -> None: + _skip_if_no_fixture_python() + config = _build_config_with_fakes() + executor: CliAgentExecutor = build_cli_executor(config) + request = _make_request(model="cli.copilot", tmp_path=tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.error is None + assert result.output_text is not None + assert result.output_text.startswith("FAKE-COPILOT:") + # Copilot does not expose tokens. + assert result.stats.total_tokens == 0 + assert result.metadata.get("tokens_unavailable") is True + + +@pytest.mark.asyncio +async def test_e2e_kiro_runner_against_fake_binary(tmp_path: Path) -> None: + _skip_if_no_fixture_python() + config = _build_config_with_fakes() + executor: CliAgentExecutor = build_cli_executor(config) + request = _make_request(model="cli.kiro", tmp_path=tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.error is None + assert result.output_text is not None + assert result.output_text.startswith("FAKE-KIRO:") + # Kiro does not expose tokens. + assert result.stats.total_tokens == 0 + assert result.metadata.get("tokens_unavailable") is True + + +@pytest.mark.asyncio +async def test_e2e_cli_executor_reports_missing_binary_clearly(tmp_path: Path) -> None: + """If the configured binary is not on disk, the runner returns an actionable error.""" + config = Config() + config.cli_providers["claude-code"] = CliProviderConfig( + command="/this/path/does/not/exist/fake-claude", + timeout_seconds=5, + ) + executor = build_cli_executor(config) + request = _make_request(model="cli.claude-code", tmp_path=tmp_path) + + handle = await executor.execute(request) + result = await executor.collect(handle) + + assert result.output_text is None + assert result.error is not None + assert "not found on PATH" in result.error + assert "claude login" in result.error # auth hint diff --git a/tests/test_cli_env_stripping.py b/tests/test_cli_env_stripping.py new file mode 100644 index 0000000..f7959d9 --- /dev/null +++ b/tests/test_cli_env_stripping.py @@ -0,0 +1,162 @@ +"""Tests that verify CLI runners strip API-key env vars from subprocesses. + +This is what keeps `claude` (and friends) on the user's subscription rather +than silently falling back to per-token API billing when an `*_API_KEY` env +var is set in the parent shell. +""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING, cast + +import pytest + +from upskill.cli_agents import CliProviderConfig, default_cli_providers +from upskill.cli_agents.base import ( + _resolve_subprocess_env, + _warn_about_stripped_env, + _warned_stripped_env, +) +from upskill.cli_agents.claude_code import ClaudeCodeRunner + +if TYPE_CHECKING: + from pathlib import Path + + +def _reset_warning_cache() -> None: + _warned_stripped_env.clear() + + +def test_default_claude_provider_strips_anthropic_env() -> None: + providers = default_cli_providers() + claude = providers["claude-code"] + assert "ANTHROPIC_API_KEY" in claude.unset_env + assert "CLAUDE_CODE_USE_BEDROCK" in claude.unset_env + assert "CLAUDE_CODE_USE_VERTEX" in claude.unset_env + # Other providers stay quiet by default. + assert providers["copilot"].unset_env == [] + assert providers["kiro"].unset_env == [] + + +def test_resolve_subprocess_env_strips_listed_vars(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + monkeypatch.setenv("OTHER_KEY", "stays") + env = _resolve_subprocess_env( + extra_env={"FOO": "bar"}, + unset_env=["ANTHROPIC_API_KEY"], + ) + assert "ANTHROPIC_API_KEY" not in env + assert env["OTHER_KEY"] == "stays" + assert env["FOO"] == "bar" + + +def test_resolve_subprocess_env_also_strips_extra_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Even if the user re-supplies the var via env=, unset_env wins.""" + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + env = _resolve_subprocess_env( + extra_env={"ANTHROPIC_API_KEY": "sk-ant-leak"}, + unset_env=["ANTHROPIC_API_KEY"], + ) + assert "ANTHROPIC_API_KEY" not in env + + +def test_warn_about_stripped_env_writes_once_per_var( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + _reset_warning_cache() + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + + _warn_about_stripped_env(runner_label="claude-code", unset_env=["ANTHROPIC_API_KEY"]) + _warn_about_stripped_env(runner_label="claude-code", unset_env=["ANTHROPIC_API_KEY"]) + + captured = capsys.readouterr() + # Exactly one stderr line, even after the second call. + assert captured.err.count("unsetting ANTHROPIC_API_KEY") == 1 + assert "subscription" in captured.err + assert "claude-code" in captured.err + + +def test_warn_does_nothing_when_var_not_set( + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + _reset_warning_cache() + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + _warn_about_stripped_env(runner_label="claude-code", unset_env=["ANTHROPIC_API_KEY"]) + + assert capsys.readouterr().err == "" + + +class _FakeProcess: + def __init__(self, *, stdout: bytes, stderr: bytes, returncode: int) -> None: + self._stdout = stdout + self._stderr = stderr + self.returncode = returncode + + async def communicate(self, input: bytes | None = None) -> tuple[bytes, bytes]: + del input + return self._stdout, self._stderr + + def kill(self) -> None: + return None + + +@pytest.mark.asyncio +async def test_claude_runner_actually_strips_anthropic_key( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """End-to-end: ClaudeCodeRunner must not pass ANTHROPIC_API_KEY to claude.""" + _reset_warning_cache() + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + + captured_envs: list[dict[str, str]] = [] + + async def fake_create_subprocess_exec(*args: str, **kwargs: object) -> _FakeProcess: + del args + env = kwargs.get("env") + if isinstance(env, dict): + captured_envs.append(cast("dict[str, str]", env)) + return _FakeProcess( + stdout=b'{"result": "ok", "usage": {"input_tokens": 1, "output_tokens": 1}}', + stderr=b"", + returncode=0, + ) + + monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec) + + workspace = tmp_path / "workspace" + workspace.mkdir() + artifact = tmp_path / "artifact" + artifact.mkdir() + + runner = ClaudeCodeRunner() + provider = default_cli_providers()["claude-code"] + result = await runner.run( + prompt="hi", + workspace_dir=workspace, + artifact_dir=artifact, + provider=provider, + timeout=10, + ) + + assert result.error is None + assert captured_envs, "subprocess was not invoked" + forwarded = captured_envs[0] + assert "ANTHROPIC_API_KEY" not in forwarded, ( + "Claude runner must strip ANTHROPIC_API_KEY so subscription billing is preserved." + ) + + +def test_user_can_override_unset_env_to_keep_key(monkeypatch: pytest.MonkeyPatch) -> None: + """Power users running against a custom API endpoint may want to keep the key.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", "sk-ant-test") + provider = CliProviderConfig(command="claude", unset_env=[]) + env = _resolve_subprocess_env( + extra_env=provider.env or None, + unset_env=provider.unset_env or None, + ) + assert env["ANTHROPIC_API_KEY"] == "sk-ant-test" diff --git a/tests/test_cli_generate_benchmark.py b/tests/test_cli_generate_benchmark.py index 9554543..50246cd 100644 --- a/tests/test_cli_generate_benchmark.py +++ b/tests/test_cli_generate_benchmark.py @@ -1,7 +1,7 @@ from __future__ import annotations from types import SimpleNamespace -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast import pytest from click.testing import CliRunner @@ -31,6 +31,7 @@ ) if TYPE_CHECKING: + from collections.abc import Callable from pathlib import Path @@ -602,7 +603,8 @@ def fake_build_executor(name: str, **kwargs: object) -> object: async def fake_run_with_skill_benchmark(*args: object, **kwargs: object): del args - assert kwargs["executor"] is fake_executor + executor_factory = cast("Callable[[str], object]", kwargs["executor_factory"]) + assert executor_factory("haiku") is fake_executor num_runs = kwargs["num_runs"] max_parallel = kwargs["max_parallel"] assert isinstance(num_runs, int) diff --git a/tests/test_cli_help.py b/tests/test_cli_help.py new file mode 100644 index 0000000..fb818e2 --- /dev/null +++ b/tests/test_cli_help.py @@ -0,0 +1,32 @@ +"""Verify the public CLI help text mentions CLI agent providers.""" + +from __future__ import annotations + +from click.testing import CliRunner + +from upskill.cli import main + + +def test_eval_help_mentions_cli_providers() -> None: + runner = CliRunner() + result = runner.invoke(main, ["eval", "--help"]) + assert result.exit_code == 0 + assert "cli.claude-code" in result.output + assert "cli.copilot" in result.output + assert "cli.kiro" in result.output + + +def test_benchmark_help_mentions_cli_providers() -> None: + runner = CliRunner() + result = runner.invoke(main, ["benchmark", "--help"]) + assert result.exit_code == 0 + assert "cli.claude-code" in result.output + + +def test_generate_help_directs_users_for_cli_models() -> None: + runner = CliRunner() + result = runner.invoke(main, ["generate", "--help"]) + assert result.exit_code == 0 + # --eval-model is the v1 entry point for cli.* during generate. + assert "cli." in result.output or "cli.claude-code" in result.output + assert "--eval-model" in result.output diff --git a/tests/test_copilot_cli_runner.py b/tests/test_copilot_cli_runner.py new file mode 100644 index 0000000..b6924d8 --- /dev/null +++ b/tests/test_copilot_cli_runner.py @@ -0,0 +1,139 @@ +"""Unit tests for the Copilot CLI runner.""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli_agents import CliProviderConfig +from upskill.cli_agents.copilot import CopilotCliRunner + +if TYPE_CHECKING: + from pathlib import Path + + +def _make_provider(args: list[str] | None = None) -> CliProviderConfig: + return CliProviderConfig(command="copilot", args=args or [], timeout_seconds=60) + + +class _FakeProcess: + def __init__(self, *, stdout: bytes, stderr: bytes, returncode: int) -> None: + self._stdout = stdout + self._stderr = stderr + self.returncode = returncode + + async def communicate(self, input: bytes | None = None) -> tuple[bytes, bytes]: + del input + return self._stdout, self._stderr + + def kill(self) -> None: + return None + + +def _patch_subprocess( + monkeypatch: pytest.MonkeyPatch, + *, + stdout: bytes = b"", + stderr: bytes = b"", + returncode: int = 0, + raise_file_not_found: bool = False, + captured_argv: list[list[str]] | None = None, +) -> None: + async def fake_create_subprocess_exec(*args: str, **_: object) -> _FakeProcess: + if captured_argv is not None: + captured_argv.append(list(args)) + if raise_file_not_found: + raise FileNotFoundError(args[0]) + return _FakeProcess(stdout=stdout, stderr=stderr, returncode=returncode) + + monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec) + + +def _make_dirs(tmp_path: Path) -> tuple[Path, Path]: + workspace = tmp_path / "workspace" + workspace.mkdir() + artifact = tmp_path / "artifact" + artifact.mkdir() + return workspace, artifact + + +@pytest.mark.asyncio +async def test_copilot_runner_returns_stdout_and_marks_tokens_unavailable( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + captured: list[list[str]] = [] + _patch_subprocess( + monkeypatch, + stdout=b"Here is the answer.\n", + captured_argv=captured, + ) + + runner = CopilotCliRunner() + result = await runner.run( + prompt="explain rebase", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(args=["--allow-all-tools"]), + ) + + assert result.error is None + assert result.output_text == "Here is the answer." + assert result.stats.total_tokens == 0 + assert result.metadata["tokens_unavailable"] is True + assert captured == [["copilot", "-p", "explain rebase", "--allow-all-tools"]] + assert (artifact / "stdout.txt").exists() + + +@pytest.mark.asyncio +async def test_copilot_runner_reports_non_zero_exit( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess( + monkeypatch, + stdout=b"", + stderr=b"login required\n", + returncode=2, + ) + + runner = CopilotCliRunner() + result = await runner.run( + prompt="x", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.output_text is None + assert result.error is not None + assert "exited with code 2" in result.error + assert "gh auth login" in result.error + assert result.metadata["tokens_unavailable"] is True + + +@pytest.mark.asyncio +async def test_copilot_runner_handles_missing_binary( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess(monkeypatch, raise_file_not_found=True) + + runner = CopilotCliRunner() + result = await runner.run( + prompt="x", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.error is not None + assert "not found on PATH" in result.error + assert "gh auth login" in result.error + assert result.metadata["tokens_unavailable"] is True + assert result.return_code == -1 diff --git a/tests/test_executor_router.py b/tests/test_executor_router.py new file mode 100644 index 0000000..c56a4f2 --- /dev/null +++ b/tests/test_executor_router.py @@ -0,0 +1,114 @@ +"""Tests for the per-model executor router.""" + +from __future__ import annotations + +import pytest + +from upskill.cli_agents.claude_code import ClaudeCodeRunner +from upskill.cli_agents.copilot import CopilotCliRunner +from upskill.cli_agents.kiro import KiroCliRunner +from upskill.config import Config +from upskill.executors.cli_agent import CliAgentExecutor +from upskill.executors.local_fast_agent import LocalFastAgentExecutor +from upskill.executors.remote_fast_agent import RemoteFastAgentExecutor +from upskill.executors.router import ( + build_cli_executor, + build_default_cli_runners, + is_cli_model, + select_executor_for_model, +) +from upskill.hf_jobs import JobsConfig + + +def _local_config() -> Config: + return Config(executor="local") + + +def _jobs_config() -> JobsConfig: + return JobsConfig( + artifact_repo="user/repo", + wait=True, + jobs_timeout="1h", + jobs_flavor="cpu-basic", + jobs_secrets="HF_TOKEN", + jobs_namespace=None, + jobs_image="ghcr.io/example/image:latest", + ) + + +def test_is_cli_model_only_matches_cli_prefix() -> None: + assert is_cli_model("cli.claude-code") is True + assert is_cli_model("cli.copilot") is True + assert is_cli_model("cli.kiro") is True + assert is_cli_model("sonnet") is False + assert is_cli_model("openai.gpt-4") is False + assert is_cli_model("generic.llama3") is False + assert is_cli_model("clientside") is False # close miss but no `.` + + +def test_build_default_cli_runners_covers_v1_set() -> None: + runners = build_default_cli_runners() + assert isinstance(runners["claude-code"], ClaudeCodeRunner) + assert isinstance(runners["copilot"], CopilotCliRunner) + assert isinstance(runners["kiro"], KiroCliRunner) + + +def test_build_cli_executor_uses_config_providers() -> None: + config = Config() + config.cli_providers["claude-code"].args = ["--model", "opus"] + + executor = build_cli_executor(config) + + assert isinstance(executor, CliAgentExecutor) + + +@pytest.mark.parametrize("model", ["cli.claude-code", "cli.copilot", "cli.kiro"]) +def test_router_returns_cli_executor_for_cli_models(model: str) -> None: + executor = select_executor_for_model(model, config=_local_config()) + assert isinstance(executor, CliAgentExecutor) + + +def test_router_returns_local_executor_for_fast_agent_alias() -> None: + executor = select_executor_for_model("sonnet", config=_local_config()) + assert isinstance(executor, LocalFastAgentExecutor) + + +def test_router_returns_local_executor_for_provider_qualified_alias() -> None: + executor = select_executor_for_model("openai.gpt-4", config=_local_config()) + assert isinstance(executor, LocalFastAgentExecutor) + + +def test_router_returns_remote_executor_when_executor_jobs_is_set() -> None: + config = Config(executor="jobs") + executor = select_executor_for_model( + "sonnet", + config=config, + jobs_config=_jobs_config(), + ) + assert isinstance(executor, RemoteFastAgentExecutor) + + +def test_router_still_routes_cli_models_to_cli_executor_under_jobs_config() -> None: + """`executor: jobs` only governs fast-agent paths; CLI models stay local.""" + config = Config(executor="jobs") + executor = select_executor_for_model( + "cli.claude-code", + config=config, + jobs_config=_jobs_config(), + ) + assert isinstance(executor, CliAgentExecutor) + + +def test_router_requires_jobs_config_when_executor_jobs_and_non_cli_model() -> None: + config = Config(executor="jobs") + with pytest.raises(ValueError): + select_executor_for_model("sonnet", config=config) + + +def test_router_returns_clear_error_for_unknown_cli_provider() -> None: + """Unknown providers do not crash the router; ``CliAgentExecutor`` surfaces the error.""" + executor = select_executor_for_model("cli.unknown-tool", config=_local_config()) + assert isinstance(executor, CliAgentExecutor) + # The error path is exercised at `execute()` time and unit-tested in + # tests/test_cli_agent_executor.py — we only assert the router itself is + # tolerant here so a typo doesn't take the whole CLI down. diff --git a/tests/test_kiro_cli_runner.py b/tests/test_kiro_cli_runner.py new file mode 100644 index 0000000..51bd4e6 --- /dev/null +++ b/tests/test_kiro_cli_runner.py @@ -0,0 +1,150 @@ +"""Unit tests for the Kiro CLI runner.""" + +from __future__ import annotations + +import asyncio +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli_agents import CliProviderConfig +from upskill.cli_agents.kiro import KiroCliRunner + +if TYPE_CHECKING: + from pathlib import Path + + +def _make_provider(args: list[str] | None = None) -> CliProviderConfig: + return CliProviderConfig(command="kiro-cli", args=args or [], timeout_seconds=60) + + +class _FakeProcess: + def __init__(self, *, stdout: bytes, stderr: bytes, returncode: int) -> None: + self._stdout = stdout + self._stderr = stderr + self.returncode = returncode + + async def communicate(self, input: bytes | None = None) -> tuple[bytes, bytes]: + del input + return self._stdout, self._stderr + + def kill(self) -> None: + return None + + +def _patch_subprocess( + monkeypatch: pytest.MonkeyPatch, + *, + stdout: bytes = b"", + stderr: bytes = b"", + returncode: int = 0, + raise_file_not_found: bool = False, + captured_argv: list[list[str]] | None = None, +) -> None: + async def fake_create_subprocess_exec(*args: str, **_: object) -> _FakeProcess: + if captured_argv is not None: + captured_argv.append(list(args)) + if raise_file_not_found: + raise FileNotFoundError(args[0]) + return _FakeProcess(stdout=stdout, stderr=stderr, returncode=returncode) + + monkeypatch.setattr(asyncio, "create_subprocess_exec", fake_create_subprocess_exec) + + +def _make_dirs(tmp_path: Path) -> tuple[Path, Path]: + workspace = tmp_path / "workspace" + workspace.mkdir() + artifact = tmp_path / "artifact" + artifact.mkdir() + return workspace, artifact + + +@pytest.mark.asyncio +async def test_kiro_runner_returns_stdout_with_default_argv( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + captured: list[list[str]] = [] + _patch_subprocess(monkeypatch, stdout=b"Kiro response\n", captured_argv=captured) + + runner = KiroCliRunner() + result = await runner.run( + prompt="summarize this repo", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.error is None + assert result.output_text == "Kiro response" + assert result.metadata["tokens_unavailable"] is True + # The default argv must NOT include --trust-all-tools (opt-in only). + assert captured == [["kiro-cli", "chat", "--no-interactive", "summarize this repo"]] + + +@pytest.mark.asyncio +async def test_kiro_runner_includes_trust_all_tools_when_opted_in( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + captured: list[list[str]] = [] + _patch_subprocess(monkeypatch, stdout=b"ok\n", captured_argv=captured) + + runner = KiroCliRunner() + await runner.run( + prompt="task", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(args=["--trust-all-tools"]), + ) + + assert captured == [["kiro-cli", "chat", "--no-interactive", "--trust-all-tools", "task"]] + + +@pytest.mark.asyncio +async def test_kiro_runner_reports_non_zero_exit( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess( + monkeypatch, + stdout=b"", + stderr=b"not authenticated\n", + returncode=3, + ) + + runner = KiroCliRunner() + result = await runner.run( + prompt="x", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.error is not None + assert "exited with code 3" in result.error + assert "kiro-cli login" in result.error + + +@pytest.mark.asyncio +async def test_kiro_runner_handles_missing_binary( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + workspace, artifact = _make_dirs(tmp_path) + _patch_subprocess(monkeypatch, raise_file_not_found=True) + + runner = KiroCliRunner() + result = await runner.run( + prompt="x", + workspace_dir=workspace, + artifact_dir=artifact, + provider=_make_provider(), + ) + + assert result.error is not None + assert "not found on PATH" in result.error + assert "kiro-cli login" in result.error diff --git a/tests/test_mixed_model_eval.py b/tests/test_mixed_model_eval.py new file mode 100644 index 0000000..bd2779f --- /dev/null +++ b/tests/test_mixed_model_eval.py @@ -0,0 +1,213 @@ +"""Test that ``upskill eval`` and ``upskill benchmark`` route per model via the router.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from upskill.cli import _eval_async +from upskill.config import Config +from upskill.executors.cli_agent import CliAgentExecutor +from upskill.executors.local_fast_agent import LocalFastAgentExecutor +from upskill.models import ( + EvalResults, + ExpectedSpec, + Skill, + SkillRecord, + SkillState, + TestCase, + TestResult, + ValidationResult, +) + +if TYPE_CHECKING: + from pathlib import Path + + +def _write_skill_fixture(path: Path) -> SkillRecord: + record = SkillRecord( + skill=Skill( + name="example-skill", + description="An example skill.", + body="Always be concise.", + ), + state=SkillState( + tests=[ + TestCase(input="prompt", expected=ExpectedSpec(contains=["concise"])), + ], + ), + ) + record.save(path) + return record + + +def _make_eval_results(*, model: str, skill: Skill, test_cases: list[TestCase]) -> EvalResults: + with_skill = [ + TestResult( + test_case=tc, + success=True, + output="concise answer", + tokens_used=10, + turns=1, + validation_result=ValidationResult( + passed=True, + assertions_passed=1, + assertions_total=1, + ), + ) + for tc in test_cases + ] + return EvalResults( + skill_name=skill.name, + model=model, + with_skill_results=with_skill, + baseline_results=[], + with_skill_success_rate=1.0, + baseline_success_rate=0.0, + ) + + +@pytest.mark.asyncio +async def test_mixed_model_benchmark_routes_per_model( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Mixed CLI + fast-agent benchmark must hit the right executor per model.""" + config = Config( + runs_dir=tmp_path / "runs", + fastagent_config=tmp_path / "fastagent.config.yaml", + ) + skill_record = _write_skill_fixture(tmp_path / "skill") + monkeypatch.setattr("upskill.cli.Config.load", lambda: config) + + executor_models: list[tuple[str, str]] = [] + + async def fake_evaluate_skill(*args: object, **kwargs: object) -> EvalResults: + del args + executor = kwargs["executor"] + model = str(kwargs["model"]) + executor_models.append((model, type(executor).__name__)) + return _make_eval_results( + skill=skill_record.skill, + model=model, + test_cases=skill_record.state.tests, + ) + + monkeypatch.setattr("upskill.cli.evaluate_skill", fake_evaluate_skill) + monkeypatch.setattr( + "upskill.cli._fast_agent_context", + lambda *_args, **_kwargs: _NoopAgentContext(), + ) + + await _eval_async( + skill_path=str(tmp_path / "skill"), + tests=None, + models=["sonnet", "cli.claude-code", "cli.kiro"], + test_gen_model=None, + num_runs=1, + no_baseline=False, + verbose=False, + executor_name="local", + artifact_repo=None, + wait=True, + jobs_timeout="2h", + jobs_flavor="cpu-basic", + jobs_secrets="HF_TOKEN", + jobs_namespace=None, + max_parallel=1, + log_runs=False, + runs_dir=str(config.runs_dir), + ) + + # Each model should have been routed to the right executor type. + assert ("sonnet", LocalFastAgentExecutor.__name__) in executor_models + assert ("cli.claude-code", CliAgentExecutor.__name__) in executor_models + assert ("cli.kiro", CliAgentExecutor.__name__) in executor_models + + +@pytest.mark.asyncio +async def test_eval_with_cli_test_gen_model_skips_fast_agent( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """When test_gen is cli.*, _agent_session must skip _fast_agent_context. + + Concretely, this proves you can run ``upskill eval -m cli.claude-code + --test-gen-model cli.claude-code`` with no Anthropic/OpenAI key set. + """ + config = Config( + runs_dir=tmp_path / "runs", + fastagent_config=tmp_path / "fastagent.config.yaml", + ) + skill_record = _write_skill_fixture(tmp_path / "skill") + monkeypatch.setattr("upskill.cli.Config.load", lambda: config) + + fast_agent_calls: list[bool] = [] + + def boom(*_args: object, **_kwargs: object) -> object: + fast_agent_calls.append(True) + raise AssertionError("_fast_agent_context must not run when all models are cli.*") + + monkeypatch.setattr("upskill.cli._fast_agent_context", boom) + + # Drop persisted tests so test generation actually triggers. + monkeypatch.setattr( + "upskill.models.SkillRecord.load", + lambda path: skill_record.model_copy(deep=True), + ) + + captured: list[str] = [] + + async def fake_evaluate_skill(*_args: object, **kwargs: object) -> EvalResults: + captured.append(str(kwargs["model"])) + return _make_eval_results( + skill=skill_record.skill, + model=str(kwargs["model"]), + test_cases=skill_record.state.tests, + ) + + async def fake_generate_tests(_task: str, *, generator: object) -> list[TestCase]: + del generator + return list(skill_record.state.tests) + + monkeypatch.setattr("upskill.cli.evaluate_skill", fake_evaluate_skill) + monkeypatch.setattr("upskill.cli.generate_tests", fake_generate_tests) + + await _eval_async( + skill_path=str(tmp_path / "skill"), + tests=None, + models=["cli.claude-code"], + test_gen_model="cli.claude-code", + num_runs=1, + no_baseline=True, + verbose=False, + executor_name="local", + artifact_repo=None, + wait=True, + jobs_timeout="2h", + jobs_flavor="cpu-basic", + jobs_secrets="HF_TOKEN", + jobs_namespace=None, + max_parallel=1, + log_runs=False, + runs_dir=str(config.runs_dir), + ) + + assert fast_agent_calls == [] # never reached + assert captured == ["cli.claude-code"] + + +class _NoopAgentContext: + """Minimal stand-in for ``_fast_agent_context`` when only eval is exercised.""" + + async def __aenter__(self) -> object: + return _NoopSession() + + async def __aexit__(self, *_args: object) -> bool: + return False + + +class _NoopSession: + skill_gen = object() + test_gen = object()