Repository navigation
Layer GIT_SSH_COMMAND on the inherited env when pushing a branch - #223
Conversation
push_branch_to_remote built env = {"GIT_SSH_COMMAND": ssh_command}, which
replaces the inherited environment rather than extending it. The one git
call that was told which key to authenticate with therefore ran with no
PATH to find ssh on, no HOME for known_hosts, and no SSH_AUTH_SOCK to
reach the agent -- naming a key was what broke the auth it set up. Same
env-replacement hazard #187 closed at the locale pin, same {**os.environ,
...} shape.
Pinned at the argv/env seam, mirroring the SSH_AUTH_SOCK assertion the
locale test already makes: the sentinel inherited variable must survive
alongside GIT_SSH_COMMAND.
Also aligns the two textual-stderr classifiers, as #211's review asked.
The fetch path guarded `e.stderr or ""`; branch creation read it
unguarded, so a failure git wrote nothing to stderr for raised a
TypeError naming neither the branch nor the cause instead of the
"failed to create branch" error every other failure raises.
Closes #212
Reviewer's GuideThis PR fixes environment handling when pushing branches with a specific SSH key and aligns stderr handling for git branch creation failures, adding tests and changelog entries for both behaviors. Sequence diagram for pushing a branch with a named ssh keysequenceDiagram
participant BranchManager
participant Subprocess as subprocess
participant Git as git
participant SSH as ssh
BranchManager->>BranchManager: push_branch_to_remote(base_repo_path, branch, remote, ssh_key_path)
alt ssh_key_path is provided
BranchManager->>BranchManager: ssh_command = "ssh -i ssh_key_path -o IdentitiesOnly=yes"
BranchManager->>BranchManager: env = {**os.environ, GIT_SSH_COMMAND: ssh_command}
else no ssh_key_path
BranchManager->>BranchManager: env = None
end
BranchManager->>Subprocess: subprocess.run(["git", "push", remote, branch], cwd=base_repo_path, env=env, check=True)
Subprocess->>Git: execute git push with env
Git->>SSH: run ssh using GIT_SSH_COMMAND and inherited PATH, HOME, SSH_AUTH_SOCK
SSH-->>Git: authenticate using specified key and agent
Git-->>Subprocess: push result
Subprocess-->>BranchManager: CompletedProcess or CalledProcessError
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
=======================================
Coverage 92.96% 92.96%
=======================================
Files 24 24
Lines 3679 3680 +1
=======================================
+ Hits 3420 3421 +1
Misses 259 259
🚀 New features to boost your workflow:
|
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two-axis review of #223 against ticket #212 at head 1f8d2b9. Preflight: all checks green, non-empty diff (3 files), MERGEABLE.
Standards
Verified clean: pixi run ci exit 0 from a fresh clone (pylint 10.00/10, 1748 passed, branch_manager.py at 100% coverage); CHANGELOG under ## [Unreleased] → Fixed; no hardcoded home paths; both tests match the file's sentence-naming and mock_run.call_args[1].get("env") idiom; no docs owed (no CLI surface changed).
- Non-blocking — Duplicated Code:
test_push_branch_to_remote_with_ssh_keyis now a strict subset of the new env test; it can only fail when the new one already has. (The builder declared keeping it deliberately — "deleting a passing test is not part of this slice" — which is sound; folding is a rider.) - Non-blocking — Shotgun Surgery (latent): env layering now hand-repeated at branch_manager.py:87/:190 + repo_manager.py:402, and the None-guard at branch_manager.py:94 + repo_manager.py:406, with dl.py:3083 stating the hazard a third way in prose. Two sites is thin for a
_git_env()helper; a third would earn it. - Non-blocking —
push_branch_to_remotestill formatse.stderrraw at :203, so a stderr-less push failure reads "Failed to push branch to remote: None" — the same complaint class this PR's changelog makes, one arm over. - Non-blocking — pre-existing, adjacent: :185 interpolates
ssh_key_pathinto the shell string unquoted; a path with a space breaks the command the fix protects. Declared out of scope by the builder with sound reasoning; worth its own ticket.
Spec
All ticket items verified with mutations reproduced. Nothing blocks.
- "
{**os.environ, ...}… is the shape that works" — :190 is exactly that; reverting the line fails the new test withKeyError: 'SSH_AUTH_SOCK', the ticket's stated failure. Restored. - "a test at the argv/env seam … mirror test_already_exists_answer_survives_a_translated_git" — same seam, same sentinel shape, adds PATH; right function (push, not creation).
- "worth aligning in the same pass" (None-guard) — both sites now
e.stderr or ""; mutating branch_manager's back is caught (TypeError, 1 failed). - Non-blocking — the other half of the aligned pair (repo_manager.py:406) mutates back to unguarded with the suite still green: pre-existing, load-bearing, unpinned. Rider for a future toucher.
- Non-blocking — ticket names PATH/HOME/SSH_AUTH_SOCK; the test pins two of three (the splat covers all).
- Nothing undeclared rode along; full suite 1748 passed / 24 deselected.
Verdict
Approve — no blocking findings on either axis. Riders for future touchers: fold the subsumed ssh-key test, pin repo_manager.py:406's guard, consider _git_env() at a third site, quote ssh_key_path (own ticket). GitHub refuses --approve from the PR author's account, so this is posted as a comment; this written verdict governs. Merging with a merge commit per repo rules.
Closes #212
BranchManager.push_branch_to_remotebuiltenv = {"GIT_SSH_COMMAND": ssh_command}— replacing the inherited environment rather than extending it. So the one git call that was told which key to authenticate with ran with noPATHto findsshon, noHOMEto read~/.ssh/known_hostsor the key's own config from, and noSSH_AUTH_SOCKto reach the agent. Naming a key was what broke the auth it was meant to set up, and only on the machines that pass one. Same env-replacement hazard #187 closed at the locale pin; same{**os.environ, ...}shape.Pinned at the argv/env seam the ticket named, mirroring the
SSH_AUTH_SOCKassertiontest_already_exists_answer_survives_a_translated_gitalready makes: with a sentinel agent socket in the environment, the env handed to the subprocess must carryGIT_SSH_COMMANDand still carry the inheritedSSH_AUTH_SOCKandPATH. Red before green — the new test failed withKeyError: 'SSH_AUTH_SOCK'against the old line.Also in scope, from #211's Standards review: the two places that classify git's outcome by reading stderr text disagreed on the
None-guard — the fetch path usede.stderr or "", branch creation reade.stderrunguarded. ACalledProcessErrorwhose stderr was never captured therefore raisedTypeError: argument of type 'NoneType' is not a container or iterable— naming neither the branch nor the cause — where every other failure raisesFailed to create branch. Its own red test drove that, and the two sites now read the same.pixi run cigreen locally: 1748 passed, pylint 10.00/10. CHANGELOG entry under## [Unreleased]→### Fixed, beside #187's.🤖 Generated with Claude Code
Summary by Sourcery
Layer ssh key configuration onto the inherited environment when pushing branches and harden branch creation error handling for stderr-less failures.
Bug Fixes:
Documentation:
Tests: