Skip to content

Commit 3697de5

Browse files
call-me-ramhaofeif
andauthored
fix(install): honor frontmatter provider with flag > frontmatter > default precedence (fixes #414) (#431)
`cao install <profile.md>` silently ignored the profile's frontmatter `provider:` key: the Click --provider option defaulted to kiro_cli and install_service.install_agent branched on that argument alone, so a profile declaring `provider: codex` installed as a kiro_cli agent with exit 0 and no warning. Install was the only entry point ignoring frontmatter — launch/assign/handoff already resolve it via resolve_provider(). The install provider now resolves with the same precedence as the launch paths: explicit --provider flag > frontmatter `provider:` > DEFAULT_PROVIDER. - CLI: --provider defaults to None; help text documents the precedence; the summary line echoes the provider the service actually resolved. - install_service.install_agent: accepts provider=None; explicit bad providers still fail fast BEFORE any URL download or env mutation; a bogus frontmatter provider logs a warning (same wording as resolve_provider) and falls back to the default. InstallResult gains a `provider` field carrying the resolved winner. - API: InstallAgentProfileRequest.provider defaults to None. - ops-MCP install_profile: provider defaults to None and is omitted from the request body when not explicit, so all three entry points behave identically. Built-in store profiles carry no frontmatter provider and keep today's default-provider behavior. Test matrix: flag wins over frontmatter; frontmatter wins when the flag is absent; both absent -> DEFAULT_PROVIDER; invalid frontmatter provider warns and falls back; explicit invalid --provider fails before download; built-in (no frontmatter) keeps the default; API and ops-MCP forward None for frontmatter resolution. Co-authored-by: haofeif <56006724+haofeif@users.noreply.github.com>
1 parent 5270588 commit 3697de5

9 files changed

Lines changed: 262 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
12+
- honor profile frontmatter `provider:` during install (flag > frontmatter > default) (#414)
1013
### Security
1114

1215
- clear the `py/clear-text-storage-sensitive-data` CodeQL false positive (code-scanning alert #142) by renaming the local `secret` fixture variable in two `memory_service` secret-gate tests to `gated_content`. CodeQL's name-based heuristic classified the variable named `secret` as a sensitive-data source and traced it into the memory-wiki write (an intentionally plaintext, by-design markdown sink). The literal is the canonical AWS documentation example key, not a real credential; the value and all assertions are unchanged, so the federated secret-gate rejection and global-scope allow paths are still exercised exactly. No production behavior change

src/cli_agent_orchestrator/api/main.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,10 +397,14 @@ class InstallAgentProfileRequest(BaseModel):
397397
398398
``env_vars`` travels in the JSON body rather than as a query parameter so
399399
that any secrets callers inject are not written to HTTP access logs.
400+
401+
``provider`` may be omitted (None): the install service then honours the
402+
profile's frontmatter ``provider:`` key, falling back to the default
403+
provider — the same flag > frontmatter > default precedence as the CLI.
400404
"""
401405

402406
source: str
403-
provider: str = DEFAULT_PROVIDER
407+
provider: Optional[str] = None
404408
env_vars: Optional[Dict[str, str]] = None
405409

406410

src/cli_agent_orchestrator/cli/commands/install.py

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,11 @@ def _copy_local_profile_to_store(agent_source: str) -> Optional[str]:
6464
@click.option(
6565
"--provider",
6666
type=click.Choice(PROVIDERS),
67-
default=DEFAULT_PROVIDER,
68-
help=f"Provider to use (default: {DEFAULT_PROVIDER})",
67+
default=None,
68+
help=(
69+
"Provider to use. Precedence: this flag > the profile's frontmatter "
70+
f"'provider:' key > default ({DEFAULT_PROVIDER})."
71+
),
6972
)
7073
@click.option(
7174
"--env",
@@ -77,7 +80,7 @@ def _copy_local_profile_to_store(agent_source: str) -> Optional[str]:
7780
"Repeatable: --env KEY=VALUE. Example: --env API_TOKEN=my-secret-token."
7881
),
7982
)
80-
def install(agent_source: str, provider: str, env_vars: tuple[str, ...]) -> None:
83+
def install(agent_source: str, provider: Optional[str], env_vars: tuple[str, ...]) -> None:
8184
"""
8285
Install an agent from local store, built-in store, URL, or file path.
8386
@@ -138,4 +141,8 @@ def install(agent_source: str, provider: str, env_vars: tuple[str, ...]) -> None
138141
if result.context_file:
139142
click.echo(f"✓ Context file: {result.context_file}")
140143
if result.agent_file:
141-
click.echo(f"✓ {provider} agent: {result.agent_file}")
144+
# The service resolves flag > frontmatter > default; result.provider
145+
# carries the winner (older mocks may omit it, hence the fallback).
146+
click.echo(
147+
f"✓ {result.provider or provider or DEFAULT_PROVIDER} agent: {result.agent_file}"
148+
)

src/cli_agent_orchestrator/ops_mcp_server/server.py

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from fastmcp import FastMCP
77
from pydantic import Field
88

9-
from cli_agent_orchestrator.constants import API_BASE_URL, DEFAULT_PROVIDER
9+
from cli_agent_orchestrator.constants import API_BASE_URL
1010
from cli_agent_orchestrator.ops_mcp_server.models import (
1111
InstallResult,
1212
LaunchResult,
@@ -194,9 +194,14 @@ async def get_profile_details(
194194
async def install_profile(
195195
source: Annotated[str, Field(description="Agent name or https:// URL to install")],
196196
provider: Annotated[
197-
str,
198-
Field(description="Target provider for the installed profile"),
199-
] = DEFAULT_PROVIDER,
197+
Optional[str],
198+
Field(
199+
description=(
200+
"Target provider for the installed profile. Omit to honour the "
201+
"profile's frontmatter provider, falling back to the default."
202+
)
203+
),
204+
] = None,
200205
env_vars: Annotated[
201206
Optional[Dict[str, str]],
202207
Field(description="Optional environment variables to inject before install"),
@@ -224,13 +229,16 @@ async def install_profile(
224229
225230
Args:
226231
source: Agent name or https:// URL from an allow-listed host
227-
provider: Target provider (default: kiro_cli)
232+
provider: Target provider. Precedence: explicit value > the profile's
233+
frontmatter ``provider:`` key > the server default (kiro_cli)
228234
env_vars: Optional env vars written to the managed .env before install
229235
230236
Returns:
231237
InstallResult with success status, file paths, and unresolved env vars
232238
"""
233-
body: Dict[str, Any] = {"source": source, "provider": provider}
239+
body: Dict[str, Any] = {"source": source}
240+
if provider is not None:
241+
body["provider"] = provider
234242
if env_vars:
235243
body["env_vars"] = env_vars
236244

src/cli_agent_orchestrator/services/install_service.py

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""Service helpers for installing agent profiles."""
22

3+
import logging
34
import os
45
import re
56
from pathlib import Path
@@ -13,6 +14,7 @@
1314
from cli_agent_orchestrator.constants import (
1415
AGENT_CONTEXT_DIR,
1516
COPILOT_AGENTS_DIR,
17+
DEFAULT_PROVIDER,
1618
KIRO_AGENTS_DIR,
1719
LOCAL_AGENT_STORE_DIR,
1820
OPENCODE_AGENTS_DIR,
@@ -40,6 +42,8 @@
4042
from cli_agent_orchestrator.utils.skill_injection import compose_agent_prompt
4143
from cli_agent_orchestrator.utils.tool_mapping import resolve_allowed_tools
4244

45+
logger = logging.getLogger(__name__)
46+
4347

4448
class InstallResult(BaseModel):
4549
"""Structured result for agent profile installation."""
@@ -51,6 +55,7 @@ class InstallResult(BaseModel):
5155
agent_file: Optional[str] = None
5256
unresolved_vars: Optional[List[str]] = None
5357
source_kind: Optional[Literal["url", "file", "name"]] = None
58+
provider: Optional[str] = None
5459

5560

5661
# Profile names are used as filesystem path segments under LOCAL_AGENT_STORE_DIR
@@ -232,11 +237,16 @@ def _build_provider_config(
232237

233238
def install_agent(
234239
source: str,
235-
provider: str,
240+
provider: Optional[str] = None,
236241
env_vars: Optional[Dict[str, str]] = None,
237242
) -> InstallResult:
238243
"""Install an agent profile for the requested provider.
239244
245+
``provider`` resolution follows the same precedence as launch/handoff
246+
(see ``resolve_provider``): an explicit argument wins, then the profile's
247+
frontmatter ``provider:`` key, then ``DEFAULT_PROVIDER``. Pass ``None``
248+
to defer to the profile.
249+
240250
``source`` must be either an https:// URL on the allowlist or a bare
241251
profile name matching ``_PROFILE_NAME_RE``. Local ``.md`` file paths
242252
are deliberately NOT accepted here — the CLI copies user files into
@@ -247,7 +257,10 @@ def install_agent(
247257
"""
248258
try:
249259
valid_providers = [provider_type.value for provider_type in ProviderType]
250-
if provider not in valid_providers:
260+
# An explicit provider is validated up front so bad input fails fast
261+
# BEFORE any URL download or env-file mutation. Frontmatter providers
262+
# are validated after the profile is parsed (below).
263+
if provider is not None and provider not in valid_providers:
251264
return InstallResult(
252265
success=False,
253266
message=(
@@ -284,6 +297,25 @@ def install_agent(
284297
resolved_content = resolve_env_vars(raw_content)
285298
profile = parse_agent_profile_text(resolved_content, agent_name)
286299

300+
# No explicit provider — honour the profile's frontmatter ``provider:``
301+
# key, mirroring resolve_provider() on the launch/handoff paths. Bogus
302+
# frontmatter values warn and fall back to the default; built-in store
303+
# profiles carry no frontmatter provider and keep the default.
304+
if provider is None:
305+
if profile.provider and profile.provider in valid_providers:
306+
provider = profile.provider
307+
else:
308+
if profile.provider:
309+
logger.warning(
310+
"Agent profile '%s' has invalid provider '%s'. "
311+
"Valid providers: %s. Falling back to '%s'.",
312+
profile.name,
313+
profile.provider,
314+
valid_providers,
315+
DEFAULT_PROVIDER,
316+
)
317+
provider = DEFAULT_PROVIDER
318+
287319
# Resolve the bundled cao-mcp-server console script to a PATH-independent
288320
# invocation before materializing provider configs. The
289321
# configs Kiro/Q write to disk are consumed verbatim by those CLIs, so
@@ -411,6 +443,7 @@ def install_agent(
411443
agent_file=str(agent_file) if agent_file else None,
412444
unresolved_vars=unresolved_vars or None,
413445
source_kind=source_kind,
446+
provider=provider,
414447
)
415448

416449
except requests.RequestException as exc:

test/api/test_api_profiles.py

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,34 @@ def test_returns_install_result(self, client) -> None:
106106
},
107107
)
108108

109+
def test_omitted_provider_forwards_none_for_frontmatter_resolution(self, client) -> None:
110+
"""Requests without a provider should forward None so the service
111+
resolves the profile's frontmatter provider (flag > frontmatter >
112+
default precedence, GH #414) — identically to the CLI."""
113+
service_result = InstallResult(
114+
success=True,
115+
message="Agent 'developer' installed successfully",
116+
agent_name="developer",
117+
provider="claude_code",
118+
)
119+
120+
with patch(
121+
"cli_agent_orchestrator.api.main.install_agent",
122+
return_value=service_result,
123+
) as mock_install:
124+
response = client.post(
125+
"/agents/profiles/install",
126+
json={"source": "developer"},
127+
)
128+
129+
assert response.status_code == 200
130+
assert response.json()["provider"] == "claude_code"
131+
mock_install.assert_called_once_with(
132+
source="developer",
133+
provider=None,
134+
env_vars=None,
135+
)
136+
109137
def test_returns_400_for_invalid_source(self, client) -> None:
110138
"""Structured service failures should be surfaced as 400s."""
111139
with patch(

test/cli/commands/test_install.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,41 @@ def test_install_success_outputs_result_details(self, runner: CliRunner) -> None
7070
{"API_TOKEN": "secret-token"},
7171
)
7272

73+
def test_install_without_provider_flag_passes_none_and_echoes_resolved_provider(
74+
self, runner: CliRunner
75+
) -> None:
76+
"""Omitting --provider should pass None so the service resolves frontmatter.
77+
78+
The final summary line must echo the provider the service actually
79+
resolved (flag > frontmatter > default), not the CLI default.
80+
"""
81+
service_result = InstallResult(
82+
success=True,
83+
message="Agent 'developer' installed successfully",
84+
agent_name="developer",
85+
context_file="/tmp/agent-context/developer.md",
86+
agent_file="/tmp/copilot/developer.agent.md",
87+
source_kind="name",
88+
provider="copilot_cli",
89+
)
90+
91+
with patch(
92+
"cli_agent_orchestrator.cli.commands.install.install_agent",
93+
return_value=service_result,
94+
) as mock_install:
95+
result = runner.invoke(install, ["developer"])
96+
97+
assert result.exit_code == 0
98+
assert "copilot_cli agent: /tmp/copilot/developer.agent.md" in result.output
99+
mock_install.assert_called_once_with("developer", None, None)
100+
101+
def test_install_help_documents_provider_precedence(self, runner: CliRunner) -> None:
102+
"""--provider help text should document flag > frontmatter > default."""
103+
result = runner.invoke(install, ["--help"])
104+
105+
assert result.exit_code == 0
106+
assert "frontmatter" in result.output
107+
73108
def test_install_url_source_prints_download_confirmation(self, runner: CliRunner) -> None:
74109
"""URL installs should print a download confirmation line."""
75110
service_result = InstallResult(

test/ops_mcp_server/test_server.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,31 @@ async def test_install_profile_returns_result_for_url_source(self) -> None:
184184
json={"source": "https://example.com/remote.md", "provider": "kiro_cli"},
185185
)
186186

187+
async def test_install_profile_omits_provider_when_not_explicit(self) -> None:
188+
"""Omitted provider should be left out of the body so the install API
189+
resolves the profile's frontmatter provider (GH #414)."""
190+
payload: InstallPayload = {
191+
"success": True,
192+
"message": "Agent 'developer' installed successfully",
193+
"agent_name": "developer",
194+
"context_file": "/tmp/developer.md",
195+
"agent_file": None,
196+
"unresolved_vars": None,
197+
}
198+
with patch(
199+
"cli_agent_orchestrator.ops_mcp_server.server.requests.request",
200+
return_value=_response(json_data=payload),
201+
) as mock_request:
202+
result = await install_profile("developer")
203+
204+
assert result == InstallResult(**payload)
205+
mock_request.assert_called_once_with(
206+
"post",
207+
"http://127.0.0.1:9889/agents/profiles/install",
208+
params=None,
209+
json={"source": "developer"},
210+
)
211+
187212
async def test_install_profile_forwards_env_vars(self) -> None:
188213
"""Env var maps should be forwarded to the API install endpoint."""
189214
payload = {

0 commit comments

Comments
 (0)