fix(github): report the raw page length from list_repository_issues - #228
Conversation
list_repository_issues fetches one GitHub page and filters pull requests out of it, which destroys the only pagination signal page-number callers have: the filtered array's length says nothing about the raw page length. A short page may be a full page with PRs mixed in, and an empty page may be 100 consecutive PRs — so any paginating consumer that stops on a short or empty page silently drops every later issue, and no sound termination rule can be built from the filtered response alone. The response now carries pageInfo.fetched — the number of items GitHub returned before filtering — declared in the output schema and in the action description: callers must continue paginating while fetched equals the requested page size, even when issues comes back short or empty. The PR filtering behavior itself is unchanged. Tests cover the mixed page (fetched=3, issues=[1,3]), the all-pull-requests page (fetched=2, issues=[]), and the schema declaration.
Summary by CodeRabbit
WalkthroughThe GitHub 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/providers/github/runtime-issue.test.ts (1)
70-80: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that
pageInfois required at the top level.This test verifies that
fetchedis required insidepageInfo, but not thatoutputSchema.requiredcontains"pageInfo". Add that assertion to protect the complete output contract from future regression.Suggested assertion
const pageInfoProperties = pageInfo?.properties as Record<string, JsonSchema> | undefined; + const outputRequired = action?.outputSchema.required as string[] | undefined; expect(perPage).toMatchObject({ type: "integer", minimum: 1, maximum: 100, default: 30 }); expect(pageInfoProperties?.fetched?.type).toBe("integer"); expect(pageInfo?.required as string[] | undefined).toContain("fetched"); + expect(outputRequired).toContain("pageInfo");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/providers/github/runtime-issue.test.ts` around lines 70 - 80, Extend the “declares the pagination contract in the action schemas” test to inspect the action output schema’s top-level required fields and assert that it contains “pageInfo”. Keep the existing nested fetched requirement and other pagination assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/providers/github/runtime-issue.test.ts`:
- Around line 70-80: Extend the “declares the pagination contract in the action
schemas” test to inspect the action output schema’s top-level required fields
and assert that it contains “pageInfo”. Keep the existing nested fetched
requirement and other pagination assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a19b626f-c764-48f3-888f-098d9307fb2d
📒 Files selected for processing (3)
src/providers/github/actions.tssrc/providers/github/runtime-issue.test.tssrc/providers/github/runtime-issue.ts
…176) fix(sources): paginate github issues on the gateway's raw page length The OC list_repository_issues action filters pull requests out AFTER paginating, so the filtered issues array's length is not a termination signal: short-page termination silently truncated the scan on the first PR-bearing page, and even empty-page termination fails on 100 consecutive PRs. Upstream now reports the raw page length (pageInfo.fetched, oomol-lab/open-connector#228); this change consumes it. Engine: PageNumber gains raw_page_size_path, mirroring total_pages_path — when declared, the scan continues while the RAW page was full regardless of how short (or empty) the filtered rows are; a missing signal propagates as RowPathNotFound and a non-integer one fails as the new PaginationRawPageSizeInvalid (kind-only), never a silent truncation. The two authoritative signals are mutually exclusive at validate time. Loader exposes raw_page_size_path on page_number pagination; the issues table declares $.pageInfo.fetched. Contract re-captured from a live gateway carrying the upstream fix (outputSchema now declares pageInfo) and the fingerprint re-pinned, so older gateways fail issues registration at the fingerprint gate instead of truncating. Live-verified: all 11 tables register with the new pin and the issues scan reaches the credential wall. Tests: engine units (continue on full raw page with short/empty rows, terminate on short raw page, missing/invalid signal failures, total/raw mutual exclusion) and a pack e2e driving a 3-page scan whose middle page is ALL pull requests — the case that defeats every filtered-count heuristic. Stubs and the demo stub gateway now emit pageInfo. Docs and spec updated with the minimum-gateway note. 246 open_connector / 803 lib tests green. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The skill predated three structural changes that alter how a pack is actually authored; the workflow now matches the shipped reality: - Packs are embedded YAML assets: the implementation checklist authors packs/<provider>.yaml (bare table keys, derived ids, per-table pagination/columns/filters blocks, rationale as YAML comments) with a small OnceLock accessor module and registry entry — and enumerates what the validating loader already rejects (unknown keys, duplicate columns/mappings, undeclared filter columns, input-namespace collisions, zero page sizes, non-finite floats) so authoring attention goes to the semantic choices the loader cannot check. - Fingerprint pinning gained the coverage-gap pin: the recipe now ends with fingerprint_uncovered_columns asserting each table's exact uncovered set, and the review checklist carries the item. - Pagination soundness is a first-class phase-1 check: executors that filter rows AFTER paginating destroy the termination signal — the skill now teaches raw_page_size_path (with the upstream-contribution precedent, oomol-lab/open-connector#228) and the rule that a missing signal means upstream fix or deferral, never a heuristic. The engine-baseline section is reframed (main carries the full baseline today; verify-by-grep stays, with a refreshed marker list), and the branch absorbed origin/main (one trivial doc-comment conflict on fingerprint_schema, resolved keeping the richer rationale). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
#174) * feat(skills): add the source-pack skill for AI-driven pack development A shared Claude Code skill that lets an AI session take a provider request (Notion, Jira, …) to a review-ready PR by encoding what the GitHub and Slack packs taught us. Lives under docs/superpowers/skills/ — .claude/ is personal workspace configuration and stays untracked; each developer installs shared skills with a one-line symlink (docs/superpowers/skills/README.md). - SKILL.md: the five-phase workflow — live contract reconciliation FIRST (the wire contract is Open Connector's, not the provider's raw API), table design under the admission gate, implementation, self-review, then PR submission in the house style. - references/contract-reconciliation.md: running the local gateway, the verified /v1 surface (uniform envelope, no /execute suffix, camelCase strict schemas, alias header, no read/write classification), probing actions, reading executor source as the row-shape authority (passthrough vs normalized), the 400-vs-credential-wall input validation trick, and contract capture for fingerprint pinning. - references/implementation.md: pagination/filter/field design rules (total_pages_path, Inexact string-enums, boundary-row protections, ValueFormat), the six fixture categories, the fingerprint pinning recipe (capture -> pin -> sync test -> contract-serving mocks -> drift e2e), the per-declaration e2e test floor, and the three doc targets. - references/review-checklist.md: the distilled review standards from every round both packs went through — silent-truncation checks, contract honesty, structural assertions and row identity, both sides of every gate, information discipline, docs/spec sync, and the final self-review pass required before any PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): pin the no-credential validation trick's mechanism and preconditions A review argued the 400-vs-credential-wall step is invalid because action-runner.ts resolves the connection before executing. The code citation is accurate but the conclusion is not: for the DEFAULT connection, connection-service.ts#resolveForExecution is non-throwing when nothing is configured (credential resolution is lazy), so the runner reaches core/execution.ts#executeAction whose first step is validateActionInput — invalid input fails as invalid_input before any credential exists, valid input proceeds until provider-runtime.ts raises the credential wall. Verified live (v1.3.1, zero connections: per_page -> 400 naming the property; perPage -> 403 authorization failure) and now explained in the doc instead of asserted. The review did expose two real preconditions, now documented: probe the DEFAULT connection only (a missing NAMED connection fails as connection_not_found BEFORE validation, collapsing the distinction), ensure no action policy blocks the action, and calibrate the two distinct responses once per session before trusting either as schema evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(sources): make fingerprint_schema pub(crate) for pack sync tests The source-pack skill instructs pack contract tests to pin expected_fingerprint through action_registry::fingerprint_schema so pin and registration can never disagree on the canonicalization — but the function was module-private, so the documented sync test could not compile from a sibling packs module, and re-deriving the hash elsewhere is exactly what the guidance forbids. Now pub(crate) with a doc note naming pack sync tests as the intended caller, and the skill's implementation reference spells out the import path so the recipe is copy-ready. No behavior change; zero new clippy warnings; 203 open_connector tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): declare the 5.2 engine baseline instead of assuming it The skill cited PaginationCursorInvalid, total_pages_path, ValueFormat, TimestampSecondsUtc, FixedValue::StrList, error_path, and EnvVarGuard as existing invariants — all seven land with milestone 5.2 (PR #172) and none exist on main today, so a pack built from this guide on a pre-5.2 base would inherit the old cursor arm where a non-string cursor and every row-path failure read as end-of-collection (silent truncation). The implementation reference now opens with an Engine baseline section: a one-command git grep to verify the baseline, the rule that a missing invariant is PREREQUISITE work (engine fix + regression tests) rather than an assumption, and the general principle that every named safety invariant is a claim about code to verify on the actual branch. SKILL.md phase 3 and the checklist's termination item point at it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): reflect ValueFormat::Verbatim in the filter design rules Non-timestamp mappings declare Verbatim (which also keeps a timestamp literal local rather than pushing a guessed spelling); Rfc3339 and EpochSeconds are for genuine timestamp inputs only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): rework the source-pack skill for the YAML pack era The skill predated three structural changes that alter how a pack is actually authored; the workflow now matches the shipped reality: - Packs are embedded YAML assets: the implementation checklist authors packs/<provider>.yaml (bare table keys, derived ids, per-table pagination/columns/filters blocks, rationale as YAML comments) with a small OnceLock accessor module and registry entry — and enumerates what the validating loader already rejects (unknown keys, duplicate columns/mappings, undeclared filter columns, input-namespace collisions, zero page sizes, non-finite floats) so authoring attention goes to the semantic choices the loader cannot check. - Fingerprint pinning gained the coverage-gap pin: the recipe now ends with fingerprint_uncovered_columns asserting each table's exact uncovered set, and the review checklist carries the item. - Pagination soundness is a first-class phase-1 check: executors that filter rows AFTER paginating destroy the termination signal — the skill now teaches raw_page_size_path (with the upstream-contribution precedent, oomol-lab/open-connector#228) and the rule that a missing signal means upstream fix or deferral, never a heuristic. The engine-baseline section is reframed (main carries the full baseline today; verify-by-grep stays, with a refreshed marker list), and the branch absorbed origin/main (one trivial doc-comment conflict on fingerprint_schema, resolved keeping the richer rationale). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(skills): add real-data integration verification phase to source-pack The Notion pack (PR #177) proved that contract reconciliation alone is insufficient: it passed every contract-level check and still mapped columns that were always-NULL on the real wire, because declared schemas under-declare, misname (archived vs is_archived), or anyOf-hide the fields passthrough executors emit — and passthrough columns raise no error at registration or scan time. - New Phase 4 (references/live-verification.md): user-configured real credentials (never handled by the agent), per-action live probes, both-direction column-vs-wire diffs, end-to-end scans of every table through skardi-server, fixture re-derivation as redacted live captures with a mechanical redaction audit, provider API version recording, and evidence requirements for the PR. - Self-review and PR become phases 5/6; checklist gains real-data items (always-NULL detection, pins-return-rows, live multi-page pagination, live-capture fixtures, credential-rotation reminder). - contract-reconciliation.md now states the captured contract is fingerprint input, not column truth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Fixes #229.
list_repository_issuesfetches one GitHub page and filters pull requests out of it, which destroys the only pagination signal page-number callers have: the filtered array's length says nothing about the raw page length. A short page may be a full page with PRs mixed in, and an empty page may be 100 consecutive PRs — so any paginating consumer that stops on a short or empty page silently drops every later issue, and no sound termination rule can be built from the filtered response alone (details in #229).The response now carries
pageInfo.fetched— the number of items GitHub returned before filtering — declared in the output schema and in the action description: callers must continue paginating whilefetchedequals the requested page size, even whenissuescomes back short or empty. The PR-filtering behavior itself is unchanged.Tests cover the mixed page (
fetched=3, filtered ids[1,3]), the all-pull-requests page (fetched=2,issues=[]— the case that defeats every downstream heuristic), and the schema declaration.npm run typecheck, oxfmt, and the full vitest suite (557 tests) pass.