From 7d11718de0008ad6fb641aa555735a8730abb809 Mon Sep 17 00:00:00 2001 From: Akhil Pindiprolu Date: Wed, 28 Oct 2026 12:42:37 -0400 Subject: [PATCH 1/3] ci(decision-model-lint): Add temporary warn-only API and security rules --- decisionModelLinter.config.ts | 102 ++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/decisionModelLinter.config.ts b/decisionModelLinter.config.ts index 77770e7c1126..7cf31bfa402b 100644 --- a/decisionModelLinter.config.ts +++ b/decisionModelLinter.config.ts @@ -87,6 +87,87 @@ const api = { }, ], }, + // The four rules below are a temporary, "warn only" safety net for API design rules while + // deterministic checks for them are built. We will remove each one once its check is enforced + returnsUnpaginatedList: { + threshold: 0.8, + prompt: + 'Does an endpoint\'s GET method return a list of objects that can keep growing, such as an organization\'s projects, members, issues, releases, or monitors, without paginating it with self.paginate(...) or a paginator class? A list inside a response object (for example {"data": [...]}) counts too. Lists with a small fixed limit, lists of fixed choices or enum values, single objects, and lists that are only built for internal use and never returned do not count.', + values: [ + { + value: 'TRUE', + description: + 'A GET handler returns a growing collection, at the top level of the response or nested in it, and nothing paginates or limits it.', + severity: 'warn', + report: + 'List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This endpoint returns a list that can keep growing without paginating it.', + }, + { + value: 'FALSE', + description: + 'Every growing list the endpoint returns is paginated, or it returns a single object, a fixed set of values, or a list with a small fixed limit.', + }, + ], + }, + replacesMembershipWithList: { + threshold: 0.8, + prompt: + 'Does a PUT, PATCH, or POST that updates an existing object accept a list of IDs, users, emails, teams, or members (for example a serializers.ListField of user IDs) and use it to replace the whole stored set, by comparing it to the current members and adding or removing the difference, clearing and re-adding, or calling set()? replayAccessMembers in organization_details.py is an example: two people editing the list at the same time overwrite each other. Lists sent once when an object is first created, lists that only add members, and lists of settings or values that are not memberships do not count.', + values: [ + { + value: 'TRUE', + description: + 'A request field holds a list of members, users, teams, or IDs, and its value replaces the whole stored set on an existing object.', + severity: 'warn', + report: + 'A list field that replaces a whole set of members lets two concurrent edits overwrite each other. Prefer separate endpoints to add and remove one member.', + }, + { + value: 'FALSE', + description: + 'No request list replaces a stored set of members, or the list is only used when creating a new object, only adds members, or is not a membership list.', + }, + ], + }, + returnsNumericIdAsInteger: { + prompt: + "Does API response code, such as a serializer's serialize() method, a response TypedDict, or the body passed to Response(...), return a numeric ID (id, or a field ending in Id or _id, such as projectId or organization_id) as an integer instead of a string? IDs converted with str(), IDs that are already strings (UUIDs, slugs, hex event IDs), counts and other numbers that are not IDs, and code that does not build an API response do not count.", + values: [ + { + value: 'TRUE', + description: + 'A numeric ID is placed in an API response as an integer, for example "id": obj.id, or a response TypedDict declares an ID field as int.', + severity: 'warn', + report: + 'API responses should return numeric IDs as strings, for example `str(obj.id)`. This file returns one as an integer.', + }, + { + value: 'FALSE', + description: + 'Every ID in the response is a string, or the code builds no API response.', + }, + ], + }, + makesExistingInputStricter: { + threshold: 0.8, + prompt: + 'Does the change make an endpoint or serializer that already existed reject input it used to accept? Code already existed when the lines around the change are unchanged or omitted; removed lines are not shown. That includes a (new) field line in an existing serializer that is required (required=True, or no required=False), which covers both a field becoming required and a new required field, new validation on an existing field (a validate_ method, validators=, min_value, max_value, max_length, a narrower choices list, or a new ValidationError raised from validate), or an allowed value or format being removed. A new endpoint or serializer where every line is new, and new optional fields, do not count.', + values: [ + { + value: 'TRUE', + description: + 'Input that an existing endpoint or serializer accepted before is now rejected: a field became required, a required field was added, or new validation was added to existing input.', + severity: 'warn', + report: + "Rejecting input an endpoint accepted before breaks existing callers, and the API spec often doesn't show it. Make the change backward compatible (keep the field optional, or accept the old values), or treat it as a breaking change and coordinate it.", + }, + { + value: 'FALSE', + description: + 'The change adds a new endpoint or serializer, adds only optional fields, or relaxes or keeps existing validation.', + }, + ], + }, }, } satisfies Suite; @@ -333,6 +414,27 @@ const security = { }, ], }, + // A temporary, warn only safety net while a deterministic check for unscoped lookups is built. + looksUpObjectWithoutOrgScope: { + threshold: 0.8, + prompt: + "Does endpoint code fetch an object using an ID from the request (request.data, request.GET, request.query_params, or a serializer's validated_data), with Model.objects.get(...), filter(...), or get_object_or_404, without also filtering by the caller's organization or project, as in Project.objects.get(id=request.data[\"project_id\"])? Lookups that filter by organization or project (organization_id=organization.id, project__organization=organization, project_id__in=...), IDs checked through self.get_projects() or a similar access-checked helper, objects checked afterwards with has_project_access or check_object_permissions, objects the endpoint's base class already resolved from the URL (organization, project, team, group), and lookups of the caller's own user do not count.", + values: [ + { + value: 'TRUE', + description: + "An object is fetched by a request-supplied ID with no filter on the caller's organization or project and no access check on the result.", + severity: 'warn', + report: + "Objects fetched by an ID from the request should also be filtered by the caller's organization or project, for example `Model.objects.get(id=..., organization_id=organization.id)`, or checked with `self.get_projects()`. Without that, one organization can read or change another's data.", + }, + { + value: 'FALSE', + description: + "Every request-supplied ID is looked up within the caller's organization or project, goes through an access-checked helper, or the object is access-checked after it is fetched, or no object is fetched by a request-supplied ID.", + }, + ], + }, }, } satisfies Suite; From 775e8c2a717e6364102b55817d0529d5cd5c9817 Mon Sep 17 00:00:00 2001 From: Akhil Pindiprolu Date: Wed, 28 Oct 2026 14:33:54 -0400 Subject: [PATCH 2/3] ci(decision-model-lint): Warn against breaking changes in rule reports --- decisionModelLinter.config.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/decisionModelLinter.config.ts b/decisionModelLinter.config.ts index 7cf31bfa402b..871d824f655c 100644 --- a/decisionModelLinter.config.ts +++ b/decisionModelLinter.config.ts @@ -100,7 +100,7 @@ const api = { 'A GET handler returns a growing collection, at the top level of the response or nested in it, and nothing paginates or limits it.', severity: 'warn', report: - 'List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This endpoint returns a list that can keep growing without paginating it.', + "List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This endpoint returns a list that can keep growing without paginating it. If the endpoint already exists, don't add pagination here: that changes what existing callers get back, so it's a breaking change that needs a deprecation plan.", }, { value: 'FALSE', @@ -120,7 +120,7 @@ const api = { 'A request field holds a list of members, users, teams, or IDs, and its value replaces the whole stored set on an existing object.', severity: 'warn', report: - 'A list field that replaces a whole set of members lets two concurrent edits overwrite each other. Prefer separate endpoints to add and remove one member.', + "A list field that replaces a whole set of members lets two concurrent edits overwrite each other. For new fields, prefer separate endpoints to add and remove one member. Don't change or remove an existing field to fix this, because that breaks its callers.", }, { value: 'FALSE', @@ -139,7 +139,7 @@ const api = { 'A numeric ID is placed in an API response as an integer, for example "id": obj.id, or a response TypedDict declares an ID field as int.', severity: 'warn', report: - 'API responses should return numeric IDs as strings, for example `str(obj.id)`. This file returns one as an integer.', + "API responses should return numeric IDs as strings, for example `str(obj.id)`. This file returns one as an integer. If the field already exists, don't change its type here, because callers expect a number. Only new fields should follow this rule.", }, { value: 'FALSE', From 2532c6cfa23dd350068eb3fac975edf99080957e Mon Sep 17 00:00:00 2001 From: Akhil Pindiprolu Date: Sun, 1 Nov 2026 12:53:20 -0500 Subject: [PATCH 3/3] ci(decision-model-lint): Give API rules the PR diff so they skip existing problems --- .github/workflows/decision-model-lint.yml | 2 + decisionModelLinter.config.ts | 93 ++++++++++++++++++----- 2 files changed, 76 insertions(+), 19 deletions(-) diff --git a/.github/workflows/decision-model-lint.yml b/.github/workflows/decision-model-lint.yml index aca58795f884..5b811e3db38f 100644 --- a/.github/workflows/decision-model-lint.yml +++ b/.github/workflows/decision-model-lint.yml @@ -35,6 +35,8 @@ jobs: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.number }} LABEL: 'Trigger: Override Decision Model Lint' + # Rules diff each file against the PR's base, it must match --base below + DECISION_MODEL_LINT_BASE: HEAD^1 OVERRIDDEN: "${{ contains(github.event.pull_request.labels.*.name, 'Trigger: Override Decision Model Lint') }}" run: | report="$RUNNER_TEMP/decision-model-lint.md" diff --git a/decisionModelLinter.config.ts b/decisionModelLinter.config.ts index 871d824f655c..81aeeecb2d35 100644 --- a/decisionModelLinter.config.ts +++ b/decisionModelLinter.config.ts @@ -1,4 +1,7 @@ // Sentry's decision-model lint rules: https://github.com/getsentry/decision-model-linter +import {execFile} from 'node:child_process'; +import {promisify} from 'node:util'; + import type {Config, Model, Suite} from '@sentry/decision-model-linter'; const jev = { @@ -8,7 +11,41 @@ const jev = { maxInputTokens: 32_768, } satisfies Model; +const diffBase = process.env.DECISION_MODEL_LINT_BASE; +const NO_DIFF = 'No diff available.'; +const execFileAsync = promisify(execFile); + +// the engine shows a changed file's new lines only, so a rule can't tell new code from code +// that was already there. The diff adds the removed lines, with no context lines so unchanged +// code stays hidden from the other rules in the suitee +async function diff({path}: {path: string}): Promise { + if (!diffBase) { + return NO_DIFF; + } + try { + const {stdout} = await execFileAsync('git', [ + 'diff', + '--unified=0', + diffBase, + '--', + path, + ]); + // a new file or a renamed one + if (!stdout || /^new file mode/m.test(stdout)) { + return NO_DIFF; + } + return stdout; + } catch { + return NO_DIFF; + } +} + +// added to the prompts of rules that should only report problems the change itself adds +const CAUSED_BY_THIS_CHANGE = + 'Use diff to tell whether this change caused it: compare its - (removed) and + (added) lines. A line that only moved or was reformatted appears as both - and +, so it already existed. If diff says No diff available., judge from the (new) lines alone.'; + const api = { + context: {diff}, rules: { addsNewOmittedFields: { prompt: @@ -92,18 +129,24 @@ const api = { returnsUnpaginatedList: { threshold: 0.8, prompt: - 'Does an endpoint\'s GET method return a list of objects that can keep growing, such as an organization\'s projects, members, issues, releases, or monitors, without paginating it with self.paginate(...) or a paginator class? A list inside a response object (for example {"data": [...]}) counts too. Lists with a small fixed limit, lists of fixed choices or enum values, single objects, and lists that are only built for internal use and never returned do not count.', + 'Does an endpoint\'s GET method return a list of objects that can keep growing, such as an organization\'s projects, members, issues, releases, or monitors, without paginating it with self.paginate(...) or a paginator class? A list inside a response object (for example {"data": [...]}) counts too. Lists with a small fixed limit, lists of fixed choices or enum values, single objects, and lists that are only built for internal use and never returned do not count. ' + + CAUSED_BY_THIS_CHANGE, values: [ { - value: 'TRUE', + value: 'INTRODUCED', description: - 'A GET handler returns a growing collection, at the top level of the response or nested in it, and nothing paginates or limits it.', + 'This change adds a GET handler, or a newly returned list, whose growing collection nothing paginates or limits, or it removes pagination from a list that had it.', severity: 'warn', report: - "List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This endpoint returns a list that can keep growing without paginating it. If the endpoint already exists, don't add pagination here: that changes what existing callers get back, so it's a breaking change that needs a deprecation plan.", + "List endpoints should paginate with `self.paginate(...)` and a `cursor` parameter. This change returns a list that can keep growing without paginating it. If the endpoint already existed, don't add pagination to it: that changes what existing callers get back.", }, { - value: 'FALSE', + value: 'PREEXISTING', + description: + 'The endpoint already returned the unpaginated list before this change: its handler and return lines are not added in diff, and the change only touches nearby code.', + }, + { + value: 'NONE', description: 'Every growing list the endpoint returns is paginated, or it returns a single object, a fixed set of values, or a list with a small fixed limit.', }, @@ -112,18 +155,24 @@ const api = { replacesMembershipWithList: { threshold: 0.8, prompt: - 'Does a PUT, PATCH, or POST that updates an existing object accept a list of IDs, users, emails, teams, or members (for example a serializers.ListField of user IDs) and use it to replace the whole stored set, by comparing it to the current members and adding or removing the difference, clearing and re-adding, or calling set()? replayAccessMembers in organization_details.py is an example: two people editing the list at the same time overwrite each other. Lists sent once when an object is first created, lists that only add members, and lists of settings or values that are not memberships do not count.', + 'Does a PUT, PATCH, or POST that updates an existing object accept a list of IDs, users, emails, teams, or members (for example a serializers.ListField of user IDs) and use it to replace the whole stored set, by comparing it to the current members and adding or removing the difference, clearing and re-adding, or calling set()? replayAccessMembers in organization_details.py is an example: two people editing the list at the same time overwrite each other. Lists sent once when an object is first created, lists that only add members, and lists of settings or values that are not memberships do not count. ' + + CAUSED_BY_THIS_CHANGE, values: [ { - value: 'TRUE', + value: 'INTRODUCED', description: - 'A request field holds a list of members, users, teams, or IDs, and its value replaces the whole stored set on an existing object.', + 'This change adds a request list field, or new code that uses one, to replace the whole stored set of members on an existing object, and diff shows no removed line for that field or logic.', severity: 'warn', report: - "A list field that replaces a whole set of members lets two concurrent edits overwrite each other. For new fields, prefer separate endpoints to add and remove one member. Don't change or remove an existing field to fix this, because that breaks its callers.", + "A list field that replaces a whole set of members lets two concurrent edits overwrite each other. Prefer separate endpoints to add and remove one member. Don't change or remove an existing field to fix this, because that breaks its callers.", }, { - value: 'FALSE', + value: 'PREEXISTING', + description: + 'A list already replaced the stored set before this change: the field and the replacing code are not added in diff, or appear as both removed and added lines.', + }, + { + value: 'NONE', description: 'No request list replaces a stored set of members, or the list is only used when creating a new object, only adds members, or is not a membership list.', }, @@ -131,18 +180,24 @@ const api = { }, returnsNumericIdAsInteger: { prompt: - "Does API response code, such as a serializer's serialize() method, a response TypedDict, or the body passed to Response(...), return a numeric ID (id, or a field ending in Id or _id, such as projectId or organization_id) as an integer instead of a string? IDs converted with str(), IDs that are already strings (UUIDs, slugs, hex event IDs), counts and other numbers that are not IDs, and code that does not build an API response do not count.", + "Does API response code, such as a serializer's serialize() method, a response TypedDict, or the body passed to Response(...), return a numeric ID (id, or a field ending in Id or _id, such as projectId or organization_id) as an integer instead of a string? IDs converted with str(), IDs that are already strings (UUIDs, slugs, hex event IDs), counts and other numbers that are not IDs, and code that does not build an API response do not count. " + + CAUSED_BY_THIS_CHANGE, values: [ { - value: 'TRUE', + value: 'INTRODUCED', description: - 'A numeric ID is placed in an API response as an integer, for example "id": obj.id, or a response TypedDict declares an ID field as int.', + 'An added line returns a numeric ID as an integer, for example "id": obj.id, or declares an ID field as int in a response TypedDict, and no removed line in diff had that field: a new field, serializer, or endpoint.', severity: 'warn', report: - "API responses should return numeric IDs as strings, for example `str(obj.id)`. This file returns one as an integer. If the field already exists, don't change its type here, because callers expect a number. Only new fields should follow this rule.", + "API responses should return numeric IDs as strings, for example `str(obj.id)`. This change adds one returned as an integer. If the field already existed, don't change its type, because callers expect a number.", }, { - value: 'FALSE', + value: 'PREEXISTING', + description: + 'An ID is returned as an integer, but it already was before this change: the field also appears in a removed line in diff, or the change only touches nearby code.', + }, + { + value: 'NONE', description: 'Every ID in the response is a string, or the code builds no API response.', }, @@ -151,20 +206,20 @@ const api = { makesExistingInputStricter: { threshold: 0.8, prompt: - 'Does the change make an endpoint or serializer that already existed reject input it used to accept? Code already existed when the lines around the change are unchanged or omitted; removed lines are not shown. That includes a (new) field line in an existing serializer that is required (required=True, or no required=False), which covers both a field becoming required and a new required field, new validation on an existing field (a validate_ method, validators=, min_value, max_value, max_length, a narrower choices list, or a new ValidationError raised from validate), or an allowed value or format being removed. A new endpoint or serializer where every line is new, and new optional fields, do not count.', + 'Does the change make an endpoint or serializer that already existed reject input it used to accept, or change what callers get when they leave a field out? Compare the - (removed) and + (added) lines in diff for the same field or method. That includes: a field that becomes required (required=False or a default removed, or required=True added); a new required field added to an existing serializer; new or tighter validation on an existing field (a validate_ method, validators=, a smaller max_length or max_value, a larger min_length or min_value, a narrower choices list, or a new ValidationError raised from validate); an allowed value or format removed; or the default of an existing optional field changed. A serializer or endpoint is new, and does not count, when its class or def line is an added line in diff. New optional fields, relaxed validation, and lines that only moved or were reformatted do not count. If diff says No diff available., nothing existed to compare against, so answer FALSE.', values: [ { value: 'TRUE', description: - 'Input that an existing endpoint or serializer accepted before is now rejected: a field became required, a required field was added, or new validation was added to existing input.', + "Diff shows existing input becoming stricter or changing meaning: a field became required, a required field was added to an existing serializer, validation was added or tightened, an allowed value was removed, or an optional field's default changed.", severity: 'warn', report: - "Rejecting input an endpoint accepted before breaks existing callers, and the API spec often doesn't show it. Make the change backward compatible (keep the field optional, or accept the old values), or treat it as a breaking change and coordinate it.", + "Rejecting input an endpoint accepted before, or changing a field's default, breaks existing callers, and the API spec often doesn't show it. Make the change backward compatible (keep the field optional, accept the old values, or keep the old default), or treat it as a breaking change and coordinate it.", }, { value: 'FALSE', description: - 'The change adds a new endpoint or serializer, adds only optional fields, or relaxes or keeps existing validation.', + 'The change adds a new endpoint, serializer, or optional field, relaxes or keeps existing validation and defaults, only moves or reformats lines, or diff is not available.', }, ], },