OpenHands / OpenHands/software-agent-sdk
CriticMixin always calls evaluate() with git_patch=None, so critics grade the transcript instead of the diff
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 1.1k
- Forks
- 539
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 137
Description
Summary
CriticBase.evaluate() is declared as
def evaluate(self, events: Sequence["LLMConvertibleEvent"], git_patch: str | None = None) -> CriticResult
but the only in-SDK caller, CriticMixin._evaluate_with_critic, always passes git_patch=None:
# Evaluate without git_patch for now
result = self.critic.evaluate(events=events, git_patch=None)
So the git_patch parameter is unreachable in normal operation, and any critic that wants to judge what the agent actually changed has to fall back to reading the conversation transcript.
Why this matters in practice
We run a custom CriticBase subclass that scores whether an agent's work genuinely satisfied its task. With git_patch=None it grades the transcript, and that produces false negatives that depend on which tools the agent happened to use:
- Agent uses
terminalwithcat/heredoc, so file contents appear in the transcript, and the critic scores correctly. - Agent uses
file_editorfor the identical, correct change, so contents never enter the transcript, the critic sees no evidence the work was done, and scores it 0.0.
Measured on agent-server 1.42.1: the same correct task scored 0.000 on the first pass and only reached 1.000 after an IterativeRefinementConfig retry. That is a wasted refinement loop and a wasted LLM call on work that was already right.
EmptyPatchCritic and AgentFinishedCritic do not hit this because neither looks at content, and APIBasedCritic targets a separate classification service, so the gap is invisible unless you implement your own critic.
Suggested fix
Compute the patch in _evaluate_with_critic and pass it through when the conversation workspace is inside a git work tree. openhands.sdk.git already exposes the read helpers (git_changes, git_diff), so most of the plumbing exists.
Two edge cases worth handling, both of which we hit:
- No
HEAD.LocalWorkspacegit-inits a fresh workspace and never commits, sogit diff --cached HEADexits 128 and yields nothing. Diffing against the empty tree (git hash-object -t tree /dev/null) works there. - Do not mutate the user index. Staging in order to compute a diff will clobber a real index. Pointing
GIT_INDEX_FILEat a throwaway file avoids touching it.
A pre-existing dirty working tree also means a naive diff includes changes the agent never made, so scoping the patch to the agent's own edits, or at least documenting that caveat, would help.
Alternative
If passing the patch by default is unwanted for performance reasons, making it opt-in via a field on CriticBase would still be better than a parameter no caller can reach.
Happy to open a PR if the approach above sounds right.
Environment
openhands-sdk/openhands-agent-server1.42.1- Custom
CriticBasesubclass loaded via--extra-python-path/--import-modules
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read openhands-sdk/openhands/sdk/agent/critic_mixin.py, especially _evaluate_with_critic, then inspect the read helpers in openhands.sdk.git. Verify how patches behave for a fresh repository without HEAD and with an existing user index. Done means CriticBase.evaluate() receives the relevant agent patch without mutating the user's index.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- git, python
- Domain
- ai, devtools
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100