Skip to content

Commit 0104831

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 0104831

File tree

14 files changed

+1200
-135
lines changed

14 files changed

+1200
-135
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: 103 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -599,41 +599,77 @@ func CreateSession(c *gin.Context) {
599599
if metadata["annotations"] == nil {
600600
metadata["annotations"] = make(map[string]interface{})
601601
}
602+
// Direct map access for plain maps (no need for unstructured helpers)
602603
annotations := metadata["annotations"].(map[string]interface{})
603604
annotations["vteam.ambient-code/parent-session-id"] = req.ParentSessionID
604605
log.Printf("Creating continuation session from parent %s (operator will handle temp pod cleanup)", req.ParentSessionID)
605606
// Note: Operator will delete temp pod when session starts (desired-phase=Running)
606607
}
607608

608609
if len(envVars) > 0 {
610+
// Direct map access for plain maps (no need for unstructured helpers)
609611
spec := session["spec"].(map[string]interface{})
610612
spec["environmentVariables"] = envVars
611613
}
612614

613615
// Interactive flag
614616
if req.Interactive != nil {
615-
session["spec"].(map[string]interface{})["interactive"] = *req.Interactive
617+
// Direct map access for plain maps (no need for unstructured helpers)
618+
spec := session["spec"].(map[string]interface{})
619+
spec["interactive"] = *req.Interactive
616620
}
617621

618622
// AutoPushOnComplete flag
619623
if req.AutoPushOnComplete != nil {
620-
session["spec"].(map[string]interface{})["autoPushOnComplete"] = *req.AutoPushOnComplete
624+
// Direct map access for plain maps (no need for unstructured helpers)
625+
spec := session["spec"].(map[string]interface{})
626+
spec["autoPushOnComplete"] = *req.AutoPushOnComplete
621627
}
622628

623629
// Set multi-repo configuration on spec (simplified format)
624-
{
630+
// Generate working branch names upfront based on session context
631+
if len(req.Repos) > 0 {
632+
// Direct map access for plain maps (no need for unstructured helpers)
625633
spec := session["spec"].(map[string]interface{})
626-
if len(req.Repos) > 0 {
627-
arr := make([]map[string]interface{}, 0, len(req.Repos))
628-
for _, r := range req.Repos {
629-
m := map[string]interface{}{"url": r.URL}
630-
if r.Branch != nil {
631-
m["branch"] = *r.Branch
632-
}
633-
arr = append(arr, m)
634+
arr := make([]map[string]interface{}, 0, len(req.Repos))
635+
for _, r := range req.Repos {
636+
// Determine the working branch to use
637+
var requestedBranch string
638+
if r.WorkingBranch != nil {
639+
requestedBranch = strings.TrimSpace(*r.WorkingBranch)
640+
} else if r.Branch != nil {
641+
requestedBranch = strings.TrimSpace(*r.Branch)
642+
}
643+
644+
// Validate user-supplied branch names to prevent command injection
645+
if requestedBranch != "" && !isValidGitBranchName(requestedBranch) {
646+
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid branch name format: %s", requestedBranch)})
647+
return
634648
}
635-
spec["repos"] = arr
649+
650+
allowProtected := false
651+
if r.AllowProtectedWork != nil {
652+
allowProtected = *r.AllowProtectedWork
653+
}
654+
655+
// Generate the actual branch name that will be used
656+
workingBranch := generateWorkingBranch(
657+
req.DisplayName,
658+
name, // session name (unique ID)
659+
requestedBranch,
660+
allowProtected,
661+
)
662+
663+
// Wrap in 'input' object to match runner expectations
664+
m := map[string]interface{}{
665+
"input": map[string]interface{}{
666+
"url": r.URL,
667+
"branch": workingBranch,
668+
},
669+
}
670+
arr = append(arr, m)
636671
}
672+
spec["repos"] = arr
637673
}
638674

639675
// Add userContext derived from authenticated caller; ignore client-supplied userId
@@ -661,7 +697,9 @@ func CreateSession(c *gin.Context) {
661697
if len(groups) == 0 && req.UserContext != nil {
662698
groups = req.UserContext.Groups
663699
}
664-
session["spec"].(map[string]interface{})["userContext"] = map[string]interface{}{
700+
// Direct map access for plain maps (no need for unstructured helpers)
701+
spec := session["spec"].(map[string]interface{})
702+
spec["userContext"] = map[string]interface{}{
665703
"userId": uid,
666704
"displayName": displayName,
667705
"groups": groups,
@@ -1406,19 +1444,21 @@ func AddRepo(c *gin.Context) {
14061444
}
14071445

14081446
var req struct {
1409-
URL string `json:"url" binding:"required"`
1410-
Branch string `json:"branch"`
1447+
URL string `json:"url" binding:"required"`
1448+
Branch string `json:"branch"`
1449+
WorkingBranch string `json:"workingBranch"`
1450+
AllowProtectedWork bool `json:"allowProtectedWork"`
1451+
Sync *struct {
1452+
URL string `json:"url"`
1453+
Branch string `json:"branch"`
1454+
} `json:"sync"`
14111455
}
14121456

14131457
if err := c.ShouldBindJSON(&req); err != nil {
14141458
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
14151459
return
14161460
}
14171461

1418-
if req.Branch == "" {
1419-
req.Branch = "main"
1420-
}
1421-
14221462
gvr := GetAgenticSessionV1Alpha1Resource()
14231463
item, err := k8sDyn.Resource(gvr).Namespace(project).Get(context.TODO(), sessionName, v1.GetOptions{})
14241464
if err != nil {
@@ -1447,10 +1487,52 @@ func AddRepo(c *gin.Context) {
14471487
repos = []interface{}{}
14481488
}
14491489

1490+
// Determine the requested branch
1491+
requestedBranch := req.WorkingBranch
1492+
if requestedBranch == "" {
1493+
requestedBranch = req.Branch
1494+
}
1495+
1496+
// Validate user-supplied branch names to prevent command injection
1497+
if requestedBranch != "" && !isValidGitBranchName(requestedBranch) {
1498+
c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("Invalid branch name format: %s", requestedBranch)})
1499+
return
1500+
}
1501+
1502+
// Get session display name for branch generation
1503+
displayName := ""
1504+
if dn, ok := spec["displayName"].(string); ok {
1505+
displayName = dn
1506+
}
1507+
1508+
// Generate the actual working branch name
1509+
workingBranch := generateWorkingBranch(
1510+
displayName,
1511+
sessionName,
1512+
requestedBranch,
1513+
req.AllowProtectedWork,
1514+
)
1515+
1516+
// Wrap in 'input' object to match runner expectations
14501517
newRepo := map[string]interface{}{
1451-
"url": req.URL,
1452-
"branch": req.Branch,
1518+
"input": map[string]interface{}{
1519+
"url": req.URL,
1520+
"branch": workingBranch,
1521+
},
14531522
}
1523+
1524+
// Add sync configuration if provided
1525+
if req.Sync != nil && strings.TrimSpace(req.Sync.URL) != "" {
1526+
syncBranch := strings.TrimSpace(req.Sync.Branch)
1527+
if syncBranch == "" {
1528+
syncBranch = "main"
1529+
}
1530+
newRepo["sync"] = map[string]interface{}{
1531+
"url": strings.TrimSpace(req.Sync.URL),
1532+
"branch": syncBranch,
1533+
}
1534+
}
1535+
14541536
repos = append(repos, newRepo)
14551537
spec["repos"] = repos
14561538

‎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)