feat: add GitHub REST client (ToolKit.GitHub.Client) - #14
Conversation
Add a thin Req-based wrapper for the GitHub REST API shared by registry-manager and thesis-monitor: - token injection via :token_provider (default: gh auth token) - generic get/post/put/patch plus endpoint helpers for contents (GET/PUT with SHA optimistic locking and base64), repository info, branches, commits, pulls (list/reviews/requested reviewers), issue comments, PR close, and repository archive - error classification: 404 -> :not_found, 401/403 -> :unauthorized, with not_found_error?/1 and unauthorized_error?/1 predicates - tests use Req.Test (Plug stub); no real HTTP in the suite Orchestration and response parsing stay in each tool (epic D2). Resolves #5 Claude-Session: https://claude.ai/code/session_015FdaeD1BANgCMUcNTUjKVV
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @doc """ | ||
| ブランチ一覧を取得する。`:per_page` を指定できる。 | ||
| """ | ||
| @spec list_branches(String.t(), keyword()) :: result() |
There was a problem hiding this comment.
🚨 [HIGH] ページネーション(複数ページ取得)のサポートが欠如しています。GitHub API の多くのエンドポイントはデフォルトで30件、最大100件しか返しません。list_branches、list_commits、list_pull_requests などは大規模リポジトリで結果が切り捨てられる可能性があります。
Link ヘッダを解析して自動的に全ページを取得するオプション(例: :paginate フラグ)、または少なくともレスポンスヘッダを呼び出し元に公開する仕組みを検討してください。現状では呼び出し側がページネーションを制御する手段がありません。
There was a problem hiding this comment.
据え置きます(設計意図)。本モジュールは epic D2 の方針で「薄い HTTP 境界」に徹し、orchestration はツール側に残します。採用予定の registry-manager / thesis-monitor はいずれも per_page(最大 100)で足りる規模で、thesis-monitor 側には「100 件超は非現実的なためページネーションは追わない」と明記されています。また「呼び出し側が制御する手段がない」は当たりません。各ヘルパは呼び出し元の :params を保持してマージするため(64e42eb で明確化)、params: [page: 2] を渡せば手動ページングできますし、汎用 get/2 でも任意のクエリを指定できます。Link ヘッダの自動追跡が必要になった時点で別 issue として検討します。
|
|
||
| defp build_headers(token, opts) do | ||
| [ | ||
| {"accept", "application/vnd.github.v3+json"}, |
There was a problem hiding this comment.
application/vnd.github.v3+json は非推奨となっており、GitHub の現在の推奨は application/vnd.github+json(バージョン番号なし)です。また、GitHub は X-GitHub-Api-Version: 2022-11-28 ヘッダによる明示的なAPIバージョン固定を推奨しています。将来的なAPI変更への耐性を高めるため、ヘッダの更新を検討してください。
There was a problem hiding this comment.
修正しました(64e42eb)。Accept を application/vnd.github+json に変更し、X-GitHub-Api-Version: 2022-11-28 を常時送るようにしました。テストのヘッダ検証も更新済みです。
| defp extract_error_message(_body, status), do: "HTTP #{status}" | ||
|
|
||
| # クエリパラメータ(nil の値は落とす)を opts の :params に載せる | ||
| defp put_params(opts, params) do |
There was a problem hiding this comment.
put_params/2 関数は既存の :params を上書きします。もし呼び出し元が opts に :params を既に設定していた場合、それが失われます。Keyword.merge や Keyword.update を使って既存のパラメータとマージする実装の方が安全です:
defp put_params(opts, params) do
present = Enum.reject(params, fn {_key, value} -> is_nil(value) end)
case present do
[] -> opts
_ ->
existing = Keyword.get(opts, :params, [])
Keyword.put(opts, :params, Keyword.merge(existing, present))
end
endThere was a problem hiding this comment.
修正しました(64e42eb)。put_params/2 を Keyword.update/4 + Keyword.merge/2 に変更し、呼び出し元の :params を保持するようにしました(同名キーはヘルパ側優先)。params: [page: 2] と per_page: 5 が両立することを確認するテストも追加しています。
| end | ||
| end | ||
|
|
||
| test "gh が見つからない場合はエラーメッセージを返す" do |
There was a problem hiding this comment.
gh が見つからない場合 のテストで System.put_env("PATH", "") を使っていますが、これはプロセス全体の環境変数を変更するため、async: true で並列実行される他のテストに影響を与える可能性があります。ExUnit.CaptureLog や Mox を使ったモック、あるいはこのテストだけ async: false にするか、テストを分離したモジュールに移動することを検討してください。
There was a problem hiding this comment.
修正しました(64e42eb)。PATH を書き換える 2 テストを ToolKit.GitHub.ClientGhCliTest(async: false)へ分離し、並列実行中の他テストへ影響しないようにしました。Mox 等の新規依存は本ライブラリの方針(依存最小)に沿って導入していません。
| """ | ||
| @spec gh_cli_token() :: {:ok, String.t()} | {:error, String.t()} | ||
| def gh_cli_token do | ||
| case System.cmd("gh", ["auth", "token"], stderr_to_stdout: true) do |
There was a problem hiding this comment.
ℹ️ [LOW] gh_cli_token/0 は ErlangError のみをrescueしていますが、System.cmd/3 が失敗する場合は ErlangError ではなく File.Error や他の例外が発生する可能性があります。また、gh コマンドが存在するが認証されていない場合(exit code != 0)のエラーメッセージが固定文字列で、実際のエラー出力が捨てられています。デバッグ性向上のため、実際の出力をエラーメッセージに含めることを検討してください:
{output, _exit_code} -> {:error, "GitHub CLI authentication failed: #{String.trim(output)}"}There was a problem hiding this comment.
一部修正しました(64e42eb)。認証失敗時(exit code != 0)は gh の実出力(stderr 込み)をエラーメッセージに含めるようにしました。rescue については、System.cmd/3 が実行ファイル不在時に送出するのは ErlangError(:enoent)であり、File.Error は System.cmd/3 からは発生しないため、現状の ErlangError rescue のままとします(PATH 不在ケースはテストで検証済み)。
- use application/vnd.github+json and pin X-GitHub-Api-Version - merge helper query params with caller-provided :params - keep gh CLI stderr output in the authentication error message - move PATH-mutating gh_cli_token tests to a non-async module Refs #5 Claude-Session: https://claude.ai/code/session_015FdaeD1BANgCMUcNTUjKVV
| ブランチ一覧を取得する。`:per_page` を指定できる。 | ||
| """ | ||
| @spec list_branches(String.t(), keyword()) :: result() | ||
| def list_branches(repo, opts \\ []) do |
There was a problem hiding this comment.
🚨 [HIGH] ページネーション(複数ページ)への対応が欠けています。GitHub REST API の多くのエンドポイントは Link ヘッダを使ったページネーションを返しますが、現在の実装は1ページ目しか取得できません。list_branches/2・list_commits/2・list_pull_requests/2 などで per_page を受け付けているにもかかわらず、全件取得のための自動ページネーション機能がないため、デフォルトの30件(または指定件数)を超えるデータが存在する場合にサイレントに切り捨てられます。少なくともドキュメントに「1ページ分のみ返す」旨を明記するか、Link ヘッダを辿る仕組みを検討してください。
There was a problem hiding this comment.
据え置きます(前ラウンドの同指摘 #14 (comment) への返信と同じ根拠)。本モジュールは epic D2 の方針で薄い HTTP 境界に徹し、採用予定の 2 ツールはいずれも per_page(最大 100)で足りる運用です。各ヘルパの @doc は「:per_page を指定できる」と単ページ取得の語彙で書いており、params: [page: N] による手動ページングも可能です(マージ挙動はテストで担保)。Link ヘッダ自動追跡が必要になった時点で別 issue にします。
| defp put_params(opts, params) do | ||
| case Enum.reject(params, fn {_key, value} -> is_nil(value) end) do | ||
| [] -> opts | ||
| present -> Keyword.update(opts, :params, present, &Keyword.merge(&1, present)) |
There was a problem hiding this comment.
put_params/2 の実装でヘルパ側のパラメータが呼び出し元の :params より優先されます(Keyword.merge(&1, present) は後者が優先)。コメントには「同名キーはヘルパ側を優先する」と書かれており意図的ですが、呼び出し元が明示的に渡したパラメータが上書きされる挙動は直感に反する可能性があります。優先順位の根拠をドキュメントに明記することを推奨します。
There was a problem hiding this comment.
据え置きます(挙動は意図どおり・根拠をここに明記)。ヘルパの名前付きオプション(since: / state: など)はそのヘルパの明示的な API であり、汎用の :params 経由の同名キーより優先されるべきです。そうしないと list_commits(repo, since: x, params: [since: y]) のような呼び出しでヘルパのシグネチャが嘘になります。優先順位は private 関数のコメントに記載済みで、:params 併用の挙動はテスト「ヘルパは呼び出し元の :params を保持したままマージする」で担保しています。
| {:error, "GitHub CLI authentication failed (run 'gh auth login'): #{String.trim(output)}"} | ||
| end | ||
| rescue | ||
| ErlangError -> {:error, "GitHub CLI (gh) not found in PATH"} |
There was a problem hiding this comment.
gh_cli_token/0 は ErlangError のみをレスキューしていますが、System.cmd/3 が gh コマンドを見つけられない場合に投げる例外は Elixir/OTP のバージョンによって異なる可能性があります。現在のテスト(PATH を空にする)で動作確認済みとのことですが、File.Error や他の例外が漏れるリスクがあります。より安全にするには rescue e -> {:error, inspect(e)} のように広くキャッチするか、:enoent を明示的に処理することを検討してください。
There was a problem hiding this comment.
据え置きます。System.cmd/3 は実行ファイル不在時に :erlang.error(:enoent) 由来の ErlangError を送出します(Elixir 本体の実装で OTP バージョンに依存しません)。File.Error は System.cmd/3 からは発生しません。rescue e -> {:error, inspect(e)} のような広いキャッチは、引数の型誤りなど本来クラッシュすべきバグまで握りつぶすため採用しません。PATH 不在ケースは ClientGhCliTest で実挙動を検証済みです。
| end | ||
|
|
||
| test "gh が見つからない場合はエラーメッセージを返す" do | ||
| original_path = System.get_env("PATH") |
There was a problem hiding this comment.
System.put_env("PATH", "") でPATHを空にするアプローチは、async: false にしているとはいえ、テスト実行中に他のプロセスが PATH を参照する場合に影響を与えます。また、try/after ブロックで元の値を復元していますが、original_path が nil(環境変数未設定)の場合に System.put_env/2 に nil を渡してしまいます。original_path || "" のようなフォールバックを追加してください。
There was a problem hiding this comment.
据え置きます。mix test を起動できている時点で PATH は必ず設定されており(シェル・CI いずれも PATH 経由で elixir を解決)、System.get_env("PATH") が nil になる実行環境は存在しません。仮に nil だった場合も after 節の System.put_env/2 が即座に例外を上げてテストが騒がしく失敗するだけで、静かな汚染は起きません。テスト専用コードにこの仮定のためのフォールバックを足すより、現状の単純さを優先します。
| Req の結果をエラー分類済みの `t:result/0` に写す(純関数)。 | ||
| """ | ||
| @spec classify_response({:ok, http_response()} | {:error, term()}) :: result() | ||
| def classify_response({:ok, %{status: status, body: body}}) when status in 200..299, |
There was a problem hiding this comment.
ℹ️ [LOW] classify_response/1 の2xxマッチ(status in 200..299)は Req.Response の status フィールドが整数であることを前提としています。一方、http_response 型定義は non_neg_integer() と広く定義されており、100番台や300番台のステータスが {:ok, body} として返される可能性があります。リダイレクト(3xx)は Req がデフォルトでフォローするため実害は少ないですが、型定義を 200..299 に絞るか、ガード節のコメントで意図を明示することを推奨します。
There was a problem hiding this comment.
据え置きます(前提の誤りを指摘)。status in 200..299 のガードは 2xx のみにマッチするため、1xx / 3xx が {:ok, body} になることはありません。両者は最後の節に落ちて {:error, {:http_error, status, message}} に分類されます(例: 304 → {:http_error, 304, "HTTP 304"})。また 3xx は Req が既定でリダイレクトを追跡するため、終端レスポンスとして現れるのは例外的です。http_response の non_neg_integer() は入力型(Req のレスポンス全般)の記述であり、分類結果を緩めるものではありません。
AI レビュー対応まとめ指摘 10 件(初回 5 + 再走 5)に対応しました。最新 commit 64e42eb で CI / AI レビューとも success です。 修正(64e42eb)
据え置き(各スレッドに根拠を返信)
|
1 participant
概要
registry-manager / thesis-monitor が共通で使える Req ベースの GitHub REST ラッパ
ToolKit.GitHub.Clientを追加します(K5)。orchestration 層と純パース層はツール側に残す方針(epic D2)のため持ち込んでいません。変更内容
lib/tool_kit/github/client.ex:token_provider(-> {:ok, token} | {:error, reason})。既定はgh auth token(gh_cli_token/0)。thesis-monitor は TokenManager をそのまま渡せる:base_url/:receive_timeout/:user_agent/:req_optionsを全関数共通オプションとして受け付けget/2post/3put/3patch/3request/3)get_file_text/3、decode_content/1、repo 情報、branches、commits(since/author/per_page)、pulls(一覧・reviews・requested_reviewers)、issue コメント、PR close、repo archive{:error, :not_found}、401/403 →{:error, :unauthorized}、その他 →{:http_error, status, message}/{:request_failed, reason}/{:token_error, reason}。述語not_found_error?/1/unauthorized_error?/1test/tool_kit/github/client_test.exsclassify_response/1/build_url/2/decode_content/1は純関数としてユニットテストmix.exs:plug(test only、Req.Test 用)を追加確認済み
mix format --check-formatted/mix credo --strict(0 issues)/mix dialyzer(0 errors)/mix test --cover全 greenResolves #5
親 epic: smkwlab/latex-ecosystem#146
https://claude.ai/code/session_015FdaeD1BANgCMUcNTUjKVV