Version Control nav group, sidebar parity, ⌘K palette

internal/web/connections_version_control_test.go

@@ -0,0 +1,130 @@
+package web_test
+
+// Regression test for the synthetic "version_control" connector-tab
+// filter that groups github + gitlab cards (and existing github/gitlab
+// connections) under one /connections sub-page. Lives next to the
+// parallel /policies?scope=version_control test so a future refactor
+// can't silently drop either branch of the filter.
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/trilitech/Sieve/internal/connector"
+	githubconn "github.com/trilitech/Sieve/internal/connectors/github"
+	gitlabconn "github.com/trilitech/Sieve/internal/connectors/gitlab"
+	"github.com/trilitech/Sieve/internal/connectors/httpproxy"
+)
+
+// TestConnectionsPage_VersionControlTab asserts:
+//   - /connections?type=version_control surfaces both the github and
+//     gitlab connector catalog cards.
+//   - It also surfaces existing connections whose ConnectorType is
+//     "github" or "gitlab", and excludes other connections.
+//   - The narrower ?type=github tab only shows GitHub (no GitLab).
+func TestConnectionsPage_VersionControlTab(t *testing.T) {
+	handler, env := newTestWebServer(t)
+
+	// Register the real github + gitlab connector metas so the catalog
+	// loop has cards to render. The mock connector seeded by
+	// newTestWebServer is registered under "mock", which is fine —
+	// it's the negative control.
+	env.Registry.Register(githubconn.Meta(), func(raw map[string]any) (connector.Connector, error) {
+		return githubconn.Factory()(raw)
+	})
+	env.Registry.Register(gitlabconn.Meta(), func(raw map[string]any) (connector.Connector, error) {
+		return gitlabconn.Factory()(raw)
+	})
+	// http_proxy is needed for the "forged category" negative control below.
+	env.Registry.Register(httpproxy.Meta, httpproxy.Factory)
+
+	// Seed one connection per connector type. Config payloads are
+	// intentionally empty: the filter under test keys off
+	// ConnectorType + display name, not the connector's typed config
+	// schema, and a too-specific seed would tie the test to a
+	// schema that may evolve.
+	if err := env.Connections.Add("my-github", "github", "My GitHub", map[string]any{}); err != nil {
+		t.Fatalf("add github connection: %v", err)
+	}
+	if err := env.Connections.Add("my-gitlab", "gitlab", "My GitLab", map[string]any{}); err != nil {
+		t.Fatalf("add gitlab connection: %v", err)
+	}
+	if err := env.Connections.Add("my-mock", "mock", "My Mock", map[string]any{}); err != nil {
+		t.Fatalf("add mock connection: %v", err)
+	}
+	// Negative control: an http_proxy whose config.category is set to
+	// "github" must NOT appear under ?type=version_control. The filter
+	// keys off ConnectorType (immutable) rather than the per-connection
+	// category override, so this stays excluded.
+	if err := env.Connections.Add("fake-vcs-proxy", "http_proxy", "Fake VCS Proxy", map[string]any{
+		"target_url": "https://example.com",
+		"category":   "github",
+	}); err != nil {
+		t.Fatalf("add fake-vcs proxy: %v", err)
+	}
+
+	t.Run("version_control tab shows both catalog cards + both connections", func(t *testing.T) {
+		rec := getRequest(handler, env, "/connections?type=version_control")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d (body: %s)", rec.Code, rec.Body.String())
+		}
+		body := rec.Body.String()
+		// Catalog cards: the page renders each connector card by
+		// emitting both the connector's Meta().Name (in an h4) and its
+		// Meta().Description (in a sibling p). Pin both Name strings —
+		// the Name is the load-bearing label and is stable text.
+		// "GitLab" alone is too soft a probe (it also appears in the
+		// connection-row display name), so we also pin a Description
+		// substring per connector so a regression that removed the
+		// catalog cards while leaving the connection list intact would
+		// fail.
+		for _, want := range []string{
+			"My GitHub",         // existing connection display name
+			"My GitLab",         // existing connection display name
+			"PAT or GitHub App", // GitHub Meta() Description substring
+			"merge requests",    // GitLab Meta() Description substring
+		} {
+			if !strings.Contains(body, want) {
+				t.Errorf("version_control tab missing %q", want)
+			}
+		}
+		if strings.Contains(body, "My Mock") {
+			t.Errorf("version_control tab incorrectly includes non-VCS connection 'My Mock'")
+		}
+		// An http_proxy with config.category="github" must NOT slip
+		// into the version_control tab — the filter keys off the
+		// immutable ConnectorType, not the per-connection override.
+		if strings.Contains(body, "Fake VCS Proxy") {
+			t.Errorf("version_control tab incorrectly includes an http_proxy whose config.category is forged to 'github'")
+		}
+	})
+
+	t.Run("github tab excludes gitlab", func(t *testing.T) {
+		rec := getRequest(handler, env, "/connections?type=github")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d", rec.Code)
+		}
+		body := rec.Body.String()
+		if !strings.Contains(body, "My GitHub") {
+			t.Errorf("github tab missing 'My GitHub' connection")
+		}
+		if strings.Contains(body, "My GitLab") {
+			t.Errorf("github tab incorrectly includes 'My GitLab' connection")
+		}
+	})
+
+	t.Run("gitlab tab excludes github", func(t *testing.T) {
+		rec := getRequest(handler, env, "/connections?type=gitlab")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d", rec.Code)
+		}
+		body := rec.Body.String()
+		if !strings.Contains(body, "My GitLab") {
+			t.Errorf("gitlab tab missing 'My GitLab' connection")
+		}
+		if strings.Contains(body, "My GitHub") {
+			t.Errorf("gitlab tab incorrectly includes 'My GitHub' connection")
+		}
+	})
+}

internal/web/policies_version_control_test.go

@@ -0,0 +1,100 @@
+package web_test
+
+// Regression test for the synthetic "version_control" policy scope that
+// groups github + gitlab policies under one sidebar tab. The filter is
+// implemented in server.go's handlePolicies; this test pins the
+// inclusion rule so a future refactor can't silently drop github/gitlab
+// from the group or accidentally widen it to other scopes.
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+
+	"github.com/trilitech/Sieve/internal/testing/testenv"
+)
+
+// TestPoliciesPage_VersionControlScope asserts:
+//   - ?scope=version_control includes policies tagged github and gitlab.
+//   - That synthetic scope does NOT include policies for unrelated
+//     connectors (slack, gmail).
+//   - The narrower ?scope=github tab shows only github (legacy/empty
+//     scopes also show, but explicitly-scoped gitlab policies do not).
+func TestPoliciesPage_VersionControlScope(t *testing.T) {
+	handler, env := newTestWebServer(t)
+
+	// Seed four policies, three of which carry an explicit scope.
+	mustSeedPolicy(t, env, "github-allow", "github")
+	mustSeedPolicy(t, env, "gitlab-allow", "gitlab")
+	mustSeedPolicy(t, env, "slack-allow", "slack")
+	mustSeedPolicy(t, env, "legacy-no-scope", "") // empty scope — shows under all tabs
+	// Hand-crafted "version_control"-scoped policy stands in for a
+	// DB row that slipped past the create-time block in
+	// handlePolicyCreate. The version_control filter MUST NOT
+	// surface it (scope is a browse-only filter, not a real scope).
+	mustSeedPolicy(t, env, "vcs-stamped", "version_control")
+
+	t.Run("version_control includes github + gitlab, excludes slack", func(t *testing.T) {
+		rec := getRequest(handler, env, "/policies?scope=version_control")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d (body: %s)", rec.Code, rec.Body.String())
+		}
+		body := rec.Body.String()
+		for _, want := range []string{"github-allow", "gitlab-allow", "legacy-no-scope"} {
+			if !strings.Contains(body, want) {
+				t.Errorf("version_control scope missing policy %q", want)
+			}
+		}
+		if strings.Contains(body, "slack-allow") {
+			t.Errorf("version_control scope incorrectly includes slack policy")
+		}
+		if strings.Contains(body, "vcs-stamped") {
+			t.Errorf("version_control scope incorrectly includes a policy literally stamped scope=version_control (this scope is browse-only and such a policy is nonsense)")
+		}
+	})
+
+	t.Run("github scope excludes gitlab", func(t *testing.T) {
+		rec := getRequest(handler, env, "/policies?scope=github")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d", rec.Code)
+		}
+		body := rec.Body.String()
+		if !strings.Contains(body, "github-allow") {
+			t.Errorf("github scope missing github-allow policy")
+		}
+		if strings.Contains(body, "gitlab-allow") {
+			t.Errorf("github scope incorrectly includes gitlab-allow")
+		}
+	})
+
+	t.Run("slack scope excludes both vcs policies", func(t *testing.T) {
+		rec := getRequest(handler, env, "/policies?scope=slack")
+		if rec.Code != http.StatusOK {
+			t.Fatalf("expected 200, got %d", rec.Code)
+		}
+		body := rec.Body.String()
+		for _, unwanted := range []string{"github-allow", "gitlab-allow"} {
+			if strings.Contains(body, unwanted) {
+				t.Errorf("slack scope incorrectly includes %q", unwanted)
+			}
+		}
+	})
+}
+
+// mustSeedPolicy creates a minimal rules-evaluator policy carrying the
+// given scope (empty string = unscoped). The rules body is intentionally
+// trivial — we're testing the scope filter, not the evaluator.
+func mustSeedPolicy(t *testing.T, env *testenv.Env, name, scope string) {
+	t.Helper()
+	cfg := map[string]any{
+		"rules": []any{
+			map[string]any{"action": "allow"},
+		},
+	}
+	if scope != "" {
+		cfg["scope"] = scope
+	}
+	if _, err := env.Policies.Create(name, "rules", cfg); err != nil {
+		t.Fatalf("seed policy %q: %v", name, err)
+	}
+}

internal/web/server.go

@@ -554,6 +554,17 @@ func (s *Server) handleConnections(w http.ResponseWriter, r *http.Request) {
 				if cat == "http_proxy" || c.ConnectorType == "mcp_proxy" {
 					conns = append(conns, c)
 				}
+			} else if connType == "version_control" {
+				// "version_control" tab groups github + gitlab.
+				// Match the immutable ConnectorType directly so an
+				// http_proxy that happens to carry config.category =
+				// "github" / "gitlab" (a legitimate generic-LLM /
+				// generic-proxy override) does NOT slip into the VCS
+				// filter. `cat` reflects the http_proxy override and
+				// would do that here; ConnectorType cannot be forged.
+				if c.ConnectorType == "github" || c.ConnectorType == "gitlab" {
+					conns = append(conns, c)
+				}
 			} else if cat == connType {
 				conns = append(conns, c)
 			}
@@ -573,6 +584,10 @@ func (s *Server) handleConnections(w http.ResponseWriter, r *http.Request) {
 				if connType == "proxy" {
 					match = m.Type == "http_proxy" || m.Type == "mcp_proxy"
 				}
+				// "version_control" tab shows both github and gitlab connectors
+				if connType == "version_control" {
+					match = m.Type == "github" || m.Type == "gitlab"
+				}
 				if match {
 					filtered[category] = append(filtered[category], m)
 				}
@@ -1434,10 +1449,22 @@ func (s *Server) handlePolicies(w http.ResponseWriter, r *http.Request) {
 
 	// Filter policies by scope. A policy's scope is stored in its config.
 	// Policies without a scope (legacy/presets) show under all tabs.
+	// The synthetic "version_control" scope is strictly defined as
+	// github + gitlab + unscoped. A policy literally stamped with
+	// scope=version_control would be nonsense (creation is blocked in
+	// handlePolicyCreate); excluding it here keeps the filter honest
+	// against any hand-crafted DB row that slipped through, and keeps
+	// the handlePolicyCreate comment accurate.
 	var pols []policies.Policy
 	for _, p := range allPols {
 		pScope, _ := p.PolicyConfig["scope"].(string)
-		if scope == "" || pScope == "" || pScope == scope {
+		var match bool
+		if scope == "version_control" {
+			match = pScope == "" || pScope == "github" || pScope == "gitlab"
+		} else {
+			match = scope == "" || pScope == "" || pScope == scope
+		}
+		if match {
 			pols = append(pols, p)
 		}
 	}
@@ -1475,6 +1502,22 @@ func (s *Server) handlePolicyCreate(w http.ResponseWriter, r *http.Request) {
 		policyConfig = make(map[string]any)
 	}
 
+	// "version_control" is a synthetic browse-only scope used in the
+	// sidebar grouping; it isn't a real connector scope. The
+	// /policies?scope=version_control list-filter (handlePolicies)
+	// pulls policies whose stored scope is github or gitlab (plus
+	// unscoped legacies). Persisting scope=version_control on a new
+	// policy would orphan it from both the github and gitlab tabs
+	// while only matching the synthetic group — almost certainly an
+	// accident, so refuse it loudly. The policies.html template
+	// hides the create form under this scope; this server-side
+	// reject is defence-in-depth against a hand-crafted POST or an
+	// out-of-date browser tab.
+	if sc, _ := policyConfig["scope"].(string); sc == "version_control" {
+		http.Error(w, "scope \"version_control\" is a browse-only filter; pick github or gitlab for a real policy", http.StatusBadRequest)
+		return
+	}
+
 	// Validate the policy rules against known operations.
 	if errs := s.validatePolicyRules(policyConfig); len(errs) > 0 {
 		http.Error(w, "Policy validation errors:\n"+strings.Join(errs, "\n"), http.StatusBadRequest)

internal/web/server_status_test.go

@@ -48,6 +48,23 @@ func authedPost(t *testing.T, env *testenv.Env, path string) (*http.Request, *ht
 	return req, httptest.NewRecorder()
 }
 
+// getRequest performs an authenticated GET against an admin handler and
+// returns the recorder. Shared by the web_test package's GET-and-assert
+// tests so a single canonical "GET with session cookie" path lives here
+// instead of being re-implemented in each test file. (internal/web/slack_test.go
+// has a same-named helper that lives in the internal `package web` for
+// the internal-package tests; this one is its external-package
+// counterpart.)
+func getRequest(handler http.Handler, env *testenv.Env, path string) *httptest.ResponseRecorder {
+	req := httptest.NewRequest(http.MethodGet, path, nil)
+	if c := env.SessionCookie(); c != nil {
+		req.AddCookie(c)
+	}
+	rec := httptest.NewRecorder()
+	handler.ServeHTTP(rec, req)
+	return rec
+}
+
 // TestServer_DisableConnection_RejectsAgentToken verifies that an
 // agent bearer token cannot disable a connection through the admin UI.
 // The requireOperatorSession middleware inspects the Authorization

internal/web/templates/connections.html

@@ -201,34 +201,22 @@ <h4 class="text-white font-medium">{{.Name}}</h4>
               </button>
             </form>
             {{else if eq .Type "github"}}
-            <div class="rounded-lg border border-slate-700 bg-slate-900/60 p-3 mb-3 text-xs text-slate-400 space-y-2">
+            <div class="rounded-lg border border-slate-700 bg-slate-900/60 p-3 mb-3 text-xs text-slate-400 space-y-3">
               <p class="text-slate-300"><strong class="text-white">Two ways to connect.</strong> Pick what matches your access pattern.</p>
-              <div class="grid grid-cols-2 gap-3 mt-2">
-                <div>
-                  <div class="flex items-baseline gap-1.5">
-                    <span class="text-slate-200 font-medium">Fine-grained PAT</span>
-                    <span class="inline-flex items-center rounded px-1.5 py-0.5 text-[10px] uppercase tracking-wider bg-emerald-500/15 text-emerald-300 ring-1 ring-emerald-500/25">Recommended</span>
-                  </div>
-                  <ul class="mt-1 space-y-0.5 list-disc list-inside text-slate-400">
-                    <li>Fastest setup — paste a token, done.</li>
-                    <li>Scoped to specific repos under one owner.</li>
-                    <li>Acts as <em>you</em> (or a service-account user).</li>
-                    <li>Tokens expire — you'll re-paste later.</li>
-                  </ul>
+              <div>
+                <div class="flex items-baseline gap-1.5 mb-1">
+                  <span class="text-slate-200 font-medium">Fine-grained PAT</span>
+                  <span class="inline-flex items-center rounded px-1.5 py-0.5 text-[10px] uppercase tracking-wider bg-emerald-500/15 text-emerald-300 ring-1 ring-emerald-500/25">Recommended</span>
                 </div>
-                <div>
-                  <div class="flex items-baseline gap-1.5">
-                    <span class="text-slate-200 font-medium">GitHub App install</span>
-                  </div>
-                  <ul class="mt-1 space-y-0.5 list-disc list-inside text-slate-400">
-                    <li>Org-wide install, finer-grained scopes per repo.</li>
-                    <li>Ephemeral 1-hour tokens — Sieve refreshes automatically.</li>
-                    <li>Acts as the App (not a real user), better for audit.</li>
-                    <li>Requires your Sieve host to be reachable from GitHub.</li>
-                  </ul>
+                <p class="leading-snug">Paste a token; done. Scoped to specific repos under one owner. Acts as <em>you</em> (or a service-account user). Tokens expire and need re-pasting.</p>
+              </div>
+              <div>
+                <div class="flex items-baseline gap-1.5 mb-1">
+                  <span class="text-slate-200 font-medium">GitHub App install</span>
                 </div>
+                <p class="leading-snug">Org-wide install with finer-grained per-repo scopes. Ephemeral 1-hour tokens — Sieve refreshes automatically. Acts as the App (better audit). Requires your Sieve host to be reachable from GitHub for the callback.</p>
               </div>
-              <p class="pt-1 text-slate-500">Use a PAT for a single repo or personal/dev access; install the App when multiple repos in an org need long-lived, auto-rotating access.</p>
+              <p class="pt-2 border-t border-slate-700/60 text-slate-500">Use a PAT for a single repo or personal/dev access; install the App when multiple repos in an org need long-lived, auto-rotating access.</p>
             </div>
             <form method="POST" action="/connections/github/pat" class="space-y-3">
               <div>