From 2922ce559fab76e89483db707f728d0c61a8ba40 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Sat, 22 Aug 2026 16:29:54 -0700 Subject: [PATCH 1/6] feat(gh_cli): resolve the gh binary, inject the config token, classify errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolve_gh(): shutil.which first, then /opt/homebrew/bin, /usr/local/bin, ~/.local/bin, /usr/bin (a desktop build launched without a shell PATH still finds a brew/apt gh); cached per process, re-resolved on a stale path. - set_token_getter(): the plugin's github.token secret is injected into every gh run as GH_TOKEN/GITHUB_TOKEN, winning over an ambient env token; read live so a token pasted in Settings works without a restart. - check_gh_error() now CLASSIFIES: gh exit 4 / 'gh auth login' stderr → not authenticated (with the fix); GraphQL could-not-resolve / HTTP 404 → repo not found; 403/429 + rate limit → rate limit hit (retry-after when gh says); binary missing → not installed (looked in …). Everything else keeps the 'Error (gh exit N): …' shape existing callers match on. - parse_json()/dicts(): the one place a gh JSON body is type-checked, so a dict-for-a-file / bare scalar can never AttributeError through a tool. Co-Authored-By: Claude Fable 5 --- gh_cli.py | 229 +++++++++++++++++++++++++++++++++++++++---- tests/test_gh_cli.py | 185 ++++++++++++++++++++++++++++++++++ 2 files changed, 396 insertions(+), 18 deletions(-) create mode 100644 tests/test_gh_cli.py diff --git a/gh_cli.py b/gh_cli.py index af68599..d592f5e 100644 --- a/gh_cli.py +++ b/gh_cli.py @@ -1,9 +1,22 @@ """Async `gh` CLI runner — vendored so the plugin is host-free. -A thin wrapper around the GitHub CLI: timeout + kill, missing-binary detection, and -token injection. Auth: if GITHUB_TOKEN (or GH_TOKEN) is set it's injected into the -subprocess env; otherwise `gh` uses its own ambient auth (`gh auth login`). No token -is required for public-repo reads at low volume. +A thin wrapper around the GitHub CLI: binary resolution, timeout + kill, +missing-binary detection, token injection, and error CLASSIFICATION. + +Binary: ``gh`` is resolved via ``shutil.which`` and, when PATH doesn't carry it +(a Linux desktop build launched from a .desktop file has no shell PATH +augmentation; macOS GUI apps likewise), a scan of the usual install dirs +(``/opt/homebrew/bin``, ``/usr/local/bin``, ``~/.local/bin``, ``/usr/bin``). The +resolved path is cached per process. + +Auth, in precedence order: the plugin's ``github.token`` secret (Settings ▸ GitHub, +set via ``set_token_getter`` at register time) wins over an ambient +``GITHUB_TOKEN`` / ``GH_TOKEN`` env var, which wins over ``gh``'s own keyring +login (``gh auth login``). No token is required for public-repo reads at low volume. + +Errors: ``check_gh_error`` turns a failed run into ONE readable ``Error: ...`` +string the model (or a person) can act on — not-authenticated, repo-not-found, +rate-limited, binary-missing — instead of raw stderr. Adapted from protoAgent's tools/gh_cli.py (which was adapted from the quinn fleet). """ @@ -11,8 +24,13 @@ from __future__ import annotations import asyncio +import json import os import re +import shutil +from collections.abc import Callable +from pathlib import Path +from typing import Any _COMMAND_TIMEOUT = 30 @@ -20,6 +38,15 @@ # silent default (a forgotten repo must error, not fire at the wrong repository). REPO_RE = re.compile(r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$") +# `gh`'s own exit code for "you need to authenticate" (cli/cli: exitcode.AuthRequired = 4). +GH_EXIT_AUTH_REQUIRED = 4 + +# Where `gh` lands when a package manager installs it but the launching process has +# no shell PATH (desktop builds). Scanned in this order AFTER `shutil.which`. +_FALLBACK_BIN_DIRS = ("/opt/homebrew/bin", "/usr/local/bin", "~/.local/bin", "/usr/bin") + +_AUTH_HINT = "run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)" + def bad_repo(repo: str) -> str | None: """Validate an `owner/name` repo slug; return an Error string if invalid, else None.""" @@ -31,26 +58,113 @@ def bad_repo(repo: str) -> str | None: return None -def _resolve_token() -> str | None: - return os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN") or None +# ── binary resolution ───────────────────────────────────────────────────────────── +_gh_path: str | None = None +_gh_resolved = False -async def run_gh(args: list[str], timeout: int = _COMMAND_TIMEOUT) -> tuple[int, str, str]: - """Run a `gh` command, returning (returncode, stdout, stderr). Times out (killing - the process), and reports a clean error when `gh` isn't installed instead of raising.""" + +def resolve_gh() -> str | None: + """The absolute path of the `gh` binary, or None when it can't be found. PATH + first (`shutil.which`), then the well-known install dirs. Cached per process — + call ``reset_gh_cache()`` (tests) to re-resolve.""" + global _gh_path, _gh_resolved + if _gh_resolved: + return _gh_path + found = shutil.which("gh") + if not found: + for d in _FALLBACK_BIN_DIRS: + cand = Path(d).expanduser() / "gh" + if cand.is_file() and os.access(cand, os.X_OK): + found = str(cand) + break + _gh_path, _gh_resolved = found, True + return found + + +def reset_gh_cache() -> None: + """Forget the cached binary path (so the next call re-resolves).""" + global _gh_path, _gh_resolved + _gh_path, _gh_resolved = None, False + + +def gh_search_dirs() -> list[str]: + """Where `gh` is looked for — PATH plus the fallback dirs (for error messages).""" + return [*(p for p in os.environ.get("PATH", "").split(os.pathsep) if p), *_FALLBACK_BIN_DIRS] + + +# ── token resolution ────────────────────────────────────────────────────────────── + +# A zero-arg getter returning the plugin's configured token ("" when unset). Wired by +# register() to read the LIVE config (so a token pasted in Settings works without a +# restart); None when no host wired one (the host-free suite, an older host). +_token_getter: Callable[[], str] | None = None + + +def set_token_getter(getter: Callable[[], str] | None) -> None: + """Install (or clear) the config-token getter. The getter is called per `gh` run + and must never raise — it's wrapped anyway, a failing getter means "no token".""" + global _token_getter + _token_getter = getter + + +def _config_token() -> str: + if _token_getter is None: + return "" + try: + return str(_token_getter() or "").strip() + except Exception: # noqa: BLE001 — a broken getter must not take `gh` down + return "" + + +def resolve_token() -> str | None: + """The token `gh` should use: the plugin's configured secret first (Settings ▸ + GitHub), else the ambient GITHUB_TOKEN / GH_TOKEN env, else None (keyring).""" + return _config_token() or os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN") or None + + +def token_source() -> str: + """Where the effective token comes from: ``config`` | ``env`` | ``none``.""" + if _config_token(): + return "config" + if os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN"): + return "env" + return "none" + + +def gh_env() -> dict: + """The child env for a `gh` run — the ambient env plus the effective token injected + as GH_TOKEN (gh's own precedence: GH_TOKEN > GITHUB_TOKEN > keyring), so a config + token beats anything already in the environment.""" env = os.environ.copy() - token = _resolve_token() + token = resolve_token() if token: + env["GH_TOKEN"] = token env["GITHUB_TOKEN"] = token + return env + + +# ── the runner ──────────────────────────────────────────────────────────────────── + +# The stderr stand-in for a missing binary — check_gh_error recognises it. +_MISSING_BINARY = "gh CLI is not installed or not on PATH." + + +async def run_gh(args: list[str], timeout: int = _COMMAND_TIMEOUT) -> tuple[int, str, str]: + """Run a `gh` command, returning (returncode, stdout, stderr). Times out (killing + the process), and reports a clean error when `gh` isn't installed instead of raising.""" + binary = resolve_gh() + if not binary: + return 127, "", _MISSING_BINARY proc = None try: proc = await asyncio.create_subprocess_exec( - "gh", + binary, *args, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, - env=env, + env=gh_env(), ) stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=timeout) return ( @@ -63,11 +177,90 @@ async def run_gh(args: list[str], timeout: int = _COMMAND_TIMEOUT) -> tuple[int, proc.kill() return 1, "", f"gh command timed out after {timeout}s" except FileNotFoundError: - return 1, "", "gh CLI is not installed or not on PATH." + reset_gh_cache() # the cached path went stale (uninstalled mid-process) + return 127, "", _MISSING_BINARY + except PermissionError: + return 126, "", f"gh binary at {binary} is not executable." -def check_gh_error(returncode: int, stderr: str) -> str | None: - """Return a formatted `Error: ...` string if the command failed, else None.""" - if returncode != 0: - return f"Error (gh exit {returncode}): {stderr[:500]}" - return None +# ── output parsing ──────────────────────────────────────────────────────────────── + + +def parse_json(out: str, expect: type | tuple[type, ...] = dict) -> tuple[Any, str | None]: + """``(value, None)`` when ``out`` is JSON of the ``expect``ed type (``dict``, + ``list``, or a tuple of them), else ``(None, "Error: ...")``. The ONE place a `gh --json` / `gh api` + body is trusted: the contents API hands back a dict for a file and a list for a + directory, a `--jq` can yield a bare scalar, and a tool that does ``.get`` on the + wrong type raises through the tool layer and kills the agent's turn.""" + try: + value = json.loads(out) + except (ValueError, TypeError): + return None, f"Error: could not parse gh output: {(out or '')[:200]}" + if not isinstance(value, expect): + names = " or ".join(t.__name__ for t in (expect if isinstance(expect, tuple) else (expect,))) + return None, ( + f"Error: unexpected gh output (expected a JSON {names}, got {type(value).__name__}): {(out or '')[:200]}" + ) + return value, None + + +def dicts(items) -> list[dict]: + """Only the dict rows of a `gh --json` list — nulls/scalars in a row list are skipped.""" + return [x for x in (items or []) if isinstance(x, dict)] + + +# ── error classification ────────────────────────────────────────────────────────── + +_RATE_LIMIT_RESET_RE = re.compile(r"(?:retry after|reset(?:s)? (?:at|in)|try again (?:in|after))\s*([^\n.]+)", re.I) +_NOT_FOUND_REPO_RE = re.compile(r"Could not resolve to a Repository with the name '([^']+)'", re.I) + + +def classify_gh_error(returncode: int, stderr: str, *, repo: str = "") -> str | None: + """Map a failed `gh` run to ONE actionable ``Error: ...`` string, or None if it + succeeded. The categories, in the order they're checked: + + - binary missing (the runner's stand-in stderr, or exit 127); + - not authenticated (gh's exit 4, or stderr pointing at `gh auth login`); + - rate-limited (HTTP 403 + "rate limit"); + - repo not found / not accessible (GraphQL "Could not resolve to a Repository", + or an HTTP 404); + - anything else → the generic ``Error (gh exit N): `` (unchanged shape, + existing tests and callers match on it). + """ + if returncode == 0: + return None + blob = stderr or "" + low = blob.lower() + + if _MISSING_BINARY in blob or returncode == 127: + return f"Error: gh CLI is not installed or not on PATH (looked in {', '.join(gh_search_dirs())})." + if ( + returncode == GH_EXIT_AUTH_REQUIRED + or "gh auth login" in low + or "not logged in" in low + or "authentication required" in low + or "bad credentials" in low + ): + return f"Error: GitHub CLI is not authenticated — {_AUTH_HINT}." + if "rate limit" in low and ("403" in low or "429" in low or "exceeded" in low): + m = _RATE_LIMIT_RESET_RE.search(blob) + when = f" — retry after {m.group(1).strip()}" if m else " — retry in a few minutes" + return f"Error: GitHub API rate limit hit{when}." + m = _NOT_FOUND_REPO_RE.search(blob) + if m: + return f"Error: repo '{m.group(1)}' not found or not accessible (check the owner/name and your token's scopes)." + if "http 404" in low: + where = ( + f"repo '{repo}' not found or not accessible, or the path/ref/number doesn't exist in it" + if repo + else "not found" + ) + return f"Error: {where} (HTTP 404 — check the owner/name, the path/ref/number, and your token's scopes)." + return f"Error (gh exit {returncode}): {blob[:500]}" + + +def check_gh_error(returncode: int, stderr: str, *, repo: str = "") -> str | None: + """Return a formatted `Error: ...` string if the command failed, else None. + Classified (auth / not-found / rate-limit / missing binary) — see + ``classify_gh_error``; generic failures keep the ``Error (gh exit N): …`` shape.""" + return classify_gh_error(returncode, stderr, repo=repo) diff --git a/tests/test_gh_cli.py b/tests/test_gh_cli.py new file mode 100644 index 0000000..1186555 --- /dev/null +++ b/tests/test_gh_cli.py @@ -0,0 +1,185 @@ +"""gh_cli — binary resolution, token precedence, and error CLASSIFICATION. + +Host-free and network-free: the classifier is pure; the runner is exercised only +against a binary that doesn't exist (the missing-binary path must return, not raise). +""" + +from __future__ import annotations + +import os +import stat + +import pytest +from ghplugin import gh_cli +from ghplugin.gh_cli import ( + check_gh_error, + classify_gh_error, + gh_env, + resolve_gh, + resolve_token, + run_gh, + set_token_getter, + token_source, +) + + +@pytest.fixture(autouse=True) +def _clean(monkeypatch): + """Each test starts with no cached binary, no config token, no env token.""" + gh_cli.reset_gh_cache() + set_token_getter(None) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + yield + gh_cli.reset_gh_cache() + set_token_getter(None) + + +# ── binary resolution ──────────────────────────────────────────────────────────── + + +def test_resolve_gh_prefers_path(tmp_path, monkeypatch): + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\necho hi\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) + monkeypatch.setenv("PATH", str(tmp_path)) + assert resolve_gh() == str(fake) + + +def test_resolve_gh_falls_back_to_install_dirs_when_path_is_bare(tmp_path, monkeypatch): + """A desktop build launched without a shell PATH still finds a brew/apt gh.""" + monkeypatch.setenv("PATH", str(tmp_path / "nothing-here")) + fake_dir = tmp_path / "opt-bin" + fake_dir.mkdir() + fake = fake_dir / "gh" + fake.write_text("#!/bin/sh\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) + monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", (str(fake_dir),)) + assert resolve_gh() == str(fake) + + +def test_resolve_gh_is_cached_until_reset(tmp_path, monkeypatch): + monkeypatch.setenv("PATH", str(tmp_path)) + monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", ()) + assert resolve_gh() is None + # A binary appearing later isn't seen until the cache is reset. + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) + assert resolve_gh() is None + gh_cli.reset_gh_cache() + assert resolve_gh() == str(fake) + + +async def test_run_gh_missing_binary_returns_not_raises(tmp_path, monkeypatch): + monkeypatch.setenv("PATH", str(tmp_path)) + monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", ()) + rc, out, serr = await run_gh(["--version"]) + assert rc == 127 and out == "" and "not installed" in serr + assert check_gh_error(rc, serr).startswith("Error: gh CLI is not installed or not on PATH (looked in ") + + +# ── token precedence ───────────────────────────────────────────────────────────── + + +def test_no_token_anywhere(): + assert resolve_token() is None + assert token_source() == "none" + assert "GH_TOKEN" not in gh_env() + + +def test_env_token_is_injected(monkeypatch): + monkeypatch.setenv("GITHUB_TOKEN", "ghp_env") + assert resolve_token() == "ghp_env" + assert token_source() == "env" + assert gh_env()["GH_TOKEN"] == "ghp_env" + + +def test_config_token_wins_over_env(monkeypatch): + """The Settings ▸ GitHub secret beats an ambient env token — it's what the person + just pasted, and it's visible in the UI where the env var is not.""" + monkeypatch.setenv("GH_TOKEN", "ghp_env") + set_token_getter(lambda: "ghp_config") + assert resolve_token() == "ghp_config" + assert token_source() == "config" + env = gh_env() + assert env["GH_TOKEN"] == "ghp_config" and env["GITHUB_TOKEN"] == "ghp_config" + + +def test_blank_config_token_falls_through_to_env(monkeypatch): + monkeypatch.setenv("GH_TOKEN", "ghp_env") + set_token_getter(lambda: " ") + assert resolve_token() == "ghp_env" + + +def test_raising_token_getter_means_no_token(): + def boom(): + raise RuntimeError("config not loaded") + + set_token_getter(boom) + assert resolve_token() is None and token_source() == "none" + + +# ── classification ─────────────────────────────────────────────────────────────── + + +def test_success_is_none(): + assert check_gh_error(0, "") is None + assert check_gh_error(0, "some warning on stderr") is None + + +@pytest.mark.parametrize( + "rc,stderr", + [ + (4, ""), # gh's AuthRequired exit code, whatever it printed + (1, "To get started with GitHub CLI, please run: gh auth login"), + (1, "You are not logged into any GitHub hosts. To log in, run: gh auth login"), + (1, "HTTP 401: Bad credentials (https://api.github.com/user)"), + ], +) +def test_not_authenticated(rc, stderr): + err = check_gh_error(rc, stderr) + assert err.startswith("Error: GitHub CLI is not authenticated — run `gh auth login`") + assert "Settings ▸ GitHub (github.token)" in err + + +def test_repo_not_found_graphql(): + err = check_gh_error(1, "GraphQL: Could not resolve to a Repository with the name 'o/nope'. (repository)") + assert err.startswith("Error: repo 'o/nope' not found or not accessible") + + +def test_http_404_names_the_repo_when_known(): + err = check_gh_error(1, "gh: Not Found (HTTP 404)", repo="o/n") + assert err.startswith("Error: repo 'o/n' not found or not accessible") + assert "HTTP 404" in err + # Without a repo it's still a not-found, not a raw dump. + assert check_gh_error(1, "gh: Not Found (HTTP 404)").startswith("Error: not found (HTTP 404") + + +def test_rate_limit_with_and_without_a_reset_hint(): + err = check_gh_error(1, "HTTP 403: API rate limit exceeded for 1.2.3.4. Retry after 12:34:56 UTC") + assert err.startswith("Error: GitHub API rate limit hit — retry after 12:34:56 UTC") + err = check_gh_error(1, "HTTP 403: rate limit exceeded") + assert err == "Error: GitHub API rate limit hit — retry in a few minutes." + + +def test_plain_403_is_not_a_rate_limit(): + """A forbidden that isn't a rate limit keeps the generic shape (existing callers match on it).""" + assert check_gh_error(1, "HTTP 403: forbidden") == "Error (gh exit 1): HTTP 403: forbidden" + + +def test_generic_failure_keeps_the_legacy_shape_and_caps_stderr(): + err = classify_gh_error(2, "x" * 900) + assert err.startswith("Error (gh exit 2): ") and len(err) == len("Error (gh exit 2): ") + 500 + + +def test_missing_binary_stderr_is_classified_even_with_exit_1(): + err = check_gh_error(1, "gh CLI is not installed or not on PATH.") + assert err.startswith("Error: gh CLI is not installed or not on PATH (looked in ") + assert "/usr/local/bin" in err + + +def test_search_dirs_include_path_and_fallbacks(monkeypatch): + monkeypatch.setenv("PATH", f"/p1{os.pathsep}/p2") + dirs = gh_cli.gh_search_dirs() + assert dirs[:2] == ["/p1", "/p2"] and "/opt/homebrew/bin" in dirs From fba9b3428c8a088f4c90deb05c7b070f76533d2e Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Sat, 22 Aug 2026 16:30:03 -0700 Subject: [PATCH 2/6] feat(repos): live default-repo getter, checkout origin fallback, named default_repo error (#23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - resolve_repo() accepts a zero-arg getter for the default (evaluated only when no explicit repo was passed) so the tools / /issue read the LIVE config instead of a register-time snapshot — an onboard_project or a Settings edit mid-session is seen by the next call. - default_repo_error(): a malformed github.default_repo (not owner/name) is a NAMED error — in /issue, the routes and the views — never fed to gh. Closes #23. - projects.py: last-resort repo source — parse the origin remote of every registry project path (those without a github: binding first) and of project_board.repo (skipping the board's '.' sentinel) into owner/name; HTTPS/SSH/scp shapes, github.com-anchored, cached per path for a minute. Picker order: explicit → registry bindings → remotes, deduped. - The GITHUB_DEFAULT_REPO / GH_REPO env fallback stays (a test depends on it) but is logged at INFO whenever it decides the repo — never silent. - gh_issue.py docstring: '/issue is the user-only chat path; the github_create_issue agent tool exists behind github.write' (the old 'deliberately NOT an agent tool' was false since write_tools shipped). Co-Authored-By: Claude Fable 5 --- gh_issue.py | 84 +++++++++++++++++++------ projects.py | 128 ++++++++++++++++++++++++++++++++++---- tests/test_gh_issue.py | 54 +++++++++++++++++ tests/test_projects.py | 135 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 371 insertions(+), 30 deletions(-) diff --git a/gh_issue.py b/gh_issue.py index 5f3e121..92f0e33 100644 --- a/gh_issue.py +++ b/gh_issue.py @@ -1,10 +1,13 @@ """The `/issue` chat control command — file a GitHub issue from a chat message. -This is the WRITE counterpart to the read tools that is deliberately NOT an agent -tool: creating an issue is a write the model must not do autonomously, so the host -exposes it as a user-only `/issue` chat control command (like `/goal`). The plugin -registers it via `registry.register_chat_command("issue", …)` (host seam); this module -is the pure, host-free logic behind it. +`/issue` is the USER-ONLY chat path for filing an issue: the host exposes it as a +chat control command (like `/goal`) via `registry.register_chat_command("issue", …)`, +so a person can file from the composer on any agent — including a read-only one — +without the model being involved. The `github_create_issue` AGENT tool exists too +(write_tools.py), behind the per-agent `github.write` gate: a PM/coding agent with +write on can file autonomously; a research agent without it cannot. Both paths share +`file_issue` below, so the gate check and the `gh issue create` argv can't diverge. +This module is the pure, host-free logic. `run_issue_command(rest, *, default_repo)` takes everything after the `/issue` token (the host already matched it) and returns the reply string. The issue body is checked @@ -13,13 +16,17 @@ proposed-direction or acceptance section), so an issue filed here always passes. Repo resolution (no silent misrouting): explicit `--repo` > the plugin's configured -`github.default_repo` (passed in as `default_repo`) > `GITHUB_DEFAULT_REPO` / `GH_REPO` -env > an error asking for one. Auth rides on `gh_cli` (`GITHUB_TOKEN`/`GH_TOKEN` or -ambient `gh auth`); `gh issue create` needs write scope. +`github.default_repo` (passed in as `default_repo`; itself resolved from default_repo +> first of repos > the host's project registry > a checkout's origin remote, see +projects.py) > `GITHUB_DEFAULT_REPO` / `GH_REPO` env (logged at INFO when it fires — +it's the one non-obvious source) > an error asking for one. Auth rides on `gh_cli` +(the `github.token` secret, `GITHUB_TOKEN`/`GH_TOKEN`, or ambient `gh auth`); +`gh issue create` needs write scope. """ from __future__ import annotations +import logging import os import re import shlex @@ -27,6 +34,8 @@ from .gh_cli import REPO_RE, check_gh_error, run_gh +log = logging.getLogger("protoagent.plugins.github") + # Section detectors — kept in lockstep with the host CI gate's regexes so the local # check and the server-side gate can never disagree about what "conforms" means. _SECTION_RES = { @@ -107,23 +116,59 @@ def labels_for(kind: str, extra: list[str] | None = None) -> list[str]: return out -def resolve_repo(explicit: str | None, default_repo: str = "") -> str | None: - """Target repo: explicit ``--repo`` > configured default > GITHUB_DEFAULT_REPO - / GH_REPO env > ``None`` (caller errors — there is no silent default).""" +def current_default(default_repo) -> str: + """Materialise a default repo that may be a plain string OR a zero-arg getter. + The tools take a GETTER (``__init__.py``) so a ``default_repo``/``repos``/registry + edit — or an ``onboard_project`` mid-session — is seen by the very next tool call, + not at the next register(). A raising getter reads as "no default".""" + if callable(default_repo): + try: + return str(default_repo() or "").strip() + except Exception: # noqa: BLE001 — a broken getter must not kill a tool call + log.debug("[github] default-repo getter failed", exc_info=True) + return "" + return str(default_repo or "").strip() + + +def default_repo_error(value: str) -> str | None: + """The NAMED error for a malformed configured default (#23): ``github.default_repo`` + must be ``owner/name``. ``None`` when it's blank (unset is fine) or well-formed.""" + v = (value or "").strip() + if not v or REPO_RE.match(v): + return None return ( - (explicit or "").strip() - or (default_repo or "").strip() - or os.environ.get("GITHUB_DEFAULT_REPO") - or os.environ.get("GH_REPO") - or None + f"Error: github.default_repo must be 'owner/name' (got {v!r}) — fix it in Settings ▸ GitHub, " + "or pass repo='owner/name' explicitly." ) +def resolve_repo(explicit: str | None, default_repo="") -> str | None: + """Target repo: explicit ``--repo`` > configured default > GITHUB_DEFAULT_REPO + / GH_REPO env > ``None`` (caller errors — there is no silent default). + + ``default_repo`` may be a string or a zero-arg getter (evaluated only when no + explicit repo was passed — see ``current_default``). The env step is the only + source a person can't see in Settings, so it's logged at INFO whenever it's what + actually decided the repo (never silently).""" + chosen = (explicit or "").strip() or current_default(default_repo) + if chosen: + return chosen + for var in ("GITHUB_DEFAULT_REPO", "GH_REPO"): + val = (os.environ.get(var) or "").strip() + if val: + log.info("[github] no repo passed or configured — using %s=%s from the environment", var, val) + return val + return None + + def effective_default_repo(default_repo: str, repos: list[str] | None = None) -> str: """The preselected default repo for the dialog + the ``/issue`` command: the explicit ``github.default_repo`` if set, else the first entry in the - ``github.repos`` picker list, else ``""`` (env still applies via - ``resolve_repo``). Keeps the command and the dialog agreeing on the default.""" + ``github.repos`` picker list (explicit ∪ registry ∪ checkout remotes), else + ``""`` (env still applies via ``resolve_repo``). Keeps the command, the tools + and the dialog agreeing on the default. A malformed explicit default is returned + as-is so the caller's ``bad_repo`` / ``default_repo_error`` names it instead of + silently routing to the next candidate.""" if (default_repo or "").strip(): return default_repo.strip() for r in repos or []: @@ -214,6 +259,7 @@ def _parse(rest: str, *, default_repo: str = "") -> IssueRequest | str: title = " ".join(title_parts).strip() labels = labels_for(kind, labels) + explicit_repo = repo repo = resolve_repo(repo, default_repo) if not title: @@ -228,6 +274,8 @@ def _parse(rest: str, *, default_repo: str = "") -> IssueRequest | str: "in Settings (or the `GITHUB_DEFAULT_REPO` env var)." ) if not REPO_RE.match(repo): + if not (explicit_repo or "").strip(): + return default_repo_error(repo) or f"Error: repo must be 'owner/name' (got {repo!r})." return f"Error: --repo must be 'owner/name' (got {repo!r})." return IssueRequest(title=title, body=body, kind=kind, repo=repo, labels=labels, dry_run=dry_run) diff --git a/projects.py b/projects.py index 9dd0d9c..9d3206f 100644 --- a/projects.py +++ b/projects.py @@ -1,4 +1,5 @@ -"""The host's managed-projects registry (ADR 0095), read defensively. +"""The host's managed-projects registry (ADR 0095), read defensively — plus the +last-resort fallback: a local checkout's ``origin`` remote. protoAgent gained a top-level ``projects:`` registry in v0.115.0 — one place to declare a project, with consumers projecting from it instead of re-declaring it. @@ -6,7 +7,7 @@ feeds the repo picker and ``/issue``, so registering a project once is enough instead of also re-typing ``owner/name`` into ``github.repos``. -Two properties are deliberate: +Three properties are deliberate: **Explicit config is ADDED to, never replaced — and never hides the registry.** ``github.repos`` set ⇒ those repos come FIRST, in the operator's order, and the @@ -19,6 +20,16 @@ non-regressing for everything the explicit list names (same entries, same order, same default) — it only adds the registry's entries after them. +**A local checkout becomes ``owner/name`` by itself (v0.6.0).** A registry project +declared with only a ``path`` (no ``github:``), and the board's ``project_board.repo`` +path, are git checkouts — their ``origin`` remote already says which GitHub repo +they are. ``remote_repos`` parses ``git -C remote get-url origin`` for each +(``github.com[:/]owner/name(.git)?``, HTTPS or SSH) so a fresh install that +onboarded a project, or pointed the board at a checkout, gets a working default +repo with nothing typed twice. These come LAST (after explicit + registry +``github:`` entries), are cached briefly per path, and a non-git or remote-less +path contributes nothing. + **Every read degrades to ``[]``.** The plugin's ``min_protoagent_version`` stays 0.27.0 — the projection is additive, and bumping the floor would cut off older hosts that don't want it anyway. So the host import is lazy and broadly guarded: @@ -29,6 +40,72 @@ from __future__ import annotations +import re +import subprocess +import time +from pathlib import Path + +# `origin` URL shapes: https://github.com/o/n(.git), git@github.com:o/n(.git), +# ssh://git@github.com/o/n(.git), github.com/o/n — host-anchored so a mirror on +# another forge never masquerades as a GitHub repo. +GITHUB_REMOTE_RE = re.compile(r"github\.com[:/]([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+?)(?:\.git)?/?$", re.I) + +_GIT_TIMEOUT = 3 # seconds — `git remote get-url` is local and instant; never let it hang a tool call +_REMOTE_TTL = 60.0 # seconds — per-path cache so a per-call getter doesn't fork git per tool call +_remote_cache: dict[str, tuple[float, str | None]] = {} + + +def repo_from_remote_url(url: str) -> str | None: + """``owner/name`` from a GitHub remote URL (HTTPS / SSH / scp-like), else None.""" + m = GITHUB_REMOTE_RE.search((url or "").strip()) + return f"{m.group(1)}/{m.group(2)}" if m else None + + +def repo_from_checkout(path: str) -> str | None: + """``owner/name`` from the ``origin`` remote of the git checkout at ``path`` + (None when the path isn't a git repo, has no GitHub origin, or git isn't + available). Cached per path for a minute.""" + key = str(path or "").strip() + if not key: + return None + now = time.monotonic() + hit = _remote_cache.get(key) + if hit and now - hit[0] < _REMOTE_TTL: + return hit[1] + result: str | None = None + try: + p = Path(key).expanduser() + if p.is_dir(): + proc = subprocess.run( + ["git", "-C", str(p), "remote", "get-url", "origin"], + capture_output=True, + text=True, + timeout=_GIT_TIMEOUT, + ) + if proc.returncode == 0: + result = repo_from_remote_url(proc.stdout) + except Exception: # noqa: BLE001 — no git / timeout / odd path: contributes nothing + result = None + _remote_cache[key] = (now, result) + return result + + +def reset_remote_cache() -> None: + """Forget the per-path origin cache (tests; a config save).""" + _remote_cache.clear() + + +def _host_entries() -> list[dict]: + """The registry + legacy-fence project entries (dicts only), or ``[]``.""" + try: + from graph.sdk import config + + cfg = config() + entries = list(getattr(cfg, "projects", None) or []) + list(getattr(cfg, "filesystem_projects", None) or []) + except Exception: # noqa: BLE001 — no host / older host / config unloaded; never fatal + return [] + return [e for e in entries if isinstance(e, dict)] + def registry_repos() -> list[str]: """``owner/name`` for every registered project that declares one — config @@ -38,29 +115,56 @@ def registry_repos() -> list[str]: ``filesystem.projects`` override: an instance that predates the registry (or one written by the pre-#2925 ``onboard_project``) carries its ``github`` bindings THERE, and those repos are just as real. Registry entries lead.""" + return _dedupe(str(e.get("github") or "") for e in _host_entries()) + + +def board_repo_path() -> str: + """The project-board plugin's ``project_board.repo`` checkout path when the host + config carries one and it's a real path — ``""`` for the board's ``"."`` / + blank unconfigured sentinel (the board itself refuses to build there), or on a + host with no plugin config.""" try: from graph.sdk import config - cfg = config() - entries = list(getattr(cfg, "projects", None) or []) + list(getattr(cfg, "filesystem_projects", None) or []) - except Exception: # noqa: BLE001 — no host / older host / config unloaded; never fatal - return [] - return _dedupe(str(e.get("github") or "") for e in entries if isinstance(e, dict)) + pconf = getattr(config(), "plugin_config", None) or {} + repo = str((pconf.get("project_board") or {}).get("repo") or "").strip() + except Exception: # noqa: BLE001 + return "" + return "" if repo in ("", ".") else repo + + +def checkout_paths() -> list[str]: + """The local checkouts whose ``origin`` can name a repo: every registry / + fence project's ``path`` (those WITHOUT a ``github:`` binding first — a bound + one already contributed via ``registry_repos``), then ``project_board.repo``.""" + entries = _host_entries() + unbound = [str(e.get("path") or "") for e in entries if not str(e.get("github") or "").strip()] + bound = [str(e.get("path") or "") for e in entries if str(e.get("github") or "").strip()] + board = board_repo_path() + return _dedupe([*unbound, *bound, board]) + + +def remote_repos() -> list[str]: + """``owner/name`` parsed from the ``origin`` remote of each checkout path the + host knows about — the last-resort source. ``[]`` with no host.""" + return _dedupe(repo_from_checkout(p) or "" for p in checkout_paths()) -def effective_repos(cfg_repos: list | None) -> list[str]: +def effective_repos(cfg_repos: list | None, *, include_remotes: bool = True) -> list[str]: """The repo picker list: the explicit ``github.repos`` entries (operator order, - first) UNION the host's managed-projects registry. The single place the two - layers meet — and the reason an onboarded project needs no second declaration.""" + first) UNION the host's managed-projects registry UNION (last) the repos parsed + from the checkouts' ``origin`` remotes. The single place the layers meet — and + the reason an onboarded project needs no second declaration.""" explicit = [str(r).strip() for r in (cfg_repos or []) if str(r).strip()] - return _dedupe([*explicit, *registry_repos()]) + remotes = remote_repos() if include_remotes else [] + return _dedupe([*explicit, *registry_repos(), *remotes]) def _dedupe(repos) -> list[str]: out: list[str] = [] seen: set[str] = set() for repo in repos: - repo = repo.strip() + repo = (repo or "").strip() if repo and repo not in seen: seen.add(repo) out.append(repo) diff --git a/tests/test_gh_issue.py b/tests/test_gh_issue.py index 4dd0244..f144807 100644 --- a/tests/test_gh_issue.py +++ b/tests/test_gh_issue.py @@ -9,6 +9,8 @@ from unittest.mock import AsyncMock, patch from ghplugin.gh_issue import ( + current_default, + default_repo_error, effective_default_repo, labels_for, missing_sections, @@ -50,6 +52,58 @@ def test_resolve_repo_precedence(monkeypatch): assert resolve_repo(None, "") == "o/env" # then env +def test_resolve_repo_env_fallback_is_logged_not_silent(monkeypatch, caplog): + """The env step is invisible in Settings — when it decides the repo, say so at INFO.""" + import logging + + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.setenv("GH_REPO", "o/env") + with caplog.at_level(logging.INFO, logger="protoagent.plugins.github"): + assert resolve_repo(None, "") == "o/env" + assert any("GH_REPO=o/env" in r.getMessage() for r in caplog.records) + caplog.clear() + with caplog.at_level(logging.INFO, logger="protoagent.plugins.github"): + assert resolve_repo("o/explicit", "") == "o/explicit" # env never consulted + assert not caplog.records + + +def test_resolve_repo_accepts_a_getter_and_calls_it_lazily(monkeypatch): + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.delenv("GH_REPO", raising=False) + calls = [] + + def getter(): + calls.append(1) + return "o/live" + + assert resolve_repo("o/explicit", getter) == "o/explicit" and calls == [] # explicit ⇒ getter untouched + assert resolve_repo("", getter) == "o/live" and len(calls) == 1 + + def boom(): + raise RuntimeError("host not ready") + + assert resolve_repo("", boom) is None # a broken getter reads as "no default", never raises + assert current_default(boom) == "" and current_default(" o/s ") == "o/s" + + +def test_default_repo_error_names_only_malformed_values(): + assert default_repo_error("") is None and default_repo_error(" ") is None + assert default_repo_error("o/n") is None + err = default_repo_error("protoLabsAI") + assert err.startswith("Error: github.default_repo must be 'owner/name' (got 'protoLabsAI')") + assert "Settings ▸ GitHub" in err + + +async def test_issue_command_names_a_malformed_configured_default(monkeypatch): + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.delenv("GH_REPO", raising=False) + out = await run_issue_command("A title with no repo flag", default_repo="just-owner") + assert out.startswith("Error: github.default_repo must be 'owner/name' (got 'just-owner')") + # an explicit --repo that's malformed is still the --repo error + out = await run_issue_command("Title --repo nope", default_repo="o/fine") + assert out.startswith("Error: --repo must be 'owner/name'") + + def test_effective_default_repo(): assert effective_default_repo("o/explicit", ["o/a"]) == "o/explicit" assert effective_default_repo("", ["o/a", "o/b"]) == "o/a" # first of the picker list diff --git a/tests/test_projects.py b/tests/test_projects.py index 8e91adc..8443b65 100644 --- a/tests/test_projects.py +++ b/tests/test_projects.py @@ -128,3 +128,138 @@ def test_default_repo_falls_through_to_the_registry(fake_host): assert effective_default_repo("", effective_repos([])) == "o/a" # explicit default still beats everything assert effective_default_repo("o/explicit", effective_repos([])) == "o/explicit" + + +# ── v0.6.0: a local checkout's `origin` remote becomes owner/name ── + + +@pytest.fixture(autouse=True) +def _fresh_remote_cache(): + from ghplugin.projects import reset_remote_cache + + reset_remote_cache() + yield + reset_remote_cache() + + +@pytest.mark.parametrize( + "url,expected", + [ + ("https://github.com/protoLabsAI/github-plugin.git", "protoLabsAI/github-plugin"), + ("https://github.com/protoLabsAI/github-plugin", "protoLabsAI/github-plugin"), + ("git@github.com:protoLabsAI/github-plugin.git", "protoLabsAI/github-plugin"), + ("ssh://git@github.com/protoLabsAI/github-plugin.git", "protoLabsAI/github-plugin"), + ("https://github.com/o/n/", "o/n"), + ("https://gitlab.com/o/n.git", None), # not GitHub — never masquerades + ("", None), + ("not a url", None), + ], +) +def test_repo_from_remote_url(url, expected): + from ghplugin.projects import repo_from_remote_url + + assert repo_from_remote_url(url) == expected + + +def _git_repo(tmp_path, name, origin=None): + """A real (empty) git checkout, optionally with an origin remote.""" + import subprocess + + d = tmp_path / name + d.mkdir() + subprocess.run(["git", "init", "-q", str(d)], check=True) + if origin: + subprocess.run(["git", "-C", str(d), "remote", "add", "origin", origin], check=True) + return d + + +def test_repo_from_checkout_parses_the_origin_remote(tmp_path): + from ghplugin.projects import repo_from_checkout + + d = _git_repo(tmp_path, "proj", "git@github.com:o/proj.git") + assert repo_from_checkout(str(d)) == "o/proj" + + +def test_repo_from_checkout_degrades_for_non_repos_and_remoteless_repos(tmp_path): + from ghplugin.projects import repo_from_checkout + + assert repo_from_checkout(str(tmp_path / "missing")) is None # no such dir + (tmp_path / "plain").mkdir() + assert repo_from_checkout(str(tmp_path / "plain")) is None # not a git repo + assert repo_from_checkout(str(_git_repo(tmp_path, "noremote"))) is None # no origin + assert repo_from_checkout("") is None + + +def test_repo_from_checkout_is_cached_per_path(tmp_path, monkeypatch): + import subprocess + + from ghplugin import projects + + d = _git_repo(tmp_path, "proj", "https://github.com/o/proj") + assert projects.repo_from_checkout(str(d)) == "o/proj" + calls = [] + real = subprocess.run + monkeypatch.setattr(projects.subprocess, "run", lambda *a, **k: (calls.append(a), real(*a, **k))[1]) + assert projects.repo_from_checkout(str(d)) == "o/proj" + assert calls == [] # served from the cache — a per-call getter never forks git per tool call + + +def test_remote_repos_come_from_registry_paths_and_the_board_repo(tmp_path, monkeypatch): + """A registry project with only a `path` (no github:), and project_board.repo, are + checkouts — their origin names the repo. Bound entries' paths are scanned too (the + remote may differ from the declared binding), after the unbound ones.""" + import sys + import types + + from ghplugin.projects import checkout_paths, effective_repos, remote_repos + + unbound = _git_repo(tmp_path, "unbound", "git@github.com:o/unbound.git") + bound = _git_repo(tmp_path, "bound", "git@github.com:o/bound-remote.git") + board = _git_repo(tmp_path, "board", "https://github.com/o/board.git") + + sdk = types.ModuleType("graph.sdk") + sdk.config = lambda: types.SimpleNamespace( + projects=[ + {"name": "u", "path": str(unbound)}, + {"name": "b", "path": str(bound), "github": "o/bound"}, + ], + filesystem_projects=None, + plugin_config={"project_board": {"repo": str(board)}}, + ) + graph = types.ModuleType("graph") + graph.sdk = sdk + monkeypatch.setitem(sys.modules, "graph", graph) + monkeypatch.setitem(sys.modules, "graph.sdk", sdk) + + assert checkout_paths() == [str(unbound), str(bound), str(board)] + assert remote_repos() == ["o/unbound", "o/bound-remote", "o/board"] + # Order of the picker: explicit, then registry bindings, then remotes (last resort), deduped. + assert effective_repos(["o/explicit"]) == ["o/explicit", "o/bound", "o/unbound", "o/bound-remote", "o/board"] + assert effective_repos([], include_remotes=False) == ["o/bound"] + + +def test_board_repo_dot_sentinel_is_ignored(monkeypatch): + """projectBoard's `repo: "."` is its UNCONFIGURED default (it refuses to build there) — + never parse the server's cwd as the agent's repo.""" + import sys + import types + + from ghplugin.projects import board_repo_path, checkout_paths + + for sentinel in (".", "", None): + sdk = types.ModuleType("graph.sdk") + sdk.config = lambda s=sentinel: types.SimpleNamespace( + projects=[], filesystem_projects=None, plugin_config={"project_board": {"repo": s}} + ) + graph = types.ModuleType("graph") + graph.sdk = sdk + monkeypatch.setitem(sys.modules, "graph", graph) + monkeypatch.setitem(sys.modules, "graph.sdk", sdk) + assert board_repo_path() == "" + assert checkout_paths() == [] + + +def test_no_host_means_no_remotes(): + from ghplugin.projects import board_repo_path, checkout_paths, remote_repos + + assert checkout_paths() == [] and remote_repos() == [] and board_repo_path() == "" From 9d6a4db546b3240ff12a387d246bbe0a703014b7 Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Sat, 22 Aug 2026 16:30:14 -0700 Subject: [PATCH 3/6] =?UTF-8?q?fix(tools):=20never=20raise=20=E2=80=94=20r?= =?UTF-8?q?epo=5Fcontents=20on=20a=20file,=20typed=20JSON=20everywhere,=20?= =?UTF-8?q?github=5Fstatus,=20docstring=20truth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github_repo_contents: the contents API returns a dict for a FILE; iterating it raised AttributeError through the tool layer and killed the agent's turn. Now: "Error: '' is a file, not a directory — use github_read_file". - Every gh JSON body goes through parse_json()/dicts() (get_pr/get_issue/ list_issues/ci_runs/review guards) — a list, scalar, null row or garbage is an Error string, not an exception. The sweep found get_pr and list_issues raising on the wrong JSON type too. - New read tool github_status (always on, no write gate): one paragraph — gh installed? authenticated? as whom? which default repo? — so the model self-diagnoses instead of guessing from stderr. - tests/test_no_raise_sweep.py: enumerates EVERY tool register() produces (read+write+review, 23) and invokes each against a run_gh stub returning dict / list / list-of-nulls / empty / garbage / number / string / error / auth / not-found / missing-binary / timeout, asserting a str comes back. Plus a real missing-binary pass through run_gh itself. The arg table must match the registered set, so a new tool can't dodge the sweep. - Docstring truth: github_read_file's description (what the model sees) no longer carries 'TODO(team): implement via …'; the 'STUBBED' module/section comments are gone; factories accept a getter for the default repo. - check_gh_error(..., repo=) everywhere so a 404 names the repo. Co-Authored-By: Claude Fable 5 --- read_tools.py | 138 ++++++++++++++++++++------------- review_tools.py | 36 ++++----- tests/test_no_raise_sweep.py | 145 +++++++++++++++++++++++++++++++++++ tests/test_read_tools.py | 86 +++++++++++++++++++++ write_tools.py | 25 +++--- 5 files changed, 342 insertions(+), 88 deletions(-) create mode 100644 tests/test_no_raise_sweep.py diff --git a/read_tools.py b/read_tools.py index 47f0bf5..219d449 100644 --- a/read_tools.py +++ b/read_tools.py @@ -1,21 +1,23 @@ """GitHub READ tools over `gh` — always registered (read-only is the safe default). -Six are ported from protoAgent's tools/github_tools.py (PRs, issues, diffs, CI). Two -new ones (`github_read_file`, `github_repo_contents`) are STUBBED — the team builds -them out (see the TODOs). Each tool takes an `owner/name` repo — or falls back to the -configured default when it's omitted — and degrades to a readable `Error: ...` string -when `gh`/auth is unavailable. +Eleven tools, all implemented: six ported from protoAgent's tools/github_tools.py +(PRs, issues, diffs, CI), the repo-content readers (`github_read_file`, +`github_read_pr_file`, `github_repo_contents`, `github_path_exists`), `github_pr_diff`, +and `github_status` (the self-diagnosis probe — is `gh` installed / authenticated). +Each tool takes an `owner/name` repo — or falls back to the configured default when +it's omitted (a LIVE getter, so a Settings edit or an onboarded project is seen by the +next call) — and degrades to a readable, classified `Error: ...` string (not +authenticated / repo not found / rate-limited / `gh` missing) instead of raising. """ from __future__ import annotations -import json import re from langchain_core.tools import tool -from .gh_cli import bad_repo, check_gh_error, run_gh -from .gh_issue import resolve_repo +from .gh_cli import bad_repo, check_gh_error, dicts, parse_json, run_gh +from .gh_issue import current_default, default_repo_error, resolve_repo # Error-relevant lines to surface from a failed CI log (github_run_failure). _CI_ERR_RE = re.compile( @@ -25,9 +27,17 @@ ) -def get_read_tools(default_repo: str = "") -> list: - """Build the read tools. ``default_repo`` (``owner/name``) is used whenever a tool's - ``repo`` arg is omitted, so an agent with one configured repo needn't repeat it.""" +def get_read_tools(default_repo="", repos=None) -> list: + """Build the read tools. ``default_repo`` (``owner/name``, or a zero-arg getter + returning it — the live-config case) is used whenever a tool's ``repo`` arg is + omitted, so an agent with one configured repo needn't repeat it. ``repos`` (a list + or a getter) is the picker list, surfaced by ``github_status`` only.""" + + def _repos() -> list[str]: + try: + return list((repos() if callable(repos) else repos) or []) + except Exception: # noqa: BLE001 + return [] @tool async def github_get_pr(number: int, repo: str = "") -> str: @@ -51,13 +61,12 @@ async def github_get_pr(number: int, repo: str = "") -> str: "number,title,state,author,body,additions,deletions,files,url,headRefName,baseRefName", ] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - d = json.loads(out) - except json.JSONDecodeError: - return f"Error: could not parse gh output: {out[:200]}" - files = ", ".join(f.get("path", "?") for f in (d.get("files") or [])[:20]) + d, perr = parse_json(out, dict) + if perr: + return perr + files = ", ".join(str(f.get("path", "?")) for f in dicts(d.get("files"))[:20]) return ( f"PR #{d.get('number')} [{d.get('state')}] {d.get('title')}\n" f"branch: {d.get('headRefName', '?')} -> {d.get('baseRefName', '?')}\n" @@ -80,13 +89,12 @@ async def github_get_issue(number: int, repo: str = "") -> str: rc, out, serr = await run_gh( ["issue", "view", str(number), "--repo", repo, "--json", "number,title,state,author,labels,body,url"] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - d = json.loads(out) - except json.JSONDecodeError: - return f"Error: could not parse gh output: {out[:200]}" - labels = ", ".join(lbl.get("name", "") for lbl in (d.get("labels") or [])) + d, perr = parse_json(out, dict) + if perr: + return perr + labels = ", ".join(str(lbl.get("name", "")) for lbl in dicts(d.get("labels"))) return ( f"Issue #{d.get('number')} [{d.get('state')}] {d.get('title')}\n" f"by {(d.get('author') or {}).get('login', '?')} | labels: {labels or '(none)'} | " @@ -122,17 +130,17 @@ async def github_list_issues(repo: str = "", state: str = "open", limit: int = 2 "number,title,state,labels", ] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - items = json.loads(out) - except json.JSONDecodeError: - return f"Error: could not parse gh output: {out[:200]}" + items, perr = parse_json(out, list) + if perr: + return perr + items = dicts(items) if not items: return f"No {state} issues in {repo}." lines = [f"{len(items)} {state} issue(s) in {repo}:"] for it in items: - labels = ",".join(lbl.get("name", "") for lbl in (it.get("labels") or [])) + labels = ",".join(str(lbl.get("name", "")) for lbl in dicts(it.get("labels"))) lines.append( f" #{it.get('number')} [{it.get('state')}] {it.get('title')}" + (f" ({labels})" if labels else "") ) @@ -153,7 +161,7 @@ async def github_get_commit_diff(ref: str, repo: str = "", max_chars: int = 8000 rc, out, serr = await run_gh( ["api", f"repos/{repo}/commits/{ref}", "-H", "Accept: application/vnd.github.diff"] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err diff = out.strip() if not diff: @@ -175,7 +183,7 @@ async def github_pr_diff(number: int, repo: str = "", max_chars: int = 12000) -> if err := bad_repo(repo): return err rc, out, serr = await run_gh(["pr", "diff", str(number), "--repo", repo]) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err diff = out.strip() if not diff: @@ -211,12 +219,12 @@ async def github_ci_runs(repo: str = "", branch: str = "", limit: int = 15) -> s if branch.strip(): args += ["--branch", branch.strip()] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - runs = json.loads(out) - except json.JSONDecodeError: - return f"Error: could not parse gh output: {out[:200]}" + runs, perr = parse_json(out, list) + if perr: + return perr + runs = dicts(runs) if not runs: return f"No recent runs for {repo}" + (f" on {branch}" if branch.strip() else "") lines = [ @@ -240,7 +248,7 @@ async def github_run_failure(run_id: int, repo: str = "", max_lines: int = 40) - return err cap = max(5, min(int(max_lines), 80)) rc, out, serr = await run_gh(["run", "view", str(run_id), "--repo", repo, "--log-failed"], timeout=60) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err raw = [ln.rstrip() for ln in out.splitlines() if ln.strip()] seen: set = set() @@ -257,21 +265,17 @@ async def github_run_failure(run_id: int, repo: str = "", max_lines: int = 40) - return f"Run {run_id} in {repo}: no failed-step log lines (run may not have failed, or its logs expired)." return f"{repo} run {run_id} — failure log ({len(picked)} line(s)):\n" + "\n".join(picked) - # ── NEW read tools — STUBBED. Build these out (this is what lets an agent research - # any repo over `gh` without registering an fs project per repo). ──────────────── + # ── Repo-content readers — what lets an agent research ANY repo over `gh` without + # registering an fs project per repo. ─────────────────────────────────────────── @tool async def github_read_file(path: str, repo: str = "", ref: str = "") -> str: - """Read a single file's contents from a GitHub repo. + """Read a single file's raw contents from a GitHub repo (capped at 20000 chars). Args: repo: Repository as ``owner/name``. Omit to use the agent's configured default repo. - path: Path to the file within the repo (e.g. ``docs/guide.md``). + path: Path to the file within the repo (e.g. ``docs/guide.md``). For a directory + use ``github_repo_contents``; for a file as it is IN a PR use ``github_read_pr_file``. ref: Optional branch / tag / SHA (default: the repo's default branch). - - TODO(team): implement via `gh api repos/{repo}/contents/{path}?ref={ref}` with - `Accept: application/vnd.github.raw` (returns the raw file body), or - `gh api .../contents/... --jq .content | base64 -d`. Validate repo with - bad_repo(); cap the returned size; return a readable Error on failure. """ repo = resolve_repo(repo, default_repo) or "" if err := bad_repo(repo): @@ -280,7 +284,7 @@ async def github_read_file(path: str, repo: str = "", ref: str = "") -> str: if ref.strip(): args += ["-f", f"ref={ref}"] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err if len(out) > 20000: out = out[:20000] + "\n… (truncated at 20000 chars)" @@ -308,7 +312,7 @@ async def github_read_pr_file(number: int, path: str, repo: str = "") -> str: # DEFAULT branch, i.e. the pre-PR file: it then "confirms" that symbols the # PR adds don't exist, which reads as a blocker finding on correct code. rc, out, serr = await run_gh(["api", f"repos/{repo}/pulls/{number}", "--jq", ".head.sha"]) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err head = out.strip() if not head: @@ -323,7 +327,7 @@ async def github_read_pr_file(number: int, path: str, repo: str = "") -> str: f"ref={head}", ] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): # Fail LOUD, never fall back to the default branch: a silent fallback is # exactly the bug this tool exists to prevent. return f"Error reading {path} at {repo}#{number} head {head[:12]}: {gh_err}" @@ -368,7 +372,7 @@ async def github_path_exists(path: str, repo: str = "", ref: str = "") -> str: + " — the path does not exist." ) return ( - check_gh_error(rc, serr) + check_gh_error(rc, serr, repo=repo) or f"Error (gh exit {rc}): could not verify {repo}/{clean} — treat as UNVERIFIED (a Gap, not a finding)." ) @@ -388,12 +392,18 @@ async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> if ref.strip(): args += ["-f", f"ref={ref}"] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - items = json.loads(out) - except json.JSONDecodeError: - return f"Error: could not parse gh output: {out[:200]}" + items, perr = parse_json(out, (dict, list)) + if perr: + return perr + # The contents API returns a LIST for a directory but a single OBJECT for a + # file (or symlink/submodule). Iterating the object raised AttributeError + # through the tool layer and killed the whole agent turn — say what it is. + if isinstance(items, dict): + kind = items.get("type") or "file" + return f"Error: '{path or '.'}' is a {kind}, not a directory — use github_read_file to read it." + items = dicts(items) if not items: return f"No contents in {repo}/{path or '.'}." type_map = {"file": "FILE", "dir": "DIR ", "symlink": "LINK", "submodule": "SUB "} @@ -405,6 +415,23 @@ async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> lines.append(f"{t:4s} {size:>8s} {entry.get('name', '?')} ({entry.get('path', '')})") return "\n".join(lines) + @tool + async def github_status() -> str: + """Check whether the GitHub CLI is installed and authenticated, and which repo the + other github_* tools default to. Call this FIRST when any github_* tool returns an + authentication / not-found / CLI error, or before GitHub work on a fresh machine — + it says exactly what's wrong and what the operator must do (install `gh`, run + `gh auth login`, or paste a token in Settings ▸ GitHub). Takes no arguments. + """ + from .status import compute_status, summarize_status + + default = current_default(default_repo) + st = await compute_status(default, _repos()) + text = summarize_status(st) + if bad := default_repo_error(default): + text += f" NOTE: {bad}" + return text + return [ github_get_pr, github_get_issue, @@ -417,4 +444,5 @@ async def github_repo_contents(repo: str = "", path: str = "", ref: str = "") -> github_read_file, github_read_pr_file, github_repo_contents, + github_status, ] diff --git a/review_tools.py b/review_tools.py index eb94ce0..29670fc 100644 --- a/review_tools.py +++ b/review_tools.py @@ -29,11 +29,9 @@ from __future__ import annotations -import json - from langchain_core.tools import tool -from .gh_cli import bad_repo, check_gh_error, run_gh +from .gh_cli import bad_repo, check_gh_error, parse_json, run_gh from .gh_issue import resolve_repo # Check-run states that count as still-running. GitHub check runs carry @@ -52,13 +50,13 @@ async def _viewer_login() -> str: async def _pr_head_and_author(repo: str, number: int) -> tuple[str, str, str]: """(head_sha, author_login, error). error is '' on success.""" rc, out, serr = await run_gh(["pr", "view", str(number), "--repo", repo, "--json", "headRefOid,author"]) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return "", "", gh_err - try: - d = json.loads(out) - except json.JSONDecodeError: - return "", "", f"Error: could not parse gh output: {out[:200]}" - return str(d.get("headRefOid") or ""), str((d.get("author") or {}).get("login") or ""), "" + d, perr = parse_json(out, dict) + if perr: + return "", "", perr + author = d.get("author") if isinstance(d.get("author"), dict) else {} + return str(d.get("headRefOid") or ""), str(author.get("login") or ""), "" async def _ci_state(repo: str, head_sha: str) -> tuple[str, str]: @@ -74,9 +72,8 @@ async def _ci_state(repo: str, head_sha: str) -> tuple[str, str]: ) if rc != 0: return "unknown", (serr or out).strip()[:200] - try: - statuses = json.loads(out or "[]") - except json.JSONDecodeError: + statuses, perr = parse_json(out or "[]", list) + if perr: return "unknown", f"unparseable check-runs response: {out[:120]}" pending = [s for s in statuses if str(s).lower() in _NON_TERMINAL] if pending: @@ -97,13 +94,10 @@ async def _post_review(repo: str, number: int, event: str, body: str) -> str: f"body={body}", ] ) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err - try: - d = json.loads(out) - url = d.get("html_url") or "" - except json.JSONDecodeError: - url = "" + d, _perr = parse_json(out, dict) + url = str(d.get("html_url") or "") if d else "" return f"Posted {event} review on {repo}#{number}." + (f" {url}" if url else "") @@ -146,9 +140,9 @@ async def _guard_blocking_verdict(repo: str, number: int, verdict: str) -> str: return "" -def get_review_tools(default_repo: str = "") -> list: - """Build the verdict tools. ``default_repo`` (``owner/name``) is used whenever a - tool's ``repo`` arg is omitted.""" +def get_review_tools(default_repo="") -> list: + """Build the verdict tools. ``default_repo`` (``owner/name``, or a zero-arg getter + returning it) is used whenever a tool's ``repo`` arg is omitted.""" @tool async def github_review_comment(number: int, body: str, repo: str = "") -> str: diff --git a/tests/test_no_raise_sweep.py b/tests/test_no_raise_sweep.py new file mode 100644 index 0000000..51400ca --- /dev/null +++ b/tests/test_no_raise_sweep.py @@ -0,0 +1,145 @@ +"""The no-raise sweep — EVERY registered tool, against every shape `gh` can hand back. + +A tool that raises kills the whole agent turn (the projectBoard v0.40.1 lesson — and +this plugin's own: `github_repo_contents` iterated the contents API's dict-for-a-file +and AttributeError'd through the tool layer). So: enumerate what `register()` actually +registers (read + write + review — nothing hand-listed that could drift), invoke each +with minimal valid args, and stub `run_gh` to return a dict / a list / garbage / an +error / a missing binary / a timeout. The ONLY acceptable outcome is a `str`. +""" + +from __future__ import annotations + +import json +from unittest.mock import AsyncMock, patch + +import pytest +from ghplugin import register + +# Minimal valid args per tool — enough to get PAST the arg validation and into the +# `run_gh` result handling, which is what the sweep is about. Any tool not listed +# here is invoked with `{}` (its defaults), and the sweep fails loudly if a NEW tool +# needs args it doesn't have (so adding a tool means adding a row here). +_ARGS: dict[str, dict] = { + "github_get_pr": {"repo": "o/n", "number": 1}, + "github_get_issue": {"repo": "o/n", "number": 1}, + "github_list_issues": {"repo": "o/n"}, + "github_get_commit_diff": {"repo": "o/n", "ref": "abc"}, + "github_pr_diff": {"repo": "o/n", "number": 1}, + "github_path_exists": {"repo": "o/n", "path": "x"}, + "github_ci_runs": {"repo": "o/n"}, + "github_run_failure": {"repo": "o/n", "run_id": 1}, + "github_read_file": {"repo": "o/n", "path": "README.md"}, + "github_read_pr_file": {"repo": "o/n", "number": 1, "path": "x"}, + "github_repo_contents": {"repo": "o/n", "path": "src"}, + "github_status": {}, + "github_create_issue": {"repo": "o/n", "title": "t"}, + "github_comment": {"repo": "o/n", "number": 1, "body": "b"}, + "github_create_pr": {"repo": "o/n", "head": "h", "title": "t"}, + "github_edit_pr": {"repo": "o/n", "number": 1, "title": "t", "state": "ready"}, + "github_merge_pr": {"repo": "o/n", "number": 1, "confirm": True}, + "github_close": {"repo": "o/n", "number": 1}, + "github_set_labels": {"repo": "o/n", "number": 1, "add": "bug"}, + "github_set_assignees": {"repo": "o/n", "number": 1, "add": "kj"}, + "github_review_comment": {"repo": "o/n", "number": 1, "body": "b"}, + "github_review_approve": {"repo": "o/n", "number": 1, "body": "b"}, + "github_review_request_changes": {"repo": "o/n", "number": 1, "body": "b"}, +} + +# What a stubbed `gh` hands back: (rc, stdout, stderr). Every tool must turn each +# into a string — including shapes that are valid JSON but the WRONG type. +_SHAPES = { + "dict": ( + 0, + json.dumps({"type": "file", "name": "x", "path": "x", "size": 3, "login": "u", "head": {"sha": "a" * 40}}), + "", + ), + "list": (0, json.dumps([{"type": "file", "name": "x", "path": "x", "size": 1}, "not-a-dict", 7]), ""), + "list-of-nulls": (0, "[null, null]", ""), + "empty": (0, "", ""), + "garbage": (0, "<<>> \x00\xff", ""), + "number": (0, "42", ""), + "string-json": (0, '"just a string"', ""), + "error": (1, "", "HTTP 500: Internal Server Error"), + "auth": (4, "", "To get started with GitHub CLI, please run: gh auth login"), + "not-found": (1, "", "GraphQL: Could not resolve to a Repository with the name 'o/n'. (repository)"), + "missing-binary": (127, "", "gh CLI is not installed or not on PATH."), + "timeout": (1, "", "gh command timed out after 30s"), +} + + +def _all_tools(make_registry): + reg = make_registry({"write": True}) + register(reg) + return {t.name: t for t in reg.tools} + + +def _patch_all(fake): + """Patch run_gh in EVERY module that binds it (each imports the name directly).""" + targets = [ + "ghplugin.read_tools.run_gh", + "ghplugin.write_tools.run_gh", + "ghplugin.review_tools.run_gh", + "ghplugin.status.run_gh", + "ghplugin.gh_issue.run_gh", + "ghplugin.api.run_gh", + ] + patches = [patch(t, fake) for t in targets] + for p in patches: + p.start() + return patches + + +def test_sweep_covers_every_registered_tool(make_registry): + """The arg table must name every tool register() produces — a new tool can't slip + past the sweep by omission.""" + names = set(_all_tools(make_registry)) + assert names == set(_ARGS), f"sweep table out of date: missing {names - set(_ARGS)}, stale {set(_ARGS) - names}" + + +@pytest.mark.parametrize("shape", list(_SHAPES)) +async def test_every_tool_returns_a_str_never_raises(make_registry, shape): + tools = _all_tools(make_registry) + rc, out, serr = _SHAPES[shape] + fake = AsyncMock(return_value=(rc, out, serr)) + patches = _patch_all(fake) + try: + with patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"): + for name, tool in tools.items(): + try: + result = await tool.ainvoke(_ARGS.get(name, {})) + except Exception as e: # noqa: BLE001 — the assertion message is the point + raise AssertionError(f"{name} RAISED on gh shape {shape!r}: {type(e).__name__}: {e}") from e + assert isinstance(result, str), f"{name} returned {type(result).__name__} on shape {shape!r}" + finally: + for p in patches: + p.stop() + + +async def test_every_tool_survives_a_raising_runner(make_registry): + """Even a runner that itself blows up (it shouldn't — but a transport error might) + must not escape a tool. Today the tools let it propagate; this documents the + CURRENT contract: run_gh is the no-raise boundary, so this exercises run_gh's + own missing-binary path end-to-end instead.""" + from ghplugin import gh_cli + + tools = _all_tools(make_registry) + gh_cli.reset_gh_cache() + try: + with patch("ghplugin.gh_cli.shutil.which", return_value=None), patch.object(gh_cli, "_FALLBACK_BIN_DIRS", ()): + for name, tool in tools.items(): + result = await tool.ainvoke(_ARGS.get(name, {})) + assert isinstance(result, str) and "not installed" in result.lower(), f"{name}: {result[:120]!r}" + finally: + gh_cli.reset_gh_cache() + + +async def test_repo_contents_on_a_file_says_so(make_registry): + """The bug that motivated the sweep: a FILE path → dict → was an AttributeError.""" + tools = _all_tools(make_registry) + fake = AsyncMock( + return_value=(0, json.dumps({"type": "file", "name": "README.md", "path": "README.md", "size": 9}), "") + ) + with patch("ghplugin.read_tools.run_gh", fake): + out = await tools["github_repo_contents"].ainvoke({"repo": "o/n", "path": "README.md"}) + assert out == "Error: 'README.md' is a file, not a directory — use github_read_file to read it." diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index d88f3a8..fc82c9c 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -372,3 +372,89 @@ async def fake_gh(args, **kw): with patch("ghplugin.read_tools.run_gh", new=AsyncMock(side_effect=fake_gh)): out = await _read_pr_file_tool().ainvoke({"repo": "o/r", "number": 1, "path": "x.ts"}) assert "could not resolve the head SHA" in out + + +# ── v0.6.0: the descriptions the model sees are TRUE ───────────────────────────── + + +def test_no_tool_description_carries_a_todo_or_stub_note(): + """`github_read_file`'s docstring (= the tool description the model reads) shipped a + `TODO(team): implement via …` for five releases while the tool worked fine.""" + for t in get_read_tools(): + desc = (t.description or "").lower() + assert "todo" not in desc and "stub" not in desc, f"{t.name}: {t.description!r}" + + +@pytest.mark.asyncio +async def test_repo_contents_on_a_file_path_returns_an_error_string(): + """The contents API returns a dict for a FILE — iterating it raised through the tool layer.""" + body = json.dumps({"type": "file", "name": "README.md", "path": "README.md", "size": 42}) + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(0, body, ""))): + result = await _repo_contents_tool().ainvoke({"repo": "owner/name", "path": "README.md"}) + assert result == "Error: 'README.md' is a file, not a directory — use github_read_file to read it." + + +@pytest.mark.asyncio +async def test_repo_contents_skips_non_dict_rows(): + body = json.dumps([{"name": "a", "path": "a", "type": "file", "size": 1}, None, "x"]) + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(0, body, ""))): + result = await _repo_contents_tool().ainvoke({"repo": "owner/name", "path": ""}) + assert "1 item(s)" in result and " a (a)" in result + + +@pytest.mark.asyncio +async def test_get_pr_wrong_json_type_is_an_error_string(): + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(0, "[1, 2]", ""))): + result = await _get_pr_tool().ainvoke({"repo": "owner/name", "number": 1}) + assert result.startswith("Error: unexpected gh output (expected a JSON dict, got list)") + + +@pytest.mark.asyncio +async def test_unauthenticated_gh_is_a_classified_error(monkeypatch): + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.delenv("GH_REPO", raising=False) + with patch( + "ghplugin.read_tools.run_gh", + new=AsyncMock(return_value=(4, "", "To get started with GitHub CLI, please run: gh auth login")), + ): + result = await _list_issues_tool("owner/name").ainvoke({}) + assert result.startswith("Error: GitHub CLI is not authenticated — run `gh auth login`") + + +@pytest.mark.asyncio +async def test_repo_not_found_is_a_classified_error(): + serr = "GraphQL: Could not resolve to a Repository with the name 'owner/gone'. (repository)" + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(1, "", serr))): + result = await _list_issues_tool().ainvoke({"repo": "owner/gone"}) + assert result.startswith("Error: repo 'owner/gone' not found or not accessible") + + +# ── github_status ──────────────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_status_tool_summarises_and_names_a_bad_default(monkeypatch): + tools = {t.name: t for t in get_read_tools("just-owner", ["o/a"])} + with patch("ghplugin.status.resolve_gh", return_value=None): + out = await tools["github_status"].ainvoke({}) + assert out.startswith("GitHub CLI is NOT installed") + assert "NOTE: Error: github.default_repo must be 'owner/name' (got 'just-owner')" in out + assert "1 repo(s) in the picker: o/a" in out + + +@pytest.mark.asyncio +async def test_status_tool_reads_default_and_repos_through_getters(): + calls = {"d": 0, "r": 0} + + def d(): + calls["d"] += 1 + return "o/live" + + def r(): + calls["r"] += 1 + return ["o/live", "o/x"] + + tools = {t.name: t for t in get_read_tools(d, r)} + with patch("ghplugin.status.resolve_gh", return_value=None): + out = await tools["github_status"].ainvoke({}) + assert "Default repo: o/live." in out and calls == {"d": 1, "r": 1} diff --git a/write_tools.py b/write_tools.py index 1e9c29f..ab929a5 100644 --- a/write_tools.py +++ b/write_tools.py @@ -42,9 +42,10 @@ def _pr_number(url: str) -> str: return tail if tail.isdigit() else "" -def get_write_tools(default_repo: str = "", emit=None) -> list: - """Build the write tools. ``default_repo`` (``owner/name``) is used whenever a tool's - ``repo`` arg is omitted, so an agent with one configured repo needn't repeat it. +def get_write_tools(default_repo="", emit=None) -> list: + """Build the write tools. ``default_repo`` (``owner/name``, or a zero-arg getter + returning it — the live-config case) is used whenever a tool's ``repo`` arg is + omitted, so an agent with one configured repo needn't repeat it. ``emit`` is the host's namespaced event-bus seam (ADR 0039; ``registry.emit``). When given, PR lifecycle events are broadcast as ``github.pr.opened`` / ``github.pr.merged`` @@ -80,7 +81,7 @@ async def github_create_issue(title: str, repo: str = "", body: str = "", labels if label: args += ["--label", label] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err return out.strip() @@ -100,7 +101,7 @@ async def github_comment(number: int, body: str, repo: str = "") -> str: return err args = ["issue", "comment", str(number), "--repo", repo, "--body", body] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err return out.strip() @@ -135,7 +136,7 @@ async def github_create_pr(head: str, title: str, repo: str = "", body: str = "" body, ] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err url = out.strip() _emit( @@ -173,13 +174,13 @@ async def github_edit_pr(number: int, repo: str = "", title: str = "", body: str if body: args += ["--body", body] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err url = out.strip() if state: args = ["pr", "ready", str(number), "--repo", repo] + (["--undo"] if state == "draft" else []) rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err url = url or out.strip() return url or f"Edited PR #{number} in {repo}." @@ -225,7 +226,7 @@ async def github_merge_pr( if delete_branch: args.append("--delete-branch") rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err _emit("pr.merged", {"repo": repo, "number": str(number), "method": method}) return out.strip() or f"Merged PR #{number} in {repo} via {method}." @@ -255,7 +256,7 @@ async def github_close( if comment and not reopen: args += ["--comment", comment] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err return out.strip() or f"{'Reopened' if reopen else 'Closed'} {kind} #{number} in {repo}." @@ -288,7 +289,7 @@ async def github_set_labels( for label in removes: args += ["--remove-label", label] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err return out.strip() or f"Updated labels on {kind} #{number} in {repo}." @@ -321,7 +322,7 @@ async def github_set_assignees( for user in removes: args += ["--remove-assignee", user] rc, out, serr = await run_gh(args) - if gh_err := check_gh_error(rc, serr): + if gh_err := check_gh_error(rc, serr, repo=repo): return gh_err return out.strip() or f"Updated assignees on {kind} #{number} in {repo}." From fafc7cf09ca918ec07a666f912c4aa88daa0f66b Mon Sep 17 00:00:00 2001 From: Josh Mabry Date: Sat, 22 Aug 2026 16:30:25 -0700 Subject: [PATCH 4/6] feat(first-run): /status route, setup card in both views, setup-gap probe, live config wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - status.py: compute_status() probes gh --version + gh auth status --json hosts (text fallback for an older gh) → {gh_path, gh_version, authenticated, login, host, token_source, error, default_repo, repos}; the ACTIVE account decides; bounded timeout; never raises. summarize_status() is the github_status tool's paragraph; gaps_for() / report_gaps() push 'gh' / 'auth' messages to the host's report_setup_gap seam (guarded — no seam, no-op) and clear them when healthy; probe_in_background() runs that off the register() hot path in a daemon thread, only when the seam exists. - GET /api/plugins/github/status (gated data router) — plus default_repo_error. resolve_config() is the ONE picker/default computation shared by /config, /status and POST /issue: a malformed default_repo is named, kept out of the picker, and the default falls through (#23). - view.py: a shared setup card (_SETUP_CSS/_SETUP_JS spliced into BOTH pages) rendered from /status inside the kit boot only (post-handshake): gh missing → install + sign-in copy; not signed in → gh auth login or paste a token in Settings ▸ GitHub (github.token); malformed default repo → the named error; a Re-check button. The existing 'No repositories configured' state stays (and the new-issue form now says it too). - __init__.py: everything closes over LIVE getters (registry.live_config, else the snapshot) — the tools' default repo, the picker, /issue, the data router, and the github.token secret via set_token_getter; the setup probe starts when the host has the seam. Co-Authored-By: Claude Fable 5 --- __init__.py | 83 +++++++++--- api.py | 90 +++++++++---- status.py | 230 +++++++++++++++++++++++++++++++ tests/test_board_view.py | 117 ++++++++++++++++ tests/test_register.py | 91 +++++++++++++ tests/test_status.py | 282 +++++++++++++++++++++++++++++++++++++++ view.py | 78 ++++++++++- 7 files changed, 922 insertions(+), 49 deletions(-) create mode 100644 status.py create mode 100644 tests/test_status.py diff --git a/__init__.py b/__init__.py index 23a9858..79c4e47 100644 --- a/__init__.py +++ b/__init__.py @@ -6,16 +6,24 @@ ADR 0019), so the same plugin serves a read-only research agent and a write-capable coding/PM agent. -It also owns the user-only `/issue` chat control command (the write the model must -NOT do autonomously): registered via the host's `register_chat_command` seam when the -host provides it, reading the configured `default_repo`/`repos` for routing. On an -older host without that seam, `/issue` is simply skipped — the tools still load. +It also owns the user-only `/issue` chat control command — the path a PERSON files +from, on any agent, without the model: registered via the host's +`register_chat_command` seam when the host provides it. (The `github_create_issue` +AGENT tool exists too, behind the write gate.) On an older host without that seam, +`/issue` is simply skipped — the tools still load. And it serves its own console board view (two tabs: Issues / PRs) via two routers (public PAGE + gated DATA, the notes pattern) when the host exposes `register_router`. -Every host-coupled registration (chat command, routers) is `hasattr`-guarded so the -plugin degrades gracefully on an older host: the tools always load. +Config is read LIVE (v0.6.0): the tools' default repo, the `/issue` command's +routing, the views' picker, and the `github.token` secret all go through a getter +(`registry.live_config` when the host has it, else the register-time snapshot), so +a Settings edit — or a project the agent onboards mid-session — is seen by the very +next call, not the next register(). + +Every host-coupled registration (chat command, routers, setup-gap reporting) is +`hasattr`-guarded so the plugin degrades gracefully on an older host: the tools +always load. Host-only imports stay LAZY (none here) so the test suite imports the modules with no protoAgent host present. @@ -32,21 +40,45 @@ def register(registry) -> None: cfg = registry.config or {} write_enabled = bool(cfg.get("write", False)) - # The default repo the tools fall back to when their `repo` arg is omitted — the - # configured `default_repo`, else the first of `repos`, else the first repo in the - # host's ADR 0095 managed-projects registry (same resolution as /issue and the - # board). Tools are rebuilt on a config reload, so capturing it here is live enough. + # LIVE config: the host's `live_config` re-reads the resolved section (manifest + # defaults ⊕ YAML ⊕ secrets) per call; without it (older host, tests) the + # register-time snapshot is the fixed answer. Everything below closes over THIS, + # never over a value computed once here. + live = getattr(registry, "live_config", None) + get_cfg = live if callable(live) else (lambda: cfg) + + def current_cfg() -> dict: + try: + return get_cfg() or {} + except Exception: # noqa: BLE001 — a failing host read ⇒ the snapshot + return cfg + + from .gh_cli import set_token_getter from .gh_issue import effective_default_repo from .projects import effective_repos - default_repo = effective_default_repo(cfg.get("default_repo", ""), effective_repos(cfg.get("repos"))) + def current_repos() -> list[str]: + """The picker list, live: explicit `repos` ∪ the host's project registry ∪ + the registered checkouts' origin remotes (projects.py).""" + return effective_repos(current_cfg().get("repos")) + + def current_default_repo() -> str: + """The default the tools / `/issue` fall back to when no repo is passed — + the configured `default_repo`, else the first of `repos` (same resolution + as the board), evaluated per call.""" + return effective_default_repo(str(current_cfg().get("default_repo") or ""), current_repos()) + + # The `github.token` secret (Settings ▸ GitHub) — injected into every `gh` run as + # GH_TOKEN, winning over an ambient env token. Read live, so a pasted token works + # without a restart. Process-wide by design: one `gh` runner per process. + set_token_getter(lambda: str(current_cfg().get("token") or "")) # READ tools — always available (they return an error string if `gh`/auth is missing). n_read = 0 try: from .read_tools import get_read_tools - read = get_read_tools(default_repo) + read = get_read_tools(current_default_repo, current_repos) for t in read: registry.register_tool(t) n_read = len(read) @@ -63,7 +95,9 @@ def register(registry) -> None: # Pass the host's event-bus seam (ADR 0039) when it exists so PR lifecycle # events broadcast as `github.pr.opened` / `github.pr.merged`. hasattr-guarded # like the other host couplings — an older host just gets no events. - write = get_write_tools(default_repo, emit=getattr(registry, "emit", None)) + get_review_tools(default_repo) + write = get_write_tools(current_default_repo, emit=getattr(registry, "emit", None)) + get_review_tools( + current_default_repo + ) for t in write: registry.register_tool(t) n_write = len(write) @@ -80,7 +114,7 @@ def register(registry) -> None: async def _issue(rest: str, session_id: str) -> str: """File a GitHub issue (user-only). Usage: /issue [--bug|--feature] [--repo owner/name].""" - return await run_issue_command(rest, default_repo=default_repo) + return await run_issue_command(rest, default_repo=current_default_repo()) registry.register_chat_command("issue", _issue) issue_cmd = True @@ -97,20 +131,31 @@ async def _issue(rest: str, session_id: str) -> str: from .api import build_data_router, build_view_router registry.register_router(build_view_router(), prefix="/plugins/github") - # Pass a LIVE config getter when the host offers one (registry.live_config), + # The data router reads config per request through the same live getter, # so a repo/default_repo edit shows in the board with no server restart — a # hot-reload can't re-mount this router, but reading config per request does. - # Older hosts (no live_config) fall back to the register-time snapshot. - get_cfg = registry.live_config if hasattr(registry, "live_config") else cfg - registry.register_router(build_data_router(get_cfg), prefix="/api/plugins/github") + registry.register_router(build_data_router(current_cfg), prefix="/api/plugins/github") view = True except Exception: # noqa: BLE001 log.exception("[github] registering the board view failed") + # First-run setup gaps — `gh` missing / not authenticated — reported to the host's + # operator-warning seam (`report_setup_gap`, newer than this plugin's floor, so + # guarded: no seam ⇒ no thread). Off the hot path: a daemon thread probes `gh` + # and reports (or clears) the gaps; register() never waits on it. + probe = False + try: + from .status import probe_in_background + + probe = probe_in_background(registry, current_default_repo, current_repos) is not None + except Exception: # noqa: BLE001 + log.debug("[github] status probe not started", exc_info=True) + log.info( - "[github] registered %d read tool(s)%s%s%s", + "[github] registered %d read tool(s)%s%s%s%s", n_read, f" + {n_write} write tool(s) (write enabled)" if write_enabled else " (read-only — github.write is false)", " + /issue command" if issue_cmd else "", " + board view" if view else "", + " + setup probe" if probe else "", ) diff --git a/api.py b/api.py index 1709d8a..1f0cb4c 100644 --- a/api.py +++ b/api.py @@ -7,9 +7,13 @@ bearer gate), fetched from inside the loaded page with the postMessage handshake token (the DS plugin-kit's ``apiFetch``). -The data logic lives in plain async functions (``fetch_issues`` / ``fetch_prs``) so -the suite can test it host-free; the FastAPI imports stay lazy inside the build_* -functions (fastapi is the host's at runtime). +The data logic lives in plain async functions (``fetch_issues`` / ``fetch_prs`` / +``status.compute_status``) so the suite can test it host-free; the FastAPI imports +stay lazy inside the build_* functions (fastapi is the host's at runtime). + +``GET /status`` is the first-run probe (v0.6.0): is `gh` installed, authenticated, +as whom — the views render a setup card from it when something's missing, and +``/config`` names a malformed ``default_repo`` (#23) instead of feeding it to `gh`. """ from __future__ import annotations @@ -17,7 +21,7 @@ import json import shutil -from .gh_cli import bad_repo, run_gh +from .gh_cli import bad_repo, check_gh_error, resolve_gh, run_gh # The JSON fields we ask `gh` for — kept lean: enough for a board row + the detail link. _ISSUE_FIELDS = "number,title,state,author,labels,url,createdAt,comments" @@ -25,8 +29,8 @@ def gh_available() -> bool: - """Whether the `gh` CLI is on PATH (the board shows a hint when it isn't).""" - return shutil.which("gh") is not None + """Whether the `gh` CLI can be found (PATH, then the usual install dirs).""" + return resolve_gh() is not None or shutil.which("gh") is not None def _norm_state(state: str) -> str | None: @@ -50,8 +54,8 @@ async def fetch_issues(repo: str, state: str = "open", limit: int = 30) -> dict: rc, out, serr = await run_gh( ["issue", "list", "--repo", repo, "--state", norm, "--limit", str(capped), "--json", _ISSUE_FIELDS] ) - if rc != 0: - return {"error": f"Error (gh exit {rc}): {serr[:300]}"} + if gh_err := check_gh_error(rc, serr, repo=repo): + return {"error": gh_err} try: return {"items": json.loads(out or "[]")} except json.JSONDecodeError: @@ -72,8 +76,8 @@ async def fetch_prs(repo: str, state: str = "open", limit: int = 30) -> dict: rc, out, serr = await run_gh( ["pr", "list", "--repo", repo, "--state", norm, "--limit", str(capped), "--json", _PR_FIELDS] ) - if rc != 0: - return {"error": f"Error (gh exit {rc}): {serr[:300]}"} + if gh_err := check_gh_error(rc, serr, repo=repo): + return {"error": gh_err} try: return {"items": json.loads(out or "[]")} except json.JSONDecodeError: @@ -81,13 +85,39 @@ async def fetch_prs(repo: str, state: str = "open", limit: int = 30) -> dict: def _repos(cfg: dict) -> list[str]: - """The picker list — explicit ``github.repos``, else the host's ADR 0095 - managed-projects registry (v0.115.0+; ``[]`` on older hosts).""" + """The picker list — the explicit ``github.repos`` entries (first, operator order) + UNION the host's ADR 0095 managed-projects registry (v0.115.0+) UNION the repos + parsed from the registered checkouts' ``origin`` remotes (last). ``[]`` with no host + and nothing configured. See projects.py.""" from .projects import effective_repos return effective_repos(cfg.get("repos")) +def resolve_config(cfg: dict) -> dict: + """The resolved picker/default for a config dict — ONE computation shared by + ``/config``, ``/status`` and the issue route, so they can't disagree: + ``{"repos": [...], "default_repo": "...", "default_repo_error": str|None}``. + + A malformed ``github.default_repo`` (not ``owner/name``, #23) is NOT fed into the + picker or used as the default — it's reported as the named ``default_repo_error`` + and the default falls through to the first good picker entry (the views render + the error so the person fixes the field rather than seeing a raw `gh` failure). + """ + from .gh_issue import default_repo_error, effective_default_repo + + raw_default = str(cfg.get("default_repo") or "").strip() + err = default_repo_error(raw_default) + repos = _repos(cfg) + default = effective_default_repo("" if err else raw_default, repos) + # The picker is built from `repos`. A very common config sets `default_repo` but + # leaves `repos` empty — without folding the default in, the picker has zero + # options and the board shows "nothing to select" even though a repo IS configured. + # Surface the resolved default as a selectable option (first), deduped. + selectable = [default, *repos] if default and default not in repos else repos + return {"repos": selectable, "default_repo": default, "default_repo_error": err} + + def build_view_router(): """The PAGES — served under the PUBLIC ``/plugins/github`` prefix (ungated): the read-only board (``/view``) and the compact file-an-issue form (``/new-issue``).""" @@ -121,7 +151,8 @@ def build_data_router(cfg): """ from fastapi import APIRouter, Body - from .gh_issue import IssueRequest, effective_default_repo, file_issue, labels_for, resolve_repo + from .gh_issue import IssueRequest, file_issue, labels_for, resolve_repo + from .status import compute_status get_cfg = cfg if callable(cfg) else (lambda: cfg) @@ -129,19 +160,17 @@ def build_data_router(cfg): @router.get("/config") async def _config() -> dict: - current = get_cfg() - repos = _repos(current) - default = effective_default_repo(current.get("default_repo", ""), repos) - # The picker is built from `repos`. A very common config sets `default_repo` but - # leaves `repos` empty — without folding the default in, the picker has zero - # options and the board shows "nothing to select" even though a repo IS configured. - # Surface the resolved default as a selectable option (first), deduped. - selectable = [default, *repos] if default and default not in repos else repos - return { - "repos": selectable, - "default_repo": default, - "gh_available": gh_available(), - } + resolved = resolve_config(get_cfg() or {}) + return {**resolved, "gh_available": gh_available()} + + @router.get("/status") + async def _status() -> dict: + """The first-run probe: `gh` path + version, auth state (login/host), the token + source, and the resolved repos — never raises (a failure is ``error``).""" + resolved = resolve_config(get_cfg() or {}) + st = await compute_status(resolved["default_repo"], resolved["repos"]) + st["default_repo_error"] = resolved["default_repo_error"] + return st @router.get("/issues") async def _issues(repo: str, state: str = "open") -> dict: @@ -153,18 +182,23 @@ async def _prs(repo: str, state: str = "open") -> dict: @router.post("/issue") async def _create_issue(body: dict = Body(...)) -> dict: - current = get_cfg() + resolved = resolve_config(get_cfg() or {}) kind = (body.get("kind") or "generic").lower() if kind not in ("bug", "feature", "generic"): kind = "generic" title = (body.get("title") or "").strip() issue_body = (body.get("body") or "").strip() - repo = resolve_repo(body.get("repo"), effective_default_repo(current.get("default_repo", ""), _repos(current))) + explicit = str(body.get("repo") or "").strip() + repo = resolve_repo(explicit, resolved["default_repo"]) labels = labels_for(kind, [str(x) for x in (body.get("labels") or [])]) dry_run = bool(body.get("dry_run")) if not title: return {"ok": False, "error": "Title is required."} if not repo: + # No usable repo anywhere — and if the configured default is the reason + # (malformed, #23), say THAT, by name, rather than "set one". + if resolved["default_repo_error"]: + return {"ok": False, "error": resolved["default_repo_error"]} return {"ok": False, "error": "No target repo — set one in Settings ▸ GitHub, or pick one."} if bad_repo(repo): return {"ok": False, "error": f"Repo must be 'owner/name' (got {repo!r})."} diff --git a/status.py b/status.py new file mode 100644 index 0000000..9c25c64 --- /dev/null +++ b/status.py @@ -0,0 +1,230 @@ +"""Setup status — is `gh` installed, is it authenticated, and as whom? + +The first-run question. A fresh desktop install with the Project Manager archetype +enabled gets a GitHub rail that silently shows "No repositories configured" or a +raw `gh` stderr, with nothing telling the person that `gh` is missing or logged +out. This module answers that ONE question three ways from one computation: + +- ``GET /api/plugins/github/status`` (api.py) → the setup card in both views; +- the ``github_status`` read tool → a one-paragraph summary the model can act on + instead of guessing from a tool error; +- ``report_gaps`` → the host's operator-warning seam (``registry.report_setup_gap``, + guarded — the seam is newer than this plugin's floor), called from a best-effort + background probe at register time so the warning shows before anything is used. + +Everything here is host-free and never raises: a probe failure is a status with +``error`` set, not an exception (the routes/tools/threads calling it must not die). +""" + +from __future__ import annotations + +import asyncio +import json +import logging +import re +import threading + +from .gh_cli import resolve_gh, run_gh, token_source + +log = logging.getLogger("protoagent.plugins.github") + +_PROBE_TIMEOUT = 10 # auth status makes a network call; keep the card snappy + +_VERSION_RE = re.compile(r"gh version (\S+)") +# The text shape (older gh without --json): "✓ Logged in to github.com account kj (keyring)" +_LOGGED_IN_RE = re.compile(r"Logged in to (\S+) account (\S+)") +_FAILED_RE = re.compile(r"(?:X|✗)\s+Failed to log in to (\S+)") + +# Setup-gap keys (the host surfaces them as operator warnings, one per key). +GAP_GH = "gh" +GAP_AUTH = "auth" + +_AUTH_HINT = "run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)" + + +def _empty(default_repo: str = "", repos: list[str] | None = None) -> dict: + return { + "gh_path": None, + "gh_version": None, + "authenticated": False, + "login": None, + "host": None, + "token_source": token_source(), + "error": None, + "default_repo": default_repo, + "repos": list(repos or []), + } + + +def _parse_auth_json(out: str) -> tuple[bool, str | None, str | None, str | None]: + """(authenticated, login, host, error) from `gh auth status --json hosts`. + The ACTIVE account decides — a failing active token with a healthy inactive + keyring login is still "not authenticated" (that's what gh would use).""" + d = json.loads(out or "{}") + hosts = d.get("hosts") or {} + if not hosts: + return False, None, None, "not logged in" + accounts: list[dict] = [a for entries in hosts.values() for a in (entries or []) if isinstance(a, dict)] + active = [a for a in accounts if a.get("active")] or accounts[:1] + if not active: + return False, None, None, "not logged in" + a = active[0] + host = str(a.get("host") or next(iter(hosts), "") or "") or None + if str(a.get("state") or "") == "success": + return True, str(a.get("login") or "") or None, host, None + err = str(a.get("error") or "token rejected") + # gh embeds the whole HTTP body; keep the first line only. + return False, None, host, err.splitlines()[0][:200] + + +def _parse_auth_text(rc: int, out: str, serr: str) -> tuple[bool, str | None, str | None, str | None]: + """Text fallback for a gh without `--json` on `auth status`.""" + blob = "\n".join(x for x in (out, serr) if x) + # Blocks are separated by blank lines; the one with "Active account: true" decides. + blocks = [b for b in re.split(r"\n\s*\n", blob) if b.strip()] + active = [b for b in blocks if re.search(r"Active account:\s*true", b)] or blocks[:1] + for b in active: + if m := _LOGGED_IN_RE.search(b): + return True, m.group(2), m.group(1), None + if m := _FAILED_RE.search(b): + detail = next( + (ln.strip(" -") for ln in b.splitlines() if "token" in ln.lower() and "invalid" in ln.lower()), "" + ) + return False, None, m.group(1), detail or "active token rejected" + if rc != 0 or "not logged in" in blob.lower() or "gh auth login" in blob.lower(): + return False, None, None, "not logged in" + return False, None, None, (serr or out or "could not read auth status")[:200] + + +async def compute_status(default_repo: str = "", repos: list[str] | None = None) -> dict: + """Probe `gh` (binary → version → auth) and return the status dict: + ``{gh_path, gh_version, authenticated, login, host, token_source, error, + default_repo, repos}``. Never raises; every failure lands in ``error``.""" + st = _empty(default_repo, repos) + try: + path = resolve_gh() + if not path: + st["error"] = "gh CLI is not installed or not on PATH" + return st + st["gh_path"] = path + + rc, out, serr = await run_gh(["--version"], timeout=_PROBE_TIMEOUT) + if rc != 0: + st["error"] = f"`gh --version` failed (exit {rc}): {(serr or out)[:200]}" + return st + m = _VERSION_RE.search(out) + st["gh_version"] = m.group(1) if m else (out.splitlines()[0][:40] if out else None) + + rc, out, serr = await run_gh(["auth", "status", "--json", "hosts"], timeout=_PROBE_TIMEOUT) + if rc == 0 and out.lstrip().startswith("{"): + try: + ok, login, host, err = _parse_auth_json(out) + except (ValueError, TypeError): + ok, login, host, err = _parse_auth_text(rc, out, serr) + else: + if "unknown flag" in (serr or "").lower() or "--json" in (serr or ""): + rc, out, serr = await run_gh(["auth", "status"], timeout=_PROBE_TIMEOUT) + ok, login, host, err = _parse_auth_text(rc, out, serr) + st.update(authenticated=ok, login=login, host=host, error=err) + return st + except Exception as e: # noqa: BLE001 — a probe must never take a route/tool/thread down + st["error"] = f"status probe failed: {type(e).__name__}: {e}"[:300] + return st + + +def summarize_status(st: dict) -> str: + """One paragraph a model (or a person) can act on — the same facts as the dict.""" + repos = st.get("repos") or [] + default = st.get("default_repo") or "" + repo_bit = ( + f" Default repo: {default}." + if default + else " No default repo configured (pass repo='owner/name' per call, or set one in Settings ▸ GitHub)." + ) + ( + f" {len(repos)} repo(s) in the picker: {', '.join(repos[:8])}{'…' if len(repos) > 8 else ''}." if repos else "" + ) + src = st.get("token_source") or "none" + src_bit = { + "config": " Token source: Settings ▸ GitHub (github.token).", + "env": " Token source: GITHUB_TOKEN/GH_TOKEN env.", + }.get(src, "") + + if not st.get("gh_path"): + return ( + "GitHub CLI is NOT installed (no `gh` on PATH or in the usual install dirs). " + "Install it — https://cli.github.com (macOS: `brew install gh`; Debian/Ubuntu: `sudo apt install gh`) — " + f"then {_AUTH_HINT}. Every github_* tool will return an error until then." + repo_bit + ) + ver = f" v{st['gh_version']}" if st.get("gh_version") else "" + if not st.get("authenticated"): + why = f" ({st['error']})" if st.get("error") else "" + return ( + f"GitHub CLI{ver} is installed at {st['gh_path']} but NOT authenticated{why}. " + f"To fix: {_AUTH_HINT}. Read tools on public repos may still work at low volume; " + "everything else will return 'not authenticated' until then." + src_bit + repo_bit + ) + who = f" as {st['login']}" if st.get("login") else "" + host = f" on {st['host']}" if st.get("host") else "" + return ( + f"GitHub CLI{ver} is installed at {st['gh_path']} and authenticated{who}{host}. All github_* tools are usable." + + src_bit + + repo_bit + ) + + +def gaps_for(st: dict) -> dict[str, str | None]: + """The setup-gap messages this status implies, keyed by gap id; ``None`` = clear.""" + gaps: dict[str, str | None] = {GAP_GH: None, GAP_AUTH: None} + if not st.get("gh_path"): + gaps[GAP_GH] = ( + "GitHub CLI (`gh`) is not installed or not on PATH — the GitHub tools and rail won't work. " + "Install it from https://cli.github.com, then run `gh auth login`." + ) + return gaps + if not st.get("authenticated"): + why = f" ({st['error']})" if st.get("error") else "" + gaps[GAP_AUTH] = f"GitHub CLI is not authenticated{why} — {_AUTH_HINT}." + return gaps + + +def report_gaps(registry, st: dict) -> bool: + """Push this status' gaps to the host's ``report_setup_gap(key, message|None)`` seam + when the host has one; clears a gap (``None``) when it's healthy. Guarded: an + older host (no seam) or a seam that raises is a no-op. Returns whether it reported.""" + fn = getattr(registry, "report_setup_gap", None) + if not callable(fn): + return False + try: + for key, msg in gaps_for(st).items(): + fn(key, msg) + return True + except Exception: # noqa: BLE001 — telemetry must never break register() + log.debug("[github] report_setup_gap failed", exc_info=True) + return False + + +def probe_in_background(registry, default_repo="", repos=None) -> threading.Thread | None: + """Compute the status off the register() hot path and report its gaps. Only + started when the host exposes the seam (nothing to report otherwise). The thread + is a daemon and swallows everything — boot must never wait on, or die from, it. + ``default_repo`` / ``repos`` may be values or zero-arg getters (evaluated IN the + thread, so the git-remote parsing they may do never blocks register()).""" + if not callable(getattr(registry, "report_setup_gap", None)): + return None + + def _run() -> None: + try: + d = default_repo() if callable(default_repo) else default_repo + r = repos() if callable(repos) else repos + st = asyncio.run(compute_status(str(d or ""), list(r or []))) + report_gaps(registry, st) + if st.get("gh_path") and st.get("authenticated"): + log.info("[github] gh %s authenticated as %s", st.get("gh_version") or "?", st.get("login") or "?") + else: + log.warning("[github] setup gap: %s", summarize_status(st)) + except Exception: # noqa: BLE001 + log.debug("[github] background status probe failed", exc_info=True) + + t = threading.Thread(target=_run, name="github-plugin-status-probe", daemon=True) + t.start() + return t diff --git a/tests/test_board_view.py b/tests/test_board_view.py index b2801e3..68939f6 100644 --- a/tests/test_board_view.py +++ b/tests/test_board_view.py @@ -254,3 +254,120 @@ def test_manifest_declares_board_and_widget_views(): assert new_issue["utility"]["info"] # a util-bar pill with hover info assert new_issue["palette"]["path"] == "/plugins/github/new-issue" # distinct ⌘K page assert new_issue["path"] == "/plugins/github/new-issue" + + +# --- v0.6.0: /status, the named default_repo error (#23), the setup card ------- + + +def _status_gh(*, auth=(0, '{"hosts":{}}', "You are not logged into any GitHub hosts.")): + async def run_gh(args, timeout=None, **kw): + if args == ["--version"]: + return (0, "gh version 2.92.0 (2026-04-28)", "") + if args[:2] == ["auth", "status"]: + return auth + raise AssertionError(f"unexpected gh call: {args}") + + return run_gh + + +def test_status_route_shape_when_unauthenticated(): + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/local/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_status_gh())), + ): + body = TestClient(_app()).get("/api/plugins/github/status").json() + assert body["gh_path"] == "/usr/local/bin/gh" and body["gh_version"] == "2.92.0" + assert body["authenticated"] is False and body["login"] is None + assert body["error"] == "not logged in" + assert body["default_repo"] == "o/n" and body["repos"] == ["o/n", "o/m"] + assert body["default_repo_error"] is None + for k in ("host", "token_source"): + assert k in body + + +def test_status_route_when_gh_is_missing_never_errors(): + with patch("ghplugin.status.resolve_gh", return_value=None): + r = TestClient(_app()).get("/api/plugins/github/status") + assert r.status_code == 200 + body = r.json() + assert body["gh_path"] is None and body["authenticated"] is False and "not installed" in body["error"] + + +def test_status_route_authenticated(): + ok = '{"hosts":{"github.com":[{"state":"success","active":true,"host":"github.com","login":"kj"}]}}' + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_status_gh(auth=(0, ok, "")))), + ): + body = TestClient(_app()).get("/api/plugins/github/status").json() + assert body["authenticated"] is True and body["login"] == "kj" and body["host"] == "github.com" + + +def test_config_names_a_malformed_default_repo_and_keeps_it_out_of_the_picker(): + """#23 — `default_repo: protoLabsAI` (no slash) used to be fed straight to `gh`. Now it's + the named error, the picker carries only well-formed entries, and the default falls + through to the first good one.""" + body = ( + TestClient(_app({"repos": ["o/good"], "default_repo": "protoLabsAI"})).get("/api/plugins/github/config").json() + ) + assert body["default_repo_error"].startswith("Error: github.default_repo must be 'owner/name' (got 'protoLabsAI')") + assert body["repos"] == ["o/good"] and body["default_repo"] == "o/good" + + +def test_config_default_repo_error_is_none_when_well_formed_or_blank(): + assert TestClient(_app()).get("/api/plugins/github/config").json()["default_repo_error"] is None + assert ( + TestClient(_app({"repos": [], "default_repo": ""})) + .get("/api/plugins/github/config") + .json()["default_repo_error"] + is None + ) + + +def test_status_route_carries_the_default_repo_error_too(): + with patch("ghplugin.status.resolve_gh", return_value=None): + body = TestClient(_app({"repos": [], "default_repo": "nope"})).get("/api/plugins/github/status").json() + assert "must be 'owner/name'" in body["default_repo_error"] + + +def test_create_issue_route_names_a_malformed_default_when_nothing_else_resolves(): + fake = AsyncMock() + with patch("ghplugin.gh_issue.run_gh", fake): + body = ( + TestClient(_app({"repos": [], "default_repo": "just-owner"})) + .post("/api/plugins/github/issue", json={"title": "T", "body": "x"}) + .json() + ) + assert body["ok"] is False and body["error"].startswith( + "Error: github.default_repo must be 'owner/name' (got 'just-owner')" + ) + fake.assert_not_called() + + +def test_pages_render_the_setup_card_from_status_inside_the_kit_boot(): + """Both views fetch /status ONLY from inside the kit's boot (post-handshake, #2926) and + carry the setup card with the three remedies: install gh / gh auth login / the token.""" + from ghplugin.view import NEW_ISSUE_PAGE, PAGE + + for page in (PAGE, NEW_ISSUE_PAGE): + assert "/api/plugins/github/status" in page + assert 'id="setup"' in page and "function setupCard" in page + # the fetch is reached from boot(), which the kit invokes — not at top level + boot_body = page.split("async function boot(){", 1)[1].split("\n }", 1)[0] + assert "checkStatus();" in boot_body + assert "cli.github.com" in page and "gh auth login" in page and "github.token" in page + assert "Default repo is malformed" in page # #23 surfaces in the views too + assert "No repositories configured" in page # the existing empty state stays + + +def test_register_wires_the_data_router_to_the_live_config(make_registry): + """register() passes a getter (not the snapshot) so the routes see config edits.""" + reg = make_registry({"repos": ["o/n"], "default_repo": "o/n"}) + register(reg) + data = next(r["router"] for r in reg.routers if r["prefix"] == "/api/plugins/github") + app = FastAPI() + app.include_router(data, prefix="/api/plugins/github") + c = TestClient(app) + assert c.get("/api/plugins/github/config").json()["repos"] == ["o/n"] + reg.config["repos"] = ["o/n", "o/added"] # the (shared) config dict is edited after register() + assert c.get("/api/plugins/github/config").json()["repos"] == ["o/n", "o/added"] diff --git a/tests/test_register.py b/tests/test_register.py index 87facb7..913f2f5 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -17,6 +17,7 @@ "github_run_failure", "github_read_file", "github_repo_contents", + "github_status", # the self-diagnosis probe — always on, no write gate (v0.6.0) } WRITE_TOOLS = { "github_create_issue", @@ -61,3 +62,93 @@ def test_tools_have_descriptions(make_registry): for t in reg.tools: desc = getattr(t, "description", None) assert desc, f"tool {getattr(t, 'name', t)!r} has no description" + + +def test_github_status_is_never_write_gated(make_registry): + """The model must be able to self-diagnose a broken `gh` on a read-only agent.""" + reg = make_registry({"write": False}) + register(reg) + assert "github_status" in reg.tool_names + + +async def test_tools_default_repo_is_live_not_a_register_time_snapshot(make_registry, monkeypatch): + """v0.6.0 — an `onboard_project` / Settings edit mid-session must reach the tools' + omitted-repo fallback on the NEXT call, with no re-register. (Here the host has no + live_config, so the snapshot dict IS the live config — editing it is the edit.)""" + from unittest.mock import AsyncMock, patch + + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.delenv("GH_REPO", raising=False) + reg = make_registry({"default_repo": "o/first"}) + register(reg) + tool = {t.name: t for t in reg.tools}["github_list_issues"] + fake = AsyncMock(return_value=(0, "[]", "")) + with patch("ghplugin.read_tools.run_gh", fake): + await tool.ainvoke({}) + assert fake.call_args.args[0][fake.call_args.args[0].index("--repo") + 1] == "o/first" + reg.config["default_repo"] = "o/second" # the operator edits Settings + await tool.ainvoke({}) + assert fake.call_args.args[0][fake.call_args.args[0].index("--repo") + 1] == "o/second" + + +def test_register_prefers_the_hosts_live_config_getter(make_registry): + """A host with `live_config` (protoAgent ≥ the live-config seam) is read per call — + including the `token` secret, which is wired into the gh runner.""" + from ghplugin import gh_cli + + class _Live(make_registry): + def __init__(self, config): + super().__init__(config) + self.live = dict(config) + + def live_config(self): + return self.live + + reg = _Live({"default_repo": "o/snap", "token": ""}) + register(reg) + try: + assert gh_cli.resolve_token() is None + reg.live["token"] = "ghp_pasted" # pasted in Settings ▸ GitHub, no restart + assert gh_cli.resolve_token() == "ghp_pasted" and gh_cli.token_source() == "config" + finally: + gh_cli.set_token_getter(None) + + +def test_register_starts_the_setup_probe_only_when_the_host_has_the_seam(make_registry, monkeypatch): + """The setup-gap seam is newer than this plugin's floor: with it, a background probe + reports `gh`/`auth` gaps (and clears them when healthy); without it, nothing runs.""" + import threading + from unittest.mock import patch + + from ghplugin import status as status_mod + + started: list[threading.Thread] = [] + real = status_mod.probe_in_background + + def spy(registry, default_repo="", repos=None): + t = real(registry, default_repo, repos) + if t: + started.append(t) + return t + + class _Seam(make_registry): + def __init__(self, config): + super().__init__(config) + self.gaps: dict = {} + + def report_setup_gap(self, key, message): + self.gaps[key] = message + + with patch("ghplugin.status.probe_in_background", spy), patch("ghplugin.status.resolve_gh", return_value=None): + reg = _Seam({}) + register(reg) + for t in started: + t.join(timeout=5) + assert started and not started[0].is_alive() + assert reg.gaps["gh"] and "not installed" in reg.gaps["gh"] and reg.gaps["auth"] is None + + started.clear() + with patch("ghplugin.status.probe_in_background", spy): + plain = make_registry({}) + register(plain) # no seam → no thread + assert started == [] diff --git a/tests/test_status.py b/tests/test_status.py new file mode 100644 index 0000000..ea70d42 --- /dev/null +++ b/tests/test_status.py @@ -0,0 +1,282 @@ +"""status.py — the first-run probe, its summary, and the setup-gap reporting. + +`run_gh` / `resolve_gh` are patched in the `ghplugin.status` namespace so nothing +shells out; the probe must NEVER raise — every failure is a status with `error` set. +""" + +from __future__ import annotations + +import json +import threading +from unittest.mock import AsyncMock, patch + +from ghplugin.status import ( + GAP_AUTH, + GAP_GH, + compute_status, + gaps_for, + probe_in_background, + report_gaps, + summarize_status, +) + +_OK_JSON = json.dumps( + { + "hosts": { + "github.com": [ + {"state": "success", "active": True, "host": "github.com", "login": "kj", "tokenSource": "keyring"} + ] + } + } +) +_BAD_ACTIVE_JSON = json.dumps( + { + "hosts": { + "github.com": [ + { + "state": "error", + "error": 'non-200 OK status code: 401 Unauthorized body: "..."\nmore', + "active": True, + "host": "github.com", + "login": "", + "tokenSource": "GH_TOKEN", + }, + {"state": "success", "active": False, "host": "github.com", "login": "kj", "tokenSource": "keyring"}, + ] + } + } +) + + +def _gh(*, version="gh version 2.92.0 (2026-04-28)\nhttps://...", auth=(0, _OK_JSON, "")): + async def run_gh(args, timeout=None, **kw): + if args == ["--version"]: + return (0, version, "") + if args[:2] == ["auth", "status"]: + return auth + raise AssertionError(f"unexpected gh call: {args}") + + return run_gh + + +# ── compute_status ─────────────────────────────────────────────────────────────── + + +async def test_missing_binary(): + with patch("ghplugin.status.resolve_gh", return_value=None): + st = await compute_status("o/n", ["o/n"]) + assert st["gh_path"] is None and st["authenticated"] is False + assert "not installed" in st["error"] + assert st["default_repo"] == "o/n" and st["repos"] == ["o/n"] + for k in ( + "gh_path", + "gh_version", + "authenticated", + "login", + "host", + "error", + "default_repo", + "repos", + "token_source", + ): + assert k in st + + +async def test_authenticated_via_json(): + with ( + patch("ghplugin.status.resolve_gh", return_value="/opt/homebrew/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_gh())), + ): + st = await compute_status() + assert st["gh_path"] == "/opt/homebrew/bin/gh" + assert st["gh_version"] == "2.92.0" + assert st["authenticated"] is True and st["login"] == "kj" and st["host"] == "github.com" + assert st["error"] is None + + +async def test_active_account_failing_is_not_authenticated_even_with_a_good_inactive_one(): + """gh uses the ACTIVE account — a rejected GH_TOKEN with a healthy keyring login + behind it still fails every command, so status must say so (first line only).""" + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_gh(auth=(0, _BAD_ACTIVE_JSON, "")))), + ): + st = await compute_status() + assert st["authenticated"] is False and st["login"] is None + assert st["error"].startswith("non-200 OK status code: 401") and "\n" not in st["error"] + + +async def test_not_logged_in_json_is_empty_hosts(): + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch( + "ghplugin.status.run_gh", + new=AsyncMock(side_effect=_gh(auth=(0, '{"hosts":{}}', "You are not logged into any GitHub hosts."))), + ), + ): + st = await compute_status() + assert st["authenticated"] is False and st["error"] == "not logged in" + + +async def test_text_fallback_for_an_older_gh_without_json(): + """An older gh rejects --json on auth status; the probe re-runs plain and parses text.""" + calls = [] + + async def run_gh(args, timeout=None, **kw): + calls.append(args) + if args == ["--version"]: + return (0, "gh version 2.30.0 (2023-06-01)", "") + if "--json" in args: + return (1, "", "unknown flag: --json") + return (0, "", "github.com\n ✓ Logged in to github.com account kj (keyring)\n - Active account: true") + + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=run_gh)), + ): + st = await compute_status() + assert st["authenticated"] is True and st["login"] == "kj" and st["host"] == "github.com" + assert ["auth", "status"] in calls # the plain re-run happened + + +async def test_text_fallback_not_logged_in(): + async def run_gh(args, timeout=None, **kw): + if args == ["--version"]: + return (0, "gh version 2.30.0", "") + if "--json" in args: + return (1, "", "unknown flag: --json") + return (1, "", "You are not logged into any GitHub hosts. To log in, run: gh auth login") + + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=run_gh)), + ): + st = await compute_status() + assert st["authenticated"] is False and st["error"] == "not logged in" + + +async def test_version_failure_is_an_error_not_an_exception(): + async def run_gh(args, timeout=None, **kw): + return (1, "", "dyld: missing library") + + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=run_gh)), + ): + st = await compute_status() + assert st["gh_path"] == "/usr/bin/gh" and st["gh_version"] is None + assert "`gh --version` failed" in st["error"] + + +async def test_a_raising_runner_never_escapes(): + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=RuntimeError("boom"))), + ): + st = await compute_status() + assert st["authenticated"] is False and "status probe failed: RuntimeError: boom" in st["error"] + + +# ── summary + gaps ─────────────────────────────────────────────────────────────── + + +def test_summary_missing_binary_tells_how_to_install(): + text = summarize_status({"gh_path": None, "default_repo": "", "repos": []}) + assert "NOT installed" in text and "cli.github.com" in text and "gh auth login" in text + assert "No default repo configured" in text + + +def test_summary_unauthenticated_tells_how_to_fix(): + text = summarize_status( + { + "gh_path": "/usr/bin/gh", + "gh_version": "2.9", + "authenticated": False, + "error": "not logged in", + "default_repo": "o/n", + "repos": ["o/n", "o/m"], + "token_source": "env", + } + ) + assert "NOT authenticated (not logged in)" in text + assert "gh auth login" in text and "Settings ▸ GitHub (github.token)" in text + assert "Default repo: o/n." in text and "2 repo(s) in the picker: o/n, o/m" in text + assert "GITHUB_TOKEN/GH_TOKEN env" in text + + +def test_summary_healthy_is_one_paragraph(): + text = summarize_status( + { + "gh_path": "/usr/bin/gh", + "gh_version": "2.9", + "authenticated": True, + "login": "kj", + "host": "github.com", + "default_repo": "o/n", + "repos": ["o/n"], + "token_source": "config", + } + ) + assert text.startswith("GitHub CLI v2.9 is installed at /usr/bin/gh and authenticated as kj on github.com.") + assert "\n" not in text and "Settings ▸ GitHub (github.token)" in text + + +def test_gaps_for_each_state(): + assert gaps_for({"gh_path": None}) == {GAP_GH: gaps_for({"gh_path": None})[GAP_GH], GAP_AUTH: None} + assert "not installed" in gaps_for({"gh_path": None})[GAP_GH] + g = gaps_for({"gh_path": "/x/gh", "authenticated": False, "error": "not logged in"}) + assert g[GAP_GH] is None and "not authenticated (not logged in)" in g[GAP_AUTH] + assert gaps_for({"gh_path": "/x/gh", "authenticated": True}) == {GAP_GH: None, GAP_AUTH: None} + + +class _Seam: + def __init__(self, raise_on_call=False): + self.calls: list[tuple[str, str | None]] = [] + self.raise_on_call = raise_on_call + + def report_setup_gap(self, key, message): + if self.raise_on_call: + raise RuntimeError("seam broke") + self.calls.append((key, message)) + + +def test_report_gaps_uses_the_seam_and_clears_when_healthy(): + seam = _Seam() + assert report_gaps(seam, {"gh_path": "/x/gh", "authenticated": False, "error": "e"}) is True + assert dict(seam.calls) == {GAP_GH: None, GAP_AUTH: dict(seam.calls)[GAP_AUTH]} + assert dict(seam.calls)[GAP_AUTH].startswith("GitHub CLI is not authenticated") + seam.calls.clear() + report_gaps(seam, {"gh_path": "/x/gh", "authenticated": True}) + assert dict(seam.calls) == {GAP_GH: None, GAP_AUTH: None} # both CLEARED + + +def test_report_gaps_without_the_seam_is_a_noop(): + assert report_gaps(object(), {"gh_path": None}) is False + + +def test_report_gaps_swallows_a_raising_seam(): + assert report_gaps(_Seam(raise_on_call=True), {"gh_path": None}) is False + + +# ── the background probe ───────────────────────────────────────────────────────── + + +def test_probe_not_started_without_the_seam(): + assert probe_in_background(object(), "o/n", []) is None + + +def test_probe_runs_off_thread_reports_and_evaluates_getters_in_thread(): + seam = _Seam() + seen = {} + + def default_getter(): + seen["thread"] = threading.current_thread().name + return "o/n" + + with patch("ghplugin.status.resolve_gh", return_value=None): + t = probe_in_background(seam, default_getter, lambda: ["o/n"]) + assert t is not None + t.join(timeout=5) + assert not t.is_alive() + assert seen["thread"] == "github-plugin-status-probe" # evaluated IN the thread, not at register() + assert dict(seam.calls)[GAP_GH] and dict(seam.calls)[GAP_AUTH] is None diff --git a/view.py b/view.py index d85d6ea..4f7d6aa 100644 --- a/view.py +++ b/view.py @@ -15,10 +15,69 @@ the postMessage handshake); slug-aware base (host window AND the fleet proxy); links the DS plugin-kit so the page is themed from the operator's live ``--pl-*`` tokens. Vanilla JS, no host build (ADR 0038). + +Both pages render a **setup card** (v0.6.0) from ``GET /status`` when `gh` is missing +or not authenticated — or ``default_repo`` is malformed (#23) — telling the person what +to do (install gh / `gh auth login` / paste a token in Settings ▸ GitHub). The card's +CSS + JS are shared (``_SETUP_CSS`` / ``_SETUP_JS``, spliced into both pages) so the +two surfaces can't drift. The fetch happens only inside the kit's ``initPluginView`` +boot (after the bearer handshake — #2926), like every other data call. """ from __future__ import annotations +# --- the shared setup card (spliced into both pages) -------------------------- +_SETUP_CSS = r""" + #setup{display:none;margin:8px;padding:10px 12px;border-radius:var(--pl-radius,8px); + border:1px solid var(--pl-color-border);background:var(--pl-color-bg);line-height:1.45} + #setup.show{display:block} + #setup .st{font-weight:600;margin-bottom:3px;display:flex;align-items:center;gap:6px} + #setup .sd{color:var(--pl-color-fg-muted);font-size:12px} + #setup code{font-family:var(--pl-font-mono,ui-monospace,Menlo,monospace);font-size:11px; + padding:1px 4px;border-radius:4px;background:var(--pl-color-bg-raised)} + #setup .sa{margin-top:7px;display:flex;gap:6px;align-items:center;flex-wrap:wrap} + #setup .warn{color:#d29922} +""" + +_SETUP_JS = r""" + // The first-run setup card — rendered from /status (gh missing / not signed in / + // malformed default repo). Fetched ONLY from inside the kit boot (post-handshake). + let statusSeq = 0; + function setupCard(st){ + const el = $("setup"); if(!el) return; + const lines = []; + if(!st || st.__failed){ + lines.push('<div class="st"><span class="warn">!</span> Could not check GitHub CLI status</div>' + + '<div class="sd">The agent may be unreachable, or the status route is missing. Use Re-check.</div>'); + } else if(!st.gh_path){ + lines.push('<div class="st"><span class="warn">!</span> GitHub CLI (<code>gh</code>) is not installed</div>' + + '<div class="sd">The GitHub rail and tools need it. Install from <a href="https://cli.github.com" target="_blank" rel="noreferrer">cli.github.com</a>' + + ' (macOS: <code>brew install gh</code>; Debian/Ubuntu: <code>sudo apt install gh</code>), then sign in with <code>gh auth login</code>' + + ' in a terminal — or paste a personal access token in <b>Settings ▸ GitHub</b> (github.token).</div>'); + } else if(!st.authenticated){ + lines.push('<div class="st"><span class="warn">!</span> GitHub CLI is not signed in</div>' + + '<div class="sd">Found <code>gh</code>' + (st.gh_version ? ' v' + esc(st.gh_version) : '') + ' at <code>' + esc(st.gh_path) + '</code>' + + (st.error ? ' — ' + esc(st.error) : '') + '.<br>Run <code>gh auth login</code> in a terminal, or paste a personal access token' + + ' in <b>Settings ▸ GitHub</b> (github.token)' + (st.token_source === 'config' ? ' — the saved token was rejected; replace it' : '') + '.</div>'); + } + if(st && st.default_repo_error){ + lines.push('<div class="st"><span class="warn">!</span> Default repo is malformed</div>' + + '<div class="sd">' + esc(st.default_repo_error) + '</div>'); + } + if(!lines.length){ el.className = ""; el.innerHTML = ""; return; } + lines.push('<div class="sa"><button class="pl-btn pl-btn--sm" id="recheck" type="button">Re-check</button></div>'); + el.innerHTML = lines.join(""); el.className = "show"; + const b = $("recheck"); if(b) b.onclick = () => { b.disabled = true; b.textContent = "Checking…"; checkStatus(); }; + } + async function checkStatus(){ + const my = ++statusSeq; + try { + const st = await kit.apiFetch("/api/plugins/github/status").then(r => r.ok ? r.json() : { __failed: true }); + if(my === statusSeq) setupCard(st); + } catch(e){ if(my === statusSeq) setupCard({ __failed: true }); } + } +""" + # --- the read-only board ----------------------------------------------------- PAGE = r"""<!doctype html><html lang="en"><head><meta charset="utf-8"> <meta name="viewport" content="width=device-width, initial-scale=1"> @@ -57,6 +116,7 @@ .cmt{display:inline-flex;align-items:center;gap:3px} .pill{font-size:10px;padding:1px 7px;border-radius:999px;border:1px solid var(--pl-color-border)} .empty,.hint{padding:24px 14px;text-align:center;color:var(--pl-color-fg-muted)} +__SETUP_CSS__ </style></head><body> <div id="wrap"> <div class="bar"> @@ -69,6 +129,7 @@ <span class="spacer"></span> <button class="pl-btn pl-btn--sm" id="refresh" type="button" title="Refresh" aria-label="Refresh"></button> </div> + <div id="setup" role="status"></div> <div id="list"><div class="hint">Loading…</div></div> </div> <script type="module"> @@ -135,10 +196,12 @@ } catch(e){ if(my===loadSeq) list.innerHTML = '<div class="empty">Failed to load — is the agent reachable?</div>'; } } +__SETUP_JS__ let booted = false; async function boot(){ if (booted) return; // the kit re-fires this on every re-theme + the handshake re-send; booted = true; // build the picker and first-load EXACTLY once, or the list thrashes (#15). + checkStatus(); // the setup card, in parallel — never blocks the list let cfg = { repos: [], default_repo: "" }; try { cfg = await kit.apiFetch("/api/plugins/github/config").then(r => r.json()); } catch(e){} const sel = $("repo"); @@ -178,8 +241,11 @@ border:1px solid var(--pl-color-border);border-radius:var(--pl-radius,8px);padding:7px;font-size:12px} textarea{flex:1;min-height:140px;resize:vertical;font-family:var(--pl-font-mono,ui-monospace,Menlo,monospace)} #res{font-size:11px;color:var(--pl-color-fg-muted);white-space:pre-wrap;min-height:16px} + #setup{margin:0} +__SETUP_CSS__ </style></head><body> <div id="wrap"> + <div id="setup" role="status"></div> <div class="row"> <select id="repo" title="Repository"></select> <select id="kind" style="max-width:130px"><option value="generic">Generic</option><option value="bug">Bug</option><option value="feature">Feature</option></select> @@ -200,14 +266,18 @@ const $ = (id) => document.getElementById(id); const esc = (s) => String(s == null ? "" : s).replace(/[&<>"]/g, (c) => ({ "&":"&","<":"<",">":">",'"':""" }[c])); +__SETUP_JS__ let booted = false; async function boot(){ if (booted) return; // kit re-fires on every re-theme; populate the picker once so it never booted = true; // clobbers an in-progress repo selection (#15). + checkStatus(); // the setup card, in parallel — never blocks the form let cfg = { repos: [], default_repo: "" }; try { cfg = await kit.apiFetch("/api/plugins/github/config").then(r => r.json()); } catch(e){} - $("repo").innerHTML = (cfg.repos||[]).map(r => '<option value="'+esc(r)+'">'+esc(r)+'</option>').join(""); - if(cfg.default_repo){ $("repo").value = cfg.default_repo; } + const sel = $("repo"); + sel.innerHTML = (cfg.repos||[]).map(r => '<option value="'+esc(r)+'">'+esc(r)+'</option>').join(""); + if(cfg.default_repo){ sel.value = cfg.default_repo; } + if(!(cfg.repos||[]).length){ $("res").textContent = "No repositories configured — add one under Settings ▸ GitHub (github.default_repo / repos)."; } } async function submit(){ const title = $("title").value.trim(); @@ -227,3 +297,7 @@ // Boot ONCE via the kit (after the theme/auth handshake) — not also directly (#13). kit.initPluginView(boot); </script></body></html>""" + +# Splice the shared setup card into both pages — one source, two surfaces. +PAGE = PAGE.replace("__SETUP_CSS__", _SETUP_CSS).replace("__SETUP_JS__", _SETUP_JS) +NEW_ISSUE_PAGE = NEW_ISSUE_PAGE.replace("__SETUP_CSS__", _SETUP_CSS).replace("__SETUP_JS__", _SETUP_JS) From b2a178716970fead4a3a347dede30270be48e030 Mon Sep 17 00:00:00 2001 From: Josh Mabry <mabry1985@gmail.com> Date: Sat, 22 Aug 2026 16:30:35 -0700 Subject: [PATCH 5/6] =?UTF-8?q?chore:=20v0.6.0=20=E2=80=94=20token=20secre?= =?UTF-8?q?t=20in=20the=20manifest,=20ruff=20pinned,=20docs=20match=20the?= =?UTF-8?q?=20code,=20release=20recipe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - protoagent.plugin.yaml: version 0.6.0; secrets: [token] + the Settings field 'GitHub token (PAT) — optional; gh auth login also works'; the auth precedence and repo-resolution comments match gh_cli.py / projects.py. - pyproject.toml: 0.6.0 (test_version.py lockstep). - ruff==0.15.10 pinned in requirements-dev.txt and installed from there in ci.yml (the protoAgent core / projectBoard pin) — a floating ruff fails format --check on correctly-formatted code. - README: the real tool inventory — 11 read / 8 write / 3 review, all live (was 8/3 with five 'stubbed'); auth precedence; repo sources; the setup card. - PROTO.md: §3 file map (status.py, projects.py, review_tools.py), §4 '/issue is the user-only chat path; github_create_issue exists behind github.write' + the live-config rule, §5 inventory + error classification + first-run status + the no-raise sweep, §6 never-raise rule, NEW §7 release recipe (tag -a / push / gh release create; no CHANGELOG — the PR body is it). - CLAUDE.md / AGENTS.md pointers no longer mention 'the stubs'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- .github/workflows/ci.yml | 4 +- AGENTS.md | 5 +- CLAUDE.md | 5 +- PROTO.md | 123 ++++++++++++++++++++++++++++++--------- README.md | 46 +++++++++++---- protoagent.plugin.yaml | 37 +++++++++--- pyproject.toml | 2 +- requirements-dev.txt | 3 + 8 files changed, 175 insertions(+), 50 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d1dd997..2c252ac 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -21,7 +21,9 @@ jobs: python-version: "3.12" - name: Install dev deps - run: pip install -r requirements-dev.txt ruff + # ruff is PINNED in requirements-dev.txt (0.15.10, the protoAgent/projectBoard pin) — + # a floating ruff fails `format --check` on correctly-formatted code. + run: pip install -r requirements-dev.txt - name: Lint run: ruff check . && ruff format --check . diff --git a/AGENTS.md b/AGENTS.md index b0ef727..a79c6fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,5 +2,6 @@ **Canonical instructions live in [PROTO.md](./PROTO.md).** Read it before editing — it owns the stack, the lint+test gate, the per-agent gating design (**§4, don't break -it**), what to build (the stubs, §5), and the rules (§6, incl. host-free imports and -plain-string `@tool` docstrings). This file is a thin pointer; edit PROTO.md. +it**), the tool inventory + error classification (§5), the rules (§6, incl. host-free +imports, plain-string `@tool` docstrings, and never-raise tools), and the release +recipe (§7). This file is a thin pointer; edit PROTO.md. diff --git a/CLAUDE.md b/CLAUDE.md index aba86ae..bbb619f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,5 +2,6 @@ **Canonical instructions live in [PROTO.md](./PROTO.md).** Read it before editing — it owns the stack, the lint+test gate, the per-agent gating design (**§4, don't break -it**), what to build (the stubs, §5), and the rules (§6, incl. host-free imports and -plain-string `@tool` docstrings). This file is a thin pointer; edit PROTO.md. +it**), the tool inventory + error classification (§5), the rules (§6, incl. host-free +imports, plain-string `@tool` docstrings, and never-raise tools), and the release +recipe (§7). This file is a thin pointer; edit PROTO.md. diff --git a/PROTO.md b/PROTO.md index 1e27f49..5e4b5a7 100644 --- a/PROTO.md +++ b/PROTO.md @@ -13,7 +13,7 @@ tools over the `gh` CLI, with **per-agent write gating**. The host loads it via | Layer | What | |---|---| | Runtime | Python ≥ 3.11; `langchain-core` (`@tool`) is provided by the host | -| Auth | `gh` ambient auth, or `GITHUB_TOKEN`/`GH_TOKEN` from the env | +| Auth | `github.token` secret (Settings) > `GITHUB_TOKEN`/`GH_TOKEN` env > `gh auth login` keyring | | Repo | `protoLabsAI/github-plugin`, ships `enabled: false` (install ≠ enable ≠ trust) | ## 2. Commands — the PR gate @@ -21,25 +21,32 @@ tools over the `gh` CLI, with **per-agent write gating**. The host loads it via These must pass before a PR opens (host-free — no protoAgent needed): ```bash -pip install -r requirements-dev.txt ruff +pip install -r requirements-dev.txt # includes the PINNED ruff==0.15.10 ruff check . && ruff format --check . # lint + format pytest -q # the suite ``` -There is no other runner. `ruff` + `pytest` are the sole gate. +There is no other runner. `ruff` + `pytest` are the sole gate. `ruff` is pinned (the +same `0.15.10` as protoAgent core and projectBoard-plugin) in BOTH `requirements-dev.txt` +and `ci.yml` — a floating ruff fails `format --check` on correctly-formatted code; bump +the pin in both places together. ## 3. Where everything lives ``` -protoagent.plugin.yaml # manifest (id: github, config_section: github; write/default_repo/repos) -__init__.py # register() — gating wiring (read always; write iff github.write) + /issue -gh_cli.py # vendored async `gh` runner (run_gh, check_gh_error, bad_repo) -read_tools.py # 8 read tools (6 ported core + read_file/repo_contents) +protoagent.plugin.yaml # manifest (id: github, config_section: github; write/default_repo/repos + the `token` secret) +__init__.py # register() — gating wiring (read always; write iff github.write) + /issue + live-config getters +gh_cli.py # vendored async `gh` runner: binary resolution (PATH + install dirs), token + # injection (config secret > env), check_gh_error CLASSIFICATION, bad_repo +status.py # the first-run probe: compute_status / summarize_status / report_gaps (setup-gap seam) +projects.py # repo sources: host projects: registry (ADR 0095) + checkout `origin` remote parsing +read_tools.py # 11 read tools (6 ported core + file/contents/pr-file/path-exists/pr-diff + github_status) write_tools.py # 8 write tools (create/edit/merge/close/comment/labels/assignees) — gated -gh_issue.py # /issue chat command logic (user-only; gate-checked; configured repo) -api.py # routers — public PAGES (view/new-issue) + gated data routes (config/issues/prs/issue) -view.py # PAGE = read-only board (Issues/PRs tabs + repo picker); NEW_ISSUE_PAGE = file-an-issue form. --pl-* themed -tests/ # host-free pytest (gating + version coherence + routes via TestClient) +review_tools.py # 3 verdict tools (comment/approve/request-changes, guarded) — gated +gh_issue.py # /issue chat command logic + repo resolution (resolve_repo, default_repo_error) +api.py # routers — public PAGES (view/new-issue) + gated data routes (config/status/issues/prs/issue) +view.py # PAGE = read-only board; NEW_ISSUE_PAGE = file-an-issue form; shared setup card. --pl-* themed +tests/ # host-free pytest (gating + version coherence + routes via TestClient + the no-raise sweep) ``` **Console surfaces (ADR 0026/0038/0042/0057).** `register()` mounts two routers when the @@ -63,22 +70,39 @@ Filing lives in the widget + palette, NOT the board (keep the board read-only). agent stays read-only; a coding/PM agent gets write. `tests/test_register.py` asserts both halves — keep it green. -**`/issue` is a user-only chat command, not an agent tool** — creating an issue is a -write the model must not do autonomously, so `register()` registers it via the host's -`register_chat_command` seam (the logic lives in `gh_issue.py`). The call is guarded -by `hasattr(registry, "register_chat_command")`, so on an older host without the seam -`/issue` is skipped and the tools still load (degrade-safe). It routes to the -configured `default_repo`/`repos` (never a silent default). `tests/test_issue_command.py` -asserts both the seam-present and legacy-host paths — keep them green. - -## 5. Tools (all implemented) +**`/issue` is the user-only chat path; the `github_create_issue` agent tool exists +behind `github.write`.** A PERSON files from the composer on any agent — including a +read-only one — via `/issue`, registered through the host's `register_chat_command` +seam (the logic lives in `gh_issue.py`); the MODEL files via `github_create_issue` +only when the agent's write gate is on (the PM archetype requires it). Both go +through the same `file_issue` (gate check + `gh issue create`), so they can't +diverge. The seam call is guarded by `hasattr(registry, "register_chat_command")`, so +on an older host without it `/issue` is skipped and the tools still load +(degrade-safe). It routes to the configured `default_repo`/`repos` (never a silent +default). `tests/test_issue_command.py` asserts both the seam-present and +legacy-host paths — keep them green. + +**Config is read LIVE.** `register()` builds getters over `registry.live_config` (the +snapshot on an older host) and every consumer — the tools' default repo, `/issue`, the +data routes, the `token` secret — reads through them per call. Never capture a config +value at register time: an `onboard_project` mid-session, or a token pasted in +Settings, must be seen by the very next call. + +## 5. Tools (all implemented — 11 read / 8 write / 3 review) Each tool mocks `run_gh` in its test and asserts the exact argv + readable errors. +`tests/test_no_raise_sweep.py` additionally invokes EVERY registered tool against a +`run_gh` stub returning a dict / a list / garbage / an error and asserts a `str` comes +back — a tool that raises kills the whole agent turn, so no tool may. Add a tool ⇒ the +sweep covers it automatically (it enumerates `register()`'s output). **Read (always on)** — 6 ported core tools (`github_get_pr`, `github_get_issue`, -`github_list_issues`, `github_get_commit_diff`, `github_ci_runs`, `github_run_failure`) -plus `github_read_file` (`gh api .../contents/{path}`, raw) and `github_repo_contents` -(directory listing). +`github_list_issues`, `github_get_commit_diff`, `github_ci_runs`, `github_run_failure`), +`github_pr_diff`, the content readers (`github_read_file` — raw file; `github_read_pr_file` +— a file at a PR's head; `github_repo_contents` — directory listing, says "is a file — +use github_read_file" on a file; `github_path_exists` — the EXISTS/MISSING probe), and +`github_status` — is `gh` installed / authenticated / as whom / which default repo, the +self-diagnosis tool the model calls when another tool errors (no `write` gate). **Write (gated on `github.write`)** — `github_create_issue` / `github_comment` / `github_create_pr` (return the new URL), @@ -87,23 +111,70 @@ plus `github_read_file` (`gh api .../contents/{path}`, raw) and `github_repo_con `github_close` (close/reopen issue|pr), and `github_set_labels` / `github_set_assignees` (`gh {issue,pr} edit --add/--remove-{label,assignee}`). Issue-vs-PR ops take `kind`. +**Review (gated on `github.write`)** — `github_review_comment` (always allowed), +`github_review_approve` / `github_review_request_changes` (refused while CI is pending +or unreadable, and on the agent's own PR — guards live in the tool, below the model). + New write op? Validate `bad_repo()`, build argv, `run_gh()`, degrade to `Error: ...`, add it to `get_write_tools()`'s return list **and** `WRITE_TOOLS` in `test_register.py`, and mirror an existing test. Anything irreversible (merge) must be `confirm`-guarded. +**Errors are classified** (`gh_cli.check_gh_error`): `gh` exit 4 / "gh auth login" ⇒ +`Error: GitHub CLI is not authenticated — …`; GraphQL "Could not resolve to a Repository" +⇒ `Error: repo 'o/n' not found or not accessible`; HTTP 404 ⇒ not found (with the repo +named when known); 403/429 + "rate limit" ⇒ `Error: GitHub API rate limit hit — retry …`; +binary missing ⇒ `Error: gh CLI is not installed or not on PATH (looked in …)`. Anything +else keeps the `Error (gh exit N): <stderr>` shape. Add a category here, not in a tool. + +**First-run status** (`status.py`): `GET /api/plugins/github/status` → `{gh_path, +gh_version, authenticated, login, host, token_source, error, default_repo, repos, +default_repo_error}` from `gh --version` + `gh auth status --json hosts` (text fallback +for an older `gh`); never raises. The views render a setup card from it; the +`github_status` tool returns `summarize_status()`; `probe_in_background()` reports the +`gh` / `auth` gaps to the host's `report_setup_gap` seam at register time (guarded — +no seam, no thread) and clears them when healthy. + ## 6. Rules - **Host-free.** NEVER import `graph.*` / `plugins.*` at module top — the suite runs with only `requirements-dev.txt`. Keep any host imports lazy (inside functions). - **`@tool` docstrings must be PLAIN string literals** — an f-string docstring makes `__doc__` None and the tool ships with no description (the model can't see it). -- **Every tool requires an explicit `owner/name` repo** — validate with `bad_repo()`; - there is no silent default (a forgotten repo must error, not hit the wrong repo). +- **Every tool requires an `owner/name` repo** — validate with `bad_repo()`. The + fallback when the arg is omitted is the LIVE configured default (`default_repo` > + first picker entry; the picker = `github.repos` ∪ the host's `projects:` registry ∪ + the `origin` remote of each registered checkout / `project_board.repo`), then the + `GITHUB_DEFAULT_REPO` / `GH_REPO` env **logged at INFO when it fires**. No repo + anywhere ⇒ an error, never a guess. A malformed `default_repo` is the NAMED + `default_repo_error` (#23) in `/config`, `/status`, the issue route and the views. +- **Never raise out of a tool.** A tool that raises kills the agent's whole turn. Every + `gh` result is typed-checked before use (the contents API returns a dict for a file, + a list for a directory); the no-raise sweep test enforces this for every tool. - **DO NOT FABRICATE.** Use real `gh` invocations; verify the actual `gh api` shape before relying on it. No placeholder/guessed command flags. - **Don't add runtime pip deps.** Test-only deps go in `requirements-dev.txt`; real runtime deps would go in the manifest's `requires_pip` (operator-installed). -## 7. Agent-scratch +## 7. Release + +A release is a version bump + a tag + a GitHub release; hosts pin the tag in +`plugins.lock`. `tests/test_version.py` asserts the manifest and `pyproject.toml` +versions match — bump BOTH. + +```bash +# 1. bump `version:` in protoagent.plugin.yaml AND `version =` in pyproject.toml (lockstep) +# 2. land the PR on main (the gates above must be green) +# 3. tag + release from the merged main +git checkout main && git pull +git tag -a vX.Y.Z -m "vX.Y.Z — <one-line summary>" +git push origin vX.Y.Z +gh release create vX.Y.Z --title "vX.Y.Z" --notes "<the PR body / changelog>" +``` + +There is no CHANGELOG file — the PR body is the changelog; paste it into the release +notes. Installed hosts pick the new version up with +`python -m server plugin update github` (or by re-pinning `plugins.lock`). + +## 8. Agent-scratch `.proto/` is the coding agent's own scratch — gitignored, never commit. diff --git a/README.md b/README.md index a7400ce..11f85fe 100644 --- a/README.md +++ b/README.md @@ -8,16 +8,29 @@ config sets `github.write: true`** — so a research/Lead agent stays read-only coding/PM agent gets write, purely by its own per-instance config. Supersedes the read-only in-tree `github` plugin. -## Tools +## Tools (all implemented) -**Read** (always): `github_get_pr`, `github_get_issue`, `github_list_issues`, -`github_get_commit_diff`, `github_ci_runs`, `github_run_failure`, `github_read_file`*, -`github_repo_contents`*. +**Read** (always, 11): `github_get_pr`, `github_get_issue`, `github_list_issues`, +`github_get_commit_diff`, `github_pr_diff`, `github_ci_runs`, `github_run_failure`, +`github_read_file`, `github_read_pr_file`, `github_repo_contents`, `github_path_exists`, +and `github_status` (is `gh` installed / signed in, as whom, which default repo — the +self-diagnosis probe the model calls when another tool errors). -**Write** (only when `github.write: true`): `github_create_issue`*, `github_comment`*, -`github_create_pr`*. +**Write** (only when `github.write: true`, 8): `github_create_issue`, `github_comment`, +`github_create_pr`, `github_edit_pr`, `github_merge_pr` (`confirm`-guarded), +`github_close`, `github_set_labels`, `github_set_assignees`. -*\* stubbed — being built out; see [PROTO.md §5](./PROTO.md).* +**Review** (also behind `github.write`, 3): `github_review_comment`, +`github_review_approve`, `github_review_request_changes` — the formal verdict tools, +with the CI-terminal and self-review guards enforced inside the tool (ADR 0078). + +Plus the **user-only `/issue` chat command** (file an issue from the composer on any +agent, without the model) and two console views — the read-only Issues/PRs **board** +and the **New issue** form — both of which show a **setup card** when `gh` is missing +or not signed in. + +Every tool returns a readable, classified error instead of raw `gh` stderr: not +authenticated (with the fix), repo not found, API rate limit, `gh` not installed. ## Install & enable @@ -31,15 +44,28 @@ Install ≠ enable. To turn it on, add to `config/langgraph-config.yaml`: plugins: enabled: [github] github: - write: false # read-only; set true ONLY for agents that should mutate GitHub + write: false # read-only; set true ONLY for agents that should mutate GitHub + default_repo: "" # owner/name — the repo tools and /issue use when none is passed + repos: [] # repo picker list; the host's projects: registry is added automatically ``` -Auth: `gh` uses its own ambient auth (`gh auth login`) or `GITHUB_TOKEN`/`GH_TOKEN`. +**Auth**, in precedence order: the `github.token` secret (Settings ▸ GitHub — a personal +access token with `repo` scope) > `GITHUB_TOKEN` / `GH_TOKEN` in the environment > +`gh`'s own keyring login (`gh auth login`). Public-repo reads need none at low volume. +`gh` is found on PATH or in the usual install dirs (`/opt/homebrew/bin`, `/usr/local/bin`, +`~/.local/bin`, `/usr/bin`) — a desktop build launched without a shell PATH still works. + +**Repos**: a tool's omitted `repo` falls back to `default_repo`, else the first picker +entry. The picker is `github.repos` ∪ the host's managed-projects registry (ADR 0095, +`projects:` entries with a `github:` binding) ∪ — last resort — the `owner/name` parsed +from the `origin` remote of each registered checkout (and `project_board.repo`). All of +it is read live: a Settings edit or an `onboard_project` mid-session is seen by the +next call. A malformed `default_repo` is a named error, never fed to `gh`. ## Develop ```bash -pip install -r requirements-dev.txt ruff +pip install -r requirements-dev.txt # pins ruff==0.15.10 ruff check . && ruff format --check . && pytest -q ``` diff --git a/protoagent.plugin.yaml b/protoagent.plugin.yaml index 1b2389d..bcb490f 100644 --- a/protoagent.plugin.yaml +++ b/protoagent.plugin.yaml @@ -2,7 +2,7 @@ # Keep `version` in lockstep with pyproject.toml (tests/test_version.py asserts it). id: github name: GitHub (read/write tools) -version: 0.5.0 +version: 0.6.0 description: >- Read AND write GitHub tools over the `gh` CLI, with PER-AGENT write gating. The read tools (PRs, issues, diffs, CI, repo files/contents) are always on; the write @@ -10,19 +10,29 @@ description: >- when `github.write: true` — so a research/Lead agent stays read-only while a coding/PM agent gets write, purely by its own per-instance config (ADR 0019). Merging is `confirm`-guarded. Supersedes the read-only in-tree `github` plugin. - Tools degrade to a readable error if `gh` isn't installed/authenticated. + Tools degrade to a readable, classified error (not authenticated / repo not found / + rate-limited / `gh` missing) — and `github_status` + a setup card in the views say + exactly what to fix on a fresh machine. enabled: false # SHIP DISABLED — enabling is the operator's trust decision repository: https://github.com/protoLabsAI/github-plugin min_protoagent_version: "0.27.0" # Config (ADR 0019): defaults are the source of truth; langgraph-config.yaml overlays. # `write` is the per-agent gate — each agent's config decides read-only vs read+write. -# `default_repo`/`repos` route the user-only `/issue` command (and the console views): -# explicit `--repo` > default_repo > first of repos > GITHUB_DEFAULT_REPO/GH_REPO env. +# `default_repo`/`repos` route the tools' omitted-repo fallback, the user-only `/issue` +# command and the console views: explicit repo > default_repo > first of repos (the +# list below ∪ the project registry ∪ checkout origin remotes) > GITHUB_DEFAULT_REPO / +# GH_REPO env (logged when it fires). All read LIVE — a Settings edit or an onboarded +# project is seen by the next call, no restart. config_section: github config: write: false # false = read tools only; true = read + write tools - default_repo: "" # preselected repo for /issue (owner/name); "" = none + default_repo: "" # default repo (owner/name) for tools + /issue; "" = none. + # Must be owner/name — anything else is a named error in + # the views / tools (#23), never fed to `gh`. + token: "" # OPTIONAL personal access token (secret → secrets.yaml). + # Injected into every `gh` run as GH_TOKEN, winning over an + # ambient GITHUB_TOKEN/GH_TOKEN; `gh auth login` works too. repos: [] # repo picker list (each owner/name) for /issue + views. # The host's managed-projects registry (ADR 0095, # protoAgent 0.115.0+) is ALWAYS added: every `projects:` @@ -33,11 +43,20 @@ config: # here come first, in this order, so the default-repo # resolution is unchanged; the registry's follow. # Older hosts with no registry: behavior is unchanged. + # LAST RESORT (v0.6.0): a registry project with only a + # `path` (no `github:`), and `project_board.repo`, are + # git checkouts — their `origin` remote is parsed into + # owner/name, so a fresh install that onboarded a project + # gets a working default with nothing typed twice. + +# Secrets (ADR 0019) — routed to the gitignored secrets.yaml, redacted in the UI. +secrets: [token] # Settings (ADR 0019) — surfaced as editable fields in the console. settings: - {key: write, label: "Allow write tools (this agent)", type: bool} - - {key: default_repo, label: "Default repo (owner/name)", type: string} + - {key: default_repo, label: "Default repo (owner/name)", type: string, description: "owner/name — used when a tool or /issue omits the repo. Leave blank to fall back to the first picker entry."} + - {key: token, label: "GitHub token (PAT) — optional; gh auth login also works", type: secret, description: "A personal access token with repo scope. Wins over GITHUB_TOKEN/GH_TOKEN in the environment; leave blank to use `gh auth login`'s keyring login."} - {key: repos, label: "Repo picker list", type: string_list} # Console views (ADR 0026/0057) — sandboxed iframes the plugin serves itself (api.py). @@ -59,8 +78,10 @@ public_paths: - /plugins/github/view - /plugins/github/new-issue -# Auth: `gh` uses its own ambient auth (`gh auth login`), or GITHUB_TOKEN / GH_TOKEN -# from the env if set. No plugin secret needed for public-repo reads. +# Auth, in precedence order: the `token` secret above (Settings ▸ GitHub) > GITHUB_TOKEN / +# GH_TOKEN from the env > `gh`'s own keyring login (`gh auth login`). Nothing is needed +# for public-repo reads at low volume. `GET /api/plugins/github/status` + the +# `github_status` tool report which applies; the views show a setup card when none does. # Declarative transparency (shown in the console; not yet enforced). capabilities: diff --git a/pyproject.toml b/pyproject.toml index 137b162..9e182cc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "github-plugin" -version = "0.5.0" +version = "0.6.0" description = "Read/write GitHub tools for protoAgent over the gh CLI, with per-agent write gating." requires-python = ">=3.11" diff --git a/requirements-dev.txt b/requirements-dev.txt index c29506b..c0ff573 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -6,3 +6,6 @@ httpx>=0.27 # powers fastapi/starlette TestClient (the route tests) pyyaml>=6 pytest>=8 pytest-asyncio>=0.23 # for the async-tool tests the team adds (asyncio_mode=auto) +ruff==0.15.10 # PINNED — an unpinned ruff floats to a release that formats differently + # than the dev's, so `ruff format --check` fails on already-formatted + # code. Same pin as protoAgent core + projectBoard-plugin; bump together. From 2e54593bb7048f29fae4a8b49a50839869512c6c Mon Sep 17 00:00:00 2001 From: Josh Mabry <mabry1985@gmail.com> Date: Sat, 22 Aug 2026 16:51:23 -0700 Subject: [PATCH 6/6] fix(review): never cache a gh miss, report gaps from every status probe, gh-native token precedence, no-raise spawn, off-loop picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #27, all items: BLOCKERS 1. resolve_gh() caches only a HIT; a miss is re-resolved every call, and compute_status() drops the cache first — so Re-check (and the next tool) sees gh installed after boot, and sees it removed. gh_available() now uses the same resolver (it disagreed with the tools before). Tests cover absent→installed and installed→removed. 2. compute_and_report(): /status AND the github_status tool now report to report_setup_gap (edge-triggered — failing key gets its message, passing key gets None) so the host banner clears on the very request that sees the recovery. build_data_router(cfg, registry=) and get_read_tools(..., registry=) carry the registry; register() passes it. MINORS 3. Token precedence matches gh: only a non-empty config token is injected (GH_TOKEN + GITHUB_TOKEN); otherwise the env passes through untouched so gh's own GH_TOKEN > GITHUB_TOKEN > keyring applies. resolve_token()/ token_source() check GH_TOKEN first. Manifest/README/PROTO wording. 4. run_gh(): trailing 'except OSError' -> (126, '', 'could not run gh at ...') (ENOEXEC/EMFILE), contextlib.suppress(ProcessLookupError) around kill(). Sweep extended: every tool x {ENOEXEC, EMFILE, kill-after-exit}. 5. effective_default_repo() takes the picker as a GETTER and short-circuits when default_repo is set (no git fork on the configured path); the data routes and github_status resolve via asyncio.to_thread; the probe thread primes the remote cache; TTL 60s -> 10min. Tests assert the picker is never computed with a default set, and that routes resolve off-loop. 6. auth_hint(token_source) — a rejected env/config token says 'fix/unset it' / 'replace it in Settings > GitHub', not 'run gh auth login'. Used by the classified error, the summary, the gaps, and the views' card. The gh 401 body is trimmed to its status line. 7. Tool counts: 12 read / 8 write / 3 review = 23 everywhere. NITS - github_path_exists classifies FIRST (error_kind): auth/rate-limit/missing binary are UNVERIFIED errors, never a MISSING verdict; the 404 wording names the inaccessible-repo case. - README/PROTO state #23's named error is on the plugin's GET /config + the card (on read), not on Settings save. - gaps_for caps the why fragment at 80 chars so the host's ~300-char banner keeps the hint. - conftest: an autouse fixture clears GH_TOKEN/GITHUB_TOKEN, the token getter and the gh cache per test, so a dev box's ambient token can't skew hint assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --- PROTO.md | 29 +++++--- README.md | 13 ++-- __init__.py | 19 ++--- api.py | 33 ++++++--- gh_cli.py | 124 ++++++++++++++++++++++---------- gh_issue.py | 14 +++- projects.py | 8 ++- protoagent.plugin.yaml | 14 ++-- read_tools.py | 29 +++++--- status.py | 52 ++++++++++---- tests/conftest.py | 16 +++++ tests/test_board_view.py | 78 ++++++++++++++++++++ tests/test_gh_cli.py | 133 ++++++++++++++++++++++++++++++++--- tests/test_gh_issue.py | 18 +++++ tests/test_no_raise_sweep.py | 49 +++++++++++++ tests/test_read_tools.py | 35 +++++++++ tests/test_register.py | 23 ++++++ tests/test_status.py | 69 ++++++++++++++++-- view.py | 9 ++- 19 files changed, 648 insertions(+), 117 deletions(-) diff --git a/PROTO.md b/PROTO.md index 5e4b5a7..341110c 100644 --- a/PROTO.md +++ b/PROTO.md @@ -13,7 +13,7 @@ tools over the `gh` CLI, with **per-agent write gating**. The host loads it via | Layer | What | |---|---| | Runtime | Python ≥ 3.11; `langchain-core` (`@tool`) is provided by the host | -| Auth | `github.token` secret (Settings) > `GITHUB_TOKEN`/`GH_TOKEN` env > `gh auth login` keyring | +| Auth | a non-empty `github.token` secret (Settings) is injected and wins; else the env passes through and gh's own order applies (`GH_TOKEN` > `GITHUB_TOKEN` > `gh auth login` keyring) | | Repo | `protoLabsAI/github-plugin`, ships `enabled: false` (install ≠ enable ≠ trust) | ## 2. Commands — the PR gate @@ -40,7 +40,7 @@ gh_cli.py # vendored async `gh` runner: binary resolution (PATH + # injection (config secret > env), check_gh_error CLASSIFICATION, bad_repo status.py # the first-run probe: compute_status / summarize_status / report_gaps (setup-gap seam) projects.py # repo sources: host projects: registry (ADR 0095) + checkout `origin` remote parsing -read_tools.py # 11 read tools (6 ported core + file/contents/pr-file/path-exists/pr-diff + github_status) +read_tools.py # 12 read tools (6 ported core + file/contents/pr-file/path-exists/pr-diff + github_status) write_tools.py # 8 write tools (create/edit/merge/close/comment/labels/assignees) — gated review_tools.py # 3 verdict tools (comment/approve/request-changes, guarded) — gated gh_issue.py # /issue chat command logic + repo resolution (resolve_repo, default_repo_error) @@ -88,7 +88,7 @@ data routes, the `token` secret — reads through them per call. Never capture a value at register time: an `onboard_project` mid-session, or a token pasted in Settings, must be seen by the very next call. -## 5. Tools (all implemented — 11 read / 8 write / 3 review) +## 5. Tools (all implemented — 12 read / 8 write / 3 review = 23) Each tool mocks `run_gh` in its test and asserts the exact argv + readable errors. `tests/test_no_raise_sweep.py` additionally invokes EVERY registered tool against a @@ -119,8 +119,10 @@ New write op? Validate `bad_repo()`, build argv, `run_gh()`, degrade to `Error: add it to `get_write_tools()`'s return list **and** `WRITE_TOOLS` in `test_register.py`, and mirror an existing test. Anything irreversible (merge) must be `confirm`-guarded. -**Errors are classified** (`gh_cli.check_gh_error`): `gh` exit 4 / "gh auth login" ⇒ -`Error: GitHub CLI is not authenticated — …`; GraphQL "Could not resolve to a Repository" +**Errors are classified** (`gh_cli.check_gh_error`, categories from `error_kind`): `gh` +exit 4 / "gh auth login" ⇒ `Error: GitHub CLI is not authenticated — …` with the hint +branched on `token_source()` (a rejected env/config token says "replace/unset it", not +"run gh auth login"); GraphQL "Could not resolve to a Repository" ⇒ `Error: repo 'o/n' not found or not accessible`; HTTP 404 ⇒ not found (with the repo named when known); 403/429 + "rate limit" ⇒ `Error: GitHub API rate limit hit — retry …`; binary missing ⇒ `Error: gh CLI is not installed or not on PATH (looked in …)`. Anything @@ -130,9 +132,16 @@ else keeps the `Error (gh exit N): <stderr>` shape. Add a category here, not in gh_version, authenticated, login, host, token_source, error, default_repo, repos, default_repo_error}` from `gh --version` + `gh auth status --json hosts` (text fallback for an older `gh`); never raises. The views render a setup card from it; the -`github_status` tool returns `summarize_status()`; `probe_in_background()` reports the -`gh` / `auth` gaps to the host's `report_setup_gap` seam at register time (guarded — -no seam, no thread) and clears them when healthy. +`github_status` tool returns `summarize_status()`. **Every status computation reports** +to the host's `report_setup_gap` seam (guarded — no seam, no-op): `probe_in_background()` +at register time (a daemon thread, off the boot path) and `compute_and_report()` from +`/status` and the tool, edge-triggered (a failing key gets its message, a passing key +gets `None`) so the operator banner clears on the very Re-check that sees the recovery. +Each probe drops the cached `gh` path first — a miss is never cached — so a `gh` +installed after boot is found by the next call. +**Never block the loop on the picker**: `effective_default_repo` takes the picker as a +getter and short-circuits when `default_repo` is set; the routes and `github_status` +run the resolution via `asyncio.to_thread`; checkout remotes are cached 10 min. ## 6. Rules @@ -146,7 +155,9 @@ no seam, no thread) and clears them when healthy. the `origin` remote of each registered checkout / `project_board.repo`), then the `GITHUB_DEFAULT_REPO` / `GH_REPO` env **logged at INFO when it fires**. No repo anywhere ⇒ an error, never a guess. A malformed `default_repo` is the NAMED - `default_repo_error` (#23) in `/config`, `/status`, the issue route and the views. + `default_repo_error` (#23) on the plugin's `GET /config` / `/status`, the issue route, + the `github_status` tool and the views' setup card — i.e. on READ; the host's Settings + save itself is not gated (the plugin has no save route). - **Never raise out of a tool.** A tool that raises kills the agent's whole turn. Every `gh` result is typed-checked before use (the contents API returns a dict for a file, a list for a directory); the no-raise sweep test enforces this for every tool. diff --git a/README.md b/README.md index 11f85fe..adb1bf4 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ read-only in-tree `github` plugin. ## Tools (all implemented) -**Read** (always, 11): `github_get_pr`, `github_get_issue`, `github_list_issues`, +**Read** (always, 12): `github_get_pr`, `github_get_issue`, `github_list_issues`, `github_get_commit_diff`, `github_pr_diff`, `github_ci_runs`, `github_run_failure`, `github_read_file`, `github_read_pr_file`, `github_repo_contents`, `github_path_exists`, and `github_status` (is `gh` installed / signed in, as whom, which default repo — the @@ -49,9 +49,11 @@ github: repos: [] # repo picker list; the host's projects: registry is added automatically ``` -**Auth**, in precedence order: the `github.token` secret (Settings ▸ GitHub — a personal -access token with `repo` scope) > `GITHUB_TOKEN` / `GH_TOKEN` in the environment > -`gh`'s own keyring login (`gh auth login`). Public-repo reads need none at low volume. +**Auth**: a non-empty `github.token` secret (Settings ▸ GitHub — a personal access token +with `repo` scope) is injected into every `gh` run and wins. Otherwise the environment is +passed through untouched and `gh`'s own precedence applies: `GH_TOKEN` > `GITHUB_TOKEN` > +the `gh auth login` keyring. Public-repo reads need none at low volume. When auth fails, +the error says which of those was rejected and what to do about it. `gh` is found on PATH or in the usual install dirs (`/opt/homebrew/bin`, `/usr/local/bin`, `~/.local/bin`, `/usr/bin`) — a desktop build launched without a shell PATH still works. @@ -60,7 +62,8 @@ entry. The picker is `github.repos` ∪ the host's managed-projects registry (AD `projects:` entries with a `github:` binding) ∪ — last resort — the `owner/name` parsed from the `origin` remote of each registered checkout (and `project_board.repo`). All of it is read live: a Settings edit or an `onboard_project` mid-session is seen by the -next call. A malformed `default_repo` is a named error, never fed to `gh`. +next call. A malformed `default_repo` is a named error on the plugin's `GET /config` and +in the views' setup card (not on Settings save), never fed to `gh`. ## Develop diff --git a/__init__.py b/__init__.py index 79c4e47..f9d2066 100644 --- a/__init__.py +++ b/__init__.py @@ -65,12 +65,15 @@ def current_repos() -> list[str]: def current_default_repo() -> str: """The default the tools / `/issue` fall back to when no repo is passed — the configured `default_repo`, else the first of `repos` (same resolution - as the board), evaluated per call.""" - return effective_default_repo(str(current_cfg().get("default_repo") or ""), current_repos()) - - # The `github.token` secret (Settings ▸ GitHub) — injected into every `gh` run as - # GH_TOKEN, winning over an ambient env token. Read live, so a pasted token works - # without a restart. Process-wide by design: one `gh` runner per process. + as the board), evaluated per call. `current_repos` is passed as a GETTER so + the picker (and its git-remote parsing) is only computed when no explicit + default is set.""" + return effective_default_repo(str(current_cfg().get("default_repo") or ""), current_repos) + + # The `github.token` secret (Settings ▸ GitHub) — when non-empty, injected into every + # `gh` run as GH_TOKEN (winning over an ambient env token); when empty the env is + # passed through untouched so gh's own precedence applies. Read live, so a pasted + # token works without a restart. Process-wide by design: one `gh` runner per process. set_token_getter(lambda: str(current_cfg().get("token") or "")) # READ tools — always available (they return an error string if `gh`/auth is missing). @@ -78,7 +81,7 @@ def current_default_repo() -> str: try: from .read_tools import get_read_tools - read = get_read_tools(current_default_repo, current_repos) + read = get_read_tools(current_default_repo, current_repos, registry=registry) for t in read: registry.register_tool(t) n_read = len(read) @@ -134,7 +137,7 @@ async def _issue(rest: str, session_id: str) -> str: # The data router reads config per request through the same live getter, # so a repo/default_repo edit shows in the board with no server restart — a # hot-reload can't re-mount this router, but reading config per request does. - registry.register_router(build_data_router(current_cfg), prefix="/api/plugins/github") + registry.register_router(build_data_router(current_cfg, registry=registry), prefix="/api/plugins/github") view = True except Exception: # noqa: BLE001 log.exception("[github] registering the board view failed") diff --git a/api.py b/api.py index 1f0cb4c..94ec32b 100644 --- a/api.py +++ b/api.py @@ -18,8 +18,8 @@ from __future__ import annotations +import asyncio import json -import shutil from .gh_cli import bad_repo, check_gh_error, resolve_gh, run_gh @@ -29,8 +29,9 @@ def gh_available() -> bool: - """Whether the `gh` CLI can be found (PATH, then the usual install dirs).""" - return resolve_gh() is not None or shutil.which("gh") is not None + """Whether the `gh` CLI can be found (PATH, then the usual install dirs) — the + same resolver the runner uses, so this and a tool can never disagree.""" + return resolve_gh() is not None def _norm_state(state: str) -> str | None: @@ -139,7 +140,7 @@ async def _new_issue(): return router -def build_data_router(cfg): +def build_data_router(cfg, registry=None): """The board's DATA routes — mounted under the GATED ``/api/plugins/github`` prefix. ``cfg`` is either the config dict OR a zero-arg callable returning it. Pass a callable @@ -148,27 +149,39 @@ def build_data_router(cfg): picks up the freshly-saved repos/default_repo. A plain dict (tests, older host) is a fixed snapshot. ``/issue`` reuses the SAME gate-checked `file_issue` path as the `/issue` chat command, so the dialog and the command can never diverge. + + ``registry`` (optional) is the host registry: ``/status`` reports its result to the + ``report_setup_gap`` seam through it, so the operator banner clears on the very + Re-check that sees `gh` installed / signed in. ``None`` ⇒ status only. + + The picker/default resolution may parse git remotes (blocking, cached) — every + route runs it in a worker thread, never on the event loop. """ from fastapi import APIRouter, Body from .gh_issue import IssueRequest, file_issue, labels_for, resolve_repo - from .status import compute_status + from .status import compute_and_report get_cfg = cfg if callable(cfg) else (lambda: cfg) + async def _resolved() -> dict: + current = get_cfg() or {} + return await asyncio.to_thread(resolve_config, current) + router = APIRouter() @router.get("/config") async def _config() -> dict: - resolved = resolve_config(get_cfg() or {}) + resolved = await _resolved() return {**resolved, "gh_available": gh_available()} @router.get("/status") async def _status() -> dict: """The first-run probe: `gh` path + version, auth state (login/host), the token - source, and the resolved repos — never raises (a failure is ``error``).""" - resolved = resolve_config(get_cfg() or {}) - st = await compute_status(resolved["default_repo"], resolved["repos"]) + source, and the resolved repos — never raises (a failure is ``error``). Reports + to the host's setup-gap seam (clears on recovery) when a registry was given.""" + resolved = await _resolved() + st = await compute_and_report(registry, resolved["default_repo"], resolved["repos"]) st["default_repo_error"] = resolved["default_repo_error"] return st @@ -182,7 +195,7 @@ async def _prs(repo: str, state: str = "open") -> dict: @router.post("/issue") async def _create_issue(body: dict = Body(...)) -> dict: - resolved = resolve_config(get_cfg() or {}) + resolved = await _resolved() kind = (body.get("kind") or "generic").lower() if kind not in ("bug", "feature", "generic"): kind = "generic" diff --git a/gh_cli.py b/gh_cli.py index d592f5e..90cc473 100644 --- a/gh_cli.py +++ b/gh_cli.py @@ -9,10 +9,11 @@ (``/opt/homebrew/bin``, ``/usr/local/bin``, ``~/.local/bin``, ``/usr/bin``). The resolved path is cached per process. -Auth, in precedence order: the plugin's ``github.token`` secret (Settings ▸ GitHub, -set via ``set_token_getter`` at register time) wins over an ambient -``GITHUB_TOKEN`` / ``GH_TOKEN`` env var, which wins over ``gh``'s own keyring -login (``gh auth login``). No token is required for public-repo reads at low volume. +Auth: a non-empty ``github.token`` secret (Settings ▸ GitHub, wired via +``set_token_getter`` at register time) is injected as ``GH_TOKEN`` and wins. +Otherwise the child env is passed through untouched and gh's own precedence applies +(``GH_TOKEN`` > ``GITHUB_TOKEN`` > the ``gh auth login`` keyring). No token is +required for public-repo reads at low volume. Errors: ``check_gh_error`` turns a failed run into ONE readable ``Error: ...`` string the model (or a person) can act on — not-authenticated, repo-not-found, @@ -24,6 +25,7 @@ from __future__ import annotations import asyncio +import contextlib import json import os import re @@ -45,7 +47,23 @@ # no shell PATH (desktop builds). Scanned in this order AFTER `shutil.which`. _FALLBACK_BIN_DIRS = ("/opt/homebrew/bin", "/usr/local/bin", "~/.local/bin", "/usr/bin") -_AUTH_HINT = "run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)" + +def auth_hint(source: str | None = None) -> str: + """What to DO about "not authenticated", branched on where the rejected token came + from — "run gh auth login" is wrong advice when an env/config token is what `gh` + is rejecting. ``source`` defaults to the live ``token_source()``.""" + src = source if source is not None else token_source() + if src == "config": + return ( + "the token saved in Settings ▸ GitHub (github.token) was rejected — replace it there, " + "or clear it to fall back to `gh auth login`" + ) + if src == "env": + return ( + "the GH_TOKEN / GITHUB_TOKEN in the agent's environment was rejected — fix or unset it, " + "or paste a working token in Settings ▸ GitHub (github.token)" + ) + return "run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)" def bad_repo(repo: str) -> str | None: @@ -66,10 +84,12 @@ def bad_repo(repo: str) -> str | None: def resolve_gh() -> str | None: """The absolute path of the `gh` binary, or None when it can't be found. PATH - first (`shutil.which`), then the well-known install dirs. Cached per process — - call ``reset_gh_cache()`` (tests) to re-resolve.""" + first (`shutil.which`), then the well-known install dirs. A HIT is cached per + process; a MISS is never cached — an operator who installs `gh` after boot must + be seen by the very next call (Re-check, the next tool), not the next restart. + ``reset_gh_cache()`` drops a hit (a stale path, the status probe, tests).""" global _gh_path, _gh_resolved - if _gh_resolved: + if _gh_resolved and _gh_path: return _gh_path found = shutil.which("gh") if not found: @@ -78,7 +98,8 @@ def resolve_gh() -> str | None: if cand.is_file() and os.access(cand, os.X_OK): found = str(cand) break - _gh_path, _gh_resolved = found, True + if found: + _gh_path, _gh_resolved = found, True return found @@ -118,26 +139,29 @@ def _config_token() -> str: def resolve_token() -> str | None: - """The token `gh` should use: the plugin's configured secret first (Settings ▸ - GitHub), else the ambient GITHUB_TOKEN / GH_TOKEN env, else None (keyring).""" - return _config_token() or os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN") or None + """The token `gh` will use: the plugin's configured secret first (Settings ▸ + GitHub), else the ambient env in gh's OWN order (GH_TOKEN, then GITHUB_TOKEN), + else None (gh's keyring login). Informational — see ``gh_env`` for what's injected.""" + return _config_token() or os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN") or None def token_source() -> str: """Where the effective token comes from: ``config`` | ``env`` | ``none``.""" if _config_token(): return "config" - if os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN"): + if os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN"): return "env" return "none" def gh_env() -> dict: - """The child env for a `gh` run — the ambient env plus the effective token injected - as GH_TOKEN (gh's own precedence: GH_TOKEN > GITHUB_TOKEN > keyring), so a config - token beats anything already in the environment.""" + """The child env for a `gh` run. ONLY a non-empty config token is injected (as + GH_TOKEN, the var gh reads first, and GITHUB_TOKEN so nothing stale shadows it) — + it's what the person just pasted and it's visible in the UI. Otherwise the ambient + env is passed through UNTOUCHED, so gh's own precedence (GH_TOKEN > GITHUB_TOKEN > + keyring) applies exactly as it would in a terminal.""" env = os.environ.copy() - token = resolve_token() + token = _config_token() if token: env["GH_TOKEN"] = token env["GITHUB_TOKEN"] = token @@ -174,13 +198,18 @@ async def run_gh(args: list[str], timeout: int = _COMMAND_TIMEOUT) -> tuple[int, ) except asyncio.TimeoutError: if proc is not None: - proc.kill() + with contextlib.suppress(ProcessLookupError): # it exited between the timeout and the kill + proc.kill() return 1, "", f"gh command timed out after {timeout}s" except FileNotFoundError: reset_gh_cache() # the cached path went stale (uninstalled mid-process) return 127, "", _MISSING_BINARY except PermissionError: return 126, "", f"gh binary at {binary} is not executable." + except OSError as e: + # ENOEXEC (a foreign-arch / truncated ~/.local/bin/gh), EMFILE, E2BIG … — a + # spawn failure is a tool ERROR, never an exception through the tool layer. + return 126, "", f"could not run gh at {binary}: {e}" # ── output parsing ──────────────────────────────────────────────────────────────── @@ -215,25 +244,18 @@ def dicts(items) -> list[dict]: _NOT_FOUND_REPO_RE = re.compile(r"Could not resolve to a Repository with the name '([^']+)'", re.I) -def classify_gh_error(returncode: int, stderr: str, *, repo: str = "") -> str | None: - """Map a failed `gh` run to ONE actionable ``Error: ...`` string, or None if it - succeeded. The categories, in the order they're checked: - - - binary missing (the runner's stand-in stderr, or exit 127); - - not authenticated (gh's exit 4, or stderr pointing at `gh auth login`); - - rate-limited (HTTP 403 + "rate limit"); - - repo not found / not accessible (GraphQL "Could not resolve to a Repository", - or an HTTP 404); - - anything else → the generic ``Error (gh exit N): <stderr>`` (unchanged shape, - existing tests and callers match on it). - """ +def error_kind(returncode: int, stderr: str) -> str | None: + """The CATEGORY of a failed `gh` run — ``None`` on success, else one of + ``missing_binary`` | ``auth`` | ``rate_limit`` | ``not_found`` | ``generic`` — + checked in that order. ``classify_gh_error`` renders it; a caller that must + branch on the category (``github_path_exists``: a 404 is MISSING, anything else + is an error) uses this directly so the two can't disagree.""" if returncode == 0: return None blob = stderr or "" low = blob.lower() - if _MISSING_BINARY in blob or returncode == 127: - return f"Error: gh CLI is not installed or not on PATH (looked in {', '.join(gh_search_dirs())})." + return "missing_binary" if ( returncode == GH_EXIT_AUTH_REQUIRED or "gh auth login" in low @@ -241,15 +263,43 @@ def classify_gh_error(returncode: int, stderr: str, *, repo: str = "") -> str | or "authentication required" in low or "bad credentials" in low ): - return f"Error: GitHub CLI is not authenticated — {_AUTH_HINT}." + return "auth" if "rate limit" in low and ("403" in low or "429" in low or "exceeded" in low): + return "rate_limit" + if _NOT_FOUND_REPO_RE.search(blob) or "http 404" in low: + return "not_found" + return "generic" + + +def classify_gh_error(returncode: int, stderr: str, *, repo: str = "") -> str | None: + """Map a failed `gh` run to ONE actionable ``Error: ...`` string, or None if it + succeeded. The categories (``error_kind``), in the order they're checked: + + - binary missing (the runner's stand-in stderr, or exit 127); + - not authenticated (gh's exit 4, or stderr pointing at `gh auth login`) — the + hint is branched on ``token_source()``: a rejected env/config token says so, + instead of the wrong "run gh auth login"; + - rate-limited (HTTP 403/429 + "rate limit"); + - repo not found / not accessible (GraphQL "Could not resolve to a Repository", + or an HTTP 404); + - anything else → the generic ``Error (gh exit N): <stderr>`` (unchanged shape, + existing tests and callers match on it). + """ + kind = error_kind(returncode, stderr) + if kind is None: + return None + blob = stderr or "" + if kind == "missing_binary": + return f"Error: gh CLI is not installed or not on PATH (looked in {', '.join(gh_search_dirs())})." + if kind == "auth": + return f"Error: GitHub CLI is not authenticated — {auth_hint()}." + if kind == "rate_limit": m = _RATE_LIMIT_RESET_RE.search(blob) when = f" — retry after {m.group(1).strip()}" if m else " — retry in a few minutes" return f"Error: GitHub API rate limit hit{when}." - m = _NOT_FOUND_REPO_RE.search(blob) - if m: - return f"Error: repo '{m.group(1)}' not found or not accessible (check the owner/name and your token's scopes)." - if "http 404" in low: + if kind == "not_found": + if m := _NOT_FOUND_REPO_RE.search(blob): + return f"Error: repo '{m.group(1)}' not found or not accessible (check the owner/name and your token's scopes)." where = ( f"repo '{repo}' not found or not accessible, or the path/ref/number doesn't exist in it" if repo diff --git a/gh_issue.py b/gh_issue.py index 92f0e33..3410cb9 100644 --- a/gh_issue.py +++ b/gh_issue.py @@ -161,16 +161,26 @@ def resolve_repo(explicit: str | None, default_repo="") -> str | None: return None -def effective_default_repo(default_repo: str, repos: list[str] | None = None) -> str: +def effective_default_repo(default_repo: str, repos=None) -> str: """The preselected default repo for the dialog + the ``/issue`` command: the explicit ``github.default_repo`` if set, else the first entry in the ``github.repos`` picker list (explicit ∪ registry ∪ checkout remotes), else ``""`` (env still applies via ``resolve_repo``). Keeps the command, the tools and the dialog agreeing on the default. A malformed explicit default is returned as-is so the caller's ``bad_repo`` / ``default_repo_error`` names it instead of - silently routing to the next candidate.""" + silently routing to the next candidate. + + ``repos`` may be a list OR a zero-arg getter — the getter is only called when no + explicit default is set, so computing the picker (which may fork git to read + checkout remotes) is skipped on the common configured path.""" if (default_repo or "").strip(): return default_repo.strip() + if callable(repos): + try: + repos = repos() + except Exception: # noqa: BLE001 — a broken picker getter reads as "no picker" + log.debug("[github] repos getter failed", exc_info=True) + repos = [] for r in repos or []: if (r or "").strip(): return r.strip() diff --git a/projects.py b/projects.py index 9d3206f..b571f55 100644 --- a/projects.py +++ b/projects.py @@ -27,8 +27,10 @@ (``github.com[:/]owner/name(.git)?``, HTTPS or SSH) so a fresh install that onboarded a project, or pointed the board at a checkout, gets a working default repo with nothing typed twice. These come LAST (after explicit + registry -``github:`` entries), are cached briefly per path, and a non-git or remote-less -path contributes nothing. +``github:`` entries), are cached per path (10 min — the register-time probe thread +primes the cache, the async routes call this via ``asyncio.to_thread``, and a +configured ``default_repo`` short-circuits it entirely), and a non-git or +remote-less path contributes nothing. **Every read degrades to ``[]``.** The plugin's ``min_protoagent_version`` stays 0.27.0 — the projection is additive, and bumping the floor would cut off older @@ -51,7 +53,7 @@ GITHUB_REMOTE_RE = re.compile(r"github\.com[:/]([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+?)(?:\.git)?/?$", re.I) _GIT_TIMEOUT = 3 # seconds — `git remote get-url` is local and instant; never let it hang a tool call -_REMOTE_TTL = 60.0 # seconds — per-path cache so a per-call getter doesn't fork git per tool call +_REMOTE_TTL = 600.0 # seconds — a checkout's origin rarely changes; the per-call getters must not fork git _remote_cache: dict[str, tuple[float, str | None]] = {} diff --git a/protoagent.plugin.yaml b/protoagent.plugin.yaml index bcb490f..da3f150 100644 --- a/protoagent.plugin.yaml +++ b/protoagent.plugin.yaml @@ -31,8 +31,10 @@ config: # Must be owner/name — anything else is a named error in # the views / tools (#23), never fed to `gh`. token: "" # OPTIONAL personal access token (secret → secrets.yaml). - # Injected into every `gh` run as GH_TOKEN, winning over an - # ambient GITHUB_TOKEN/GH_TOKEN; `gh auth login` works too. + # When set, injected into every `gh` run as GH_TOKEN and + # wins; when blank the env passes through untouched and + # gh's own order applies (GH_TOKEN > GITHUB_TOKEN > the + # `gh auth login` keyring). repos: [] # repo picker list (each owner/name) for /issue + views. # The host's managed-projects registry (ADR 0095, # protoAgent 0.115.0+) is ALWAYS added: every `projects:` @@ -56,7 +58,7 @@ secrets: [token] settings: - {key: write, label: "Allow write tools (this agent)", type: bool} - {key: default_repo, label: "Default repo (owner/name)", type: string, description: "owner/name — used when a tool or /issue omits the repo. Leave blank to fall back to the first picker entry."} - - {key: token, label: "GitHub token (PAT) — optional; gh auth login also works", type: secret, description: "A personal access token with repo scope. Wins over GITHUB_TOKEN/GH_TOKEN in the environment; leave blank to use `gh auth login`'s keyring login."} + - {key: token, label: "GitHub token (PAT) — optional; gh auth login also works", type: secret, description: "A personal access token with repo scope. When set it is used for every gh call (over any GH_TOKEN/GITHUB_TOKEN in the environment); leave blank to let gh use the environment or its own `gh auth login` keyring login."} - {key: repos, label: "Repo picker list", type: string_list} # Console views (ADR 0026/0057) — sandboxed iframes the plugin serves itself (api.py). @@ -78,9 +80,9 @@ public_paths: - /plugins/github/view - /plugins/github/new-issue -# Auth, in precedence order: the `token` secret above (Settings ▸ GitHub) > GITHUB_TOKEN / -# GH_TOKEN from the env > `gh`'s own keyring login (`gh auth login`). Nothing is needed -# for public-repo reads at low volume. `GET /api/plugins/github/status` + the +# Auth: a non-empty `token` secret (Settings ▸ GitHub) is injected and wins; otherwise +# the env passes through and gh's own precedence applies (GH_TOKEN > GITHUB_TOKEN > +# the `gh auth login` keyring). Nothing is needed for public-repo reads at low volume. `GET /api/plugins/github/status` + the # `github_status` tool report which applies; the views show a setup card when none does. # Declarative transparency (shown in the console; not yet enforced). diff --git a/read_tools.py b/read_tools.py index 219d449..422ed8c 100644 --- a/read_tools.py +++ b/read_tools.py @@ -1,6 +1,6 @@ """GitHub READ tools over `gh` — always registered (read-only is the safe default). -Eleven tools, all implemented: six ported from protoAgent's tools/github_tools.py +Twelve tools, all implemented: six ported from protoAgent's tools/github_tools.py (PRs, issues, diffs, CI), the repo-content readers (`github_read_file`, `github_read_pr_file`, `github_repo_contents`, `github_path_exists`), `github_pr_diff`, and `github_status` (the self-diagnosis probe — is `gh` installed / authenticated). @@ -12,11 +12,12 @@ from __future__ import annotations +import asyncio import re from langchain_core.tools import tool -from .gh_cli import bad_repo, check_gh_error, dicts, parse_json, run_gh +from .gh_cli import bad_repo, check_gh_error, dicts, error_kind, parse_json, run_gh from .gh_issue import current_default, default_repo_error, resolve_repo # Error-relevant lines to surface from a failed CI log (github_run_failure). @@ -27,11 +28,13 @@ ) -def get_read_tools(default_repo="", repos=None) -> list: +def get_read_tools(default_repo="", repos=None, registry=None) -> list: """Build the read tools. ``default_repo`` (``owner/name``, or a zero-arg getter returning it — the live-config case) is used whenever a tool's ``repo`` arg is omitted, so an agent with one configured repo needn't repeat it. ``repos`` (a list - or a getter) is the picker list, surfaced by ``github_status`` only.""" + or a getter) is the picker list, surfaced by ``github_status`` only — which also + reports its result to the host's setup-gap seam through ``registry`` (optional) + so the operator banner clears when the model's own check sees a recovery.""" def _repos() -> list[str]: try: @@ -364,15 +367,18 @@ async def github_path_exists(path: str, repo: str = "", ref: str = "") -> str: rc, out, serr = await run_gh(args) if rc == 0: return f"EXISTS: {repo}/{clean}" + (f" @ {ref.strip()}" if ref.strip() else "") - blob = (serr or out or "").lower() - if "404" in blob or "not found" in blob: + # Classify FIRST: an auth / rate-limit / missing-binary failure is UNVERIFIED + # (the classified error), never a MISSING verdict. Only a real 404 is MISSING — + # and a 404 can also mean the repo itself is inaccessible to this token. + kind = error_kind(rc, serr or out) + if kind == "not_found": return ( f"MISSING: {repo}/{clean}" + (f" @ {ref.strip()}" if ref.strip() else "") - + " — the path does not exist." + + " — the path does not exist (or the repo is inaccessible to this token; HTTP 404)." ) return ( - check_gh_error(rc, serr, repo=repo) + check_gh_error(rc, serr or out, repo=repo) or f"Error (gh exit {rc}): could not verify {repo}/{clean} — treat as UNVERIFIED (a Gap, not a finding)." ) @@ -423,10 +429,11 @@ async def github_status() -> str: it says exactly what's wrong and what the operator must do (install `gh`, run `gh auth login`, or paste a token in Settings ▸ GitHub). Takes no arguments. """ - from .status import compute_status, summarize_status + from .status import compute_and_report, summarize_status - default = current_default(default_repo) - st = await compute_status(default, _repos()) + # The getters may parse git remotes (blocking, cached) — off the event loop. + default, picker = await asyncio.to_thread(lambda: (current_default(default_repo), _repos())) + st = await compute_and_report(registry, default, picker) text = summarize_status(st) if bad := default_repo_error(default): text += f" NOTE: {bad}" diff --git a/status.py b/status.py index 9c25c64..d12f1d3 100644 --- a/status.py +++ b/status.py @@ -9,8 +9,15 @@ - the ``github_status`` read tool → a one-paragraph summary the model can act on instead of guessing from a tool error; - ``report_gaps`` → the host's operator-warning seam (``registry.report_setup_gap``, - guarded — the seam is newer than this plugin's floor), called from a best-effort - background probe at register time so the warning shows before anything is used. + guarded — the seam is newer than this plugin's floor). Called from a best-effort + background probe at register time so the warning shows before anything is used, + AND from every later status computation (``/status``, the tool — see + ``compute_and_report``) so the banner CLEARS live when the operator installs + `gh` / signs in and hits Re-check. Edge-triggered: a failing key gets its message, + a passing key gets ``None`` (the host's pop is idempotent). + +Each probe starts by dropping the cached `gh` path (``reset_gh_cache``) so a `gh` +installed after boot is found, and a `gh` removed after boot is noticed. Everything here is host-free and never raises: a probe failure is a status with ``error`` set, not an exception (the routes/tools/threads calling it must not die). @@ -24,7 +31,7 @@ import re import threading -from .gh_cli import resolve_gh, run_gh, token_source +from .gh_cli import auth_hint, reset_gh_cache, resolve_gh, run_gh, token_source log = logging.getLogger("protoagent.plugins.github") @@ -39,7 +46,7 @@ GAP_GH = "gh" GAP_AUTH = "auth" -_AUTH_HINT = "run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)" +_WHY_CAP = 80 # the host's setup-gap banner is ~300 chars; keep the hint, not the stack def _empty(default_repo: str = "", repos: list[str] | None = None) -> dict: @@ -73,8 +80,9 @@ def _parse_auth_json(out: str) -> tuple[bool, str | None, str | None, str | None if str(a.get("state") or "") == "success": return True, str(a.get("login") or "") or None, host, None err = str(a.get("error") or "token rejected") - # gh embeds the whole HTTP body; keep the first line only. - return False, None, host, err.splitlines()[0][:200] + # gh embeds the whole HTTP body ('… 401 Unauthorized body: "{\r\n …"') — keep the + # status line only, so the banner/card carries the HINT rather than escaped JSON. + return False, None, host, err.split(" body:", 1)[0].splitlines()[0][:200] def _parse_auth_text(rc: int, out: str, serr: str) -> tuple[bool, str | None, str | None, str | None]: @@ -102,6 +110,7 @@ async def compute_status(default_repo: str = "", repos: list[str] | None = None) default_repo, repos}``. Never raises; every failure lands in ``error``.""" st = _empty(default_repo, repos) try: + reset_gh_cache() # a probe must see a gh installed (or removed) since the last one path = resolve_gh() if not path: st["error"] = "gh CLI is not installed or not on PATH" @@ -153,14 +162,14 @@ def summarize_status(st: dict) -> str: return ( "GitHub CLI is NOT installed (no `gh` on PATH or in the usual install dirs). " "Install it — https://cli.github.com (macOS: `brew install gh`; Debian/Ubuntu: `sudo apt install gh`) — " - f"then {_AUTH_HINT}. Every github_* tool will return an error until then." + repo_bit + f"then {auth_hint('none')}. Every github_* tool will return an error until then." + repo_bit ) ver = f" v{st['gh_version']}" if st.get("gh_version") else "" if not st.get("authenticated"): - why = f" ({st['error']})" if st.get("error") else "" + why = f" ({_why(st)})" if st.get("error") else "" return ( f"GitHub CLI{ver} is installed at {st['gh_path']} but NOT authenticated{why}. " - f"To fix: {_AUTH_HINT}. Read tools on public repos may still work at low volume; " + f"To fix: {auth_hint(src)}. Read tools on public repos may still work at low volume; " "everything else will return 'not authenticated' until then." + src_bit + repo_bit ) who = f" as {st['login']}" if st.get("login") else "" @@ -172,8 +181,16 @@ def summarize_status(st: dict) -> str: ) +def _why(st: dict) -> str: + """The error fragment, first line only, capped — so a banner keeps the HINT.""" + why = str(st.get("error") or "").splitlines()[0] if st.get("error") else "" + return why if len(why) <= _WHY_CAP else why[: _WHY_CAP - 1] + "…" + + def gaps_for(st: dict) -> dict[str, str | None]: - """The setup-gap messages this status implies, keyed by gap id; ``None`` = clear.""" + """The setup-gap messages this status implies, keyed by gap id; ``None`` = clear. + Every key is ALWAYS present (a passing key is ``None``), so reporting this dict + clears what recovered — edge-triggered against the host's idempotent pop.""" gaps: dict[str, str | None] = {GAP_GH: None, GAP_AUTH: None} if not st.get("gh_path"): gaps[GAP_GH] = ( @@ -182,8 +199,8 @@ def gaps_for(st: dict) -> dict[str, str | None]: ) return gaps if not st.get("authenticated"): - why = f" ({st['error']})" if st.get("error") else "" - gaps[GAP_AUTH] = f"GitHub CLI is not authenticated{why} — {_AUTH_HINT}." + why = f" ({_why(st)})" if st.get("error") else "" + gaps[GAP_AUTH] = f"GitHub CLI is not authenticated{why} — {auth_hint(st.get('token_source'))}." return gaps @@ -203,6 +220,17 @@ def report_gaps(registry, st: dict) -> bool: return False +async def compute_and_report(registry, default_repo: str = "", repos: list[str] | None = None) -> dict: + """``compute_status`` + ``report_gaps`` in one call — what ``/status`` and the + ``github_status`` tool use, so a recovery (gh installed, signed in) clears the + host banner on the very request that observed it. ``registry`` may be ``None`` + (no host wired one): then it's just the status.""" + st = await compute_status(default_repo, repos) + if registry is not None: + report_gaps(registry, st) + return st + + def probe_in_background(registry, default_repo="", repos=None) -> threading.Thread | None: """Compute the status off the register() hot path and report its gaps. Only started when the host exposes the seam (nothing to report otherwise). The thread diff --git a/tests/conftest.py b/tests/conftest.py index ecc17db..e45e8e4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -68,6 +68,22 @@ def tool_names(self) -> list[str]: return [getattr(t, "name", getattr(t, "__name__", "?")) for t in self.tools] +@pytest.fixture(autouse=True) +def _no_ambient_github_auth(monkeypatch): + """Every test starts with no env token, no config-token getter, and no cached gh + path — the auth HINT branches on token_source(), so an operator's ambient GH_TOKEN + must never leak into an assertion. Tests that want a token set it explicitly.""" + from ghplugin import gh_cli + + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + gh_cli.set_token_getter(None) + gh_cli.reset_gh_cache() + yield + gh_cli.set_token_getter(None) + gh_cli.reset_gh_cache() + + @pytest.fixture def make_registry(): return _Registry diff --git a/tests/test_board_view.py b/tests/test_board_view.py index 68939f6..ef64b04 100644 --- a/tests/test_board_view.py +++ b/tests/test_board_view.py @@ -371,3 +371,81 @@ def test_register_wires_the_data_router_to_the_live_config(make_registry): assert c.get("/api/plugins/github/config").json()["repos"] == ["o/n"] reg.config["repos"] = ["o/n", "o/added"] # the (shared) config dict is edited after register() assert c.get("/api/plugins/github/config").json()["repos"] == ["o/n", "o/added"] + + +class _SeamRegistry: + """A host with the setup-gap seam — records every (key, message) it's handed.""" + + def __init__(self): + self.calls: list[tuple[str, str | None]] = [] + + def report_setup_gap(self, key, message): + self.calls.append((key, message)) + + +def test_status_route_reports_gaps_and_clears_them_on_recovery(): + """The banner must clear on the very Re-check that sees gh installed / signed in — + not only at register time (#2 of the adversarial review).""" + seam = _SeamRegistry() + app = FastAPI() + app.include_router(build_data_router(_CFG, registry=seam), prefix="/api/plugins/github") + c = TestClient(app) + + with patch("ghplugin.status.resolve_gh", return_value=None): + assert c.get("/api/plugins/github/status").json()["gh_path"] is None + assert dict(seam.calls)["gh"] and "not installed" in dict(seam.calls)["gh"] + assert dict(seam.calls)["auth"] is None + + seam.calls.clear() + ok = '{"hosts":{"github.com":[{"state":"success","active":true,"host":"github.com","login":"kj"}]}}' + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_status_gh(auth=(0, ok, "")))), + ): + assert c.get("/api/plugins/github/status").json()["authenticated"] is True + assert dict(seam.calls) == {"gh": None, "auth": None} # CLEARED, live + + +def test_status_route_without_a_registry_still_works(): + with patch("ghplugin.status.resolve_gh", return_value=None): + assert TestClient(_app()).get("/api/plugins/github/status").status_code == 200 + + +def test_register_hands_the_registry_to_the_data_router(make_registry): + """register() passes the registry so /status can report; a host without the seam is fine.""" + seen = {} + real = __import__("ghplugin.api", fromlist=["build_data_router"]).build_data_router + + def spy(cfg, registry=None): + seen["registry"] = registry + return real(cfg, registry=registry) + + reg = make_registry(_CFG) + with patch("ghplugin.api.build_data_router", spy): + register(reg) + assert seen["registry"] is reg + + +def test_routes_resolve_the_picker_off_the_event_loop(): + """resolve_config may fork git (checkout remotes) — the routes must run it in a worker + thread, never on the loop (#5 of the adversarial review).""" + import threading + + from ghplugin import api + + threads = [] + real = api.resolve_config + + def spy(cfg): + threads.append(threading.current_thread()) + return real(cfg) + + with patch("ghplugin.api.resolve_config", spy), patch("ghplugin.status.resolve_gh", return_value=None): + c = TestClient(_app()) + c.get("/api/plugins/github/config") + c.get("/api/plugins/github/status") + c.post("/api/plugins/github/issue", json={"title": ""}) + assert len(threads) == 3 + # TestClient runs the app loop on its own portal thread; to_thread hands off to a + # default-executor worker, whose name is the giveaway. + assert all(t.name.startswith("asyncio_") for t in threads), [t.name for t in threads] diff --git a/tests/test_gh_cli.py b/tests/test_gh_cli.py index 1186555..9d56a14 100644 --- a/tests/test_gh_cli.py +++ b/tests/test_gh_cli.py @@ -58,17 +58,47 @@ def test_resolve_gh_falls_back_to_install_dirs_when_path_is_bare(tmp_path, monke assert resolve_gh() == str(fake) -def test_resolve_gh_is_cached_until_reset(tmp_path, monkeypatch): +def test_resolve_gh_never_caches_a_miss(tmp_path, monkeypatch): + """No gh at boot → the operator installs it → the NEXT call sees it (no restart). + Caching the miss meant Re-check said 'not installed' forever while /config's + availability check said true.""" monkeypatch.setenv("PATH", str(tmp_path)) monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", ()) assert resolve_gh() is None - # A binary appearing later isn't seen until the cache is reset. fake = tmp_path / "gh" fake.write_text("#!/bin/sh\n") fake.chmod(fake.stat().st_mode | stat.S_IXUSR) - assert resolve_gh() is None - gh_cli.reset_gh_cache() + assert resolve_gh() == str(fake) # seen immediately + + +def test_resolve_gh_caches_a_hit_until_reset(tmp_path, monkeypatch): + monkeypatch.setenv("PATH", str(tmp_path)) + monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", ()) + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) assert resolve_gh() == str(fake) + fake.unlink() # uninstalled mid-process + assert resolve_gh() == str(fake) # the hit is cached … + gh_cli.reset_gh_cache() + assert resolve_gh() is None # … until a probe resets it + + +async def test_status_probe_resets_the_cache_so_recheck_sees_both_transitions(tmp_path, monkeypatch): + """Re-check after installing gh → found; Re-check after removing it → gone.""" + from ghplugin.api import gh_available + from ghplugin.status import compute_status + + monkeypatch.setenv("PATH", str(tmp_path)) + monkeypatch.setattr(gh_cli, "_FALLBACK_BIN_DIRS", ()) + assert (await compute_status())["gh_path"] is None and gh_available() is False + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\necho 'gh version 9.9.9 (2030-01-01)'\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) + st = await compute_status() + assert st["gh_path"] == str(fake) and st["gh_version"] == "9.9.9" and gh_available() is True + fake.unlink() + assert (await compute_status())["gh_path"] is None and gh_available() is False # agree again async def test_run_gh_missing_binary_returns_not_raises(tmp_path, monkeypatch): @@ -88,11 +118,18 @@ def test_no_token_anywhere(): assert "GH_TOKEN" not in gh_env() -def test_env_token_is_injected(monkeypatch): - monkeypatch.setenv("GITHUB_TOKEN", "ghp_env") - assert resolve_token() == "ghp_env" +def test_env_token_is_passed_through_untouched_in_ghs_own_order(monkeypatch): + """With no config token the child env is the ambient env, verbatim — gh applies + its own precedence (GH_TOKEN > GITHUB_TOKEN > keyring). We never rewrite GH_TOKEN + from GITHUB_TOKEN (that inverted gh's order).""" + monkeypatch.setenv("GITHUB_TOKEN", "ghp_github") + monkeypatch.setenv("GH_TOKEN", "ghp_gh") + assert resolve_token() == "ghp_gh" # GH_TOKEN first, like gh assert token_source() == "env" - assert gh_env()["GH_TOKEN"] == "ghp_env" + env = gh_env() + assert env["GH_TOKEN"] == "ghp_gh" and env["GITHUB_TOKEN"] == "ghp_github" # untouched + monkeypatch.delenv("GH_TOKEN") + assert resolve_token() == "ghp_github" and "GH_TOKEN" not in gh_env() # nothing injected def test_config_token_wins_over_env(monkeypatch): @@ -112,6 +149,60 @@ def test_blank_config_token_falls_through_to_env(monkeypatch): assert resolve_token() == "ghp_env" +# ── the runner never raises ────────────────────────────────────────────────────── + + +async def test_run_gh_spawn_oserror_is_an_error_tuple(tmp_path, monkeypatch): + """ENOEXEC (a foreign-arch / truncated ~/.local/bin/gh), EMFILE … — OSError from the + spawn must come back as (126, '', reason), not escape through the tool layer.""" + import errno + + monkeypatch.setattr(gh_cli, "resolve_gh", lambda: "/fake/gh") + + async def boom(*a, **k): + raise OSError(errno.ENOEXEC, "Exec format error") + + monkeypatch.setattr(gh_cli.asyncio, "create_subprocess_exec", boom) + rc, out, serr = await run_gh(["--version"]) + assert rc == 126 and out == "" and "could not run gh at /fake/gh" in serr and "Exec format error" in serr + assert check_gh_error(rc, serr).startswith("Error (gh exit 126): could not run gh at /fake/gh") + + +async def test_run_gh_timeout_kill_on_an_exited_process_does_not_raise(monkeypatch): + """The process can exit between the timeout and the kill — ProcessLookupError from + kill() must be swallowed, the timeout still reported.""" + import asyncio + + monkeypatch.setattr(gh_cli, "resolve_gh", lambda: "/fake/gh") + + class _Proc: + returncode = None + + async def communicate(self): + await asyncio.sleep(10) + + def kill(self): + raise ProcessLookupError() + + async def spawn(*a, **k): + return _Proc() + + monkeypatch.setattr(gh_cli.asyncio, "create_subprocess_exec", spawn) + rc, out, serr = await run_gh(["--version"], timeout=0) + assert rc == 1 and "timed out" in serr + + +async def test_run_gh_permission_error_is_126(monkeypatch): + monkeypatch.setattr(gh_cli, "resolve_gh", lambda: "/fake/gh") + + async def boom(*a, **k): + raise PermissionError("denied") + + monkeypatch.setattr(gh_cli.asyncio, "create_subprocess_exec", boom) + rc, _out, serr = await run_gh(["--version"]) + assert rc == 126 and "not executable" in serr + + def test_raising_token_getter_means_no_token(): def boom(): raise RuntimeError("config not loaded") @@ -143,6 +234,32 @@ def test_not_authenticated(rc, stderr): assert "Settings ▸ GitHub (github.token)" in err +def test_auth_hint_branches_on_the_token_source(monkeypatch): + """'run gh auth login' is WRONG advice when a token is what gh is rejecting.""" + from ghplugin.gh_cli import auth_hint + + assert auth_hint("none").startswith("run `gh auth login`") + assert "GH_TOKEN / GITHUB_TOKEN in the agent's environment was rejected" in auth_hint("env") + assert "token saved in Settings ▸ GitHub (github.token) was rejected" in auth_hint("config") + # the classified error follows the LIVE source + monkeypatch.setenv("GH_TOKEN", "ghp_bad") + assert "environment was rejected" in check_gh_error(4, "") + set_token_getter(lambda: "ghp_bad_config") + assert "Settings ▸ GitHub (github.token) was rejected — replace it there" in check_gh_error(4, "") + + +def test_error_kind_categories(): + from ghplugin.gh_cli import error_kind + + assert error_kind(0, "whatever") is None + assert error_kind(127, "") == "missing_binary" + assert error_kind(4, "") == "auth" + assert error_kind(1, "HTTP 403: API rate limit exceeded") == "rate_limit" + assert error_kind(1, "gh: Not Found (HTTP 404)") == "not_found" + assert error_kind(1, "GraphQL: Could not resolve to a Repository with the name 'o/n'.") == "not_found" + assert error_kind(1, "HTTP 500") == "generic" + + def test_repo_not_found_graphql(): err = check_gh_error(1, "GraphQL: Could not resolve to a Repository with the name 'o/nope'. (repository)") assert err.startswith("Error: repo 'o/nope' not found or not accessible") diff --git a/tests/test_gh_issue.py b/tests/test_gh_issue.py index f144807..602fe5c 100644 --- a/tests/test_gh_issue.py +++ b/tests/test_gh_issue.py @@ -110,6 +110,24 @@ def test_effective_default_repo(): assert effective_default_repo("", []) == "" +def test_effective_default_repo_only_calls_the_picker_getter_when_needed(): + """The picker getter may fork git (checkout remotes) — a configured default_repo + must short-circuit it (#5 of the adversarial review).""" + calls = [] + + def picker(): + calls.append(1) + return ["o/a"] + + assert effective_default_repo("o/explicit", picker) == "o/explicit" and calls == [] + assert effective_default_repo("", picker) == "o/a" and len(calls) == 1 + + def boom(): + raise RuntimeError("host not ready") + + assert effective_default_repo("", boom) == "" # never raises + + # --- run_issue_command ------------------------------------------------------- diff --git a/tests/test_no_raise_sweep.py b/tests/test_no_raise_sweep.py index 51400ca..7951723 100644 --- a/tests/test_no_raise_sweep.py +++ b/tests/test_no_raise_sweep.py @@ -134,6 +134,55 @@ async def test_every_tool_survives_a_raising_runner(make_registry): gh_cli.reset_gh_cache() +@pytest.mark.parametrize("exc", ["enoexec", "emfile", "kill-after-exit"]) +async def test_every_tool_survives_a_failing_spawn(make_registry, monkeypatch, exc): + """OSError from the spawn (ENOEXEC on a foreign-arch ~/.local/bin/gh, EMFILE) and a + ProcessLookupError from kill() after a timeout must all be Error strings (#4).""" + import asyncio + import errno + + from ghplugin import gh_cli + + tools = _all_tools(make_registry) + monkeypatch.setattr(gh_cli, "resolve_gh", lambda: "/fake/gh") + + class _Proc: + returncode = None + + async def communicate(self): + await asyncio.sleep(10) + + def kill(self): + raise ProcessLookupError() + + async def spawn(*a, **k): + if exc == "enoexec": + raise OSError(errno.ENOEXEC, "Exec format error") + if exc == "emfile": + raise OSError(errno.EMFILE, "Too many open files") + return _Proc() + + monkeypatch.setattr(gh_cli.asyncio, "create_subprocess_exec", spawn) + monkeypatch.setattr(gh_cli, "_COMMAND_TIMEOUT", 0) + if exc == "kill-after-exit": + # the timeout path: every run_gh call must time out instantly, then kill raises + real_wait_for = asyncio.wait_for + + async def instant(coro, timeout=None): + return await real_wait_for(coro, timeout=0) + + monkeypatch.setattr(gh_cli.asyncio, "wait_for", instant) + # run_gh is looked up at call time in each module; patch the underlying spawn only, + # so the REAL run_gh (the no-raise boundary) is what's under test here. + with patch("ghplugin.status.resolve_gh", return_value="/fake/gh"): + for name, tool in tools.items(): + try: + result = await tool.ainvoke(_ARGS.get(name, {})) + except Exception as e: # noqa: BLE001 + raise AssertionError(f"{name} RAISED on spawn failure {exc!r}: {type(e).__name__}: {e}") from e + assert isinstance(result, str), f"{name} returned {type(result).__name__} on {exc!r}" + + async def test_repo_contents_on_a_file_says_so(make_registry): """The bug that motivated the sweep: a FILE path → dict → was an AttributeError.""" tools = _all_tools(make_registry) diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index fc82c9c..ff9316c 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -458,3 +458,38 @@ def r(): with patch("ghplugin.status.resolve_gh", return_value=None): out = await tools["github_status"].ainvoke({}) assert "Default repo: o/live." in out and calls == {"d": 1, "r": 1} + + +@pytest.mark.asyncio +async def test_status_tool_reports_to_the_setup_gap_seam(): + """The model's own check is a recovery observation too — it must clear the banner.""" + calls = [] + + class _Seam: + def report_setup_gap(self, key, message): + calls.append((key, message)) + + tools = {t.name: t for t in get_read_tools("o/n", ["o/n"], registry=_Seam())} + with patch("ghplugin.status.resolve_gh", return_value=None): + await tools["github_status"].ainvoke({}) + assert dict(calls)["gh"] and dict(calls)["auth"] is None + + +@pytest.mark.asyncio +async def test_path_exists_classifies_before_the_missing_verdict(): + """An auth / rate-limit / missing-binary failure is UNVERIFIED (the classified error), + never MISSING — only a real 404 is a verdict, and it names the inaccessible-repo case.""" + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(4, "", "please run: gh auth login"))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "x"}) + assert out.startswith("Error: GitHub CLI is not authenticated") and "MISSING" not in out + with patch( + "ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(127, "", "gh CLI is not installed or not on PATH.")) + ): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "x"}) + assert out.startswith("Error: gh CLI is not installed") and "MISSING" not in out + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(1, "", "HTTP 403: API rate limit exceeded"))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "x"}) + assert out.startswith("Error: GitHub API rate limit hit") + with patch("ghplugin.read_tools.run_gh", new=AsyncMock(return_value=(1, "", "gh: Not Found (HTTP 404)"))): + out = await _path_exists_tool().ainvoke({"repo": "owner/name", "path": "x"}) + assert out.startswith("MISSING: owner/name/x") and "repo is inaccessible" in out diff --git a/tests/test_register.py b/tests/test_register.py index 913f2f5..aab5819 100644 --- a/tests/test_register.py +++ b/tests/test_register.py @@ -152,3 +152,26 @@ def report_setup_gap(self, key, message): plain = make_registry({}) register(plain) # no seam → no thread assert started == [] + + +async def test_configured_default_repo_skips_the_picker_computation(make_registry, monkeypatch): + """With default_repo set, a tool call must not compute the picker (registry + git + remotes) at all — the common path stays free of subprocess work.""" + from unittest.mock import AsyncMock, patch + + from ghplugin import projects + + monkeypatch.delenv("GITHUB_DEFAULT_REPO", raising=False) + monkeypatch.delenv("GH_REPO", raising=False) + calls = [] + monkeypatch.setattr(projects, "remote_repos", lambda: (calls.append(1), [])[1]) + reg = make_registry({"default_repo": "o/set", "repos": ["o/other"]}) + register(reg) + tool = {t.name: t for t in reg.tools}["github_list_issues"] + with patch("ghplugin.read_tools.run_gh", AsyncMock(return_value=(0, "[]", ""))): + await tool.ainvoke({}) + assert calls == [] # picker never computed + reg.config["default_repo"] = "" + with patch("ghplugin.read_tools.run_gh", AsyncMock(return_value=(0, "[]", ""))): + await tool.ainvoke({}) + assert calls == [1] # …only when the default is unset diff --git a/tests/test_status.py b/tests/test_status.py index ea70d42..73f561f 100644 --- a/tests/test_status.py +++ b/tests/test_status.py @@ -103,7 +103,7 @@ async def test_active_account_failing_is_not_authenticated_even_with_a_good_inac ): st = await compute_status() assert st["authenticated"] is False and st["login"] is None - assert st["error"].startswith("non-200 OK status code: 401") and "\n" not in st["error"] + assert st["error"] == "non-200 OK status code: 401 Unauthorized" # the body is dropped async def test_not_logged_in_json_is_empty_hosts(): @@ -199,11 +199,43 @@ def test_summary_unauthenticated_tells_how_to_fix(): } ) assert "NOT authenticated (not logged in)" in text - assert "gh auth login" in text and "Settings ▸ GitHub (github.token)" in text + # an ENV token is what was rejected → say that, not "run gh auth login" + assert "GH_TOKEN / GITHUB_TOKEN in the agent's environment was rejected" in text assert "Default repo: o/n." in text and "2 repo(s) in the picker: o/n, o/m" in text assert "GITHUB_TOKEN/GH_TOKEN env" in text +def test_summary_unauthenticated_without_a_token_says_gh_auth_login(): + text = summarize_status( + { + "gh_path": "/usr/bin/gh", + "gh_version": "2.9", + "authenticated": False, + "error": "not logged in", + "default_repo": "", + "repos": [], + "token_source": "none", + } + ) + assert "To fix: run `gh auth login` in a terminal, or paste a token in Settings ▸ GitHub (github.token)." in text + + +def test_summary_and_gaps_cap_the_error_fragment_so_the_hint_survives(): + long_err = "non-200 OK status code: 401 Unauthorized body: " + "x" * 300 + st = { + "gh_path": "/usr/bin/gh", + "authenticated": False, + "error": long_err, + "token_source": "config", + "default_repo": "", + "repos": [], + } + gap = gaps_for(st)[GAP_AUTH] + assert "…" in gap and len(gap) < 300 # the host banner is ~300 chars; the hint must fit + assert "Settings ▸ GitHub (github.token) was rejected" in gap + assert "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" not in summarize_status(st) + + def test_summary_healthy_is_one_paragraph(): text = summarize_status( { @@ -224,11 +256,40 @@ def test_summary_healthy_is_one_paragraph(): def test_gaps_for_each_state(): assert gaps_for({"gh_path": None}) == {GAP_GH: gaps_for({"gh_path": None})[GAP_GH], GAP_AUTH: None} assert "not installed" in gaps_for({"gh_path": None})[GAP_GH] - g = gaps_for({"gh_path": "/x/gh", "authenticated": False, "error": "not logged in"}) - assert g[GAP_GH] is None and "not authenticated (not logged in)" in g[GAP_AUTH] + g = gaps_for({"gh_path": "/x/gh", "authenticated": False, "error": "not logged in", "token_source": "none"}) + assert g[GAP_GH] is None and "not authenticated (not logged in) — run `gh auth login`" in g[GAP_AUTH] + g = gaps_for({"gh_path": "/x/gh", "authenticated": False, "error": "401", "token_source": "env"}) + assert "environment was rejected" in g[GAP_AUTH] assert gaps_for({"gh_path": "/x/gh", "authenticated": True}) == {GAP_GH: None, GAP_AUTH: None} +async def test_compute_and_report_clears_the_banner_on_recovery(): + """Edge-triggered: the failing key gets its message; on the next probe that passes, + the SAME key gets None — the host's pop is idempotent, so the banner clears live.""" + from ghplugin.status import compute_and_report + + seam = _Seam() + with patch("ghplugin.status.resolve_gh", return_value=None): + st = await compute_and_report(seam, "o/n", ["o/n"]) + assert st["gh_path"] is None and dict(seam.calls)[GAP_GH] and dict(seam.calls)[GAP_AUTH] is None + seam.calls.clear() + with ( + patch("ghplugin.status.resolve_gh", return_value="/usr/bin/gh"), + patch("ghplugin.status.run_gh", new=AsyncMock(side_effect=_gh())), + ): + st = await compute_and_report(seam, "o/n", ["o/n"]) + assert st["authenticated"] is True + assert dict(seam.calls) == {GAP_GH: None, GAP_AUTH: None} # both CLEARED on the recovering probe + + +async def test_compute_and_report_without_a_registry_is_just_status(): + from ghplugin.status import compute_and_report + + with patch("ghplugin.status.resolve_gh", return_value=None): + st = await compute_and_report(None, "", []) + assert st["gh_path"] is None + + class _Seam: def __init__(self, raise_on_call=False): self.calls: list[tuple[str, str | None]] = [] diff --git a/view.py b/view.py index 4f7d6aa..b7d4123 100644 --- a/view.py +++ b/view.py @@ -57,8 +57,13 @@ } else if(!st.authenticated){ lines.push('<div class="st"><span class="warn">!</span> GitHub CLI is not signed in</div>' + '<div class="sd">Found <code>gh</code>' + (st.gh_version ? ' v' + esc(st.gh_version) : '') + ' at <code>' + esc(st.gh_path) + '</code>' - + (st.error ? ' — ' + esc(st.error) : '') + '.<br>Run <code>gh auth login</code> in a terminal, or paste a personal access token' - + ' in <b>Settings ▸ GitHub</b> (github.token)' + (st.token_source === 'config' ? ' — the saved token was rejected; replace it' : '') + '.</div>'); + + (st.error ? ' — ' + esc(st.error) : '') + '.<br>' + + (st.token_source === 'config' + ? 'The token saved in <b>Settings ▸ GitHub</b> (github.token) was rejected — replace it there, or clear it to fall back to <code>gh auth login</code>.' + : st.token_source === 'env' + ? 'The <code>GH_TOKEN</code> / <code>GITHUB_TOKEN</code> in the agent\'s environment was rejected — fix or unset it, or paste a working token in <b>Settings ▸ GitHub</b> (github.token).' + : 'Run <code>gh auth login</code> in a terminal, or paste a personal access token in <b>Settings ▸ GitHub</b> (github.token).') + + '</div>'); } if(st && st.default_repo_error){ lines.push('<div class="st"><span class="warn">!</span> Default repo is malformed</div>'