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..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 | `gh` ambient auth, or `GITHUB_TOKEN`/`GH_TOKEN` from the env | +| 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 @@ -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 # 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 -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 — 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 +`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,81 @@ 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`, 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 +else keeps the `Error (gh exit N): ` 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()`. **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 - **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) 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. - **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 — " +git push origin vX.Y.Z +gh release create vX.Y.Z --title "vX.Y.Z" --notes "" +``` + +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..adb1bf4 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, 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 +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,31 @@ 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**: 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. + +**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 on the plugin's `GET /config` and +in the views' setup card (not on Settings save), 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/__init__.py b/__init__.py index 23a9858..f9d2066 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,48 @@ 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. `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). 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, registry=registry) for t in read: registry.register_tool(t) n_read = len(read) @@ -63,7 +98,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 +117,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 +134,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, registry=registry), 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..94ec32b 100644 --- a/api.py +++ b/api.py @@ -7,17 +7,21 @@ 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 +import asyncio 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,9 @@ 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) — 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: @@ -50,8 +55,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 +77,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 +86,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``).""" @@ -109,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 @@ -118,30 +149,41 @@ 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, effective_default_repo, file_issue, labels_for, resolve_repo + from .gh_issue import IssueRequest, file_issue, labels_for, resolve_repo + 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: - 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 = 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``). 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 @router.get("/issues") async def _issues(repo: str, state: str = "open") -> dict: @@ -153,18 +195,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 = await _resolved() 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/gh_cli.py b/gh_cli.py index af68599..90cc473 100644 --- a/gh_cli.py +++ b/gh_cli.py @@ -1,9 +1,23 @@ """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: 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, +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 +25,14 @@ from __future__ import annotations import asyncio +import contextlib +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 +40,31 @@ # 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") + + +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: """Validate an `owner/name` repo slug; return an Error string if invalid, else None.""" @@ -31,26 +76,119 @@ 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. 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 and _gh_path: + 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 + if found: + _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` 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("GH_TOKEN") or os.environ.get("GITHUB_TOKEN"): + return "env" + return "none" + + +def gh_env() -> dict: + """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 + 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 ( @@ -60,14 +198,119 @@ 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: - 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." + 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}" -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 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 "missing_binary" + 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 "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}." + 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 + 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/gh_issue.py b/gh_issue.py index 5f3e121..3410cb9 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,25 +116,71 @@ 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 effective_default_repo(default_repo: str, repos: list[str] | None = None) -> str: +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=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. + + ``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() @@ -214,6 +269,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 +284,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..b571f55 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,18 @@ 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 <path> 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 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 hosts that don't want it anyway. So the host import is lazy and broadly guarded: @@ -29,6 +42,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 = 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]] = {} + + +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 +117,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/protoagent.plugin.yaml b/protoagent.plugin.yaml index 1b2389d..da3f150 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,31 @@ 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). + # 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:` @@ -33,11 +45,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. 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). @@ -59,8 +80,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: 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). 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/read_tools.py b/read_tools.py index 47f0bf5..422ed8c 100644 --- a/read_tools.py +++ b/read_tools.py @@ -1,21 +1,24 @@ """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. +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). +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 asyncio 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, 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). _CI_ERR_RE = re.compile( @@ -25,9 +28,19 @@ ) -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, 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 — 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: + 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 +64,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 +92,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 +133,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 +164,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 +186,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 +222,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 +251,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 +268,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 +287,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 +315,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 +330,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}" @@ -360,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) + 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)." ) @@ -388,12 +398,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 +421,24 @@ 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_and_report, summarize_status + + # 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}" + return text + return [ github_get_pr, github_get_issue, @@ -417,4 +451,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/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. 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/status.py b/status.py new file mode 100644 index 0000000..d12f1d3 --- /dev/null +++ b/status.py @@ -0,0 +1,258 @@ +"""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, + 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). +""" + +from __future__ import annotations + +import asyncio +import json +import logging +import re +import threading + +from .gh_cli import auth_hint, reset_gh_cache, 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" + +_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: + 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 ('… 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]: + """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: + 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" + 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('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" ({_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(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 "" + 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 _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. + 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] = ( + "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" ({_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 + + +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 + + +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 + 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/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 b2801e3..ef64b04 100644 --- a/tests/test_board_view.py +++ b/tests/test_board_view.py @@ -254,3 +254,198 @@ 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"] + + +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 new file mode 100644 index 0000000..9d56a14 --- /dev/null +++ b/tests/test_gh_cli.py @@ -0,0 +1,302 @@ +"""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_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 + fake = tmp_path / "gh" + fake.write_text("#!/bin/sh\n") + fake.chmod(fake.stat().st_mode | stat.S_IXUSR) + 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): + 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_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" + 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): + """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" + + +# ── 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") + + 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_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") + + +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 diff --git a/tests/test_gh_issue.py b/tests/test_gh_issue.py index 4dd0244..602fe5c 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,12 +52,82 @@ 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 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 new file mode 100644 index 0000000..7951723 --- /dev/null +++ b/tests/test_no_raise_sweep.py @@ -0,0 +1,194 @@ +"""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, "<<<not json>>> \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() + + +@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) + 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_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() == "" diff --git a/tests/test_read_tools.py b/tests/test_read_tools.py index d88f3a8..ff9316c 100644 --- a/tests/test_read_tools.py +++ b/tests/test_read_tools.py @@ -372,3 +372,124 @@ 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} + + +@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 87facb7..aab5819 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,116 @@ 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 == [] + + +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 new file mode 100644 index 0000000..73f561f --- /dev/null +++ b/tests/test_status.py @@ -0,0 +1,343 @@ +"""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"] == "non-200 OK status code: 401 Unauthorized" # the body is dropped + + +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 + # 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( + { + "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", "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]] = [] + 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..b7d4123 100644 --- a/view.py +++ b/view.py @@ -15,10 +15,74 @@ 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>' + + (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>' + + '<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 +121,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 +134,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 +201,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 +246,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 +271,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 +302,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) 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}."