Skip to content

ci(sdks): Warn against non-trivial changes to multiple SDKs - #17397

Closed
szokeasaurusrex wants to merge 3 commits into
masterfrom
szokeasaurusrex/single-sdk-change-enforcement
Closed

ci(sdks): Warn against non-trivial changes to multiple SDKs#17397
szokeasaurusrex wants to merge 3 commits into
masterfrom
szokeasaurusrex/single-sdk-change-enforcement

Conversation

@szokeasaurusrex

@szokeasaurusrex szokeasaurusrex commented Apr 20, 2026

Copy link
Copy Markdown
Member

Add a CI job which fails and posts a comment when non-trivial changes to multiple SDK docs pages are detected. We define non-trivial as affecting more than five lines.

We do not intend to make this CI job required; rather, I hope PR authors and reviewers will heed the comments posted by this workflow.

Example runs: The job fails against #17398 and passes against #17400.

@vercel

vercel Bot commented Apr 20, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
develop-docs Ready Ready Preview, Comment Apr 20, 2026 4:36pm
sentry-docs Ready Ready Preview, Comment Apr 20, 2026 4:36pm

Request Review

Add a CI job which fails and posts a comment when non-trivial changes to multiple SDK docs pages are detected. We define non-trivial as affecting more than five lines.

We do not intend to make this CI job required; rather, I hope PR authors and reviewers will heed the comments posted by this workflow.
@szokeasaurusrex
szokeasaurusrex force-pushed the szokeasaurusrex/single-sdk-change-enforcement branch from 23f7ea8 to 51b9566 Compare April 20, 2026 16:12
@szokeasaurusrex
szokeasaurusrex marked this pull request as ready for review April 20, 2026 16:13
Comment thread scripts/check-sdk-diff-scope.ts Outdated
Change the filter condition from >= to > so that SDKs with exactly
LINE_THRESHOLD (5) changed lines are not treated as non-trivial.
This aligns the code with the documented behavior stating changes
must 'exceed' the threshold ('more than five lines').

Co-Authored-By: Claude <noreply@anthropic.com>

Co-authored-by: Daniel Szoke <szokeasaurusrex@users.noreply.github.com>

@cursor cursor Bot left a comment

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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6d2cb4d. Configure here.

}
if (segs[0] === 'platform-includes') {
return segs.at(-1)!.split('.')[0];
}

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.

Non-SDK files misclassified as SDK names cause false positives

Low Severity

The sdkForPath function doesn't filter out non-SDK files, causing them to be counted as separate "SDKs." For platform-includes, files like _default.mdx (which exists in platform-includes/capture-error/) return _default as the SDK name. For docs/platforms, the file index.mdx returns index.mdx as the SDK name. If any of these non-SDK files accumulate >5 changed lines alongside a real SDK, the check produces a false-positive violation, which could erode trust in the advisory CI job.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6d2cb4d. Configure here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@cursoragent please propose how we can rewrite this to filter out those non-SDK files

@cursor cursor Bot Apr 29, 2026

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.

Now I understand the issue. Here's my proposal for filtering out non-SDK files:

📋 Proposal to Fix Non-SDK File Misclassification

The issue is that sdkForPath returns invalid "SDK names" for:

  1. docs/platforms/index.mdx → returns index.mdx (should be null)
  2. platform-includes/*/_default.mdx → returns _default (should be null)

🔧 Option 1: Allowlist of Valid SDKs (Recommended)

Create an explicit set of known SDK names derived from docs/platforms/ directories. This is the most robust approach:

const VALID_SDKS = new Set([
  'android', 'apple', 'dart', 'dotnet', 'elixir', 'go', 'godot',
  'java', 'javascript', 'kotlin', 'native', 'nintendo-switch', 'php',
  'playstation', 'powershell', 'python', 'react-native', 'ruby', 'rust',
  'unity', 'unreal', 'xbox',
]);

function sdkForPath(path: string): string | null {
  const segs = path.split('/');
  if (segs[0] === 'docs' && segs[1] === 'platforms') {
    const sdk = segs[2];
    return sdk && VALID_SDKS.has(sdk) ? sdk : null;
  }
  if (segs[0] === 'platform-includes') {
    const sdk = segs.at(-1)!.split('.')[0];
    return VALID_SDKS.has(sdk) ? sdk : null;
  }
  return null;
}

Pros: Explicit, no ambiguity, handles edge cases like _default, index.mdx, or any future non-SDK files
Cons: Needs updating when new SDKs are added (but this is rare)


🔧 Option 2: Blocklist of Known Non-SDK Names

Filter out known bad patterns:

function sdkForPath(path: string): string | null {
  const segs = path.split('/');
  if (segs[0] === 'docs' && segs[1] === 'platforms') {
    const sdk = segs[2];
    // Ignore files directly in docs/platforms/ (like index.mdx)
    if (!sdk || sdk.includes('.')) return null;
    return sdk;
  }
  if (segs[0] === 'platform-includes') {
    const sdk = segs.at(-1)!.split('.')[0];
    // Ignore _default and other underscore-prefixed names
    if (sdk.startsWith('_')) return null;
    return sdk;
  }
  return null;
}

Pros: Simpler, no maintenance for new SDKs
Cons: May miss other edge cases


🔧 Option 3: Dynamic SDK Discovery

Read the docs/platforms/ directory at runtime to build the allowlist:

import {readdirSync} from 'fs';

const VALID_SDKS = new Set(
  readdirSync('docs/platforms', {withFileTypes: true})
    .filter(d => d.isDirectory())
    .map(d => d.name)
);

Pros: Always up-to-date
Cons: Adds filesystem I/O, assumes the script runs from repo root


💡 My Recommendation

Option 2 (Blocklist) strikes the best balance—it's simple, requires no maintenance, and handles the known problematic patterns. Would you like me to implement it?

Open in Web Open in Cursor 

@coolguyzone
coolguyzone requested a review from sergical April 23, 2026 21:33

@sergical sergical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice

@szokeasaurusrex

Copy link
Copy Markdown
Member Author

The logic seems not to be robust enough, Cursor's bug report is valid, but its suggested solutions are not optimal. Marking as draft until I have some time to take a closer look

@szokeasaurusrex
szokeasaurusrex marked this pull request as draft April 29, 2026 12:46
@szokeasaurusrex
szokeasaurusrex removed the request for review from stephanie-anderson April 29, 2026 12:46
@szokeasaurusrex

Copy link
Copy Markdown
Member Author

Not a priority to pick this up again, so I am closing

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.

4 participants