Skip to content

fix(git): report a detached HEAD from git_checkout - #4805

Open
CryoThrust wants to merge 2 commits into
modelcontextprotocol:mainfrom
CryoThrust:fix/git-checkout-detached
Open

CryoThrust wants to merge 2 commits into
modelcontextprotocol:mainfrom
CryoThrust:fix/git-checkout-detached

Conversation

@CryoThrust

Copy link
Copy Markdown
Contributor

Summary

git_checkout validates its argument with rev_parse, which resolves any revision — a sha, a tag, HEAD~1, a remote-tracking ref, a full refs/... name. For everything that is not a branch, git checkout detaches HEAD, but the tool still returned Switched to branch '<name>'.

Report the resulting state instead:

repo.git.checkout(branch_name)
if repo.head.is_detached:
    return f"HEAD is now detached at {repo.head.commit.hexsha[:7]}"
return f"Switched to branch '{repo.active_branch.name}'"

The reply is the only thing the calling model sees — content is a single text block, structuredContent is null and isError is false — so a wrong success sentence is not corrected downstream. The branch wording is now taken from active_branch too, so it cannot disagree with the ref that was actually checked out.

Rejecting non-branch revisions would also close the issue, but it removes a capability some callers use deliberately (checking out a tag or a remote ref), so I kept it and made the message truthful instead.

Tests

Added three cases to src/git/tests/test_server.py:

  • a sha detaches HEAD and the reply says so, without "Switched to branch"
  • a tag behaves the same
  • a real branch still returns Switched to branch '<name>' and leaves HEAD attached

On main the first two fail (assert 'detached' in "Switched to branch 'v1'").

Validation: uv run pytest in src/git — 46 passed; pyright — 0 errors.

Closes #4804.

git_checkout validated branch_name with rev_parse, which resolves any revision
(a sha, tag, HEAD~1, or a remote-tracking ref), then always replied
"Switched to branch '<name>'". For anything that is not a branch git actually
detached HEAD, so the tool handed the model a success sentence over a state
where new commits belong to no branch.

Report the resulting state instead: say HEAD is detached, with the short sha,
when it is; keep the branch wording when a branch was really checked out.

@yangshuaiyufalv yangshuaiyufalv left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[MCP demo] 测试评审(只读组织,预期权限错误)。

@buyan201430-code

Copy link
Copy Markdown

Reviewed as the reporter of #4804. I ran the head (d37bd1e) locally: pytest in src/git gives 46 passed, matching your note.

The fix also covers the three rows of the #4804 table that the new tests don't exercise. Checked each against this branch:

HEAD~1              -> HEAD is now detached at <sha>   (head.is_detached: True)
refs/heads/feature  -> HEAD is now detached at <sha>   (head.is_detached: True)
origin/main         -> HEAD is now detached at <sha>   (head.is_detached: True)

A name that is both a branch and a tag (dup) still attaches to the branch and returns Switched to branch 'dup', so taking the wording from active_branch holds there too.

One non-blocking suggestion: origin/main and refs/heads/<branch> are the two an agent is most likely to send, and they are the only ones not pinned by a test. This case passes on this branch and fails on main (3 failed with the old return line); ruff format --check is clean:

@pytest.mark.parametrize("revision", ["HEAD~1", "refs/heads/feature", "origin/main"])
def test_git_checkout_non_branch_revisions_report_detached_head(
    test_repository, tmp_path, revision
):
    test_repository.git.branch("feature")
    test_repository.index.commit("second commit")
    git.Repo.init(tmp_path / "remote.git", bare=True)
    test_repository.create_remote("origin", str(tmp_path / "remote.git"))
    test_repository.git.push("origin", "HEAD:refs/heads/main")

    result = git_checkout(test_repository, revision)

    assert test_repository.head.is_detached
    assert result.startswith("HEAD is now detached at ")

Otherwise this looks right to me — keeping non-branch checkouts and making the reply truthful is a better trade than rejecting them.

…a branch

Adds the parametrized case suggested in review: HEAD~1, refs/heads/feature and
origin/main are the revisions a caller is most likely to send and none of them
is a branch, so none may be reported as a branch switch. All three fail against
the old unconditional return and pass with the fix.
@CryoThrust

Copy link
Copy Markdown
Contributor Author

Thanks for running it against the head and for checking the rows the tests didn't cover — that's exactly the gap I left, so I took the suggestion verbatim in 723b128.

test_git_checkout_non_branch_revisions_report_detached_head is parametrized over HEAD~1, refs/heads/feature and origin/main, with the bare-remote scaffolding from your snippet. Confirmed the signal both ways: with the old unconditional return all three fail, and with the fix they pass and assert result.startswith("HEAD is now detached at ") rather than just the is_detached flag.

src/git: 49 passed   (46 before, +3 from the parametrization)

Your point about the branch-and-tag name resolving to the branch is a good one to have on record too — that's the case where taking the wording from active_branch matters most, since echoing the argument would have been ambiguous there.

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.

git_checkout reports "Switched to branch 'X'" when the checkout actually detached HEAD

3 participants