fix: address PR review feedback for git SSH signature

- removeSignatureHelper now preserves user-owned keys in [gpg "ssh"]
  sections (e.g., allowedSignersFile). Only strips devpod-managed
  program entry. Drops the section header only if no user keys remain.
- Shell-escape signingKey in setupGitSSHSignature to handle keys with
  spaces or special characters.
- Convert git_ssh_signature_test.go to testify suite, add positional
  arg assertion and dedicated buffer file test.
- Add two new helper tests: PreservesUserOwnedGpgSSHKeys and
  DropsEmptyGpgSSHSection.

Author: skevetter

cmd/agent/git_ssh_signature_test.go

@@ -4,34 +4,63 @@ import (
 	"testing"
 
 	"github.com/skevetter/devpod/cmd/flags"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/suite"
 )
 
-func TestGitSSHSignatureCmd_AcceptsUnknownFlags(t *testing.T) {
+type GitSSHSignatureTestSuite struct {
+	suite.Suite
+}
+
+func TestGitSSHSignatureSuite(t *testing.T) {
+	suite.Run(t, new(GitSSHSignatureTestSuite))
+}
+
+func (s *GitSSHSignatureTestSuite) TestAcceptsUnknownFlags() {
 	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
 
-	// Simulate what git passes: -Y sign -n git -f /path/to/key -U /tmp/buffer
-	// We expect flag parsing to succeed (no "unknown shorthand flag" error).
+	// Git may pass: -Y sign -n git -f /path/to/key -U /tmp/buffer
+	// With FParseErrWhitelist, -U is treated as an unknown flag consuming
+	// /tmp/buffer as its value. This is fine because git always puts the
+	// buffer file as the last argument. We test with the buffer as a
+	// separate positional arg (no unknown flag consuming it).
 	err := cmd.ParseFlags(
 		[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "-U", "/tmp/buffer"},
 	)
-	if err != nil {
-		t.Fatalf("expected flag parsing to succeed with unknown flag -U, got: %v", err)
-	}
+	assert.NoError(s.T(), err, "flag parsing should succeed with unknown flag -U")
+}
+
+func (s *GitSSHSignatureTestSuite) TestBufferFileAsPositionalArg() {
+	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
+
+	// Standard git invocation: -Y sign -n git -f /path/to/key /tmp/buffer
+	// The buffer file is the last positional argument.
+	err := cmd.ParseFlags(
+		[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
+	)
+	assert.NoError(s.T(), err)
+
+	args := cmd.Flags().Args()
+	assert.NotEmpty(s.T(), args, "should have positional args")
+	assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1],
+		"last positional arg should be the buffer file")
 }
 
-func TestGitSSHSignatureCmd_KnownFlagsParsed(t *testing.T) {
+func (s *GitSSHSignatureTestSuite) TestKnownFlagsParsed() {
 	cmd := NewGitSSHSignatureCmd(&flags.GlobalFlags{})
 
-	err := cmd.ParseFlags([]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"})
-	if err != nil {
-		t.Fatalf("expected flag parsing to succeed, got: %v", err)
-	}
+	err := cmd.ParseFlags(
+		[]string{"-Y", "sign", "-n", "git", "-f", "/path/to/key", "/tmp/buffer"},
+	)
+	assert.NoError(s.T(), err, "flag parsing should succeed")
 
 	val, err := cmd.Flags().GetString("command")
-	if err != nil {
-		t.Fatalf("expected to get 'command' flag, got: %v", err)
-	}
-	if val != "sign" {
-		t.Fatalf("expected command flag to be 'sign', got: %q", val)
-	}
+	assert.NoError(s.T(), err)
+	assert.Equal(s.T(), "sign", val, "command flag should be 'sign'")
+
+	// The buffer file should be the last positional argument
+	args := cmd.Flags().Args()
+	assert.NotEmpty(s.T(), args, "should have positional args")
+	assert.Equal(s.T(), "/tmp/buffer", args[len(args)-1],
+		"last positional arg should be the buffer file")
 }

cmd/up.go

@@ -15,6 +15,7 @@ import (
 	"strings"
 	"syscall"
 
+	"al.essio.dev/pkg/shellescape"
 	"github.com/blang/semver/v4"
 	"github.com/sirupsen/logrus"
 	"github.com/skevetter/devpod/cmd/flags"
@@ -1565,7 +1566,10 @@ func setupGitSSHSignature(
 		"--context",
 		client.Context(),
 		client.Workspace(),
-		"--command", fmt.Sprintf("devpod agent git-ssh-signature-helper %s", signingKey),
+		"--command",
+		shellescape.QuoteCommand(
+			[]string{"devpod", "agent", "git-ssh-signature-helper", signingKey},
+		),
 	).CombinedOutput()
 	if err != nil {
 		return fmt.Errorf("setup git ssh signature helper: %w, output: %s", err, string(out))

pkg/gitsshsigning/helper.go

@@ -10,7 +10,6 @@ import (
 	"github.com/skevetter/devpod/pkg/command"
 	"github.com/skevetter/devpod/pkg/file"
 	"github.com/skevetter/log"
-	"github.com/skevetter/log/scanner"
 )
 
 const (
@@ -150,41 +149,72 @@ func removeGitConfigHelper(gitConfigPath, userName string) error {
 }
 
 func removeSignatureHelper(content string) string {
-	scan := scanner.NewScanner(strings.NewReader(content))
 	inGpgSSHSection := false
 	inGpgSection := false
-	out := []string{}
+	var gpgSSHBuffer []string
+	var out []string
 
-	for scan.Scan() {
-		line := scan.Text()
+	for line := range strings.Lines(content) {
+		line = strings.TrimRight(line, "\n")
 		trimmed := strings.TrimSpace(line)
 
-		// Track section transitions
-		if len(trimmed) > 0 && trimmed[0] == '[' {
+		if isSectionHeader(trimmed) {
+			if inGpgSSHSection {
+				out = append(out, filterGpgSSHSection(gpgSSHBuffer)...)
+				gpgSSHBuffer = nil
+			}
 			inGpgSSHSection = trimmed == `[gpg "ssh"]`
 			inGpgSection = trimmed == "[gpg]"
-
-			// Skip the entire [gpg "ssh"] section header (devpod-managed)
 			if inGpgSSHSection {
+				gpgSSHBuffer = append(gpgSSHBuffer, line)
 				continue
 			}
 		}
 
-		// Skip all lines inside [gpg "ssh"] section
 		if inGpgSSHSection {
+			gpgSSHBuffer = append(gpgSSHBuffer, line)
 			continue
 		}
 
-		// Inside [gpg] section, only skip devpod-managed keys
-		if inGpgSection && len(trimmed) > 0 && trimmed[0] != '[' {
-			if strings.HasPrefix(trimmed, "format = ssh") ||
-				strings.HasPrefix(trimmed, "program = devpod-ssh-signature") {
-				continue
-			}
+		if !isDevpodManagedGpgKey(inGpgSection, trimmed) {
+			out = append(out, line)
 		}
+	}
 
-		out = append(out, line)
+	if inGpgSSHSection {
+		out = append(out, filterGpgSSHSection(gpgSSHBuffer)...)
 	}
 
 	return strings.Join(out, "\n")
 }
+
+func isSectionHeader(trimmed string) bool {
+	return len(trimmed) > 0 && trimmed[0] == '['
+}
+
+func isDevpodManagedGpgKey(inGpgSection bool, trimmed string) bool {
+	if !inGpgSection || len(trimmed) == 0 || trimmed[0] == '[' {
+		return false
+	}
+	return strings.HasPrefix(trimmed, "format = ssh") ||
+		strings.HasPrefix(trimmed, "program = devpod-ssh-signature")
+}
+
+// filterGpgSSHSection removes devpod-managed keys from a buffered [gpg "ssh"]
+// section. Returns the header + remaining user keys, or nil if no user keys remain.
+func filterGpgSSHSection(lines []string) []string {
+	if len(lines) == 0 {
+		return nil
+	}
+	var kept []string
+	for _, line := range lines[1:] {
+		trimmed := strings.TrimSpace(line)
+		if !strings.HasPrefix(trimmed, "program = devpod-ssh-signature") {
+			kept = append(kept, line)
+		}
+	}
+	if len(kept) == 0 {
+		return nil
+	}
+	return append([]string{lines[0]}, kept...)
+}

pkg/gitsshsigning/helper_test.go

@@ -56,3 +56,36 @@ func (s *HelperTestSuite) TestRemoveSignatureHelper_NoGpgSections() {
 
 	assert.Equal(s.T(), input, result)
 }
+
+func (s *HelperTestSuite) TestRemoveSignatureHelper_PreservesUserOwnedGpgSSHKeys() {
+	input := strings.Join([]string{
+		"[user]", "\tname = Test User",
+		`[gpg "ssh"]`, "\tprogram = devpod-ssh-signature",
+		"\tallowedSignersFile = ~/.ssh/allowed_signers",
+		"[commit]", "\tgpgsign = true",
+	}, "\n")
+
+	result := removeSignatureHelper(input)
+
+	assert.NotContains(s.T(), result, "devpod-ssh-signature")
+	assert.Contains(s.T(), result, `[gpg "ssh"]`,
+		"section header should be preserved when user keys remain")
+	assert.Contains(s.T(), result, "allowedSignersFile",
+		"user-owned key should be preserved")
+	assert.Contains(s.T(), result, "[commit]")
+}
+
+func (s *HelperTestSuite) TestRemoveSignatureHelper_DropsEmptyGpgSSHSection() {
+	input := strings.Join([]string{
+		"[user]", "\tname = Test User",
+		`[gpg "ssh"]`, "\tprogram = devpod-ssh-signature",
+		"[commit]", "\tgpgsign = true",
+	}, "\n")
+
+	result := removeSignatureHelper(input)
+
+	assert.NotContains(s.T(), result, "devpod-ssh-signature")
+	assert.NotContains(s.T(), result, `[gpg "ssh"]`,
+		"empty section should be dropped entirely")
+	assert.Contains(s.T(), result, "[commit]")
+}