Skip to content

Conversation

@daniel-de-vera
Copy link
Contributor

This PR includes the following changes:

  • Improves context handling across various CLI commands by properly linking contexts to signals, making undo operations more reliable.
  • Fixes the auth token refresh issue reported in [CLI] Override exiting with 401 mid session #228.
  • Normalizes and aligns code between the override and traffic record commands.

Brief demo (has audio):

screen-capture.43.webm

@gitar-bot
Copy link

gitar-bot bot commented Oct 23, 2025

Gitar analyzes/fixes CI failures, runs prompts as workflows and addresses comments starting with Gitar.

⚙️ Options:

  • Automatically accept changes

📝 Submit Feedback

Copy link
Member

@scott-cotton scott-cotton left a comment

Choose a reason for hiding this comment

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

LGTM

But maybe some of those context.Background calls on cleanup should somehow take the overall command invocation context instead.

Unless the extra plumbing complexity doesn't seem worth it...

@daniel-de-vera
Copy link
Contributor Author

But maybe some of those context.Background calls on cleanup should somehow take the overall command invocation context instead.

Unless the extra plumbing complexity doesn't seem worth it...

The problem is that in some cases (e.g. on ctr-c), the context may be already cancelled.

@scott-cotton
Copy link
Member

But maybe some of those context.Background calls on cleanup should somehow take the overall command invocation context instead.
Unless the extra plumbing complexity doesn't seem worth it...

The problem is that in some cases (e.g. on ctr-c), the context may be already cancelled.

I think the problem is also that there is a containing context whose cancelation is not respected. One could have a context that does not cancel on control-C as the root, for example.

@daniel-de-vera
Copy link
Contributor Author

But maybe some of those context.Background calls on cleanup should somehow take the overall command invocation context instead.
Unless the extra plumbing complexity doesn't seem worth it...

The problem is that in some cases (e.g. on ctr-c), the context may be already cancelled.

I think the problem is also that there is a containing context whose cancelation is not respected. One could have a context that does not cancel on control-C as the root, for example.

Ok, let me see what I can do.

@daniel-de-vera
Copy link
Contributor Author

@scott-cotton, PTAL:

In bc1c763 I implemented:

I think the problem is also that there is a containing context whose cancelation is not respected. One could have a context that does not cancel on control-C as the root, for example.

In 9d50091 a minor file reorg in override package (to make it uniform with the rest).

@scott-cotton
Copy link
Member

@scott-cotton, PTAL:

In bc1c763 I implemented:

I think the problem is also that there is a containing context whose cancelation is not respected. One could have a context that does not cancel on control-C as the root, for example.

In 9d50091 a minor file reorg in override package (to make it uniform with the rest).

LGTM

@daniel-de-vera daniel-de-vera merged commit b0f8053 into main Oct 24, 2025
@daniel-de-vera daniel-de-vera deleted the context-handling-improvements branch October 24, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants