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 77770e7c1126..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: @@ -87,6 +124,105 @@ 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. ' + + CAUSED_BY_THIS_CHANGE, + values: [ + { + value: 'INTRODUCED', + description: + '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 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: '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.', + }, + ], + }, + 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. ' + + CAUSED_BY_THIS_CHANGE, + values: [ + { + value: 'INTRODUCED', + description: + '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. 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: '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.', + }, + ], + }, + 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. " + + CAUSED_BY_THIS_CHANGE, + values: [ + { + value: 'INTRODUCED', + description: + '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 change adds one returned as an integer. If the field already existed, don't change its type, because callers expect a number.", + }, + { + 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.', + }, + ], + }, + makesExistingInputStricter: { + threshold: 0.8, + prompt: + '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: + "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, 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, serializer, or optional field, relaxes or keeps existing validation and defaults, only moves or reformats lines, or diff is not available.', + }, + ], + }, }, } satisfies Suite; @@ -333,6 +469,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;