[refactor]: call load_openhands_config in resolver

[kotauchisunsun]

…r and move sandbox setup to a dedicated function

• This change is worth documenting at https://docs.all-hands.dev/
• Include this change in the Release Notes. If checked, you must provide an end-user friendly description for your change below

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

---

Summarize what the PR does, explaining any non-trivial design decisions.

To decouple the command-line parsing logic from the processing logic, I’ve refactored setup_sandbox_config from a class method into a standalone function.

To keep the diff manageable, I’m temporarily passing both args and sandbox_config to the IssueResolver constructor. In a future update, the constructor will no longer depend on args, and the necessary values will be passed as individual variables instead.

---

Link of any specific issues this addresses:

#8516
#8619

[enyst]

To keep the diff manageable, I’m temporarily passing both args and sandbox_config to the IssueResolver constructor. In a future update, the constructor will no longer depend on args, and the necessary values will be passed as individual variables instead.

Hey @kotauchisunsun ! Just to understand where we're heading: IMHO in the future we should not construct sandbox_config in the resolver at all. The resolver, just like other entry points in the application, should use load_app_config to get the current values, then override them with its args.

At least, that's my thought about the way forward here. What do you think of this goal? Do you think this refactoring is heading there or do you have a different take?

[kotauchisunsun]

@enyst
I also believe that AppConfig should be created inside load_app_config. As I understand it, this was discussed previously in the following PR:
#8414

However, I'm currently facing several issues around this and am unsure about the best solution.

The main problem is that AppConfig is currently instantiated inside process_issue:
https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L380

I'm considering whether this should be replaced with load_app_config.
But if I go down that route, I have a concern that mocking load_app_config in test code will become cumbersome.
Another question is whether it's acceptable for IssueResolver to override values inside the config returned by load_app_config, specifically those related to sandbox_config. I'm personally not a fan of modifying public fields from outside the class, though I realize that might come down to personal preference.

I'm starting to think that if we do override sandbox_config, perhaps it should be done not in IssueResolver but rather in resolve_issue.py (or possibly somewhere else—I'm open to other options). That would mean calling load_app_config in resolve_issue.py, and injecting the resulting AppConfig into the IssueResolver constructor (which would also improve testability).

However, doing so would require generating workspace_base inside resolve_issue.py, which is currently handled in AppConfig:

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L387-L388

The problem is that generating workspace_base depends on both issue_handler and the issue, which makes things a bit tricky.
One idea is to pass AppConfig as an argument to process_issue, but resolve_issue.py currently only calls the resolve_issue function:
https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/resolve_issue.py#L125

That leads me to wonder whether it makes sense to add AppConfig just to pass it through for the sake of process_issue, especially since the same issue of needing to precompute workspace_base would arise.

Do you have any suggestions or thoughts on a clean way to resolve this?

That's why I've been working on refactoring and breaking things up incrementally—hoping that by reducing visible complexity (like class variables), a clearer solution will eventually emerge.
So for now, I've focused on separating command-line validation logic from the issue-processing logic, as it's likely a safe step.

[enyst]

A few thoughts, please do let me know if I'm missing something.

Another question is whether it's acceptable for IssueResolver to override values inside the config returned by load_app_config, specifically those related to sandbox_config. I'm personally not a fan of modifying public fields from outside the class, though I realize that might come down to personal preference.

I hear you, we even used to have an unwritten rule that the config attributes were not overridden from outside config module. In effect we were treating them as immutable -ish, even though they weren't. I'm pretty sure that's long gone though, now we are doing it. Hopefully only during initialization phases... but I'm not totally sure anymore, I may have lost track on that.

So we have a choice here, do it somewhere in the resolver, for the resolver overrides, or make sure to do it only in config module (if not the class itself).

The second option is to have a standard way to do overrides from various entry points (such as a resolver override method in config module, or a factory for overriding classes, e.g. Settings in the web server entry point is a specific override class). Maybe we can give some thought to that.

Or maybe we can do 1), as long as we're doing it all in a single place. So that we can refactor it later easily and move to 2).

The problem is that generating workspace_base depends on both issue_handler and the issue, which makes things a bit tricky.

Please correct me if I'm not seeing this right, it seems to me that it depends only on things we have in the IssueResolver constructor already. So maybe we could do load_app_config() + resolver-specific override as early as the constructor?

Thank you for the discussion and the work on this! It's so important to get it cleaned up, so that we reduce risks of the code fragility and maybe be able to move forward easier.

[kotauchisunsun]

In effect we were treating them as immutable -ish, even though they weren't. I'm pretty sure that's long gone though, now we are doing it.

Yeah, that often happens as development progresses. Software development is hard. 😅

Or maybe we can do 1), as long as we're doing it all in a single place. So that we can refactor it later easily and move to 2).

I also think option 1 is fine, at least for now. As long as we avoid scattering the config initialization logic, I believe it's acceptable.

So maybe we could do load_app_config() + resolver-specific override as early as the constructor?

You're right that both issue and issue_handler can be injected at construction time. However, there’s a part of the logic in resolve_issue that uses issue_handler to build the issue:

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L506-L518

If we decide to inject AppConfig into the constructor, this part would also need to be moved into resolve_issue.py.
And if we go that route, we would likely also need to move the logic for parsing self.owner and self.repo into resolve_issue.py:

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L81-L84

Personally, I like this kind of change. But I also realize that since it touches a lot of areas, others might feel differently or have concerns about the size of the change.

There's also one more point I forgot to mention — the dependency on the workspace path.
Within IssueResolver’s runtime, it seems like operations are being performed under self.output_dir / workspace.
If we inject AppConfig into the IssueResolver constructor, the fact that the runtime should be initialized under the workspace subpath is no longer visible within the IssueResolver code itself.
However, functions like copy_code, initialize_runtime, and complete_runtime all assume the presence of a workspace path:

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L378

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L248

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/openhands/resolver/issue_resolver.py#L292

So, if we were to move the workspace_base logic into resolve_issue.py, that would effectively split the definition of the workspace path across two places — resolve_issue.py and IssueResolver — which could lead to future confusion. For example, if someone changes the workspace path in resolve_issue but not in IssueResolver, or vice versa, it might cause subtle bugs.

That’s why I was thinking that if we go down that route, we should also refactor things to unify the handling of the workspace path, potentially by moving it to a shared class-level variable. But of course, that would expand the scope of the change quite a bit. 😅

[enyst]

Thank you! OK, I think I follow, although please note that I'm not at the computer right now, so just from github:

Re: we could get the final config in resolve_issue and inject it in the constructor: you're right it's a larger change, we could perhaps instead:

• get the final config in the constructor, from args, and
• save it in IssueResolver if we want to read it later (self.config).
• then once that is all cleared (no writing to config elsewhere), we can maybe look to move it in the caller (resolve_issue).

Re: shutil.copytree
It would use self.config.workspace_base?

Re: cd /workspace
This path is workspace_mount_path_in_sandbox, and we hardcode the same value as its default value, so this isn't writing/overriding the value. We could maybe leave it alone (it's hardcoded since a long time), and I think people rarely change it, or we can change it to read self.config.workspace_mount_path_in_sandbox and cd to it. Not totally sure we need that at all, though...

[kotauchisunsun]

Glad we could have this discussion.

I now understand that we’ll need to approach this in stages, and that we probably don’t need to worry too much about the workspace path for now.

With that in mind, how should we modify this PR in the end?

[enyst]

Maybe we could go the other way around than this PR is doing: instead of taking args away from init, we could bring them all in init, and then try to call load_app_config(), and see what becomes redundant: for example, from line 101 to 125 (here) hopefully we can remove them all?

This depends on us not writing to config classes (or creating config classes) anywhere else in the resolver code. Not sure if we're there yet?

[kotauchisunsun]

I performed a step-by-step refactoring.
Within process_issue, AppConfig depends on workspace_base (via create_app_config).
workspace_base in turn depends on issue and issue_handler, and issue itself depends on issue_handler (via extract_issue).

I attempted to move the initialization of AppConfig into the constructor.
To do so, I first tried to initialize issue inside the constructor as well.
However, as can be seen in extract_issue, issue depends on issue_handler, which is a complex dependency.
issue_handler is created by generating an issue_handler_factory from args passed into the constructor, and then calling issue_handler_factory.create_handler, making the instantiation process somewhat complex (which I myself implemented, to be fair).
Therefore, while mocking this in tests is not impossible, it seems likely to result in convoluted test code.
Another factor contributing to the complexity of changes is that the implementation supports both GitHub and GitLab.

This issue doesn’t affect the existing test for resolve_issue in github/test_resolver.py.
The current code doesn’t instantiate the Issue in the constructor of IssueResolver; instead, it sidesteps the issue by externally replacing the issue_handler with a mock before calling resolve_issue in the test code. (Though I don't think this is a good practice.)

https://github.com/All-Hands-AI/OpenHands/blob/4b6f2aeb4dfc7084fb9c5344ec5d4a77d348f700/tests/unit/resolver/github/test_resolve_issues.py#L152

There are a few possible solutions.

First, the fundamental question is whether or not we should move the initialization of AppConfig into the constructor in this PR.
Another question is whether or not AppConfig should be injected into the constructor of IssueResolver in this PR.

If we consider moving the AppConfig initialization into the constructor, there are two options:

1. Remove args from the IssueResolver constructor to make it easier to inject mocks.
   (However, this would require changes to the test code.)

2. Use pytest’s patch functionality to mock extract_issue and skip its execution.
   (I don’t think this is a good idea, as I’m unsure whether a method called from the constructor can be forcibly overridden from the outside.)

If we consider injecting AppConfig into the constructor instead:

1. We would need to rewrite tests that currently rely on mocking issue_handler.

I believe mocking issue_handler in tests is necessary, but it seems like it would require a significant amount of change to the test code.

When I previously suggested passing issue as an argument to resolve_issue, this context was also part of my thinking.
While I hadn’t thought through everything in depth at the time, my intuition was that IssueResolver should be initialized using runtime information (e.g., whether it’s running on GitHub or GitLab), and that issue should be passed as an argument to resolve_issue.
This is why it felt slightly off to have issue information embedded in workspace_base.
That said, I also sensed that this might be necessary for parallel execution reasons.
On the other hand, if workspace_base can only be determined based on the issue, then perhaps it would be more reasonable (from another perspective) to initialize AppConfig inside process_issue, rather than in the constructor.

This ultimately may come down to a matter of design taste and the desire to keep this PR small and manageable.

How should we proceed with the changes in this PR?

[enyst]

How about, in IssueResolver.init(), something like (pseudocode)

# IssueResolver.__init__(args):
  # compute now and store for later read-only use
  self.workspace_base = args.output_dir + 'workspace' + args.issue_type + args.issue_number
  ...
  # afterwards
  factory = IssueHandlerFactory(
            owner=self.owner,
            repo=self.repo,
            ...
            issue_type=self.issue_type,
  )
...
def process_issue:
...
  # use self.workspace_base

Maybe that's missing some complexity? It seems that issue_type is here, issue_number is here.

Please feel free to shut it down if wrong!

[enyst]

This is shaping up great!

[enyst]

What do you think, @kotauchisunsun , is it ready to merge or close to? I looked at every commit, and you did the refactoring step by step carefully, and it looks safe to me, unlikely to break things. Also, the tests pass.
Do you give it a try locally, to see that it does work?

[kotauchisunsun]

@enyst
I think it's okay to merge.

I’ve confirmed the runtime behavior locally using the following command, and all tests pass:

poetry run pytest --cov=openhands/resolver/ ./tests/unit/resolver/

I also verified the behavior in my own sample repository:
kotauchisunsun/openhands-debug-demo#25

Here is the execution log:
https://github.com/kotauchisunsun/openhands-debug-demo/actions/runs/15375182896/job/43259119584

By adding the fix-me-experimental label, I was able to check out and run this branch:
https://github.com/kotauchisunsun/openhands-debug-demo/actions/runs/15375182896/workflow#L241

The pull request was successfully created:
kotauchisunsun/openhands-debug-demo#26

[enyst]

OK, thank you! I really really appreciate this work, @kotauchisunsun , it's something I think we've been missing for quite a while now. ❤️

The changes in this PR are around loading and overriding the app configuration. For a bit of history, the resolver used to be developed as an external script, it initialized itself first and then loaded openhands, basically, and that is why it has quite a number of duplicate code or things that can be simplified. Loading - and therefore, following - the app configuration, overriding where needed and otherwise using it normally, is something I think we needed to do ever since we moved the resolver into core openhands: it's deeper integration. That is, it runs the resolver as a normal part of the system, a well-behaved component 😅, as opposed to doing its own thing from outside -ish.

Cc: @malhotra5 @neubig