diff --git a/app/config/test.exs b/app/config/test.exs index e880193..e1414e7 100644 --- a/app/config/test.exs +++ b/app/config/test.exs @@ -1,6 +1,6 @@ import Config config :ash, policies: [show_policy_breakdowns?: true] -config :linear_cli, req_options: [plug: {Req.Test, LinearCli.Api}] +config :linear_cli, req_options: [plug: {Req.Test, LinearCli.Api}, retry: false] # LinearCli.ObanRepo's connection details live in config/runtime.exs - see # that file and ObanRepo's moduledoc. Doesn't matter functionally today diff --git a/app/lib/linear_cli/api.ex b/app/lib/linear_cli/api.ex index e1f778e..33d00bd 100644 --- a/app/lib/linear_cli/api.ex +++ b/app/lib/linear_cli/api.ex @@ -8,6 +8,8 @@ defmodule LinearCli.Api do the Ruby `Issue#add_comment` path. """ + require Logger + @base_url "https://api.linear.app/graphql" @doc """ @@ -50,6 +52,21 @@ defmodule LinearCli.Api do # Guard: only match when data is a map (the expected shape). A null top-level # "data" means the entire operation failed; in that case the errors clause # below provides the more informative result. + # + # The more-specific partial-success clause (both keys present) runs first so + # the field-level errors are logged before the data is returned. The general + # data-only clause follows as a fallback. + defp handle_response( + {:ok, %Req.Response{status: 200, body: %{"data" => data, "errors" => [_ | _] = errors}}} + ) + when is_map(data) do + Logger.warning( + "Linear API partial-success: #{length(errors)} field error(s) discarded, data returned" + ) + + {:ok, data} + end + defp handle_response({:ok, %Req.Response{status: 200, body: %{"data" => data}}}) when is_map(data) do {:ok, data} diff --git a/app/lib/linear_cli/cli.ex b/app/lib/linear_cli/cli.ex index 536b3fc..c3b1b32 100644 --- a/app/lib/linear_cli/cli.ex +++ b/app/lib/linear_cli/cli.ex @@ -353,6 +353,63 @@ defmodule LinearCli.CLI do halt.(88) end + # LinearCli.Api.call/2's {:error, {:http_error, status, body}} for 401/403 - + # bad or expired API key. Give a targeted message instead of dumping the raw + # error structure. ManualRead else clauses normalize the 3-tuple to 2-tuple + # {:http_error, status} so Ash (via Splode) stores it in value: [{:http_error, status}]. + defp handle_error( + %Ash.Error.Unknown{errors: [%{value: [{:http_error, status}]} | _]}, + debug, + halt + ) + when status in [401, 403] do + IO.puts(:stderr, "Linear API authentication failed (HTTP #{status}).") + IO.puts(:stderr, "Check that LINEAR_API_KEY is valid.") + IO.puts(:stderr, "** Authentication error, cannot continue **") + maybe_print_backtrace(debug) + halt.(77) + end + + # LinearCli.Api.call/2's {:error, {:http_error, status, body}} for any other + # non-200 status (rate-limit 429, server errors 5xx, etc.). + defp handle_error( + %Ash.Error.Unknown{errors: [%{value: [{:http_error, status}]} | _]}, + debug, + halt + ) do + IO.puts(:stderr, "Linear API returned HTTP #{status}.") + IO.puts(:stderr, "** API Error, Cannot Continue **") + maybe_print_backtrace(debug) + halt.(88) + end + + # LinearCli.Api.call/2's {:error, {:transport_error, exception}} - DNS failure, + # timeout, connection refused, etc. Ash wraps as %{value: [{:transport_error, ...}]}. + defp handle_error( + %Ash.Error.Unknown{errors: [%{value: [{:transport_error, _exception}]} | _]}, + debug, + halt + ) do + IO.puts(:stderr, "Could not reach the Linear API.") + IO.puts(:stderr, "** Network error, cannot continue **") + maybe_print_backtrace(debug) + halt.(69) + end + + # LinearCli.Api.call/2's {:error, {:unexpected_response, body}} - a 200 with + # neither "data" nor "errors", or a caller that received an unexpected data shape. + # More specific than the catch-all so users get a targeted message. + defp handle_error( + %Ash.Error.Unknown{errors: [%{value: [{:unexpected_response, _body}]} | _]}, + debug, + halt + ) do + IO.puts(:stderr, "Linear API returned an unexpected response.") + IO.puts(:stderr, "** API Error, Cannot Continue **") + maybe_print_backtrace(debug) + halt.(88) + end + # Ported from CLI::Caller#call's catch-all `rescue StandardError` clause. defp handle_error(error, debug, halt) do IO.puts(:stderr, "What the heck is this? #{Exception.format_banner(:error, error)}") diff --git a/app/lib/linear_cli/linear/label.ex b/app/lib/linear_cli/linear/label.ex index 35397a4..030373f 100644 --- a/app/lib/linear_cli/linear/label.ex +++ b/app/lib/linear_cli/linear/label.ex @@ -63,9 +63,18 @@ defmodule LinearCli.Linear.Label.Read.ByNames do def read(query, _ecto_query, _opts, _context) do names = query.arguments.names - with {:ok, %{"issueLabels" => %{"edges" => edges}}} <- - Api.call(@document, %{"names" => names}) do - {:ok, Enum.map(edges, &Label.from_map(&1["node"]))} + case Api.call(@document, %{"names" => names}) do + {:ok, %{"issueLabels" => %{"edges" => edges}}} -> + {:ok, Enum.map(edges, &Label.from_map(&1["node"]))} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end end @@ -104,14 +113,23 @@ defmodule LinearCli.Linear.Label.Read.ByTeam do def read(query, _ecto_query, _opts, _context) do team_id = query.arguments.team_id - with {:ok, %{"team" => %{"labels" => %{"nodes" => nodes}}}} <- - Api.call(@document, %{"teamId" => team_id}) do - labels = - nodes - |> Enum.reject(&(&1["isGroup"] || &1["parent"])) - |> Enum.map(&Label.from_map/1) + case Api.call(@document, %{"teamId" => team_id}) do + {:ok, %{"team" => %{"labels" => %{"nodes" => nodes}}}} -> + labels = + nodes + |> Enum.reject(&(&1["isGroup"] || &1["parent"])) + |> Enum.map(&Label.from_map/1) + + {:ok, labels} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} - {:ok, labels} + {:error, reason} -> + {:error, reason} end end end diff --git a/app/lib/linear_cli/linear/paginate.ex b/app/lib/linear_cli/linear/paginate.ex index f8a1677..c369b68 100644 --- a/app/lib/linear_cli/linear/paginate.ex +++ b/app/lib/linear_cli/linear/paginate.ex @@ -23,8 +23,9 @@ defmodule LinearCli.Linear.Paginate do end defp do_all(document, field_name, variables_fun, decode_fun, after_cursor, max, acc) do - with {:ok, data} <- Api.call(document, variables_fun.(after_cursor)) do - %{"edges" => edges, "pageInfo" => page_info} = Map.fetch!(data, field_name) + with {:ok, data} <- Api.call(document, variables_fun.(after_cursor)), + {:ok, %{"edges" => edges, "pageInfo" => page_info}} <- + fetch_connection(data, field_name) do acc = acc ++ Enum.map(edges, &decode_fun.(&1["node"])) if length(acc) >= max or !page_info["hasNextPage"] do @@ -32,6 +33,25 @@ defmodule LinearCli.Linear.Paginate do else do_all(document, field_name, variables_fun, decode_fun, page_info["endCursor"], max, acc) end + else + {:error, {:http_error, status, _body}} -> {:error, {:http_error, status}} + {:error, reason} -> {:error, reason} + end + end + + # Safely extracts the named connection from the response data. Returns + # {:error, {:unexpected_response, ...}} instead of crashing with KeyError + # when the field is absent or not the expected connection shape. + defp fetch_connection(data, field_name) do + case data do + %{^field_name => %{"edges" => _, "pageInfo" => _} = connection} -> + {:ok, connection} + + %{^field_name => other} -> + {:error, {:unexpected_response, other}} + + _ -> + {:error, {:unexpected_response, data}} end end end diff --git a/app/lib/linear_cli/linear/project.ex b/app/lib/linear_cli/linear/project.ex index 47985e5..9e29393 100644 --- a/app/lib/linear_cli/linear/project.ex +++ b/app/lib/linear_cli/linear/project.ex @@ -231,23 +231,42 @@ defmodule LinearCli.Linear.Project.Read.ByTeam do do: %{"teamId" => team_id, "after" => after_cursor}, else: %{"teamId" => team_id} - with {:ok, %{"team" => %{"projects" => projects}}} <- Api.call(@document, variables) do - acc = acc ++ Enum.map(projects["nodes"] || [], &Project.from_map/1) + case Api.call(@document, variables) do + {:ok, %{"team" => %{"projects" => projects}}} -> + acc = acc ++ Enum.map(projects["nodes"] || [], &Project.from_map/1) - case projects["pageInfo"] do - %{"hasNextPage" => true, "endCursor" => cursor} when is_binary(cursor) -> - page(team_id, cursor, acc) + case projects["pageInfo"] do + %{"hasNextPage" => true, "endCursor" => cursor} when is_binary(cursor) -> + page(team_id, cursor, acc) - _ -> - {:ok, acc} - end + _ -> + {:ok, acc} + end + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end defp fetch(document, variables) do - with {:ok, %{"team" => %{"projects" => %{"nodes" => nodes}}}} <- - Api.call(document, variables) do - {:ok, Enum.map(nodes, &Project.from_map/1)} + case Api.call(document, variables) do + {:ok, %{"team" => %{"projects" => %{"nodes" => nodes}}}} -> + {:ok, Enum.map(nodes, &Project.from_map/1)} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end @@ -295,9 +314,18 @@ defmodule LinearCli.Linear.Project.Read.ByName do def read(query, _ecto_query, _opts, _context) do name = query.arguments.name - with {:ok, %{"projects" => %{"nodes" => nodes}}} <- - Api.call(document(), %{"name" => name}) do - {:ok, Enum.map(nodes, &Project.from_map/1)} + case Api.call(document(), %{"name" => name}) do + {:ok, %{"projects" => %{"nodes" => nodes}}} -> + {:ok, Enum.map(nodes, &Project.from_map/1)} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end diff --git a/app/lib/linear_cli/linear/team.ex b/app/lib/linear_cli/linear/team.ex index 91b7564..ec39049 100644 --- a/app/lib/linear_cli/linear/team.ex +++ b/app/lib/linear_cli/linear/team.ex @@ -94,11 +94,21 @@ defmodule LinearCli.Linear.Team.Read.Find do def read(query, _ecto_query, _opts, _context) do id = query.arguments.id - with {:ok, %{"team" => team_map}} <- Api.call(document(), %{"id" => id}) do - case team_map do - nil -> {:ok, []} - _ -> {:ok, [Team.from_map(team_map)]} - end + case Api.call(document(), %{"id" => id}) do + {:ok, %{"team" => nil}} -> + {:ok, []} + + {:ok, %{"team" => team_map}} -> + {:ok, [Team.from_map(team_map)]} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end diff --git a/app/lib/linear_cli/linear/user.ex b/app/lib/linear_cli/linear/user.ex index 6a6dc74..69b68bc 100644 --- a/app/lib/linear_cli/linear/user.ex +++ b/app/lib/linear_cli/linear/user.ex @@ -55,8 +55,18 @@ defmodule LinearCli.Linear.User.Read.Me do def read(_query, _ecto_query, _opts, _context) do document = "{ viewer { #{User.fields_with_teams()} } }" - with {:ok, %{"viewer" => viewer}} <- Api.call(document) do - {:ok, [User.from_map(viewer)]} + case Api.call(document) do + {:ok, %{"viewer" => viewer}} when is_map(viewer) -> + {:ok, [User.from_map(viewer)]} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end end diff --git a/app/lib/linear_cli/linear/workflow_state.ex b/app/lib/linear_cli/linear/workflow_state.ex index 9250215..0530608 100644 --- a/app/lib/linear_cli/linear/workflow_state.ex +++ b/app/lib/linear_cli/linear/workflow_state.ex @@ -59,9 +59,18 @@ defmodule LinearCli.Linear.WorkflowState.Read.ByTeam do def read(query, _ecto_query, _opts, _context) do team_id = query.arguments.team_id - with {:ok, %{"team" => %{"states" => %{"nodes" => nodes}}}} <- - Api.call(@document, %{"teamId" => team_id}) do - {:ok, Enum.map(nodes, &WorkflowState.from_map/1)} + case Api.call(@document, %{"teamId" => team_id}) do + {:ok, %{"team" => %{"states" => %{"nodes" => nodes}}}} -> + {:ok, Enum.map(nodes, &WorkflowState.from_map/1)} + + {:ok, other} -> + {:error, {:unexpected_response, other}} + + {:error, {:http_error, status, _body}} -> + {:error, {:http_error, status}} + + {:error, reason} -> + {:error, reason} end end end diff --git a/app/test/linear_cli/cli_test.exs b/app/test/linear_cli/cli_test.exs index 2ede2ef..f5ff571 100644 --- a/app/test/linear_cli/cli_test.exs +++ b/app/test/linear_cli/cli_test.exs @@ -332,16 +332,12 @@ defmodule LinearCli.CLITest do assert output =~ "Start or update development status of an issue" end - test "a catch-all error halts with exit code 88" do + test "an unexpected_response error halts with exit code 88 and a specific message" do # A malformed API response (neither "data" nor "errors") makes # LinearCli.Api return {:error, {:unexpected_response, body}}, which Ash - # wraps into a generic %Ash.Error.Unknown{} matching neither the - # not-found nor smells_bad handle_error/3 clauses - it should fall - # through to the catch-all. This module is async: true, so (unlike an - # env-var-based approach, which would race with every other - # concurrently-running test file that needs LINEAR_API_KEY present - - # exactly the class of bug this codebase already hit and fixed once) - # a stubbed response is the safe way to trigger this path. + # wraps and the explicit unexpected_response handle_error/3 clause catches. + # This module is async: true, so a stubbed response is the safe way to + # trigger this path (avoids races on LINEAR_API_KEY). Req.Test.stub(LinearCli.Api, fn conn -> Req.Test.json(conn, %{"wat" => true}) end) test_pid = self() @@ -353,8 +349,70 @@ defmodule LinearCli.CLITest do end) assert_received {:halted, 88} - assert output =~ "What the heck is this?" - assert output =~ "** WTH? Cannot Continue **" + assert output =~ "Linear API returned an unexpected response." + assert output =~ "** API Error, Cannot Continue **" + end + + test "an HTTP 401/403 error halts with exit code 77 and an auth-specific message" do + Req.Test.stub(LinearCli.Api, fn conn -> + conn |> Plug.Conn.put_status(401) |> Req.Test.json(%{"error" => "unauthorized"}) + end) + + test_pid = self() + halt = fn code -> send(test_pid, {:halted, code}) end + + output = capture_io(:stderr, fn -> LinearCli.CLI.main(["whoami"], halt) end) + + assert_received {:halted, 77} + assert output =~ "Linear API authentication failed (HTTP 401)" + assert output =~ "Check that LINEAR_API_KEY is valid." + end + + test "an HTTP 403 error halts with exit code 77" do + Req.Test.stub(LinearCli.Api, fn conn -> + conn |> Plug.Conn.put_status(403) |> Req.Test.json(%{"error" => "forbidden"}) + end) + + test_pid = self() + halt = fn code -> send(test_pid, {:halted, code}) end + + output = capture_io(:stderr, fn -> LinearCli.CLI.main(["whoami"], halt) end) + + assert_received {:halted, 77} + assert output =~ "Linear API authentication failed (HTTP 403)" + end + + test "a non-auth HTTP error halts with exit code 88 and includes the status code" do + Req.Test.stub(LinearCli.Api, fn conn -> + conn |> Plug.Conn.put_status(500) |> Req.Test.json(%{"error" => "internal"}) + end) + + test_pid = self() + halt = fn code -> send(test_pid, {:halted, code}) end + + output = capture_io(:stderr, fn -> LinearCli.CLI.main(["whoami"], halt) end) + + assert_received {:halted, 88} + assert output =~ "Linear API returned HTTP 500" + assert output =~ "** API Error, Cannot Continue **" + end + + test "a transport error halts with exit code 69 and a network-specific message" do + # Req.Test.transport_error/2 simulates a transport failure (connection + # refused, DNS failure, etc.) - Req wraps it as {:error, %Req.TransportError{}} + # which Api.call converts to {:error, {:transport_error, exception}}. + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.transport_error(conn, :econnrefused) + end) + + test_pid = self() + halt = fn code -> send(test_pid, {:halted, code}) end + + output = capture_io(:stderr, fn -> LinearCli.CLI.main(["whoami"], halt) end) + + assert_received {:halted, 69} + assert output =~ "Could not reach the Linear API." + assert output =~ "** Network error, cannot continue **" end test "a bare parent command (no leaf subcommand) shows that path's help and exits 1" do @@ -374,15 +432,26 @@ defmodule LinearCli.CLITest do end test "an unexpected raise (not a returned error) still degrades to exit 88, not a raw crash" do - # A malformed API response (no "viewer" key at all) makes the manual - # read return {:ok, %{}} instead of {:ok, [records]}, which Ash's own - # manual-action-return validation *raises* on - a genuine exception, not - # a {:error, reason} tuple. run/3's handle_error/3 only ever sees - # returned values; this proves the main/1-level rescue (Ruby's - # Caller#call had a blanket `rescue StandardError` - ours previously - # only caught returned errors, not actual crashes) catches real bugs - # too, not just this one known case. - Req.Test.stub(LinearCli.Api, fn conn -> Req.Test.json(conn, %{"data" => %{}}) end) + # A response that passes User.Read.Me's shape checks (viewer is a map) + # but contains a nil teams node causes Team.from_map(nil) to raise a + # Protocol.UndefinedError inside User.from_map - a genuine exception, not + # a {:error, reason} tuple. This proves the main/1-level rescue catches + # real code bugs too, not just the known {:error, reason} paths. + # "nodes" is a string, not a list — Enum.map/2 raises Protocol.UndefinedError + # because String doesn't implement Enumerable. (nil nodes don't raise: nil["id"] + # returns nil in Elixir since Atom implements Access.) + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.json(conn, %{ + "data" => %{ + "viewer" => %{ + "id" => "u1", + "name" => "Ada", + "email" => "ada@example.com", + "teams" => %{"nodes" => "not_a_list"} + } + } + }) + end) test_pid = self() halt = fn code -> send(test_pid, {:halted, code}) end diff --git a/app/test/linear_cli/linear/issue_test.exs b/app/test/linear_cli/linear/issue_test.exs index 075d888..f51fb3d 100644 --- a/app/test/linear_cli/linear/issue_test.exs +++ b/app/test/linear_cli/linear/issue_test.exs @@ -119,6 +119,18 @@ defmodule LinearCli.Linear.IssueTest do assert issue.state.name == "Done" end + test "issues/0 returns an unexpected_response error when the connection field is absent" do + # Paginate.all/5 used to crash with KeyError when the expected "issues" + # field was missing from the API response. fetch_connection/2 now converts + # that to {:error, {:unexpected_response, ...}} instead. + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.json(conn, %{"data" => %{"something_else" => %{}}}) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:unexpected_response, _}]}]}} = + Linear.issues() + end + test "issues/1 with an unknown id returns a not_found error" do Req.Test.stub(LinearCli.Api, fn conn -> Req.Test.json(conn, %{"data" => %{"issue" => nil}}) diff --git a/app/test/linear_cli/linear/user_test.exs b/app/test/linear_cli/linear/user_test.exs index 41a174b..539be77 100644 --- a/app/test/linear_cli/linear/user_test.exs +++ b/app/test/linear_cli/linear/user_test.exs @@ -55,6 +55,27 @@ defmodule LinearCli.Linear.UserTest do assert {:error, %Ash.Error.Unknown{}} = Linear.team_members("t1") end + test "me/0 returns an unexpected_response error when viewer is null" do + # Linear returns {"data": {"viewer": null}} when the API key is valid but + # refers to an account that no longer exists. Guard `when is_map(viewer)` + # prevents User.from_map(nil) from crashing. + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.json(conn, %{"data" => %{"viewer" => nil}}) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:unexpected_response, _}]}]}} = + Linear.me() + end + + test "me/0 returns an unexpected_response error when viewer key is absent" do + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.json(conn, %{"data" => %{}}) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:unexpected_response, _}]}]}} = + Linear.me() + end + test "me/0 decodes the viewer, including nested teams" do Req.Test.stub(LinearCli.Api, fn conn -> Req.Test.json(conn, %{