diff --git a/app/lib/linear_cli/linear/paginate.ex b/app/lib/linear_cli/linear/paginate.ex index de0c647..69e45c2 100644 --- a/app/lib/linear_cli/linear/paginate.ex +++ b/app/lib/linear_cli/linear/paginate.ex @@ -14,12 +14,18 @@ defmodule LinearCli.Linear.Paginate do `max` records are collected or the API reports no more pages, decoding each raw node through `decode_fun`. - `field_name` is the top-level response key (e.g. `"teams"`) holding - `edges`/`pageInfo`. `variables_fun` receives the current `after` cursor - (`nil` on the first page) and returns the GraphQL variables map. + `field_name` is the response key holding `edges`/`pageInfo`. Pass a list of + keys for a nested connection, such as `["team", "members"]`. + `variables_fun` receives the current `after` cursor (`nil` on the first page) + and returns the GraphQL variables map. """ def all(document, field_name, variables_fun, decode_fun, max \\ 100) do - do_all(document, field_name, variables_fun, decode_fun, nil, max, []) + do_all(document, field_name, variables_fun, decode_fun, %{ + after_cursor: nil, + max: max, + acc: [], + seen_cursors: :bounded + }) end @doc """ @@ -27,9 +33,22 @@ defmodule LinearCli.Linear.Paginate do This variant has no record limit. Use it only for lookup candidate sets that must be complete before matching, rather than for bounded issue operations. + It rejects a null continuation cursor and every continuation cursor that it + has already used. """ def all_pages(document, field_name, variables_fun, decode_fun) do - do_all(document, field_name, variables_fun, decode_fun, nil, :unbounded, []) + do_all( + document, + field_name, + variables_fun, + decode_fun, + %{ + after_cursor: nil, + max: :unbounded, + acc: [], + seen_cursors: MapSet.new([nil]) + } + ) end @doc """ @@ -51,22 +70,17 @@ defmodule LinearCli.Linear.Paginate do end 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)), + defp do_all(document, field_name, variables_fun, decode_fun, state) do + with {:ok, data} <- Api.call(document, variables_fun.(state.after_cursor)), {:ok, %{"edges" => edges, "pageInfo" => page_info}} <- fetch_connection(data, field_name) do - acc = acc ++ Enum.map(edges, &decode_fun.(&1["node"])) + state = %{state | acc: state.acc ++ Enum.map(edges, &decode_fun.(&1["node"]))} - if reached_limit?(acc, max) or !page_info["hasNextPage"] do - {:ok, take_max(acc, max)} + if reached_limit?(state.acc, state.max) or !page_info["hasNextPage"] do + {:ok, take_max(state.acc, state.max)} else next_cursor = page_info["endCursor"] - - if next_cursor == after_cursor do - {:error, {:non_advancing_cursor, next_cursor}} - else - do_all(document, field_name, variables_fun, decode_fun, next_cursor, max, acc) - end + continue(document, field_name, variables_fun, decode_fun, state, next_cursor) end else {:error, {:http_error, status, _body}} -> {:error, {:http_error, status}} @@ -74,6 +88,53 @@ defmodule LinearCli.Linear.Paginate do end end + defp continue( + document, + field_name, + variables_fun, + decode_fun, + %{after_cursor: after_cursor, seen_cursors: :bounded} = state, + next_cursor + ) do + if next_cursor == after_cursor do + {:error, {:non_advancing_cursor, next_cursor}} + else + do_all(document, field_name, variables_fun, decode_fun, %{state | after_cursor: next_cursor}) + end + end + + defp continue( + _document, + _field_name, + _variables_fun, + _decode_fun, + %{seen_cursors: _}, + nil + ) do + {:error, {:non_advancing_cursor, nil}} + end + + defp continue( + document, + field_name, + variables_fun, + decode_fun, + %{seen_cursors: seen_cursors} = state, + next_cursor + ) do + if MapSet.member?(seen_cursors, next_cursor) do + {:error, {:non_advancing_cursor, next_cursor}} + else + state = %{ + state + | after_cursor: next_cursor, + seen_cursors: MapSet.put(seen_cursors, next_cursor) + } + + do_all(document, field_name, variables_fun, decode_fun, state) + end + end + defp reached_limit?(_acc, :unbounded), do: false defp reached_limit?(acc, max), do: length(acc) >= max @@ -83,7 +144,11 @@ defmodule LinearCli.Linear.Paginate do # 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 + defp fetch_connection(data, field_name) when is_binary(field_name) do + fetch_connection(data, [field_name]) + end + + defp fetch_connection(data, [field_name]) do case data do %{^field_name => %{"edges" => _, "pageInfo" => _} = connection} -> {:ok, connection} @@ -95,4 +160,11 @@ defmodule LinearCli.Linear.Paginate do {:error, {:unexpected_response, data}} end end + + defp fetch_connection(data, [field_name | rest]) do + case data do + %{^field_name => nested} -> fetch_connection(nested, rest) + _ -> {:error, {:unexpected_response, data}} + end + end end diff --git a/app/lib/linear_cli/linear/user.ex b/app/lib/linear_cli/linear/user.ex index c1d797c..df30509 100644 --- a/app/lib/linear_cli/linear/user.ex +++ b/app/lib/linear_cli/linear/user.ex @@ -82,18 +82,11 @@ defmodule LinearCli.Linear.User.Read.ByTeam do @moduledoc false use Ash.Resource.ManualRead - def read(query, ecto_query, opts, context) do - LinearCli.Linear.User.Read.ByTeamForLookup.read(query, ecto_query, opts, context) - end -end - -defmodule LinearCli.Linear.User.Read.ByTeamForLookup do - @moduledoc false - use Ash.Resource.ManualRead - alias LinearCli.Api alias LinearCli.Linear.User + # Keep the team-scoped action separate from the strict workspace lookup path. + # EXT-75 owns this action's response behavior. @document """ query($id: String!, $after: String) { team(id: $id) { @@ -146,3 +139,33 @@ defmodule LinearCli.Linear.User.Read.ByTeamForLookup do end end end + +defmodule LinearCli.Linear.User.Read.ByTeamForLookup do + @moduledoc false + use Ash.Resource.ManualRead + + alias LinearCli.Linear.Paginate + alias LinearCli.Linear.User + + @document """ + query($id: String!, $after: String) { + team(id: $id) { + members(first: 50, after: $after) { + edges { node { #{User.base_fields()} } cursor } + pageInfo { hasNextPage endCursor } + } + } + } + """ + + def read(query, _ecto_query, _opts, _context) do + team_id = query.arguments.team_id + + Paginate.all_pages( + @document, + ["team", "members"], + fn after_cursor -> %{"id" => team_id, "after" => after_cursor} end, + &User.from_map/1 + ) + end +end diff --git a/app/test/linear_cli/linear/paginate_test.exs b/app/test/linear_cli/linear/paginate_test.exs index 4b906ac..4e2b36e 100644 --- a/app/test/linear_cli/linear/paginate_test.exs +++ b/app/test/linear_cli/linear/paginate_test.exs @@ -16,6 +16,19 @@ defmodule LinearCli.Linear.PaginateTest do defp variables_fun(after_cursor), do: %{"after" => after_cursor} + defp nested_response(ids, has_next_page, end_cursor) do + %{ + "data" => %{ + "team" => %{ + "members" => %{ + "edges" => Enum.map(ids, &%{"node" => %{"id" => &1}, "cursor" => "row-#{&1}"}), + "pageInfo" => %{"hasNextPage" => has_next_page, "endCursor" => end_cursor} + } + } + } + } + end + test "uses the default 100-record limit" do test_pid = self() @@ -82,6 +95,109 @@ defmodule LinearCli.Linear.PaginateTest do Paginate.all_pages("query", "issues", &variables_fun/1, & &1["id"]) end + test "all_pages rejects a cursor that appeared on an earlier page" do + test_pid = self() + + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + send(test_pid, {:cursor, cursor}) + + response = + case cursor do + nil -> response([1], true, "c1") + "c1" -> response([2], true, "c2") + "c2" -> response([3], true, "c1") + end + + Req.Test.json(conn, response) + end) + + assert {:error, {:non_advancing_cursor, "c1"}} = + Paginate.all_pages("query", "issues", &variables_fun/1, & &1["id"]) + + assert_receive {:cursor, nil} + assert_receive {:cursor, "c1"} + assert_receive {:cursor, "c2"} + refute_receive {:cursor, "c1"} + end + + test "all_pages rejects a null continuation cursor" do + test_pid = self() + + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + send(test_pid, {:cursor, cursor}) + + response = + case cursor do + nil -> response([1], true, "c1") + "c1" -> response([2], true, nil) + end + + Req.Test.json(conn, response) + end) + + assert {:error, {:non_advancing_cursor, nil}} = + Paginate.all_pages("query", "issues", &variables_fun/1, & &1["id"]) + + assert_receive {:cursor, nil} + assert_receive {:cursor, "c1"} + refute_receive {:cursor, nil} + end + + test "all_pages reads a nested connection path" do + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + + response = + case cursor do + nil -> nested_response([1], true, "c1") + "c1" -> nested_response([2], false, "c2") + end + + Req.Test.json(conn, response) + end) + + assert {:ok, [1, 2]} = + Paginate.all_pages( + "query", + ["team", "members"], + &variables_fun/1, + & &1["id"] + ) + end + + test "bounded all preserves its limit when cursors cycle" do + test_pid = self() + + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + send(test_pid, {:cursor, cursor}) + + response = + case cursor do + nil -> response(1..20, true, "c1") + "c1" -> response(21..40, true, "c2") + "c2" -> response(41..60, true, "c1") + end + + Req.Test.json(conn, response) + end) + + assert {:ok, values} = Paginate.all("query", "issues", &variables_fun/1, & &1["id"], 100) + assert length(values) == 100 + assert_receive {:cursor, nil} + assert_receive {:cursor, "c1"} + assert_receive {:cursor, "c2"} + assert_receive {:cursor, "c1"} + assert_receive {:cursor, "c2"} + refute_receive {:cursor, "c1"} + end + test "returns the first page and reports more records without following the cursor" do test_pid = self() diff --git a/app/test/linear_cli/linear/user_test.exs b/app/test/linear_cli/linear/user_test.exs index 2846fc3..0bf6d04 100644 --- a/app/test/linear_cli/linear/user_test.exs +++ b/app/test/linear_cli/linear/user_test.exs @@ -155,6 +155,87 @@ defmodule LinearCli.Linear.UserTest do Linear.workspace_team_members("t1") end + test "workspace_team_members/1 reports a malformed first page" do + Req.Test.stub(LinearCli.Api, fn conn -> + Req.Test.json(conn, %{"data" => %{"team" => %{"members" => %{}}}}) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:unexpected_response, _}]}]}} = + Linear.workspace_team_members("t1") + end + + test "workspace_team_members/1 rejects a cursor that appeared on an earlier page" do + test_pid = self() + + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + send(test_pid, {:cursor, cursor}) + + members = [%{"id" => "u1", "name" => "Member", "email" => "member@example.com"}] + + page_info = + case cursor do + nil -> %{"hasNextPage" => true, "endCursor" => "member-1"} + "member-1" -> %{"hasNextPage" => true, "endCursor" => "member-2"} + "member-2" -> %{"hasNextPage" => true, "endCursor" => "member-1"} + end + + Req.Test.json(conn, %{ + "data" => %{ + "team" => %{ + "members" => %{ + "edges" => Enum.map(members, &%{"node" => &1, "cursor" => &1["id"]}), + "pageInfo" => page_info + } + } + } + }) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:non_advancing_cursor, "member-1"}]}]}} = + Linear.workspace_team_members("t1") + + assert_receive {:cursor, nil} + assert_receive {:cursor, "member-1"} + assert_receive {:cursor, "member-2"} + refute_receive {:cursor, "member-1"} + end + + test "workspace_team_members/1 rejects a null continuation cursor" do + test_pid = self() + + Req.Test.stub(LinearCli.Api, fn conn -> + {:ok, body, conn} = Plug.Conn.read_body(conn) + cursor = Jason.decode!(body)["variables"]["after"] + send(test_pid, {:cursor, cursor}) + + page_info = + case cursor do + nil -> %{"hasNextPage" => true, "endCursor" => "member-1"} + "member-1" -> %{"hasNextPage" => true, "endCursor" => nil} + end + + Req.Test.json(conn, %{ + "data" => %{ + "team" => %{ + "members" => %{ + "edges" => [%{"node" => %{"id" => "u1"}, "cursor" => "member-1"}], + "pageInfo" => page_info + } + } + } + }) + end) + + assert {:error, %Ash.Error.Unknown{errors: [%{value: [{:non_advancing_cursor, nil}]}]}} = + Linear.workspace_team_members("t1") + + assert_receive {:cursor, nil} + assert_receive {:cursor, "member-1"} + refute_receive {:cursor, nil} + end + test "team_members/1 propagates API errors" do Req.Test.stub(LinearCli.Api, fn conn -> Req.Test.json(conn, %{"errors" => [%{"message" => "Unauthorized"}]}) diff --git a/documents/agent_learnings.adoc b/documents/agent_learnings.adoc new file mode 100644 index 0000000..4c29a4d --- /dev/null +++ b/documents/agent_learnings.adoc @@ -0,0 +1,8 @@ += Active agent learnings +:generated: true + +// GENERATED from documents/agent_learnings/*.adoc. Do not edit by hand. +// The aggregate contains at most 20 active, actionable learnings. See +// documents/agent_learnings/README.adoc for the entry and retirement contract. + +include::agent_learnings/EXT-76_learned.adoc[] diff --git a/documents/agent_learnings/EXT-76_learned.adoc b/documents/agent_learnings/EXT-76_learned.adoc new file mode 100644 index 0000000..2bf1484 --- /dev/null +++ b/documents/agent_learnings/EXT-76_learned.adoc @@ -0,0 +1,30 @@ +== EXT-76 — Pagination termination invariant +:status: active +:risk: high +:owner: EXT-76 +:evidence-date: 2026-10-04 + +=== Failure + +Removing the 100-record lookup cap left cursor loops with no complete +termination invariant. A repeated cursor or a null continuation cursor caused +the lookup to request pages forever. + +=== Rule + +When a pagination cap is removed, add a termination invariant in the same +change. The invariant must reject every repeated continuation cursor and every +null continuation cursor that claims another page exists. + +=== Guardrail + +`Paginate.all_pages/4` records every cursor that it uses. It rejects a repeated +or null continuation cursor. Workspace member lookup uses this paginator for +the nested `team.members` connection. Bounded `Paginate.all/5` keeps its record +limit and behavior. + +=== Retirement + +Retire this entry when the paginator guard and its cycle regressions remain in +the supported implementation, and no related lookup path uses a separate +unbounded pagination loop. diff --git a/documents/agent_learnings/README.adoc b/documents/agent_learnings/README.adoc new file mode 100644 index 0000000..077ea12 --- /dev/null +++ b/documents/agent_learnings/README.adoc @@ -0,0 +1,44 @@ += Agent learning entries + +This directory stores one source document for each active, actionable learning +from an agent run. An implementation follow-up creates the entry for its owner +issue. + +== Naming and ownership + +Name each entry `EXT-_learned.adoc`. Use the owning Linear issue ID. +The same change regenerates `../agent_learnings.adoc` from the active entries. +Do not add entries to the aggregate without an owner issue. + +== Entry contents + +Each entry is a short technical-debt record. Include: + +* the repeatable fault or missing safeguard; +* the evidence and workflow where it occurred; +* the guardrail or the remaining remediation; +* the owner issue and its status; and +* a condition for retirement. + +Do not create an entry when there is no recurring fault, concrete guardrail, or +owner issue. + +== Bounded aggregate + +`../agent_learnings.adoc` contains active operational learnings only. Keep at +most 20 entries. Sort entries by risk first, then by recency. + +Remove a resolved entry from the aggregate. Keep its source file for issue and +pull-request history. Retire, merge, or assign a follow-up to an entry that the +20-entry limit displaces. + +== Regeneration process + +. Read every `*_learned.adoc` file in this directory. +. Keep only entries with an active status. +. Sort active entries by risk and then by the newest evidence date. +. Write the aggregate header and one `include::agent_learnings/[]` line per entry. +. Verify that the aggregate has no more than 20 entries. + +The aggregate is generated output. Edit the source entry or this contract when +the content or the process changes. diff --git a/documents/ash-domain-erd.adoc b/documents/ash-domain-erd.adoc index c0aa87c..ee60f2a 100644 --- a/documents/ash-domain-erd.adoc +++ b/documents/ash-domain-erd.adoc @@ -312,7 +312,7 @@ manual-implementation module, and the Linear GraphQL operation it calls. | `:by_team_for_lookup` | read | `Linear.User.Read.ByTeamForLookup` -| `team(id: $id) { members(first: 50, after: $after) { ... } }` — follows every page for workspace-wide matching +| `team(id: $id) { members(first: 50, after: $after) { ... } }` — follows every page through `Paginate` for workspace-wide matching | `Team` | `teams` @@ -596,7 +596,10 @@ Provides `all/5`: fetches cursor-paginated GraphQL connections until `max` records are collected or the API signals no more pages. It returns an error for a repeated continuation cursor instead of looping forever. `all_pages/4` follows every page without a record limit. Lookup-only actions use -this variant. The bounded `all/5` path remains unchanged for issue operations. +this variant. It accepts a top-level field name or a nested response path. It +tracks every continuation cursor. It returns `{:non_advancing_cursor, cursor}` +when a continuation cursor is null or repeats. The bounded `all/5` path remains +unchanged for issue operations. `first_page/4` fetches one page, returns its `hasNextPage` value, and never follows a continuation cursor. Used by: @@ -610,8 +613,9 @@ Used by: * `Issue.Actions.ListFirstPage` — wraps `Issue.Read.List.first_page/1` in an Ash generic action for the filter-only unassign path -`User.Read.ByTeamForLookup` follows the nested member connection with the same -cursor advancement guard. It returns every member for one team before the CLI +`User.Read.ByTeamForLookup` passes the nested `team.members` path to +`Paginate.all_pages/4`. The shared paginator validates the connection and its +cursor sequence. The action returns every member for one team before the CLI deduplicates the workspace-wide candidate set. == Maintenance contract