Skip to content

Use new bearer auth scheme and offset+limit pagination#25

Open
hi-rai wants to merge 3 commits into
dev/himanshu/add-e2e-testsfrom
dev/himanshu/auth-pagination-update
Open

Use new bearer auth scheme and offset+limit pagination#25
hi-rai wants to merge 3 commits into
dev/himanshu/add-e2e-testsfrom
dev/himanshu/auth-pagination-update

Conversation

@hi-rai

@hi-rai hi-rai commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@hi-rai hi-rai requested review from ramilamparo and satvik007 June 10, 2026 10:12
@hi-rai hi-rai self-assigned this Jun 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the API authorization header to use Bearer tokens instead of ApiKey. It also migrates pagination from page-based to offset-based for both test cases and folders listing, updating schemas, tools, and E2E tests accordingly. Additionally, it introduces support and tests for fetching only the total count when the limit is set to 0. There are no review comments to address, and I have no further feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR makes two related API changes:

  1. Auth scheme: switches the Authorization header from ApiKey <key> to Bearer <key>
  2. Pagination model: replaces page-based pagination (page + limit) with offset-based pagination (offset + limit) across both list_test_cases and list_folders

The changes are clean, consistent, and well-tested. A few things worth flagging below.


Breaking Changes — needs documentation

The PR description is empty, but this PR contains two breaking changes that will silently fail for existing users:

  • Auth: Any deployment already running against the API with ApiKey tokens will get 401s immediately after upgrade with no indication. Worth calling out in a release note or PR description so deployers know to rotate/reissue tokens if needed.
  • Pagination: Callers passing page: 2 previously would have gotten results; now that parameter is silently dropped and they'd get the first page instead of an error. If downstream clients use the old page field, this is a silent regression.

Schema Observations

listFoldersInputSchema default limit changed from 100 → 20 — the old default was 100 for folders vs 20 for test cases. This PR aligns both at 20, but for users with fewer than 100 folders who previously got everything in one call, this is a subtle behavior change. Was this intentional, or should folders stay at 100?

Output schemas use z.number() not z.number().int() — the input schemas correctly use .int() for offset and limit, but the output schemas (listTestCasesOutputSchema, listFoldersOutputSchema) use bare z.number(). Adding .int() to the output schemas too would make validation symmetric and catch any unexpected server behavior.

limit: 0 as a count-only sentinel — this is a nice pattern and well-documented in the schema description. One minor note: the data field is typed as nullable for folders. The test uses result.data ?? [] which masks a potential null return when limit=0. Worth confirming the API returns [] vs null in this case so the nullish coalescing is verified as necessary, not just defensive.


Test Coverage

The new tests are solid:

  • Default metadata test (offset=0, limit=20 echoed back) — good addition
  • Non-overlapping offset pagination test — correctly replaces the page-based version
  • limit=0 count-only test — good coverage of the new feature

One gap: no test for offset exceeding total (e.g., offset: 9999). The expectation would be data: [] and total > 0. Minor, but useful for robustness.


Minor

  • The destructured parameter rename from page to offset in src/tools/tcases.ts and src/tools/folders.ts is correct and complete.
  • The comment update in folders.test.ts (page=2offset=2) is a nice housekeeping touch.

Summary

Area Status
Correctness Logic is correct
Auth change Breaking — needs changelog/migration note
Pagination change Breaking for callers using page param
Schema validation Well-validated with int/min/max
Default limit for folders 100 → 20 may be unintentional
Test coverage Good, minor edge case gap
Output schema types Minor: add .int() to output offset/limit

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code Review

Overview

This PR makes two independent API-contract changes:

  1. Auth scheme: Authorization: ApiKey …Authorization: Bearer …
  2. Pagination scheme: page-based (page param) → offset-based (offset + limit params) for list_test_cases and list_folders

Both changes are clean and well-tested. A few things worth discussing below.


Breaking change / versioning

Both changes are breaking for existing callers:

  • Any client that passes page: N to either tool will silently have that param ignored (Zod strips unknown keys by default) and get results starting from offset 0, which is incorrect rather than an error.
  • The list_folders default limit quietly dropped from 100 → 20 — callers that relied on getting up to 100 folders in a single call will now get truncated results with no indication.

The bump to 0.5.0 is consistent with the pre-1.0 convention of using minor versions for breaking changes, but it's worth making the breaking nature explicit in the PR description / changelog so consumers know to audit their usage.

Suggestion: Consider adding a Zod .passthrough() note or at least documenting that page is no longer accepted, to make debugging easier for consumers who upgrade and see unexpected pagination.


Schema validation quality — positive

The new validators are a clear improvement:

offset: z.number().int().min(0).default(0)
limit:  z.number().int().min(0).max(1000).default(20)

.int() prevents accidental floats, .min(0) and .max(1000) give useful client-side error messages before the request ever leaves the process. Good defensive design.


limit=0 semantics

The schema allows limit: 0 and the description documents it as "fetch only the total count". This is a non-obvious but valid pattern. The e2e test for it is great.

One edge case to verify: if the backend returns data: null rather than data: [] when limit=0, the test assertion (result.data ?? []).toHaveLength(0) will still pass — but callers that do result.data.length without the nullish coalesce will throw. The output schema marks data as nullable() for folders but not for test cases — worth checking for consistency.

// folders output schema (nullable)
data: z.array(testFolderSchema).nullable().describe('List of folders')

// tcases output schema — check if this is also nullable

Folder default limit regression

-  limit: z.number().optional().default(100).describe('Number of items per page'),
+  limit: z.number().int().min(0).max(1000).default(20)

Changing the folders default from 100 → 20 is a silent behavioral regression for any client that didn't pass an explicit limit. It might be intentional for consistency with test cases, but it should be called out explicitly in the PR description.


Test coverage — positive

New tests added:

  • 'returns a paginated list with metadata' — verifies the defaults are echoed back correctly
  • 'returns only the total when limit=0' — covers the new special-case behaviour
  • 'returns non-overlapping items with offset pagination' — mirrors the old page-2 test cleanly

The tests verify both the response shape (offset/limit echoed back) and the data correctness (non-overlapping IDs). This is solid e2e coverage.

Minor: The 'returns a paginated list with metadata' test for folders was added but the equivalent test in tcases.test.ts only updated the existing typeof result.page check rather than being a new test. Both are fine, just noting the asymmetry.


Summary

Assessment
Auth change Clean one-liner, correct
Offset pagination Well-implemented, validated, tested
Breaking change handling Needs documentation; limit default regression should be noted
Schema validation Improved with .int(), .min(), .max()
Test coverage Good; new limit=0 and metadata tests are valuable
Version bump Appropriate for pre-1.0; PR body should describe breaking changes

The core implementation is correct. Main ask before merging: update the PR description (or a CHANGELOG entry) to explicitly list the breaking changes so consumers can upgrade safely.

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code Review: Use new bearer auth scheme and offset+limit pagination

Overview

This PR makes two distinct changes:

  1. Switches the API authentication header from ApiKey to Bearer token scheme.
  2. Replaces page-based pagination (page number) with offset-based pagination (offset + limit) for both test cases and folders.

Both changes are implemented consistently across schemas, tool handlers, and e2e tests. The version bump to 0.5.0 is appropriate given the breaking API surface changes.


Strengths

  • Consistent implementation: All three layers (schemas, tool handlers, tests) are updated in lock-step — no dangling references to page.
  • Good Zod constraints: The new offset and limit fields use .int().min(0).max(5000) and .default(0) / .default(20), which is precise and self-documenting.
  • limit=0 count-only feature: A useful addition, clearly described in the schema and covered by tests.
  • Test improvements: The new returns a paginated list with metadata test (folders) verifies echoed defaults, and the limit=0 tests for both resources are valuable.

Issues & Suggestions

Breaking change: folder limit default silently dropped from 100 → 20

- limit: z.number().optional().default(100).describe('Number of items per page'),
+ limit: z.number().int().min(0).max(5000).default(20).describe(...)

The previous default for folders was 100; it is now 20. Any existing caller that lists folders without specifying a limit will silently receive fewer results. This is a behavioural breaking change that should be called out explicitly in the PR description (and potentially in a CHANGELOG/README note). Consider whether 20 is intentional here or if it should match the previous 100 (or align with a different business rationale).


limit=0 boundary: min(0) allows ambiguous intent

min(0) on limit is correct for the count-only use-case, but it may surprise callers who pass limit=0 without reading the description. A small documentation note in the MCP tool description (beyond the schema .describe()) or a clear API contract note would help. Not blocking, but worth considering.


No negative-offset guard in tests

The schema correctly enforces min(0) on offset, but there is no e2e test asserting that a negative offset is rejected. Given that this is a new param with a non-obvious lower bound, one test like:

it('rejects negative offset', async () => {
  await expect(callTool(..., { offset: -1 })).rejects.toThrow(...)
})

...would make the contract explicit and guard against regressions.


Auth change has no test coverage

The ApiKeyBearer change is a one-liner and looks correct, but no test (unit or e2e) asserts the header value sent to the API. If the backend ever rejects the old scheme with a clear error, the e2e suite will catch it indirectly — but a unit test on apiQuery that inspects the Authorization header would make the contract explicit.


Base branch is a development branch, not main

This PR targets dev/himanshu/add-e2e-tests, not main. Verify this is intentional and that the eventual merge to main accounts for the full change surface.


Summary

Area Status
Auth header change ✅ Clean
Offset pagination schema ✅ Well-validated
Tool handler updates ✅ Consistent
Test coverage (happy path) ✅ Good
Folder default limit regression ⚠️ Needs explicit callout
Negative-offset boundary test 💡 Nice-to-have
Auth header unit test 💡 Nice-to-have

The core implementation is solid. The main thing worth addressing before merge is the unannounced default-limit change for folders, which could silently break existing integrations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant