Support multiple linting rules #9859

aemous · 2025-11-20T16:13:13Z

Description of changes:

Updated interactive and auto-fix mode to support multiple linting rules. The main technical complexity with supporting multiple rules is that as we apply fixes to the script, older linter findings become stale and would need to be
'refreshed'. We avoid this problem by linting the script one-rule-at-a-time.
Updated Base64BinaryFormatRule so that it does not require file:// prefix in an argument value to trigger a detection. It now flags all AWS CLI commands.
Added PaginationRule that flags all AWS CLI commands, and adds --no-cli-paginate as a fix.
Replaced ScriptLinter class with utility functions. Updated their interface to let callers pass in their own AST.
Added save and exit mode.
Added tests for save-and-exit mode and the new PaginationRule.
Misc. refactoring for maintainability/readability.

Description of tests:

Ran and passed all tests.
Manually tested the tool in interactive and auto-fix mode on the example script, and verified correct behavior.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ter value.

…detection.

… fix.

codecov-commenter · 2025-11-20T16:22:24Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (upgrade-linting-tool@6ebbb50). Learn more about missing BASE report.

Additional details and impacted files

@@                   Coverage Diff                   @@
##             upgrade-linting-tool    #9859   +/-   ##
=======================================================
  Coverage                        ?   93.33%           
=======================================================
  Files                           ?      209           
  Lines                           ?    16807           
  Branches                        ?        0           
=======================================================
  Hits                            ?    15687           
  Misses                          ?     1120           
  Partials                        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aemous · 2025-12-03T15:25:04Z

Just a note, the pagination linting rule should actually be based around the --no-cli-pager argument instead of the --no-paginate argument. This will be addressed in a follow-up PR, I already have the fix staged locally with the next round of changes.

kdaily · 2025-12-04T19:28:29Z

awsclilinter/awsclilinter/cli.py

-        choice = input(prompt).lower().strip()
-        if choice in ["y", "n", "u", "s"]:
+        choice = (
+            input("\nApply this fix? [y]es, [n]o, [u]pdate all, [s]ave and exit, [q]uit: ")


For accessibility, we should make the letter to select separate from the word that describes. A screen reader would not be able to read this well. How about:

Suggested change

input("\nApply this fix? [y]es, [n]o, [u]pdate all, [s]ave and exit, [q]uit: ")

input("\nApply this fix? [y] yes, [n] no, [u] update all, [s] save and exit, [q] quit: ")

kdaily

Thanks, nice work so far! Mostly a few basic comments and suggestions.

I think the biggest feedback is the main() method is quite long and a bit hard to follow. I would recommend breaking it out into a few helpers to clarify the branching behavior a bit better.

awsclilinter/awsclilinter/rules/pagination_rule.py

kdaily · 2025-12-09T20:31:19Z

awsclilinter/awsclilinter/rules/pagination_rule.py

+    def description(self) -> str:
+        return (
+            "AWS CLI v2 uses pagination by default for commands that return large result sets. "
+            "Add --no-cli-paginate to disable pagination and match v1 behavior."


Users can also disable pagination via env variable or config file option, which they may be already doing. Would that make detection a false positive?. If so, how do we account for that?

Good question. Since this is a static linter, we cannot resolve the user's config profile values, and if that behavior is desired, we can point users to use the --v2-debug runtime migration tool instead, which has that functionality. Being static, we should be processing these scripts independent of the environment: the only input is a script, and the only output is a script (possibly with a detections summary).

I generally agree that would be a false positive case. As a workaround, if a user knows they configured pagination to disable the pager, then they can reject the auto-fix in interactive mode by entering "n" when the fix is suggested. In any case, even if the auto-fix is made, the command-line option overrides the configuration in precedence order, and the behavior would not change (pager gets disabled anyways). So, the worst case scenario is that the flag is added even when it's unnecessary, however, it being unnecessary does not cause breakage.

kdaily · 2025-12-09T20:38:34Z

awsclilinter/awsclilinter/cli.py

+    # Group findings by rule
+    findings_by_rule: Dict[str, List[LintFinding]] = {}
+    for finding, rule in findings_with_rules:
+        if rule.name not in findings_by_rule:


nit: could use collections.defaultdict to reduce checking here, not a strong preference.

kdaily · 2025-12-09T20:42:43Z

awsclilinter/awsclilinter/cli.py

+                ast = parse(linter.apply_fixes(ast, accepted_findings))
+            return ast, len(accepted_findings) > 0, last_choice
+        elif last_choice == "q":
+            print("Quit without saving.")


nit:

Suggested change

print("Quit without saving.")

print("Quitting without saving.")

kdaily · 2025-12-09T20:52:10Z

awsclilinter/awsclilinter/cli.py

-        findings = interactive_mode(findings, script_content)
-        if not findings:
+        # Do an initial parse-and-lint with all the rules simultaneously to compute the total
+        # number of findings for displaying progress in interactive mode.


What's the value of displaying progress here? It seems a bit unnecessary to completely run through the entire set of possible changes only to do it again without just saving the diffs of what would have been changed and then accepting/denying them in the later loop. That would increase complexity of the solution, and may not matter for performance (scripts might not be that big).

I would also be fine with a summary at the end that said how many changes could have been applied and how many were.

Good point, I was second-guessing this myself. I figured users may want to know how many findings are left especially if the script is large. However, that may not be super valuable, can always add it back if users request it.

Will remove this in next revision.

kdaily · 2025-12-09T20:53:37Z

awsclilinter/awsclilinter/cli.py

+        any_changes = False
+        finding_offset = 0
+
+        # Calculate total findings for display


See comment above, but this should probably go above the comment "# Process one rule at a time" for clarity of where the logic starts.

kdaily · 2025-12-09T20:58:46Z

awsclilinter/awsclilinter/cli.py

        output_path = Path(args.output) if args.output else script_path
-        output_path.write_text(fixed_content)
+        output_path.write_text(current_script)
+        print(f"Fixed script written to: {output_path}")


nit:

Suggested change

print(f"Fixed script written to: {output_path}")

print(f"Modified script written to: {output_path}")

aemous added 12 commits November 12, 2025 14:48

Add quit without saving option.

b4130a8

Modify the base64 parameter rule to not require file:// in the parame…

77e93a9

…ter value.

Test for multiple rules

0411695

Working prototype for multiple linting rules.

c7f013a

Refactor to use a apply_all_fixes helper function.

f4a1be2

Add performance optimization to only parse into AST at most once per …

f5ef14c

…detection.

Add assertion to verify one LintRule fix won't resolve another rule's…

10caf05

… fix.

Progress on multiple linting rules.

7a6a159

Finalize refactor for multiple linting rules.

e496ac3

Add LICENSE.txt to MANIFEST.in.

a46c004

Update MANIFEST and pyproject.

f2e3633

Fix tests.

a855a4a

aemous changed the title ~~Multiple linting rules testeiifcbncfctiknvurhjditvnhjuvkbrvenhvngirvjgi~~ Multiple linting rules test Nov 20, 2025

aemous changed the title ~~Multiple linting rules test~~ Support multiple linting rules Nov 20, 2025

Update base.py docs.

500117f

Remove leftover print from testing.

3f56a63

aemous requested a review from aws-sdk-python-automation November 20, 2025 16:25

aemous marked this pull request as ready for review November 20, 2025 17:01

kdaily requested changes Dec 4, 2025

View reviewed changes

kdaily reviewed Dec 9, 2025

View reviewed changes

	input("\nApply this fix? [y]es, [n]o, [u]pdate all, [s]ave and exit, [q]uit: ")
	input("\nApply this fix? [y] yes, [n] no, [u] update all, [s] save and exit, [q] quit: ")

	print("Quit without saving.")
	print("Quitting without saving.")

	print(f"Fixed script written to: {output_path}")
	print(f"Modified script written to: {output_path}")

Support multiple linting rules #9859

Are you sure you want to change the base?

Support multiple linting rules #9859

Conversation

aemous commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aemous commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdaily left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aemous commented Nov 20, 2025 •

edited

Loading

codecov-commenter commented Nov 20, 2025 •

edited

Loading