Skip to content

Fix pagination in exemption fetching behaviour in STO#175

Open
anuragtiwari005 wants to merge 2 commits into
mainfrom
feat/STO-11498/fix-pagination
Open

Fix pagination in exemption fetching behaviour in STO#175
anuragtiwari005 wants to merge 2 commits into
mainfrom
feat/STO-11498/fix-pagination

Conversation

@anuragtiwari005
Copy link
Copy Markdown
Contributor

@anuragtiwari005 anuragtiwari005 commented May 12, 2026

Description

Fix exemption list pagination & repeated rows

Problem

The MCP exemption listing had three compounding issues:

  1. Default page size of 5 never applied. harness_list's shared input schema defaults size=20 via Zod. The registry applies defaultQueryParams before mapping input→queryParams, so size=20 always overrode the STO-specific pageSize=5. Net effect: server always received pageSize=20.
  2. Repeated / skipped rows across "show more" turns. STO-Core computes offset = page × pageSize. When the LLM changed size between calls (e.g. first call size=20, "next 5" → size=5), rows were silently skipped or repeated.
  3. Noisy item payload. The raw API response leaked long IDs into the table, and the generic compactItems pass was stripping intentional display fields (severity, requested_by, etc.).

Fix

STO-scoped where possible; minimal, additive, opt-in infra changes where not.

  • src/registry/toolsets/sto.ts — Added a preflight hook on the exemption list op that normalizes input.size (treat the harness_list schema default 20 as "unspecified" → 5; clamp to [1, 50]) and defaults page to 0. Removed the now-dead defaultQueryParams. Description/filter fields tightened so the LLM keeps size constant across a session.
  • src/registry/extractors.ts — New stoExemptionsExtract projects the STO response into a display-ready shape, pulls IDs out of items into a _action_id_by_row lookup, and appends a _nextPageHint string so the LLM copies the exact next call (same size, page+1).
  • src/registry/types.ts + src/registry/index.ts + src/tools/harness-list.ts — New opt-in skipCompact?: boolean on EndpointSpec. When set, the registry tags the result with a non-enumerable __skipCompact marker (so it never leaks to the user-visible JSON) and harness_list skips its generic compaction. Default behavior for every existing resource is unchanged.

Behavior after this PR

  • LLM omits size → server gets pageSize=5, page 0 → rows 1–5.
  • User says "next 5" → LLM follows _nextPageHint → page=1, size=5 → rows 6–10.
  • User asks "show 10" → LLM passes size=10, preflight leaves it alone, size stays 10 for subsequent pages.

Scope check

  • Only the STO toolset changes business logic.
  • skipCompact is a generic, backwards-compatible opt-in mechanism (no other spec sets it today).
  • No sto-core changes.

Verification

  • pnpm build clean.
  • pnpm test — 1194/1194 passing.
  • Manual: chatbot now shows 5 by default, "next 5" advances correctly, no row repetition.

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Other

Checklist

  • Tests pass
  • Typecheck passes

@anuragtiwari005 anuragtiwari005 force-pushed the feat/STO-11498/fix-pagination branch from 979533b to b05d805 Compare May 13, 2026 05:04
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Stale comment

Findings

  1. Important: src/registry/extractors.ts
    The new _nextPageHint is not self-consistent for filtered exemption listings. It only reconstructs status and omits the active search filter, so following the hint after a search-scoped page can fetch a different result set than the one the user is paging through.

  2. Important: src/registry/toolsets/sto.ts, src/tools/harness-list.ts
    The STO-specific pagination default is being recovered by rewriting the shared harness_list inputs after Zod has already applied the generic size=20 default. That means an explicit size: 20 request is silently changed to 5, and pagination values that arrive through filters are normalized instead of failing loudly. This breaks the shared-tool contract and makes the fix broader than a narrowly scoped STO pagination adjustment.

Missing Tests

  • A tool-level harness_list regression for resource_type="security_exemption" that covers omitted size => 5, explicit custom sizes (especially explicit 20) remaining intact, and skipCompact preserving the projected STO fields.
  • An extractor/preflight regression that verifies _nextPageHint preserves the active filter set (status plus search) and that invalid pagination inputs fail loudly instead of being silently clamped/defaulted.

Overall Verdict

Not yet aligned with the repo’s architecture standards. The intent is good, but the current implementation still leaves a shared-tool contract mismatch and a non-fail-loud pagination path, so I would not approve it as-is.

Open in Web View Automation 

Sent by Cursor Automation: Sunil On Demand Architecture Review

Comment thread src/registry/extractors.ts Outdated
Comment thread src/registry/toolsets/sto.ts Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Stale comment

Not yet aligned with Sunil's architecture standards. I found two correctness issues in the new pagination path:

  1. src/registry/toolsets/sto.ts: the STO preflight overloads size=20 to mean "unspecified", so an explicit public harness_list(..., size=20) request can never be honored and is rewritten to 5. That also sets a precedent of silently rewriting shared-tool inputs instead of failing loudly.
  2. src/registry/extractors.ts: _nextPageHint is advertised as the exact follow-up call, but it drops active filters from the current request. In particular, a paged search flow will switch datasets on page 2.

Test gap: I couldn't find focused coverage for the public harness_list(resource_type="security_exemption") path that proves explicit size=20 is preserved or that _nextPageHint carries forward the active filters.

CI note: the PR's current GitHub checks are green, but these two cases still look like behavioral regressions.

Open in Web View Automation 

Sent by Cursor Automation: Sunil On Demand Architecture Review

Comment thread src/registry/toolsets/sto.ts Outdated
Comment thread src/registry/extractors.ts Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

1 important finding:

  • src/tools/harness-list.ts, src/registry/toolsets/sto.ts, tests/registry/sto-exemptions.test.ts: the public harness_list path still defaults omitted size to 20 before STO-specific logic runs. On current HEAD, parsing the registered harness_list schema for { resource_type: "security_exemption", filters: { status: "Pending" } } materializes { page: 0, size: 20, ... }, and the dispatched request goes out with pageSize=20. That means the new STO pagination guidance only holds when callers explicitly override filters.size; the runtime does not actually back the advertised first-page size of 5. The added tests miss this live path because they only exercise preflight / Registry.dispatch() directly, and the existing tool-handler stubs invoke handlers without SDK/Zod validation, so they never see the defaulted harness_list entrypoint. I did not find additional issues beyond this live-path size-default gap.
Open in Web View Automation 

Sent by Cursor Automation: Sunil On Demand Architecture Review

}
});

it("leaves size untouched when caller omits it (no silent default)", async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assertion only proves the internal preflight helper path. It doesn't cover the live harness_list contract, where the MCP SDK applies the shared Zod defaults before the handler runs. On current HEAD, harness_list({ resource_type: "security_exemption", filters: { status: "Pending" } }) still materializes size: 20 / page: 0, and the request goes out with pageSize=20. Please add a tool-level regression that goes through the registered harness_list schema/handler (or an actual MCP server call), otherwise this suite misses the remaining public-path bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right on the live-path finding — confirmed empirically that with
size: z.number().min(1).max(100).default(20).optional() in
src/tools/harness-list.ts, an omitted size materializes as 20 after Zod
parsing, so the request goes out with pageSize=20. The current STO-side
preflight cannot distinguish "Zod-defaulted 20" from "caller-explicit 20",
so any STO-only attempt to enforce default-5 would either:

(a) silently rewrite size=20 → 5, which violates the shared-tool contract
and is exactly the regression flagged in your earlier review, or
(b) require removing .default(20) from harness-list.ts — a cross-team
platform change touching the shared harness_list schema used by
23 other listable toolsets.

Option (b) is the correct architectural fix but is out of scope for
STO-11498 (STO ticket cannot change shared platform behavior without
platform-owner sign-off and regression testing across all listable
resources).

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