[Enhancement]: Handle GH token refresh inside runtime #6632

malhotra5 · 2025-02-06T01:31:00Z

End-user friendly description of the problem this fixes or functionality that this introduces

Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

Give a summary of what the PR does, explaining any non-trivial design decisions

The GH token can expire inside the runtime (either during the session or when returning to one after a long time). This PR does the following for remote runtimes only

Checks incoming CmdRunActions that are attempting to use $GITHUB_TOKEN
Before executing the incoming command, it will first perform export GITHUB_TOKEN={token} where the token is fetched from GitHubService (this diff explains this exact behavior)

This ensures that the latest Github token has been exported to the env BEFORE any Github API request is executed by the agent

Link of any specific issues this addresses

To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:79bc289-nikolaik   --name openhands-app-79bc289   docker.all-hands.dev/all-hands-ai/openhands:79bc289

xingyaoww

LGTM!

openhands/server/session/agent_session.py

enyst

Thanks for working on this, @malhotra5 !

I'll defer to Xingyao on the actual implementation of the refresh, I just would like to stop a moment here to look at the architecture of this: this creates a dependency from the runtime to the server.

Can we do this differently?

enyst · 2025-02-06T13:28:27Z

To elaborate, we have contributions to different runtimes, an e2b runtime, a modal runtime. Those implementations depend on the abstract runtime interface, and they don't depend at all on the existence of a Web UI, and surely not on our particular Web UI.

That's for a good reason, I believe, the runtime doesn't ever need to know it will be used from a web UI.

To put it differently, openhands is not a web application. It has a web interface, it also has a cli interface, the GitHub resolver is also an interface, and it can have others. I think of the server component as a fairly standalone component. It could in theory even be outside openhands, kinda like the resolver has been, and things would still work. WDYT?

openhands/runtime/base.py

openhands/server/config/config_init.py

enyst · 2025-02-06T16:34:36Z

Unrelated to the above concern:

it's not fully clear to me what is it that this PR needs to solve, and this PR didn't solve: Fix issue #6531: [Bug]: GITHUB_TOKEN would missing when the runtime resume #6533 .

I mean, if the token is invalid, then the agent will receive an error, right? It did for me when I deleted my tokens on github, and that's okay, the LLM will eventually ask for a valid token.

malhotra5 · 2025-02-06T18:09:25Z

To put it differently, openhands is not a web application. It has a web interface, it also has a cli interface, the GitHub resolver is also an interface, and it can have others. I think of the server component as a fairly standalone component. It could in theory even be outside openhands, kinda like the resolver has been, and things would still work. WDYT?

Ah this is a really good point! I agree it should be done differently. On a separate note, can we consider the GitHubService part of the server? Technically speaking it was made to be an entirely separate component that the server, runtime, or any service that requires access to Github APIs could use; another way of thinking about it is its meant to be a better replacement for PyGithub package - so maybe just relocating it and removing dependency on the SettingsStore would make sense?

It is, however, mainly used in the server at the moment so maybe it is a server component after all - regardless I agree this PR should probably be implemented differently, thanks a bunch for the thoughtful feedback!

enyst · 2025-02-06T23:25:05Z

Unrelated to the above concern:

it's not fully clear to me what is it that this PR needs to solve, and this PR didn't solve: Fix issue #6531: [Bug]: GITHUB_TOKEN would missing when the runtime resume #6533 .

I mean, if the token is invalid, then the agent will receive an error, right? It did for me when I deleted my tokens on github, and that's okay, the LLM will eventually ask for a valid token.

Thanks to Rohit and Xingyao for the clarifications on this question: this solves a saas-only problem, where the GITHUB_TOKEN sent to the runtime from the hosted site is not the token set in the user account, but a temporary user access token as defined here. Stopping the agent would be a pretty bad experience, and it may be worse to have the LLM ask for a token so, so very often.

malhotra5 · 2025-02-07T19:28:16Z

PR has been updated so that GithubService is a standalone component responsible for interacting with the Github API

GithubService is used within in runtime or server components as needed.

enyst · 2025-02-08T01:24:16Z

openhands/services/github/github_service.py


-    def __init__(self, user_id: str | None):
+    def __init__(self, user_id: str | None = None, token: str | None = None):


A little curious thing: we have made the token stored in Settings a SecretStr, but in this diff I see it treated as a string.

I think we should fix the type hints, but I also expected to see some code picking the actual value from SecretStr when needed (get_secret_value), and I don't see any in the diff. Does the PR work as expected?🤔

Yeah tested the PR and it works; the secret string is extracted and set as a string in the middleware

Because GithubService is meant to be a standalone component, I feel it makes sense to keep the str typing as is.

Idk, the point of SecretStr is to pass it around as much as possible as a SecretStr, until we really need the value. Otherwise, it can be accidentally dumped in logs or prints etc. 🤔 I might be missing something, sorry but could you please clarify why do the service and the middleware need to store a clear text?

The idea behind initializing GithubService with a str token is to enforce separation of concerns. The caller (in this case the server component) should be responsible for decoding the SecretStr into a str when initializing a GithubService. This way, GithubService can strictly be a general purpose/single use object (very similar to how PyGithub library behaves).

However, it seems we have a different problem as well - the server component uses BOTH SecretStr and str representations for the Github token. For instance ConversationInitData extends the Settings class, allowing for a str based token. Then, the str representation is plumbed all way through to the runtime, starting from _create_new_conversation until it gets set in the runtime env

It seems the way to rectify this would be to enforce SecretStr is used in runtime as well - this is likely very necessary but beyond the scope of this PR

On a separate note, the reason the middleware sets the token as str in the request state is because all the /github endpoints need to utilize the token. So ALL the endpoints need to fetch the SecretStr from Settings and decode it anyways, and the middleware helps deduplicate that logic

From what it seems, SecretStr has only really been standardized completely for LLM_API_KEY, but not GITHUB_TOKEN -> EDIT: if this were to be standardized across our entire ecosystem (runtime included) I think it would make sense for GithubService to use a SecretStr instead of str on initialization

@enyst I feel the concerns are valid, we should be using SecretStr everywhere (as well as the GithubService class) and only be converting to str at the last possible moment when required

I'm working on implementing those refactors in #6660; after which, hopefully it unblocks this

enyst · 2025-02-08T01:25:02Z

openhands/runtime/base.py

+                    gh_client = GithubServiceImpl(user_id=self.user_id)
+                    token = await gh_client.get_latest_token()
+                    if token:
+                        export_cmd = CmdRunAction(f"export GITHUB_TOKEN='{token}'")


For example here, does this get and export a str or a SecretStr?

This will get a str value

openhands/runtime/base.py

malhotra5 added 4 commits February 5, 2025 20:17

fix circular import issues

59f5a4c

get token from gh service

d36aa29

plumb user_id and get latest token when needed

4191310

retry build

aef8ba9

malhotra5 marked this pull request as draft February 6, 2025 01:42

malhotra5 added 5 commits February 5, 2025 21:14

fix build err

4560f86

relocate filestore init

006a692

pass user_id to remote runtime init

32ffc00

debug logs + fix async err

639eb03

remove debug logs

a2e4f5b

malhotra5 changed the title ~~[Experimental]: Handle GH token refresh inside runtime~~ [Enhancement]: Handle GH token refresh inside runtime Feb 6, 2025

malhotra5 marked this pull request as ready for review February 6, 2025 03:54

xingyaoww approved these changes Feb 6, 2025

View reviewed changes

openhands/server/session/agent_session.py Outdated Show resolved Hide resolved

enyst requested changes Feb 6, 2025

View reviewed changes

enyst reviewed Feb 6, 2025

View reviewed changes

openhands/runtime/base.py Outdated Show resolved Hide resolved

enyst reviewed Feb 6, 2025

View reviewed changes

openhands/server/config/config_init.py Outdated Show resolved Hide resolved

malhotra5 added 10 commits February 7, 2025 11:48

make gh service standalone

021e64c

remove server_config dependency from gh service

289870d

move gh types from server to services

8fefdff

rm gh service cls route from server

c4b0955

use impl from services

944dc17

rm refresh param + fetch token from ghservice

9345dc6

revert container changes

2762997

use kwargs

f9e836c

rm config init

bb8dfbd

revert shared.py changes

82bb4e2

move service types to service folder

dda8ecd

malhotra5 requested a review from enyst February 7, 2025 19:28

enyst reviewed Feb 8, 2025

View reviewed changes

openhands/runtime/base.py Outdated Show resolved Hide resolved

cmp user_id first

79bc289

malhotra5 mentioned this pull request Feb 8, 2025

[Bug fix]: Standardize SecretStr use #6660

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement]: Handle GH token refresh inside runtime #6632

[Enhancement]: Handle GH token refresh inside runtime #6632

malhotra5 commented Feb 6, 2025 •

edited by github-actions bot

Loading

xingyaoww left a comment

enyst left a comment

enyst commented Feb 6, 2025 •

edited

Loading

enyst commented Feb 6, 2025

malhotra5 commented Feb 6, 2025 •

edited

Loading

enyst commented Feb 6, 2025

malhotra5 commented Feb 7, 2025

enyst Feb 8, 2025

malhotra5 Feb 8, 2025 •

edited

Loading

enyst Feb 8, 2025

malhotra5 Feb 8, 2025 •

edited

Loading

malhotra5 Feb 8, 2025

enyst Feb 8, 2025

malhotra5 Feb 8, 2025


		def __init__(self, user_id: str \| None):
		def __init__(self, user_id: str \| None = None, token: str \| None = None):

[Enhancement]: Handle GH token refresh inside runtime #6632

Are you sure you want to change the base?

[Enhancement]: Handle GH token refresh inside runtime #6632

Conversation

malhotra5 commented Feb 6, 2025 • edited by github-actions bot Loading

xingyaoww left a comment

Choose a reason for hiding this comment

enyst left a comment

Choose a reason for hiding this comment

enyst commented Feb 6, 2025 • edited Loading

enyst commented Feb 6, 2025

malhotra5 commented Feb 6, 2025 • edited Loading

enyst commented Feb 6, 2025

malhotra5 commented Feb 7, 2025

enyst Feb 8, 2025

Choose a reason for hiding this comment

malhotra5 Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

enyst Feb 8, 2025

Choose a reason for hiding this comment

malhotra5 Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

malhotra5 Feb 8, 2025

Choose a reason for hiding this comment

enyst Feb 8, 2025

Choose a reason for hiding this comment

malhotra5 Feb 8, 2025

Choose a reason for hiding this comment

malhotra5 commented Feb 6, 2025 •

edited by github-actions bot

Loading

enyst commented Feb 6, 2025 •

edited

Loading

malhotra5 commented Feb 6, 2025 •

edited

Loading

malhotra5 Feb 8, 2025 •

edited

Loading

malhotra5 Feb 8, 2025 •

edited

Loading