Skip to content

Commit 39731f5

Browse files
sallyomclaude
sallyom
and
claude
committed
feat: Improve git repository context UX (Issue #376)
Enhances the user experience for adding git repositories as context. Frontend UX Improvements: 1. Enhanced Add Context Modal - Repository options with base/feature branch configuration, protected branch detection, sync configuration 2. Improved Repository Dialog - Branch configuration with branch fetching, protected branch warnings, sync support 3. Enhanced Repository Display - Visual badges, color-coded branch pills, improved layout Co-Authored-By: Claude Sonnet 4.5 <[email protected]> Signed-off-by: sallyom <[email protected]>
1 parent f5fac44 commit 39731f5

File tree

14 files changed

+1133
-131
lines changed

14 files changed

+1133
-131
lines changed

‎components/backend/handlers/helpers.go‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"log"
77
"math"
8+
"strings"
89
"time"
910

1011
authv1 "k8s.io/api/authorization/v1"
@@ -74,3 +75,127 @@ func ValidateSecretAccess(ctx context.Context, k8sClient kubernetes.Interface, n
7475

7576
return nil
7677
}
78+
79+
// isProtectedBranch checks if a branch name is commonly protected
80+
func isProtectedBranch(branch string) bool {
81+
if branch == "" {
82+
return false
83+
}
84+
protectedNames := []string{
85+
"main", "master", "develop", "dev", "development",
86+
"production", "prod", "staging", "stage", "qa", "test", "stable",
87+
}
88+
branchLower := strings.ToLower(strings.TrimSpace(branch))
89+
for _, protected := range protectedNames {
90+
if branchLower == protected {
91+
return true
92+
}
93+
}
94+
return false
95+
}
96+
97+
// isValidGitBranchName validates a user-supplied branch name against git branch naming rules
98+
// and shell injection risks. Returns true if the branch name is safe to use.
99+
// Security: This prevents command injection by rejecting shell metacharacters.
100+
func isValidGitBranchName(branch string) bool {
101+
if branch == "" {
102+
return false
103+
}
104+
105+
// Reject if longer than 255 characters (git limit)
106+
if len(branch) > 255 {
107+
return false
108+
}
109+
110+
// Reject shell metacharacters that could lead to command injection
111+
// CRITICAL: These characters could break out of git commands in wrapper.py
112+
shellMetachars := []rune{';', '&', '|', '$', '`', '\\', '\n', '\r', '\t', '<', '>', '(', ')', '{', '}', '\'', '"', ' '}
113+
for _, char := range shellMetachars {
114+
if strings.ContainsRune(branch, char) {
115+
return false
116+
}
117+
}
118+
119+
// Reject git control characters and patterns
120+
gitControlChars := []string{"..", "~", "^", ":", "?", "*", "[", "@{"}
121+
for _, pattern := range gitControlChars {
122+
if strings.Contains(branch, pattern) {
123+
return false
124+
}
125+
}
126+
127+
// Cannot start or end with dot or slash
128+
if strings.HasPrefix(branch, ".") || strings.HasSuffix(branch, ".") ||
129+
strings.HasPrefix(branch, "/") || strings.HasSuffix(branch, "/") {
130+
return false
131+
}
132+
133+
// Cannot contain consecutive slashes
134+
if strings.Contains(branch, "//") {
135+
return false
136+
}
137+
138+
// Must contain only valid characters: alphanumeric, dash, underscore, slash, dot
139+
for _, r := range branch {
140+
valid := (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') ||
141+
(r >= '0' && r <= '9') || r == '-' || r == '_' || r == '/' || r == '.'
142+
if !valid {
143+
return false
144+
}
145+
}
146+
147+
return true
148+
}
149+
150+
// sanitizeBranchName converts a display name to a valid git branch name
151+
func sanitizeBranchName(name string) string {
152+
// Replace spaces with hyphens
153+
name = strings.ReplaceAll(name, " ", "-")
154+
// Remove or replace invalid characters for git branch names
155+
// Valid: alphanumeric, dash, underscore, slash, dot (but not at start/end)
156+
var result strings.Builder
157+
for _, r := range name {
158+
if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') ||
159+
(r >= '0' && r <= '9') || r == '-' || r == '_' || r == '/' {
160+
result.WriteRune(r)
161+
}
162+
}
163+
sanitized := result.String()
164+
// Trim leading/trailing dashes or slashes
165+
sanitized = strings.Trim(sanitized, "-/")
166+
return sanitized
167+
}
168+
169+
// generateWorkingBranch generates a working branch name based on the session and repo context
170+
// Returns the branch name to use for the session
171+
func generateWorkingBranch(sessionDisplayName, sessionID, requestedBranch string, allowProtectedWork bool) string {
172+
// If user explicitly requested a branch
173+
if requestedBranch != "" {
174+
// Check if it's protected and user hasn't allowed working on it
175+
if isProtectedBranch(requestedBranch) && !allowProtectedWork {
176+
// Create a temporary working branch to protect the base branch
177+
sessionIDShort := sessionID
178+
if len(sessionID) > 8 {
179+
sessionIDShort = sessionID[:8]
180+
}
181+
return fmt.Sprintf("work/%s/%s", requestedBranch, sessionIDShort)
182+
}
183+
// User requested non-protected branch or explicitly allowed protected work
184+
return requestedBranch
185+
}
186+
187+
// No branch requested - generate from session name
188+
if sessionDisplayName != "" {
189+
sanitized := sanitizeBranchName(sessionDisplayName)
190+
if sanitized != "" {
191+
return sanitized
192+
}
193+
}
194+
195+
// Fallback: use session ID
196+
sessionIDShort := sessionID
197+
if len(sessionID) > 8 {
198+
sessionIDShort = sessionID[:8]
199+
}
200+
return fmt.Sprintf("session-%s", sessionIDShort)
201+
}

‎components/backend/handlers/sessions.go‎

Lines changed: 108 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -599,36 +599,94 @@ func CreateSession(c *gin.Context) {
599599
if metadata["annotations"] == nil {
600600
metadata["annotations"] = make(map[string]interface{})
601601
}
602-
annotations := metadata["annotations"].(map[string]interface{})
602+
annotations, ok := metadata["annotations"].(map[string]interface{})
603+
if !ok {
604+
log.Printf("Failed to get annotations from metadata for parent session %s", req.ParentSessionID)
605+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session metadata"})
606+
return
607+
}
603608
annotations["vteam.ambient-code/parent-session-id"] = req.ParentSessionID
604609
log.Printf("Creating continuation session from parent %s (operator will handle temp pod cleanup)", req.ParentSessionID)
605610
// Note: Operator will delete temp pod when session starts (desired-phase=Running)
606611
}
607612

608613
if len(envVars) > 0 {
609-
spec := session["spec"].(map[string]interface{})
614+
spec, ok := session["spec"].(map[string]interface{})
615+
if !ok {
616+
log.Printf("Failed to get spec from session %s", name)
617+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session spec"})
618+
return
619+
}
610620
spec["environmentVariables"] = envVars
611621
}
612622

613623
// Interactive flag
614624
if req.Interactive != nil {
615-
session["spec"].(map[string]interface{})["interactive"] = *req.Interactive
625+
spec, ok := session["spec"].(map[string]interface{})
626+
if !ok {
627+
log.Printf("Failed to get spec from session %s", name)
628+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session spec"})
629+
return
630+
}
631+
spec["interactive"] = *req.Interactive
616632
}
617633

618634
// AutoPushOnComplete flag
619635
if req.AutoPushOnComplete != nil {
620-
session["spec"].(map[string]interface{})["autoPushOnComplete"] = *req.AutoPushOnComplete
636+
spec, ok := session["spec"].(map[string]interface{})
637+
if !ok {
638+
log.Printf("Failed to get spec from session %s", name)
639+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session spec"})
640+
return
641+
}
642+
spec["autoPushOnComplete"] = *req.AutoPushOnComplete
621643
}
622644

623645
// Set multi-repo configuration on spec (simplified format)
646+
// Generate working branch names upfront based on session context
624647
{
625-
spec := session["spec"].(map[string]interface{})
648+
spec, ok := session["spec"].(map[string]interface{})
649+
if !ok {
650+
log.Printf("Failed to get spec from session %s", name)
651+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session spec"})
652+
return
653+
}
626654
if len(req.Repos) > 0 {
627655
arr := make([]map[string]interface{}, 0, len(req.Repos))
628656
for _, r := range req.Repos {
629-
m := map[string]interface{}{"url": r.URL}
630-
if r.Branch != nil {
631-
m["branch"] = *r.Branch
657+
// Determine the working branch to use
658+
var requestedBranch string
659+
if r.WorkingBranch != nil {
660+
requestedBranch = strings.TrimSpace(*r.WorkingBranch)
661+
} else if r.Branch != nil {
662+
requestedBranch = strings.TrimSpace(*r.Branch)
663+
}
664+
665+
// Validate user-supplied branch names to prevent command injection
666+
if requestedBranch != "" && !isValidGitBranchName(requestedBranch) {
667+
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid branch name format: %s", requestedBranch)})
668+
return
669+
}
670+
671+
allowProtected := false
672+
if r.AllowProtectedWork != nil {
673+
allowProtected = *r.AllowProtectedWork
674+
}
675+
676+
// Generate the actual branch name that will be used
677+
workingBranch := generateWorkingBranch(
678+
req.DisplayName,
679+
name, // session name (unique ID)
680+
requestedBranch,
681+
allowProtected,
682+
)
683+
684+
// Wrap in 'input' object to match runner expectations
685+
m := map[string]interface{}{
686+
"input": map[string]interface{}{
687+
"url": r.URL,
688+
"branch": workingBranch,
689+
},
632690
}
633691
arr = append(arr, m)
634692
}
@@ -661,7 +719,13 @@ func CreateSession(c *gin.Context) {
661719
if len(groups) == 0 && req.UserContext != nil {
662720
groups = req.UserContext.Groups
663721
}
664-
session["spec"].(map[string]interface{})["userContext"] = map[string]interface{}{
722+
spec, ok := session["spec"].(map[string]interface{})
723+
if !ok {
724+
log.Printf("Failed to get spec from session %s", name)
725+
c.JSON(http.StatusInternalServerError, gin.H{"error": "Invalid session spec"})
726+
return
727+
}
728+
spec["userContext"] = map[string]interface{}{
665729
"userId": uid,
666730
"displayName": displayName,
667731
"groups": groups,
@@ -1406,19 +1470,17 @@ func AddRepo(c *gin.Context) {
14061470
}
14071471

14081472
var req struct {
1409-
URL string `json:"url" binding:"required"`
1410-
Branch string `json:"branch"`
1473+
URL string `json:"url" binding:"required"`
1474+
Branch string `json:"branch"`
1475+
WorkingBranch string `json:"workingBranch"`
1476+
AllowProtectedWork bool `json:"allowProtectedWork"`
14111477
}
14121478

14131479
if err := c.ShouldBindJSON(&req); err != nil {
14141480
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
14151481
return
14161482
}
14171483

1418-
if req.Branch == "" {
1419-
req.Branch = "main"
1420-
}
1421-
14221484
gvr := GetAgenticSessionV1Alpha1Resource()
14231485
item, err := k8sDyn.Resource(gvr).Namespace(project).Get(context.TODO(), sessionName, v1.GetOptions{})
14241486
if err != nil {
@@ -1447,9 +1509,38 @@ func AddRepo(c *gin.Context) {
14471509
repos = []interface{}{}
14481510
}
14491511

1512+
// Determine the requested branch
1513+
requestedBranch := req.WorkingBranch
1514+
if requestedBranch == "" {
1515+
requestedBranch = req.Branch
1516+
}
1517+
1518+
// Validate user-supplied branch names to prevent command injection
1519+
if requestedBranch != "" && !isValidGitBranchName(requestedBranch) {
1520+
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid branch name format: %s", requestedBranch)})
1521+
return
1522+
}
1523+
1524+
// Get session display name for branch generation
1525+
displayName := ""
1526+
if dn, ok := spec["displayName"].(string); ok {
1527+
displayName = dn
1528+
}
1529+
1530+
// Generate the actual working branch name
1531+
workingBranch := generateWorkingBranch(
1532+
displayName,
1533+
sessionName,
1534+
requestedBranch,
1535+
req.AllowProtectedWork,
1536+
)
1537+
1538+
// Wrap in 'input' object to match runner expectations
14501539
newRepo := map[string]interface{}{
1451-
"url": req.URL,
1452-
"branch": req.Branch,
1540+
"input": map[string]interface{}{
1541+
"url": req.URL,
1542+
"branch": workingBranch,
1543+
},
14531544
}
14541545
repos = append(repos, newRepo)
14551546
spec["repos"] = repos

‎components/backend/types/session.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,10 @@ type AgenticSessionSpec struct {
2828

2929
// SimpleRepo represents a simplified repository configuration
3030
type SimpleRepo struct {
31-
URL string `json:"url"`
32-
Branch *string `json:"branch,omitempty"`
31+
URL string `json:"url"`
32+
Branch *string `json:"branch,omitempty"`
33+
WorkingBranch *string `json:"workingBranch,omitempty"` // User-requested working branch (input only)
34+
AllowProtectedWork *bool `json:"allowProtectedWork,omitempty"` // Allow work directly on protected branches (input only)
3335
}
3436

3537
type AgenticSessionStatus struct {

‎components/frontend/package-lock.json‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎components/frontend/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
"@radix-ui/react-accordion": "^1.2.12",
1414
"@radix-ui/react-avatar": "^1.1.10",
1515
"@radix-ui/react-checkbox": "^1.3.3",
16+
"@radix-ui/react-collapsible": "^1.1.12",
1617
"@radix-ui/react-dropdown-menu": "^2.1.16",
1718
"@radix-ui/react-label": "^2.1.7",
1819
"@radix-ui/react-progress": "^1.1.7",

0 commit comments

Comments
 (0)