[akhilp117]

Linear issue

This PR adds 5 decision-model lint rules as a temporary safety net while we build deterministic API checks in CI. All five are warn only, so they show up in the lint comment but never fail the check. Each check though will be removed once its deterministic check is enforced

Rules:

• api suite

  • returnsUnpaginatedList: A GET that returns a list that can keep growing without self.paginate(xyz). Covers lists nested in the response ({"data": [xyz]}), which the runtime pagination check in base.py misses because it only looks at top level lists
  • replacesMembershipWithList: A PUT/PATCH/POST list of IDs or members that replaces the whole stored set, so two overlapping edits overwrite each other. replayAccessMembers in organization_details.py is an example of how we should be using a sperate endpoint to update the perms
  • returnsNumericIdAsInteger: A numeric ID returned as an integer instead of a string, this was based on our API design rules
  • makesExistingInputStricter: An existing endpoint rejecting input it used to accept, or changing what an omitted field means: when a field becomes required, a required field is added, new or tighter validation is added, or a default changes. These are breaking changes since they break callers like removing a field does, but the spec often doesn't change, so spec-diff checks can't catch them. I added this rule so that we have something for detecting breaking changes (note that new endpoints and new optional fields don't count)

• security suite

  • looksUpObjectWithoutOrgScope: An object fetched by an ID from the request without filtering by the caller's organization or project, i.e, Project.objects.get(id=request.data["project_id"]). This is the first security rule in src/AGENTS.md, and no existing rule covers it. Lookups scoped by org or project, self.get_projects(), later has_project_access/check_object_permissions checks, and objects the endpoint base class resolved don't count

The api suite rules use threshold: 0.8, like the existing rules that had false positives. returnsNumericIdAsInteger keeps the default

The linter shows the model only the lines a PR changed, never what they were before. So it couldn't tell new code from code that was already there. Touching a line next to an old integer ID got a comment, and the fix it suggested would break clients obviosuly. The stricter input rule also couldn't see that required=False became required=True.

I made it so that the api suite now also gets each file's git diff against the PR's base, with removed and added lines only, so unchanged code stays hidden as before. The workflow sets DECISION_MODEL_LINT_BASE: HEAD^1 for this so that local runs get no diff unless you set it (ideally to git merge-base origin/master HEAD). The three design rules now answer INTRODUCED, PREEXISTING, or NONE, and only INTRODUCED is reported, so existing problems never get a comment. The stricter input rule compares the removed and added lines, which also lets it catch default changes again.

I tested this in #129449 on six cases, with the old and new config. The old config made three mistakes: it flagged an existing integer ID on a reformatted line, flagged an integer ID in a file with none, and missed a field made required. The new config got all six right, for more input tokens but the cost is insignificant.

For whole-file runs: running the CLI on whole files gives no diff, so the three INTRODUCED/PREEXISTING rules now treat the whole file as under review and report any problem they find. I added this since before, they split their answer between INTRODUCED and PREEXISTING and reported nothing.

For renamed files: a renamed file is diffed against its old path, so its real changes show up instead of the whole file looking new. This fixes a missed case where a field was made required in a renamed file

[linear-code]

ENG-8873

[narsaynorath]

Just some small comments, r.e. diffing and one of the variables, otherwise looks good to me

[narsaynorath]

If I understand correctly, HEAD^1 should just get the previous commit, right? Would we prefer to diff between the current HEAD and the target branch HEAD? There's a merge-base command that will return the common ancestor comment, i.e. git merge-base master branchA will get you the commit in master that was last common with branchA, which I imagine would be a better point to do the diff if I'm understanding the intent correctly

[akhilp117]

Good thought, but here HEAD^1 already gives the merge-base result. On pull_request runs, actions/checkout doesn't check out the PR's last commit. It checks out a merge commit GitHub makes by merging the PR into master. That commit's first parent (so HEAD^1) is master's tip, so HEAD^1..HEAD is exactly what the PR changes, even if master has moved on. It also has to match the --base HEAD^1 the linter already gets, and with fetch-depth: 2 there isn't enough history for git merge-base to work anyway. For local runs I'd set it to git merge-base origin/master HEAD

[narsaynorath]

It's a little weird that this variable is outside of the renamedFrom function and not used globally, can we push it into the renamedFrom function body?

[akhilp117]

Yeah I can see why you think that, but it has to stay outside because it's a cache cuz diff() runs only once per changed file, and renamedFrom returns the saved result after the first call, so git diff --name-status runs once per lint run instead of once per file. If it were inside the function, it would reset to undefined on every call and git would run for every file. I think I'll add a comment above the line to make that clear