From 2742372b6bc877b1305b211e329252bf1d7fe57d Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:21:18 -0700 Subject: [PATCH 1/2] fix: read Windows-authored agent .md + credentials.json as UTF-8 Two Windows-only text-decoding gaps in the CLI/lib. Both are silent or misleading failures rather than clean errors. 1. spawn.hydrate_agent_overlay read the agent .md as encoding="utf-8". A file authored on Windows (Notepad, PowerShell Out-File/Set-Content) is UTF-8 WITH a BOM, so the retained leading U+FEFF makes `text.startswith("---")` False and the ENTIRE file is silently treated as a plain instruction -- tools, hooks, model_role, and meta are dropped from the sub-agent overlay with no error or warning. A Windows-authored custom agent then spawns with the wrong (or no) tools. Now reads encoding="utf-8-sig" (strips a leading BOM if present; identical to utf-8 otherwise). 2. auth._load_credentials read credentials.json with a bare read_text() (no encoding). On Windows that decodes with cp1252. Three distinct problems, all now closed: - A BOM-prefixed credentials.json (same Windows tooling as above) decoded into a string with a leading U+FEFF, and json.loads then failed with "Unexpected UTF-8 BOM (decode using utf-8-sig)" -- reported to the user as "not valid JSON", which is both wrong and unactionable. Now reads encoding="utf-8-sig", matching hydrate_agent_overlay. - Bytes that are genuinely not valid UTF-8 (a cp1252-era file from an older build) raised UnicodeDecodeError, which the surrounding try caught only json.JSONDecodeError for. It propagated uncaught and crashed `auth list/status/set` plus every provider credential lookup behind `run`/`models`/`serve` with a raw traceback. Now caught and re-raised as a click.ClickException naming the file and the remediation. - The atomic write is pinned to encoding="utf-8" for symmetry, so we never author a non-UTF-8 credentials file ourselves. No-op on POSIX (utf-8 and utf-8-sig are identical for BOM-less files). Verified behaviorally against the real functions -- 13/13 checks, covering: BOM agent .md yields tools/model_role/meta and a frontmatter-free instruction; BOM-less agent .md parses identically (no regression); BOM + non-ASCII body decodes correctly; invalid-UTF-8 credentials raise ClickException with the file path and `auth clear --force` hint; BOM-prefixed credentials.json now loads; valid and malformed JSON both behave exactly as before. Fast gate clean: ruff check, ruff format --check, pyright src/ (0 errors). Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- src/amplifier_agent_cli/admin/auth.py | 32 ++++++++++++++++++++++++--- src/amplifier_agent_lib/spawn.py | 8 ++++++- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/amplifier_agent_cli/admin/auth.py b/src/amplifier_agent_cli/admin/auth.py index f2288c85..3aef4d50 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) 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("---"): From 1ff58a6bf2e90db1a42eef29d0da33fbc9d34bc3 Mon Sep 17 00:00:00 2001 From: sadlilas <11658960+sadlilas@users.noreply.github.com> Date: Tue, 18 Aug 2026 18:48:48 -0700 Subject: [PATCH 2/2] fix: surface unreadable credentials file instead of degrading silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a credentials file is not valid UTF-8, the auth command now raises a clean ClickException with remediation guidance. However, the resolver functions (used by 'auth list' and other credential lookups) deliberately never raise — a single bad file must not brick every subsequent invocation — so they caught that exception and logged it at DEBUG level only. In practice, this rendered the fix invisible: users saw every provider as with no stderr output, making them believe their credentials were lost rather than unreadable. This commit keeps the never-raise contract but makes the degradation visible. A new _warn_credentials_unreadable_once() helper emits the remediation hint to stderr exactly once per process, guarded by a module-level latch. Both resolve functions now route their caught ClickException through it. The latch prevents duplicate warnings when resolving credentials for multiple providers (a single 'auth list' can resolve 10+ times). Stderr keeps stdout clean for callers that parse it. Result: a corrupt credentials file now exits cleanly (0) with exactly one diagnostic line on stderr, carries the file path and parse failure for troubleshooting, and correctly degrades providers to — visible and actionable, rather than silent and misleading. Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> --- src/amplifier_agent_cli/admin/auth.py | 46 +++++++++++++++++++++------ 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/src/amplifier_agent_cli/admin/auth.py b/src/amplifier_agent_cli/admin/auth.py index 3aef4d50..105b63e0 100644 --- a/src/amplifier_agent_cli/admin/auth.py +++ b/src/amplifier_agent_cli/admin/auth.py @@ -185,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. @@ -199,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 {} @@ -217,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 {}