Repository navigation
tech-debt: analyze_diff is 117 body lines against a 65 threshold, and it blocked a security fix in a function it does not touch #16186
Description
Activity
Filed the pattern this is an instance of: #16191 —
Function Length Check (#620)is whole-file, so a legacy long function blocks every unrelated change to its file.Second instance hit today:
_execute_claimed(159 body lines) inautobot-backend/a2a/task_executor.pyblocked a one-line isort fix needed to unblock #16175. Splitting it to get the isort fix through nearly shipped an inverted eval gate — a droppedelse:would have marked every task that PASSED self-eval as FAILED, and that version was green on flake8, isort, black and the repo-wide F821 audit.That is the cost this issue's 2-line security fix is also paying, and it is why #16191 argues the guard should be scoped or ratcheted rather than have its threshold raised.
This issue stays the instance and is still being fixed on its own terms —
analyze_diffgets extracted, and the last unscrubbed git call site (#16179) lands in the same PR.- added a commit that references this issue
on Sep 10, 2026 Closing: all four criteria verified against base
8e7d16918, delivered by #16212#16212 merged with
Closes #16186. Closing keywords never fire on this branch, so this is the closure by hand. Each criterion was checked against merged code, not against the PR.Criterion (verbatim) Verdict Evidence analyze_diffis under the 65-line threshold, by extraction rather than by raising the threshold✅ analyze_diffis 57 body lines. The code moved to a new module,analytics_code_review_diff.py. #16212 changed no guard or threshold: its only other files are the two size-ceiling entries, which it lowered to match a smaller fileEach extracted helper is named for what it decides, not _part_1/_part_2✅ collect_file_commentsandpersist_reviewBehaviour is unchanged: verified by the existing tests for this endpoint passing without modification, and by a diff review that the extraction is a pure move ✅ No endpoint test was modified: the only test-tree file #16212 touched is the size-ratchet baseline, a ceiling, not a test of this endpoint. Its CI was green at merge. Diff review, computed rather than eyeballed: of 49 non-blank, non-comment lines removed from analytics_code_review.py, 43 appear verbatim in the new module. 5 are the rawcreate_subprocess_execgit call replaced bystart_git, which is the next row's security fix, not extraction drift. 1 ("total_comments": len(all_comments)) was parameterised:persist_reviewreceiveslen(all_comments)and writes it ascomment_count, so the value is the sameThe #16179 call site lands in the same PR or immediately after ✅ get_git_diffruns git throughstart_git, the scrubbed-environment runner. It landed in #16212 itselfThe structural cause, hooks that read whole changed files, is not this issue. It's tracked under umbrella #16238, with #16191 for function length specifically.
analyze_diffis 117 body lines, and it is blocking a security fix in a file it does not touchautobot-backend/api/analytics_code_review.py—analyze_diffhas 117 bodylines against
function-length-check's threshold of 65.Measured on
origin/Dev_new_guiand on a branch that changes a different functionin the same file: identical, 117 both sides, zero hunks mentioning it.
Why this is filed now
It blocked the last of the seven unscrubbed git call sites from #16179.
That change is two lines — route
get_git_diffthroughstart_git(), the canonicalscrubbed-environment runner — and it makes the file smaller (972 → 971). It
cannot land, because staging the file makes the commit answerable for
analyze_diff, which the diff never reads.So six of seven git call sites are now scrubbed and one is not, for a reason
unrelated to git, security, or the change itself.
Acceptance criteria
analyze_diffis under the 65-line threshold, by extraction rather than by raising the threshold_part_1/_part_2— the function is long because it does several things, and the split should say whichNot the fix
Raising the threshold. 65 is the repo's number and
analyze_diffis 80% overit; the constant is not the problem.
Bypassing the hook.
--no-verifyis prohibited, and correctly.Related
#16178 — the structural issue: three different hooks failed three commits tonight
for violations they did not introduce, because each reads the changed file whole
rather than the changed lines. This is the instance that cost a security fix.
#620 is the function-length guideline itself.