fix(cli): pagination cursor lookup recurses into well-known wrapper objects (#157 F3) (#453)

a197d4146d53ffb17db372b2bc1cea161fc9a9d8 · 2026-04-30 17:44:46 -0700 · Trevin Chow

* fix(cli): pagination cursor lookup recurses into well-known wrapper objects

Closes the last open finding from retro #157 (Slack — F3, nested
cursor objects not handled in pagination).

Slack-style pagination puts the cursor inside response_metadata:

  {
    "ok": true,
    "messages": [...],
    "response_metadata": {"next_cursor": "abc123"}
  }

extractPaginationFromEnvelope only checked top-level keys, so paginated
syncs against Slack/MongoDB-Atlas/etc. terminated after the first page
even though more data was available. The OLD path didn't fail loudly —
nextCursor stayed empty, hasMore stayed false, sync silently stopped.

Fix: when no top-level cursor is found, look one level deeper into
well-known wrapper objects (response_metadata, pagination, meta,
paging) using the same cursorKeys set. Purely additive — only runs
when the existing path returned empty, so APIs with top-level cursors
(Stripe, GitHub, Linear, Notion) hit the fast path and short-circuit.

Cross-API confidence: this is an industry-standard envelope pattern.
Slack uses response_metadata, MongoDB Atlas uses pagination, several
REST APIs use meta or paging. The fix actively widens compatibility
without changing behavior for APIs that already worked.

Test coverage: TestSyncExtractPaginationNestedCursor combines
generator-level emission asserts (paginationWrapperKeys ships,
includes response_metadata + pagination) with a generated-tree
behavioral test that exercises extractPageItems against four cases —
Slack envelope (positive), MongoDB envelope (positive), top-level
cursor wins over wrapper (negative), no cursor anywhere (negative).
The behavioral test is written into internal/cli as a same-package
_test.go so it can call the unexported helper.

Status of retro #157 after this PR:
- WU-1 F2 (smart wrapper-key extraction): done by intervening work
- WU-1 F3 (nested cursor): done HERE
- WU-2 (parent-child dependent sync): done by intervening work
- WU-3 (SQLite nested-query safety): done by intervening work
- WU-4 (resource priority for large specs): effectively done — cap
  raised 50→500
- WU-5 (Swagger 2.0 auth scoring): done by intervening work
All 5 WUs resolved.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(cli): apply /simplify findings to nested-cursor PR

Findings from the three-agent /simplify pass on PR #453.

Reuse must-fix:
- Switch runGoCommand → runGoCommandRequired for the tier-2 behavioral
  test invocation. The standard runGoCommand short-circuits on
  testing.Short() for build/test args, which would silently skip the
  whole behavioral tier under `go test -short`. Matches the precedent
  in mcp_novel_features_test.go:144.

Reuse + Quality nice-to-have applied:
- Extract findCursorInMap(m, cursorKeys) helper in sync.go.tmpl. The
  top-level scan and the wrapper-key recursion both did the same
  range-cursorKeys + json.Unmarshal-to-string + accept-non-empty
  loop. The doc comment already promised "uses the same cursorKeys
  set" — making it a helper enforces that mechanically rather than
  by convention. Two callers collapse to one-liners; nesting drops a
  level. Future cursor-key additions only need one update site.

Efficiency nice-to-have applied:
- Add TestExtractPageItemsTopLevelCursorOnly. The existing four cases
  (Slack envelope, MongoDB envelope, top-level wins over wrapper,
  no cursor) didn't isolate the no-wrapper fast path — TopLevelWins
  pairs both forms. The new test proves the common case (Stripe,
  GitHub, Linear, Notion) still works with no wrapper present at all.

Quality nice-to-have applied:
- Trim TestSyncExtractPaginationNestedCursor function comment from 13
  lines to 5. The two-tier rationale (grep + behavioral test) is now
  one sentence; the WHY is the cobratree wrapper-key contract, not
  the test architecture.

Skipped:
- Heredoc-as-Go-source for the inline _test.go (could be a testdata
  fixture). Stylistic preference; agent itself called it acceptable.
- Promote paginationWrapperKeys to package-level var. False positive,
  matches the existing cursorKeys/hasMoreKeys pattern.
- Double-nested cursor support (data.meta.next_cursor JSON:API style).
  YAGNI — no concrete request for this shape today.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Files touched

M internal/generator/generator_test.go
M internal/generator/templates/sync.go.tmpl

Diff

commit a197d4146d53ffb17db372b2bc1cea161fc9a9d8
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Thu Apr 30 17:44:46 2026 -0700

    fix(cli): pagination cursor lookup recurses into well-known wrapper objects (#157 F3) (#453)
    
    * fix(cli): pagination cursor lookup recurses into well-known wrapper objects
    
    Closes the last open finding from retro #157 (Slack — F3, nested
    cursor objects not handled in pagination).
    
    Slack-style pagination puts the cursor inside response_metadata:
    
      {
        "ok": true,
        "messages": [...],
        "response_metadata": {"next_cursor": "abc123"}
      }
    
    extractPaginationFromEnvelope only checked top-level keys, so paginated
    syncs against Slack/MongoDB-Atlas/etc. terminated after the first page
    even though more data was available. The OLD path didn't fail loudly —
    nextCursor stayed empty, hasMore stayed false, sync silently stopped.
    
    Fix: when no top-level cursor is found, look one level deeper into
    well-known wrapper objects (response_metadata, pagination, meta,
    paging) using the same cursorKeys set. Purely additive — only runs
    when the existing path returned empty, so APIs with top-level cursors
    (Stripe, GitHub, Linear, Notion) hit the fast path and short-circuit.
    
    Cross-API confidence: this is an industry-standard envelope pattern.
    Slack uses response_metadata, MongoDB Atlas uses pagination, several
    REST APIs use meta or paging. The fix actively widens compatibility
    without changing behavior for APIs that already worked.
    
    Test coverage: TestSyncExtractPaginationNestedCursor combines
    generator-level emission asserts (paginationWrapperKeys ships,
    includes response_metadata + pagination) with a generated-tree
    behavioral test that exercises extractPageItems against four cases —
    Slack envelope (positive), MongoDB envelope (positive), top-level
    cursor wins over wrapper (negative), no cursor anywhere (negative).
    The behavioral test is written into internal/cli as a same-package
    _test.go so it can call the unexported helper.
    
    Status of retro #157 after this PR:
    - WU-1 F2 (smart wrapper-key extraction): done by intervening work
    - WU-1 F3 (nested cursor): done HERE
    - WU-2 (parent-child dependent sync): done by intervening work
    - WU-3 (SQLite nested-query safety): done by intervening work
    - WU-4 (resource priority for large specs): effectively done — cap
      raised 50→500
    - WU-5 (Swagger 2.0 auth scoring): done by intervening work
    All 5 WUs resolved.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    * refactor(cli): apply /simplify findings to nested-cursor PR
    
    Findings from the three-agent /simplify pass on PR #453.
    
    Reuse must-fix:
    - Switch runGoCommand → runGoCommandRequired for the tier-2 behavioral
      test invocation. The standard runGoCommand short-circuits on
      testing.Short() for build/test args, which would silently skip the
      whole behavioral tier under `go test -short`. Matches the precedent
      in mcp_novel_features_test.go:144.
    
    Reuse + Quality nice-to-have applied:
    - Extract findCursorInMap(m, cursorKeys) helper in sync.go.tmpl. The
      top-level scan and the wrapper-key recursion both did the same
      range-cursorKeys + json.Unmarshal-to-string + accept-non-empty
      loop. The doc comment already promised "uses the same cursorKeys
      set" — making it a helper enforces that mechanically rather than
      by convention. Two callers collapse to one-liners; nesting drops a
      level. Future cursor-key additions only need one update site.
    
    Efficiency nice-to-have applied:
    - Add TestExtractPageItemsTopLevelCursorOnly. The existing four cases
      (Slack envelope, MongoDB envelope, top-level wins over wrapper,
      no cursor) didn't isolate the no-wrapper fast path — TopLevelWins
      pairs both forms. The new test proves the common case (Stripe,
      GitHub, Linear, Notion) still works with no wrapper present at all.
    
    Quality nice-to-have applied:
    - Trim TestSyncExtractPaginationNestedCursor function comment from 13
      lines to 5. The two-tier rationale (grep + behavioral test) is now
      one sentence; the WHY is the cobratree wrapper-key contract, not
      the test architecture.
    
    Skipped:
    - Heredoc-as-Go-source for the inline _test.go (could be a testdata
      fixture). Stylistic preference; agent itself called it acceptable.
    - Promote paginationWrapperKeys to package-level var. False positive,
      matches the existing cursorKeys/hasMoreKeys pattern.
    - Double-nested cursor support (data.meta.next_cursor JSON:API style).
      YAGNI — no concrete request for this shape today.
    
    Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
    
    ---------
    
    Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 internal/generator/generator_test.go      | 155 ++++++++++++++++++++++++++++++
 internal/generator/templates/sync.go.tmpl |  45 +++++++--
 2 files changed, 194 insertions(+), 6 deletions(-)

diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 3394365c..e49575d2 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -1499,6 +1499,161 @@ func TestGenerateStoreWithBatchResourceDoesNotDuplicateUpsertBatch(t *testing.T)
 // generates a spec with a typed table, then runs the generated store
 // tests — the emitted TestUpsertBatch_Populates*Table tests fail if the
 // dispatch ever regresses.
+// TestSyncExtractPaginationNestedCursor verifies the emitted
+// extractPaginationFromEnvelope finds cursors inside well-known wrapper
+// objects (Slack response_metadata, etc.). Combines a generator-level
+// emission canary with a behavioral test written into the generated
+// tree, since either tier alone misses real regressions.
+func TestSyncExtractPaginationNestedCursor(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := &spec.APISpec{
+		Name:    "slacky",
+		Version: "0.1.0",
+		BaseURL: "https://slacky.example.com",
+		Auth:    spec.AuthConfig{Type: "none"},
+		Config: spec.ConfigSpec{
+			Format: "toml",
+			Path:   "~/.config/slacky-pp-cli/config.toml",
+		},
+		Resources: map[string]spec.Resource{
+			"messages": {
+				Description: "Messages",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {
+						Method:      "GET",
+						Path:        "/messages",
+						Description: "List messages",
+						Response:    spec.ResponseDef{Type: "array"},
+						Params: []spec.Param{
+							{Name: "channel", Type: "string"},
+							{Name: "cursor", Type: "string"},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{Store: true, Sync: true}
+	require.NoError(t, gen.Generate())
+
+	syncSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "sync.go"))
+	require.NoError(t, err)
+	src := string(syncSrc)
+
+	// Tier 1: emission canary. The wrapper-key recursion must ship.
+	assert.Contains(t, src, "paginationWrapperKeys",
+		"sync.go must declare paginationWrapperKeys for nested-cursor support")
+	assert.Contains(t, src, `"response_metadata"`,
+		"paginationWrapperKeys must include response_metadata (Slack's envelope)")
+	assert.Contains(t, src, `"pagination"`,
+		"paginationWrapperKeys must include pagination (MongoDB Atlas-style)")
+
+	// Tier 2: behavioral test, written into the generated tree as a
+	// same-package _test.go so it can call the unexported helper.
+	const inlineTest = `package cli
+
+import (
+	"encoding/json"
+	"testing"
+)
+
+func TestExtractPageItemsSlackEnvelope(t *testing.T) {
+	body := []byte(` + "`" + `{
+		"ok": true,
+		"messages": [{"text":"hello"},{"text":"world"}],
+		"response_metadata": {"next_cursor": "abc123"}
+	}` + "`" + `)
+	items, cursor, hasMore := extractPageItems(json.RawMessage(body), "cursor")
+	if len(items) != 2 {
+		t.Fatalf("want 2 items, got %d", len(items))
+	}
+	if cursor != "abc123" {
+		t.Fatalf("want cursor abc123, got %q", cursor)
+	}
+	if !hasMore {
+		t.Fatalf("want hasMore=true when cursor is present")
+	}
+}
+
+func TestExtractPageItemsMongoPaginationEnvelope(t *testing.T) {
+	body := []byte(` + "`" + `{
+		"results": [{"_id":"a"},{"_id":"b"}],
+		"pagination": {"next_page_token": "tok-2"}
+	}` + "`" + `)
+	items, cursor, hasMore := extractPageItems(json.RawMessage(body), "cursor")
+	if len(items) != 2 {
+		t.Fatalf("want 2 items, got %d", len(items))
+	}
+	if cursor != "tok-2" {
+		t.Fatalf("want cursor tok-2, got %q", cursor)
+	}
+	if !hasMore {
+		t.Fatalf("want hasMore=true when nested cursor is present")
+	}
+}
+
+// Fast path: top-level cursor with no wrapper present at all. Proves
+// the common case (Stripe, GitHub, Linear, Notion) doesn't enter the
+// wrapper recursion. Pairs with TopLevelCursorWinsOverWrapper which
+// pairs both forms; this isolates the no-wrapper shape.
+func TestExtractPageItemsTopLevelCursorOnly(t *testing.T) {
+	body := []byte(` + "`" + `{
+		"items": [{"id":1},{"id":2}],
+		"next_cursor": "page-2"
+	}` + "`" + `)
+	items, cursor, hasMore := extractPageItems(json.RawMessage(body), "cursor")
+	if len(items) != 2 {
+		t.Fatalf("want 2 items, got %d", len(items))
+	}
+	if cursor != "page-2" {
+		t.Fatalf("want top-level cursor page-2, got %q", cursor)
+	}
+	if !hasMore {
+		t.Fatalf("want hasMore=true when cursor is present")
+	}
+}
+
+// Negative case: top-level cursor should still win even when a
+// non-cursor field happens to live in a wrapper-named key.
+func TestExtractPageItemsTopLevelCursorWinsOverWrapper(t *testing.T) {
+	body := []byte(` + "`" + `{
+		"items": [{"id":1}],
+		"next_cursor": "top-level-wins",
+		"meta": {"next_cursor": "should-not-be-used"}
+	}` + "`" + `)
+	_, cursor, _ := extractPageItems(json.RawMessage(body), "cursor")
+	if cursor != "top-level-wins" {
+		t.Fatalf("want top-level cursor, got %q", cursor)
+	}
+}
+
+// Negative case: no cursor anywhere returns empty without spurious
+// nested matches on unrelated wrapper-named objects.
+func TestExtractPageItemsNoCursor(t *testing.T) {
+	body := []byte(` + "`" + `{
+		"items": [{"id":1}],
+		"meta": {"count": 1, "page": 1}
+	}` + "`" + `)
+	_, cursor, hasMore := extractPageItems(json.RawMessage(body), "cursor")
+	if cursor != "" {
+		t.Fatalf("want empty cursor, got %q", cursor)
+	}
+	if hasMore {
+		t.Fatalf("want hasMore=false when no cursor present")
+	}
+}
+`
+	testPath := filepath.Join(outputDir, "internal", "cli", "sync_pagination_test.go")
+	require.NoError(t, os.WriteFile(testPath, []byte(inlineTest), 0o644))
+
+	runGoCommandRequired(t, outputDir, "mod", "tidy")
+	runGoCommandRequired(t, outputDir, "test", "-run", "TestExtractPageItems", "./internal/cli")
+}
+
 func TestGenerateStoreUpsertBatchDispatchesToTypedTable(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/generator/templates/sync.go.tmpl b/internal/generator/templates/sync.go.tmpl
index 6960816e..77849161 100644
--- a/internal/generator/templates/sync.go.tmpl
+++ b/internal/generator/templates/sync.go.tmpl
@@ -616,7 +616,6 @@ func extractPageItems(data json.RawMessage, cursorParam string) ([]json.RawMessa
 
 // extractPaginationFromEnvelope extracts cursor and has_more from a response envelope.
 func extractPaginationFromEnvelope(envelope map[string]json.RawMessage, cursorParam string) (string, bool) {
-	var nextCursor string
 	var hasMore bool
 
 	// Try common cursor field names
@@ -624,11 +623,27 @@ func extractPaginationFromEnvelope(envelope map[string]json.RawMessage, cursorPa
 		"next_cursor", "nextCursor", "cursor", "next_page_token",
 		"nextPageToken", "page_token", "after", "end_cursor", "endCursor",
 	}
-	for _, key := range cursorKeys {
-		if raw, ok := envelope[key]; ok {
-			var s string
-			if err := json.Unmarshal(raw, &s); err == nil && s != "" {
-				nextCursor = s
+	nextCursor := findCursorInMap(envelope, cursorKeys)
+
+	// If no top-level cursor was found, look one level deeper into well-known
+	// pagination wrapper objects. Slack returns {"messages":[...],
+	// "response_metadata":{"next_cursor":"..."}}; MongoDB Atlas uses
+	// "pagination"; many APIs use "meta" or "paging". Purely additive — only
+	// runs when the top-level scan returned empty — and uses the same
+	// cursorKeys set so wrapper contents go through the same name match.
+	if nextCursor == "" {
+		paginationWrapperKeys := []string{"response_metadata", "pagination", "meta", "paging"}
+		for _, wrapperKey := range paginationWrapperKeys {
+			rawWrapper, ok := envelope[wrapperKey]
+			if !ok {
+				continue
+			}
+			var inner map[string]json.RawMessage
+			if json.Unmarshal(rawWrapper, &inner) != nil {
+				continue
+			}
+			if c := findCursorInMap(inner, cursorKeys); c != "" {
+				nextCursor = c
 				break
 			}
 		}
@@ -652,6 +667,24 @@ func extractPaginationFromEnvelope(envelope map[string]json.RawMessage, cursorPa
 	return nextCursor, hasMore
 }
 
+// findCursorInMap returns the first non-empty string-typed value in m
+// whose key matches one of cursorKeys. Used by extractPaginationFromEnvelope
+// to scan both the top-level envelope and well-known wrapper objects with
+// the same name-match rules — extracted so the two scans can't drift.
+func findCursorInMap(m map[string]json.RawMessage, cursorKeys []string) string {
+	for _, key := range cursorKeys {
+		raw, ok := m[key]
+		if !ok {
+			continue
+		}
+		var s string
+		if err := json.Unmarshal(raw, &s); err == nil && s != "" {
+			return s
+		}
+	}
+	return ""
+}
+
 // upsertSingleObject stores a non-array API response as a single record.
 func upsertSingleObject(db *store.Store, resource string, data json.RawMessage) error {
 	var obj map[string]any

← 5920d005 docs(cli): document OpenAPI extensions and link it from AGEN · back to Cli Printing Press · Fix docs-only test filtering (#455) d909d6cf →