style: restore import order in dashboard token_auth (unblocks all PRs) - #2478
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe diff is a pure import-block reorder (isort output), moving the No findings. [OPUS-REVIEWED] 06d5136 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Mechanical isort output restoring alphabetical order broken by a semantic merge of two independently-clean PRs; unblocks a red [DESIGN-REVIEWED] 06d5136 |
|
Note on overlap: the same 7-line reorder is now also carried in #2476, so that PR can reach green without waiting on this one. This PR stays open on purpose — it unblocks every other open PR in the repo, and a 7-line formatter reorder can land far faster than a feature PR under review. Whichever merges first makes the other's hunk a no-op; it is pure |
What is the problem?
mainis red.Backend Lint & Type Checkfails on both 3.10 and 3.12 with:In
token_auth.pythekiro_crew.dashboard.tailnetimport block sits abovekiro_crew.dashboard.revocation_gen, which is out of alphabetical order (tailnet>revocation_gen).Neither contributing PR could have caught this.
#2424merged at 04:49:37Z and added thetailnetblock;#2388merged at 04:55:14Z, six minutes later, and inserted therevocation_genblock — with an explanatory comment between the two — at a position that was correct against its base, wheretailnetdid not yet exist. Each branch was isort-clean on its own base; only the merged result is unsorted. Git produced no textual conflict because the two insertions do not overlap.Why this issue matters to the user
Backend Lint & Type Checkis a blocking CI job, so every open PR in the repository fails it regardless of that PR's own content — the failure is inherited through the merge ref. Authors see a red check pointing at a file they never touched, and the natural triage steps all mislead: the file is absent from their diff, their local gate is green against their (older) base, and re-running the job reproduces the failure identically because the re-run stays pinned to the same merge ref. The likely outcomes are wasted investigation or, worse, someone concluding the isort gate is unreliable.How our fix solves it
Move the
tailnetimport block belowrevocation_gen, exactly asisortorders it.Symptom → root cause: the symptom is one file failing an ordering check. The cause is that two non-overlapping insertions into the same import list each satisfied the ordering invariant locally while jointly violating it. The root cause is that import order is a whole-list property, but CI only ever evaluated it against each branch's own base — so a semantic merge conflict of this shape lands green and is discovered only by the next PR to inherit it.
The change is the exact output of
isort src/kiro_crew/dashboard/token_auth.pywith the repo's pinnedisort==6.0.0— 7 lines moved, no import added, removed, or renamed. The comment explaining therevocation_genre-export stays attached to that block; the# noqa: F401 # re-exportspragma is untouched.What tests we did
isort --check-only src/kiro_crew test— reproduced the failure on a cleanmaincheckout at584bbb05fbefore the change, and confirmed clean after. This is the same command the failing CI job runs, at the version pinned inpyproject.toml.flake8 src/kiro_crew testandmypy src/kiro_crew/(862 files) clean, confirming the reorder does not disturb theF401re-export pragma or the import-cycle structure the comments describe.Manual verification: N/A — the change is mechanical formatter output, verified by the formatter itself.
Any other suggestions on the work
This will happen again. The gap is that lint runs against each branch's base, so a pair of independently-clean insertions into one sorted list can only be caught after the fact. Two options worth considering, neither in scope here:
main.mainso the breakage is reported against the commit that caused it, instead of surfacing on an unrelated PR minutes later.I can file that as a follow-up issue if it is worth tracking.