Skip to content

Fix auto-triage user_confirm accepting unrecognized keys as default answer#63579

Merged
choo121600 merged 1 commit intoapache:mainfrom
choo121600:fix-user-unrecognized-keys
Mar 16, 2026
Merged

Fix auto-triage user_confirm accepting unrecognized keys as default answer#63579
choo121600 merged 1 commit intoapache:mainfrom
choo121600:fix-user-unrecognized-keys

Conversation

@choo121600
Copy link
Member

@choo121600 choo121600 commented Mar 14, 2026

During breeze pr auto-triage, when approving workflow runs, user_confirm silently treats unrecognized keys as the default answer.

Since the approval prompt defaults to YES (when no sensitive changes are detected), pressing a wrong key unintentionally approves workflow runs.

Change user_confirm to re-prompt on unrecognized input instead of falling through to the default.

Before

image

After

Screenshot 2026-03-14 at 3 26 05 PM
Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
    claude opus 4.6

  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, small nit

@Vamsi-klu
Copy link
Contributor

Hey @choo121600, good find — accidentally approving workflow runs by pressing a random key is definitely a safety concern for the triage tool. The while True loop approach is the right fix and it matches how prompt_triage_action in the same file already handles this.

One thing to watch out for: there's a merge conflict with current main. Commit 7cea0b8 landed a refactor that replaced get_console().print() with console_print() across the codebase. You'll need to rebase and update those calls.

A couple of other thoughts:

  • Tests would help here. There are no existing tests for user_confirm, and since the behavioral change is significant (re-prompt instead of silent default), even a couple of targeted tests would go a long way. For example, mocking _read_char to return "x" then "y" and asserting Answer.YES would prove unrecognized keys trigger a re-prompt. Same for multi-byte escape sequences.

  • Reviewer @bugraoz93 suggested a flag approach instead of while True — I think while True is the right call here since it's idiomatic Python for this pattern and matches the existing code in the same file. Might be worth responding to that comment to close the loop.

The logic itself looks correct to me across all the edge cases I checked (Enter with/without default, case sensitivity, quit_allowed=False, special characters). Clean and well-scoped fix.

@bugraoz93
Copy link
Contributor

bugraoz93 commented Mar 14, 2026

Hey @choo121600, good find — accidentally approving workflow runs by pressing a random key is definitely a safety concern for the triage tool. The while True loop approach is the right fix and it matches how prompt_triage_action in the same file already handles this.

One thing to watch out for: there's a merge conflict with current main. Commit 7cea0b8 landed a refactor that replaced get_console().print() with console_print() across the codebase. You'll need to rebase and update those calls.

A couple of other thoughts:

  • Tests would help here. There are no existing tests for user_confirm, and since the behavioral change is significant (re-prompt instead of silent default), even a couple of targeted tests would go a long way. For example, mocking _read_char to return "x" then "y" and asserting Answer.YES would prove unrecognized keys trigger a re-prompt. Same for multi-byte escape sequences.

  • Reviewer @bugraoz93 suggested a flag approach instead of while True — I think while True is the right call here since it's idiomatic Python for this pattern and matches the existing code in the same file. Might be worth responding to that comment to close the loop.

The logic itself looks correct to me across all the edge cases I checked (Enter with/without default, case sensitivity, quit_allowed=False, special characters). Clean and well-scoped fix.

I don't know why AI goes with while true which generally hard to maintain because it is always error prune. You always need to break correctly and handle exceptions properly. Otherwise process can do busy waits that can cause harder debugging. You wait process to throw exceptions but waiting 15 minutes on while true because one if statement wasn't properly set. Maybe Python makes some parts more foregiving because it wraps one additional layer on top of c-assembly-hex interpretion. This doesn't directly make while true is a good approach in general.

As your entire message looks fully AI generated, so I assume AI suggested the approach without you spending any time on reviewing the actual code to easily say it is the best approach to use while true while I am feeling spending more time explaining AI my reasoning :D

@Vamsi-klu
Copy link
Contributor

Vamsi-klu commented Mar 15, 2026

Hey @bugraoz93, appreciate you taking the time to share your thoughts, that's a fair concern in general about while True. I've run into cases myself where a missed exit condition in a loop caused headaches, especially in background services or networking code, where you can end up with a silent busy-wait.

That said, I think the context here makes it a pretty safe choice:

  • _read_char() is a blocking call that waits on TTY input (or inputimeout in CI), so each iteration either returns immediately on valid input or blocks until the user presses something. There's no spinning or CPU burn happening.

  • prompt_triage_action right below in the same file uses while True for the exact same re-prompt-on-invalid-key pattern. Having two different patterns for the same behavior in the same file would be a bit confusing for anyone reading the code later.

  • I did think about what a flag-based version would look like, something like valid = False; while not valid: but since every recognized key path does a return, the flag would never actually flip to True. It'd just sit there unused. You could restructure to store the result and break, but that adds a variable and a branch for no real clarity gain here.

And yeah, fair enough on the AI comment, I do use AI tools as part of my workflow and polishing the content(eg: comments, asking questions to improve grammar before publishing, I think most of us do these days to some degree). But I did read through the file and trace the code paths before commenting. Things like the merge conflict with console_print() and pointing to prompt_triage_action as precedent came from actually looking at the code, not from a prompt. Happy to dig into any specific point further if you think there's something I missed!

@bugraoz93
Copy link
Contributor

Hey @bugraoz93, appreciate you taking the time to share your thoughts, that's a fair concern in general about while True. I've run into cases myself where a missed exit condition in a loop caused headaches, especially in background services or networking code, where you can end up with a silent busy-wait.

That said, I think the context here makes it a pretty safe choice:

  • _read_char() is a blocking call that waits on TTY input (or inputimeout in CI), so each iteration either returns immediately on valid input or blocks until the user presses something. There's no spinning or CPU burn happening.

  • prompt_triage_action right below in the same file uses while True for the exact same re-prompt-on-invalid-key pattern. Having two different patterns for the same behavior in the same file would be a bit confusing for anyone reading the code later.

  • I did think about what a flag-based version would look like, something like valid = False; while not valid: but since every recognized key path does a return, the flag would never actually flip to True. It'd just sit there unused. You could restructure to store the result and break, but that adds a variable and a branch for no real clarity gain here.

And yeah, fair enough on the AI comment, I do use AI tools as part of my workflow and polishing the content(eg: comments, asking questions to improve grammar before publishing, I think most of us do these days to some degree). But I did read through the file and trace the code paths before commenting. Things like the merge conflict with console_print() and pointing to prompt_triage_action as precedent came from actually looking at the code, not from a prompt. Happy to dig into any specific point further if you think there's something I missed!

The current state of the code can work and there could be no problem which I wasn't stated that the current code is flaky nor have incomplete checks. My observation is more on future, lots of different people touching each of the code pieces. This doesn't mean we always see all the exits clearly via just looking at the code you need to expand entire method to ensure all new exits align with all others. That was my concern where we missed an if in the future, not running code in main. One of the thing generally breeze commands end up in our CI where then these commands also outside of your local usage territory that can do busy waits and cost hours of GH Action hours of processing until someone goes and check if Python won't be forgiving in certain cases.

I would say if you checked the code yourself, the best way to express it not tagging me to tell the PR owners to close my comment, rather answer in the comment, share your thoughts then we can discuss the solution while all of us can improve while Airflow is improving. Even answering to the comment with Claude saying this, what do you think? Or I think this could be useful because of X. There is no harm on this and this is human interaction after all to make things better which I am having lots of fun discussing these technical details. I am and the Airflow not against usage of AI but if you think of you spending most of your free time to the community to make Airflow better while I am every now and then just copy pasting entire thing from AI without even talking with you but directing PR owner to close the comment. That is not a human behavior at all. We use AI but if we lose our behavior over it, it looks to me fully AI generation or automation rather you spent a minute. We should leverage as tools and it should not change the way how we express our thoughts as a community. To each other, not as a 3rd person. That is rude.

I stated as a small nit not a blocker already gives the PR owner to either discuss with me or close the thread with thoughts. This was pretty clear while if AI wasn't generated all, you would also skip and left the part to PR author if you don't have objection to it. If you have lets discuss in the threads as humans rather pointing the ideas are wrong from AI context (with AI help is always okay until it do everything for you which makes you losing the human behavior).

@choo121600 choo121600 closed this Mar 16, 2026
@choo121600 choo121600 force-pushed the fix-user-unrecognized-keys branch from 40a5b99 to 998e722 Compare March 16, 2026 03:58
@choo121600 choo121600 reopened this Mar 16, 2026
@choo121600 choo121600 force-pushed the fix-user-unrecognized-keys branch from e8dd7cd to 2c5916f Compare March 16, 2026 07:11
@choo121600
Copy link
Member Author

I think many of us contribute here because we enjoy discussing different ideas and improving Airflow together as a community.

From my perspective, both points raised here are valuable — @Vamsi-klu provided helpful context on why the while True pattern can be reasonable in this specific case, and @bugraoz93 raised a valid broader concern around long-term maintainability.

In situations like this, continuing the discussion in the comment thread can sometimes help keep the context clearer and make the communication flow more naturally.

Discussions like this are one of the things that make open source collaboration valuable 😉

@choo121600 choo121600 merged commit 3155c29 into apache:main Mar 16, 2026
255 of 257 checks passed
@github-actions
Copy link

Backport failed to create: v3-1-test. View the failure log Run details

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
v3-1-test Commit Link

You can attempt to backport this manually by running:

cherry_picker 3155c29 v3-1-test

This should apply the commit to the v3-1-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.

After you have resolved the conflicts, you can continue the backport process by running:

cherry_picker --continue

If you don't have cherry-picker installed, see the installation guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants