feat: cursor based pagination for sites endpoint

[martinst06]

Please ensure your pull request adheres to the following guidelines:

• make sure to link the related issues in this description. Or if there's no issue created, make sure you
  describe here the problem you're solving.
• when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

If the PR is changing the API specification:

• make sure you add a "Not implemented yet" note the endpoint description, if the implementation is not ready
  yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
• make sure you add at least one example of the request and response.

If the PR is changing the API implementation or an entity exposed through the API:

• make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

• make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot-Dev]

Hey @martinst06,

Strengths

• src/controllers/sites.js:411-413 - Clean backward-compatible branching. The hasText(limitParam) || hasText(cursor) gate gives a tidy opt-in to pagination while preserving the legacy contract for existing consumers with zero response shape change.
• src/controllers/sites.js:419-422 - Solid limit validation. parseInt with radix, Number.isInteger positivity check, and clamping to MAX_LIMIT correctly handle NaN, floats, negatives, zero, and over-large values.
• test/controllers/sites.test.js:623-744 - Thorough unit test suite covering the paginated envelope, legacy flat array, limit clamping, invalid input, cursor passthrough, DTO shape, and auth denial. Good boundary condition coverage.
• Removing the org-exclusion workaround in favor of proper pagination is the correct long-term solution. The old approach was data-dependent and would silently break as the dataset grew.
• Auth check is preserved and runs before any pagination logic on both paths.
• Integration tests updated to reflect actual behavior (6 sites returned vs the old filtered 4).

Issues

Important (Should Fix)

1. Legacy path lost its protection against the 6MB Lambda response size limit
src/controllers/sites.js:436 - The old code excluded DEFAULT_ORGANIZATION_ID and ORGANIZATION_ID_FRIENDS_FAMILY specifically to avoid 413 responses from exceeding the 6MB Lambda response limit. The legacy path (no limit/cursor params) now calls Site.all({}, { fetchAllPages: true }) with no filtering, returning strictly more data than before. Existing consumers hitting GET /sites without pagination params will receive a larger payload than yesterday.

Why it matters: The org exclusion existed because this endpoint already hit the 6MB limit in production. Removing it without protecting the legacy path re-exposes the original problem before consumers have had time to adopt pagination.

How to fix: Either (a) cap the legacy path with a warning log when the result set is large, (b) restore the org exclusion on the legacy path only, or (c) at minimum, add a deprecation log on the legacy path and document a sunset timeline so the risk is explicitly acknowledged. The comment on MAX_LIMIT ("lower to 500 if 6mb lambda limit is hit again") should note that the legacy path is the unprotected surface.

2. Cursor passed to data access layer without validation
src/controllers/sites.js:412 - The cursor parameter is taken directly from user input and forwarded to Site.all() without type or format validation. While this endpoint requires admin or S2S readAll capability (limiting the attacker pool), the cursor is still untrusted input passed to the data layer. A malformed cursor could trigger unexpected errors that leak internal state in error responses.

Why it matters: Defense in depth. Even behind auth, validating input shape at the controller boundary prevents classes of issues in downstream layers.

How to fix: Add basic validation before passing to the DAL - validate type is string and apply a reasonable length limit:

if (cursor !== null && (typeof cursor !== 'string' || cursor.length > 1024)) {
  return badRequest('Invalid cursor parameter');
}

3. No integration test for the paginated path
test/it/shared/tests/sites.js - The integration tests verify only the legacy flat-array response. There is no integration test calling GET /sites?limit=2 to verify the paginated envelope shape, cursor behavior, and hasMore field work end-to-end against real data access.

Why it matters: Unit tests mock Site.all to return { data, cursor }, but the integration between the controller, the actual data access layer's limit/cursor/returnCursor options, and the response serialization is only proven via integration tests. If Site.all returns a different shape under real conditions, the unit tests would still pass.

How to fix: Add at least one integration test that calls GET /sites?limit=2 as admin, asserts the response has sites and pagination keys, and verifies hasMore behaves correctly when results exceed the requested limit.

Minor (Nice to Have)

4. Silent empty-array fallback on contract violation
src/controllers/sites.js:425 - (results.data || []) will silently return an empty sites array if Site.all with returnCursor: true returns an unexpected shape (missing data property). This masks a data-access contract violation from the caller, who gets { sites: [], pagination: { hasMore: false } } with no signal that something went wrong.

How to fix: Consider logging a warning or failing explicitly if results lacks a data property.

5. Test claims negative-limit coverage but only tests zero
test/controllers/sites.test.js:695 - The test is named "falls back to DEFAULT_LIMIT when limit is zero or negative" but only sends limit: '0'. A negative value ('-5') exercises a subtly different code path. The logic is correct, but the test does not prove it for negatives as claimed.

How to fix: Add a second assertion or separate test case with limit: '-5'.

Recommendations

• Log a deprecation notice on the legacy path so you can track which consumers haven't migrated. The existing S2S log covers S2S consumers, but admin callers on the legacy path are invisible. This gives the data needed to set a removal timeline.
• Consider adding a total or totalCount field to the pagination envelope if the data access layer can provide it cheaply. If it is expensive, document explicitly why it is absent - that decision will be re-asked.

Assessment

Ready to merge? With fixes

The pagination implementation is clean, well-tested at the unit level, and architecturally sound - cursor-based pagination is the right pattern. The backward-compatible branching is well-structured. The main concern is that the legacy path lost its 6MB protection (the org exclusion) without gaining an alternative. If you are confident the current production dataset fits in 6MB with the previously-excluded orgs included, this could merge as-is with a documented follow-up to deprecate the legacy path - but the risk should be explicitly acknowledged.

Next Steps

1. Address the legacy path 6MB protection gap - either restore a safeguard or explicitly document the risk and deprecation plan.
2. Add cursor validation at the controller boundary.
3. Add an integration test for the paginated path.
4. Minor items are optional.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 28s | Cost: $4.65 | Commit: 4f4e8785f43f6af024e50ad8a1fce82f945154c5
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot-Dev]

Hey @martinst06,

Strengths

• src/controllers/sites.js:430-443 - Previously flagged issue now addressed: the legacy path correctly retains the DEFAULT_ORGANIZATION_ID and ORGANIZATION_ID_FRIENDS_FAMILY exclusion to stay under the 6MB Lambda response limit. The comment explaining the rationale is a nice touch for future maintainers.
• src/controllers/sites.js:411-413 - Clean backward-compatible branching. The hasText(limitParam) || hasText(cursor) gate provides a tidy opt-in to pagination while preserving the legacy contract for existing consumers with zero response shape change.
• src/controllers/sites.js:419-422 - Solid limit validation. parseInt with radix, Number.isInteger positivity check, and clamping to MAX_LIMIT correctly handle NaN, floats, negatives, zero, and over-large values.
• test/controllers/sites.test.js:625-744 - Thorough unit test suite covering the paginated envelope, legacy flat array, limit clamping, invalid input, cursor passthrough, DTO shape, and auth denial. Good boundary condition coverage.
• Auth check is preserved and runs before any pagination logic on both paths.
• test/it/shared/tests/sites.js:90-96 - Integration test updated to accurately reflect the restored exclusion behavior.

Issues

Important (Should Fix)

1. Cursor passed to data-access layer without validation
src/controllers/sites.js:412 - The cursor parameter is taken directly from user input and forwarded to Site.all() without type, format, or length validation. While this endpoint requires admin or S2S readAll capability (limiting the attacker pool), if the underlying data layer does not defensively validate cursor format, a malformed cursor could trigger unexpected query behavior, error-based information disclosure, or resource exhaustion. The limit parameter gets thorough validation; the cursor gets none.

How to fix - add a basic guard before passing to the data layer:

if (cursor !== null && (typeof cursor !== 'string' || cursor.length > 2048)) {
  return badRequest('Invalid cursor parameter');
}

If the data access layer already validates cursors and returns safe errors, a brief comment documenting that assumption would suffice as an alternative.

2. No integration test for the paginated path
test/it/shared/tests/sites.js - The integration tests only verify the legacy flat-array response. There is no integration test calling GET /sites?limit=2 to verify the paginated envelope shape, cursor behavior, and hasMore field work end-to-end against the real data access layer.

Why it matters: unit tests mock Site.all to return { data, cursor }, but the integration between the controller, the actual data access layer's limit/cursor/returnCursor options, and the response serialization is only proven via integration tests. The ci / it-postgres check is currently failing, which makes this gap more concerning.

How to fix: add at least one integration test that calls GET /sites?limit=2 as admin, asserts the response has sites and pagination keys, and verifies hasMore behaves correctly.

Minor (Nice to Have)

3. Silent empty-array fallback on contract violation
src/controllers/sites.js:425 - (results.data || []) will silently return an empty sites array if Site.all with returnCursor: true returns an unexpected shape (missing data property). This masks a data-access contract violation from the caller, who gets { sites: [], pagination: { hasMore: false } } with no signal that something went wrong.

How to fix: consider logging a warning when results.data is falsy so the failure is observable.

4. Test claims negative-limit coverage but only tests zero
test/controllers/sites.test.js:695 - The test is named "falls back to DEFAULT_LIMIT when limit is zero or negative" but only sends limit: '0'. A negative value ('-5') exercises a subtly different code path through the parsedLimit > 0 guard. The logic is correct by inspection, but the test does not prove it for negatives as claimed.

How to fix: add a second assertion or separate test case with limit: '-5'.

Recommendations

• Log a deprecation notice on the legacy path so you can track which consumers have not migrated to pagination. This gives the data needed to set a removal timeline.
• Consider documenting the pagination contract (envelope shape, cursor opaqueness, limit bounds) in the API specification or OpenAPI file so consumers have a stable reference.

Assessment

Ready to merge? With fixes

The pagination implementation is clean, well-tested at the unit level, and architecturally sound. The backward-compatible branching is well-structured, and the critical issue from the prior review (missing org exclusion) is resolved. The remaining gaps are: (1) cursor validation to avoid leaking persistence-layer errors as 500s, and (2) at least one integration test covering the paginated path to prove the data-access contract works end-to-end.

Note: CI check ci / it-postgres is currently failing - this should be investigated before merge.

Next Steps

1. Add cursor validation at the controller boundary (type check + length cap).
2. Add an integration test for the paginated path.
3. Investigate the failing ci / it-postgres check.
4. Minor items are optional.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 16s | Cost: $2.52 | Commit: 8b7e7867ff8ff9823c07e9e18c86880d5af12830
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[martinst06]

Thanks for the review. Addressed #3 (added a log.warn when Site.all returns an unexpected shape with returnCursor: true).

Pushing back on the rest:

1. Cursor validation — the endpoint requires admin or S2S site:readAll capability, so the attacker pool is effectively internal. The data-access layer's decodeCursor already handles malformed input gracefully (returns 0 for bad base64, treated as offset). Adding a redundant 3-line guard at the controller boundary for an internal-only endpoint feels like cargo-culted defense-in-depth without a concrete threat model. Happy to add it if there's a specific scenario I'm missing.

2. Paginated integration test — fair point in principle, but the cursor encoding/decoding logic lives entirely in spacecat-shared-data-access and is already covered by tests there. The controller-level integration only adds a limit/cursor query param to a method that's exercised by the existing IT tests on the legacy path. The risk that "real data access returns a different shape than mocks" is low because the same Site.all() call is used by both paths. I'd rather defer this to follow-up if/when we see a real issue.

4. Negative limit test — parseInt('-5', 10) === -5, which fails the > 0 check and falls back to DEFAULT_LIMIT. Same code path as '0'. The test name is accurate to behavior even if the assertion is only on one value. Will add -5 as an explicit case if there's appetite for it, but it's testing the same branch.

`ci / it-postgres` failing — this is stale; the latest commit (`8b7e786`) shows that job as success. The bot was looking at an older commit state.

Recommendations:

• Deprecation log on legacy path — can add if there's appetite, but until we agree on a sunset timeline it's just noise. Happy to do this once we have a date.
• OpenAPI spec update — this is on the ticket's acceptance criteria and will be done in a follow-up commit.

[MysticatBot-Dev]

Hey @martinst06,

Strengths

• src/controllers/sites.js:425 - Previously flagged silent empty-array fallback now addressed with a log.warn when Site.all returns an unexpected shape with returnCursor: true. Good responsiveness to review feedback.
• src/controllers/sites.js:411-413 - Clean backward-compatible branching. The hasText(limitParam) || hasText(cursor) gate provides a tidy opt-in to pagination while preserving the legacy contract for existing consumers with zero response shape change. This is how you migrate a live API without a version bump.
• src/controllers/sites.js:419-422 - Solid limit validation. parseInt with radix, Number.isInteger positivity check, and clamping to MAX_LIMIT correctly handle NaN, floats, negatives, zero, and over-large values.
• src/controllers/sites.js:430-443 - Legacy path correctly retains the org exclusion with a clear comment explaining why it exists. The TODO is resolved; this is now intentional behavior, not a workaround.
• test/controllers/sites.test.js:625-744 - Comprehensive unit test suite with good boundary condition coverage. Each test follows clean AAA structure and asserts on observable behavior (response status + body shape), not just stub interactions.
• Cursor-based pagination is the right architectural choice over offset-based for a growing dataset. This will still be the right pattern in 3-5 years.
• Auth check is preserved and runs before any pagination logic on both paths.

Prior findings resolved by author response: Cursor validation (DAL's decodeCursor handles malformed input gracefully behind admin/S2S auth), paginated integration test (cursor logic tested in spacecat-shared-data-access, same Site.all() entry point), negative limit test (same code path as zero - parseInt('-5', 10) fails > 0 identically). All three pushbacks are technically sound.

Issues

Minor (Nice to Have)

1. S2S consumer + paginated path combination untested
src/controllers/sites.js:449 - The s2sResult.allowed log statement fires on both paths, but all new pagination tests use an admin profile context, not an S2S consumer. The S2S + pagination combination has no unit-level exercise.

How to fix: Add one test case in the pagination describe block that uses an S2S consumer context with a limit param, and asserts the log is called with the correct count. Low risk since the log statement is trivial and the S2S check happens before the pagination branch.

Recommendations

• Consider documenting the cursor validation assumption inline (e.g., // cursor validated by DAL's decodeCursor) so future maintainers know the contract depends on that behavior in the shared data-access layer.
• Consider documenting a sunset timeline for the legacy path once S2S consumers adopt pagination. Removing the dual-path branch will eliminate the org-exclusion workaround and the 6MB operational constraint entirely.

Assessment

Ready to merge? Yes

The pagination implementation is clean, well-tested, architecturally sound, and backward-compatible. The prior review's concerns have been either addressed in code (log.warn on unexpected shape) or resolved by the author's responses with valid reasoning. No security concerns, no data integrity risks, no architectural issues. Ship it.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 40s | Cost: $4.61 | Commit: 7bc08378f0c0be2996c03cec72f29e88ab242a1b
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot-Dev]

Hey @martinst06,

Strengths

• src/controllers/sites.js:408-413 - Clean backward-compatible branching via hasText(limitParam) || hasText(cursor). Existing consumers see zero behavioral change; new consumers opt in explicitly. This is how you migrate a live API without a version bump.
• src/controllers/sites.js:414-415 - Cursor validation added (type check + 256 char cap) matching the OpenAPI spec declaration. Defense at the boundary is the right call for a public-facing controller.
• src/controllers/sites.js:419-422 - Solid limit validation: parseInt with radix, Number.isInteger, positivity check, and clamping to MAX_LIMIT correctly handles NaN, floats, negatives, zero, and over-large values.
• src/controllers/sites.js:424-430 - The unexpected-shape guard with log.error is a textbook justified fallback: logs the anomaly at the right level, provides diagnostic context, and returns a well-formed empty response rather than a 500.
• src/dto/config.js:71-75 - Stripping headers from fetchConfig in list responses is a solid security improvement. Bulk-listing endpoints should never expose credentials. The comment explains the reasoning.
• src/controllers/sites.js:436-443 - Legacy path retains org exclusion with clear comments explaining the operational reason (6MB Lambda limit) and the sunset mechanism (Coralogix zero-hit gate). The [sites][legacy-shape] log marker with clientId gives a concrete, queryable signal for sunsetting.
• test/it/shared/tests/sites.js:605-628 - Integration test proves the cursor round-trip end-to-end: fetch page 1, verify envelope shape and hasMore, fetch page 2, verify no ID overlap. This anchors the controller-DAL contract.
• test/controllers/sites.test.js:450-461 - Parameterized bad-limit tests covering ['abc', '0', '-1', '-100'] prove validation handles the full failure domain.
• test/controllers/sites.test.js:726-750 - Excellent boundary testing on cursor length (256 accepted, 257 rejected) plus type coercion tests for non-string cursors.
• Cursor-based over offset-based pagination is the correct architectural choice - stable under concurrent writes, scales to arbitrarily large datasets, and does not leak cardinality.
• All five prior review findings are resolved. The author pushed back with technically sound arguments and then addressed the items anyway - good engineering judgment.

Issues

Minor (Nice to Have)

1. Cursor validation error message is imprecise for non-string types
src/controllers/sites.js:414 - The guard typeof cursor !== 'string' || cursor.length > 256 returns 'cursor exceeds maximum length' for both conditions. When cursor is a number or object, the message is misleading. This endpoint is admin-only so the audience is internal, but precise error messages reduce debugging time.

How to fix:

if (cursor !== null) {
  if (typeof cursor !== 'string') return badRequest('cursor must be a string');
  if (cursor.length > 256) return badRequest('cursor exceeds maximum length');
}

2. oneOf in OpenAPI response without a discriminator
docs/openapi/sites-api.yaml - The 200 response uses oneOf: [SiteList, SitePagedResponse] but provides no discriminator. Code generators and SDK tooling cannot determine which schema to deserialize into without inspecting the response body shape at runtime. In practice consumers know which shape they requested (they sent limit/cursor or they did not), and this resolves itself when the legacy path is removed.

3. No test verifying fetchConfig.headers is stripped from the list DTO
src/dto/config.js:70-77 - The credential-stripping change has no dedicated test proving that a fetchConfig with { overrideBaseURL: '...', headers: { Authorization: 'Bearer ...' } } returns only { overrideBaseURL }, or that fetchConfig with only headers (no overrideBaseURL) is omitted entirely. Codecov confirms line coverage, but a regression test for security-relevant behavior would be stronger.

Recommendations

• Set a calendar date for legacy path removal. The TODO ("remove once Coralogix shows zero hits for 30 days") is an unbounded timeline. Consider a hard deprecation date (e.g., 90 days from merge) and communicate it to known consumers.
• Consider standardizing the pagination envelope across endpoints. The { items, pagination: { limit, cursor, hasMore } } shape is good - documenting "all new list endpoints use this envelope" makes the pattern durable as other endpoints paginate.

Assessment

Ready to merge? Yes

The pagination implementation is clean, well-tested, architecturally sound, and backward-compatible. Cursor-based pagination is the correct long-term solution for a growing dataset behind a Lambda with response size constraints. The dual-path complexity is justified and temporary, with a concrete observability mechanism for sunsetting. All prior review findings are addressed. The DTO credential stripping is a good opportunistic hardening. No blocking issues remain.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 45s | Cost: $5.41 | Commit: 03733bcb7fad1a4eb85660c74c547a0ab4d3d74a
If this code review was useful, please react with 👍. Otherwise, react with 👎.}