diff --git a/src/amplifier_agent_cli/admin/auth.py b/src/amplifier_agent_cli/admin/auth.py index f2288c85..105b63e0 100644 --- a/src/amplifier_agent_cli/admin/auth.py +++ b/src/amplifier_agent_cli/admin/auth.py @@ -95,13 +95,39 @@ def _load_credentials() -> dict[str, Any]: Tolerant of legacy shapes: if the existing file is missing the ``version`` envelope, treat the whole body as the ``providers`` dict and silently upgrade on next write. Raises ``click.ClickException`` - on JSON-decode failure with a clear remediation hint. + with a clear remediation hint on either failure mode -- bytes that + are not valid UTF-8, or text that is not valid JSON. Neither is + allowed to surface as a raw traceback. """ path = credentials_path() if not path.exists(): return {"version": CREDENTIALS_VERSION, "providers": {}} try: - data = json.loads(path.read_text() or "{}") + # encoding="utf-8-sig", matching hydrate_agent_overlay: a file touched + # by Windows tooling (Notepad's "UTF-8 with BOM", Windows PowerShell + # 5.1 Set-Content/Out-File) is UTF-8 WITH a BOM. Read as plain "utf-8" + # the leading U+FEFF survives into the string and json.loads then dies + # with "Unexpected UTF-8 BOM (decode using utf-8-sig)" -- surfaced to + # the user as "not valid JSON", which is both wrong and unactionable. + # utf-8-sig strips a leading BOM if present and is identical to utf-8 + # for files without one. We always write plain utf-8 (_atomic_write), + # so this only ever forgives a file some other tool re-saved. + raw = path.read_text(encoding="utf-8-sig") + except UnicodeDecodeError as exc: + # Reachable whenever the bytes on disk are not valid UTF-8: a file + # written by an older build under a non-UTF-8 locale default (Windows + # cp1252), hand-edited in a legacy encoding, or truncated mid-sequence. + # Without this branch the UnicodeDecodeError escapes uncaught and + # every `auth` command -- plus each provider credential lookup behind + # `run`/`models`/`serve` -- dies with a raw traceback. + raise click.ClickException( + f"Credentials file at {path} is not valid UTF-8 ({exc}). " + "It was likely written by an older build under a non-UTF-8 locale. " + "Re-save the file as UTF-8, or clear all stored credentials with " + "`amplifier-agent auth clear --force` and re-add them." + ) from exc + try: + data = json.loads(raw or "{}") except json.JSONDecodeError as exc: raise click.ClickException( f"Credentials file at {path} is not valid JSON ({exc}). " @@ -139,7 +165,7 @@ def _atomic_write(path: Path, payload: dict[str, Any]) -> None: pass tmp_path = path.with_suffix(path.suffix + ".tmp") - tmp_path.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n") + tmp_path.write_text(json.dumps(payload, indent=2, sort_keys=True) + "\n", encoding="utf-8") os.chmod(tmp_path, CREDENTIALS_FILE_MODE) os.replace(tmp_path, path) @@ -159,13 +185,43 @@ def _save_credentials(data: dict[str, Any]) -> Path: # Resolver helper (consumed by provider_sources._resolve_env_credential) # --------------------------------------------------------------------------- +# Module-level latch for _warn_credentials_unreadable_once. A single command +# resolves credentials once per provider (and Azure-style providers resolve an +# extra endpoint field), so an unlatched warning would print a dozen identical +# lines for one broken file. +_credentials_warning_emitted = False + + +def _warn_credentials_unreadable_once(exc: click.ClickException) -> None: + """Surface an unreadable credentials file to the user exactly once. + + The resolvers below deliberately never raise: one bad write must not + brick every subsequent invocation. But degrading silently is its own + failure -- with only a DEBUG log, a corrupt file makes every provider + report ```` and exit 0, which reads as "no credentials + configured" rather than "your credentials file is broken." The user + then re-adds keys that were never actually lost. + + So: resilient *and* visible. Emit the remediation hint (which carries + the path and the specific parse failure) once per process on stderr, + leaving stdout clean for callers that parse it. + """ + global _credentials_warning_emitted + logger.debug("credentials.json unreadable; resolving as empty (%s)", exc.message) + if _credentials_warning_emitted: + return + _credentials_warning_emitted = True + click.echo(f"Warning: {exc.message}", err=True) + def resolve_credential_from_file(provider_name: str) -> str: """Look up ``provider_name``'s ``api_key`` in the credentials file. Returns ``""`` if no file exists or the entry is missing. Never - raises -- a malformed file is logged at DEBUG and treated as empty, - so a one-time bad write doesn't break every subsequent invocation. + raises -- a malformed file is treated as empty so a one-time bad + write doesn't break every subsequent invocation -- but the failure is + reported once per process via :func:`_warn_credentials_unreadable_once` + so the degradation is never silent. The caller (``_resolve_env_credential``) chains this AFTER the env var lookup so shell env always wins. @@ -173,11 +229,7 @@ def resolve_credential_from_file(provider_name: str) -> str: try: data = _load_credentials() except click.ClickException as exc: - logger.debug( - "credentials.json unreadable; resolving %r as empty (%s)", - provider_name, - exc.message, - ) + _warn_credentials_unreadable_once(exc) return "" providers = data.get("providers") or {} entry = providers.get(provider_name) or {} @@ -191,11 +243,13 @@ def resolve_field_from_file(provider_name: str, field: str) -> str: """Read an arbitrary string field for a provider entry. Used by Azure-style providers that store endpoint URLs alongside - the api_key. Returns ``""`` when absent. + the api_key. Returns ``""`` when absent. Shares the never-raise / + warn-once contract of :func:`resolve_credential_from_file`. """ try: data = _load_credentials() - except click.ClickException: + except click.ClickException as exc: + _warn_credentials_unreadable_once(exc) return "" providers = data.get("providers") or {} entry = providers.get(provider_name) or {} diff --git a/src/amplifier_agent_lib/spawn.py b/src/amplifier_agent_lib/spawn.py index 0164ad09..c24f7416 100644 --- a/src/amplifier_agent_lib/spawn.py +++ b/src/amplifier_agent_lib/spawn.py @@ -102,7 +102,13 @@ def hydrate_agent_overlay(agent_md_path: Path) -> dict[str, Any]: """ import yaml - text = agent_md_path.read_text(encoding="utf-8") + # encoding="utf-8-sig": a Windows-authored agent .md (Notepad, PowerShell + # Out-File/Set-Content) is UTF-8 WITH a BOM. Read as plain "utf-8" the + # leading U+FEFF survives, so `text.startswith("---")` is False and the + # whole file is silently treated as a plain instruction -- tools, hooks, + # model_role, meta are dropped with no error. utf-8-sig strips a leading BOM + # if present and is identical to utf-8 for files without one. + text = agent_md_path.read_text(encoding="utf-8-sig") # Split on the YAML frontmatter delimiters: ---\nYAML\n--- if not text.startswith("---"):