diff --git a/workspace-server/internal/handlers/discovery.go b/workspace-server/internal/handlers/discovery.go index 79315016..5c798c81 100644 --- a/workspace-server/internal/handlers/discovery.go +++ b/workspace-server/internal/handlers/discovery.go @@ -292,8 +292,8 @@ func filterPeersByQuery(peers []map[string]interface{}, q string) []map[string]i needle := strings.ToLower(q) out := make([]map[string]interface{}, 0, len(peers)) for _, p := range peers { - name := p["name"].(string) - role := p["role"].(string) + name, _ := p["name"].(string) // nil → "" — safe on empty-role rows + role, _ := p["role"].(string) // nil → "" — queryPeerMaps sets nil when DB role is empty if strings.Contains(strings.ToLower(name), needle) || strings.Contains(strings.ToLower(role), needle) { out = append(out, p) diff --git a/workspace-server/internal/handlers/discovery_test.go b/workspace-server/internal/handlers/discovery_test.go index 892a1f0a..3070fcf4 100644 --- a/workspace-server/internal/handlers/discovery_test.go +++ b/workspace-server/internal/handlers/discovery_test.go @@ -394,6 +394,80 @@ func TestPeers_Q_NoMatches_RawBodyIsArrayNotNull(t *testing.T) { } } +// TestFilterPeersByQuery_NilRoleRegression is the regression gate for +// mc#730/#731: queryPeerMaps sets peer["role"] = nil when the DB role column +// is empty (discovery.go lines 337-341). filterPeersByQuery did a bare +// type assertion p["role"].(string) which panics on nil. The fix uses the +// comma-ok form so nil → "". The test passes a map with nil name and nil +// role and asserts no panic + correct filter behaviour. +func TestFilterPeersByQuery_NilRoleRegression(t *testing.T) { + cases := []struct { + name string + peers []map[string]interface{} + q string + wantLen int + wantIDs []string + }{ + { + name: "nil role matches on name", + peers: []map[string]interface{}{ + {"id": "ws-a", "name": nil, "role": nil}, + {"id": "ws-b", "name": "Alpha Builder", "role": nil}, + {"id": "ws-c", "name": "Beta Builder", "role": nil}, + }, + q: "alpha", + wantLen: 1, + wantIDs: []string{"ws-b"}, + }, + { + name: "nil name matches on nil role (empty string)", + peers: []map[string]interface{}{ + {"id": "ws-x", "name": nil, "role": nil}, + {"id": "ws-y", "name": "Dev Workspace", "role": nil}, + }, + q: "", + wantLen: 2, // empty q is a no-op + wantIDs: []string{"ws-x", "ws-y"}, + }, + { + name: "all nil — no panic, returns input", + peers: []map[string]interface{}{ + {"id": "ws-z", "name": nil, "role": nil}, + }, + q: "anything", + wantLen: 0, + wantIDs: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := filterPeersByQuery(tc.peers, tc.q) + if len(got) != tc.wantLen { + t.Fatalf("len: got %d, want %d", len(got), tc.wantLen) + } + gotIDs := make([]string, len(got)) + for i, p := range got { + gotIDs[i] = p["id"].(string) + } + if tc.wantIDs != nil { + for _, id := range tc.wantIDs { + found := false + for _, g := range gotIDs { + if g == id { + found = true + break + } + } + if !found { + t.Errorf("missing id %q; got IDs: %v", id, gotIDs) + } + } + } + }) + } +} + func keysOf(m map[string]struct{}) []string { out := make([]string, 0, len(m)) for k := range m { diff --git a/workspace-server/internal/handlers/terminal_diagnose.go b/workspace-server/internal/handlers/terminal_diagnose.go index b78c8955..cf1422e2 100644 --- a/workspace-server/internal/handlers/terminal_diagnose.go +++ b/workspace-server/internal/handlers/terminal_diagnose.go @@ -43,6 +43,27 @@ func (s *syncBuf) String() string { return s.b.String() } +// unwrapGoError extracts subprocess stderr from a Go-wrapped error that +// includes combined output. e.g. from sendSSHPublicKey's +// fmt.Errorf("send-ssh-public-key: %w (%s)", err, combinedOut), this +// returns the " (%s)" portion — the actionable subprocess signal like +// "AccessDeniedException: ... is not authorized to perform: +// ec2-instance-connect:OpenTunnel". Returns "" when the output is +// identical to the error string (no stderr captured). +func unwrapGoError(errMsg string) string { + // Extract content between the last '(' and trailing ')'. The + // sendSSHPublicKey wrapper uses fmt.Errorf("...: %w (%s)", err, combinedOut) + // so the subprocess stderr is always the last parenthesised segment, + // e.g. "send-ssh-public-key: exit status 1 (AccessDeniedException: ...)" + // — note the closing ')' is at the very end with no trailing space. + open := strings.LastIndex(errMsg, "(") + if open < 0 { + return "" + } + inner := errMsg[open+1:] + return strings.TrimSuffix(inner, ")") +} + // HandleDiagnose handles GET /workspaces/:id/terminal/diagnose. It runs the // same per-step pipeline as HandleConnect (ssh-keygen → EIC send-key → tunnel // → ssh) but non-interactively, captures the first failing step and its @@ -214,12 +235,18 @@ func (h *TerminalHandler) diagnoseRemote(ctx context.Context, workspaceID, insta } // Step 2: send-ssh-public-key (AWS Instance Connect) + // mc#687: populate Detail so the E2E smoke sees the AWS permission error + // verbatim. The subprocess stderr (e.g. "AccessDeniedException: ... is not + // authorized to perform: ec2-instance-connect:OpenTunnel") is captured by + // sendSSHPublicKey's CombinedOutput() and embedded in the Go error string. t0 = time.Now() if err := sendSSHPublicKey(ctx, region, instanceID, osUser, strings.TrimSpace(string(pubKey))); err != nil { + errMsg := err.Error() return stop("send-ssh-public-key", diagnoseStep{ Name: "send-ssh-public-key", DurationMs: time.Since(t0).Milliseconds(), - Error: err.Error(), + Error: errMsg, + Detail: unwrapGoError(errMsg), }) } res.Steps = append(res.Steps, diagnoseStep{Name: "send-ssh-public-key", OK: true, DurationMs: time.Since(t0).Milliseconds()}) diff --git a/workspace-server/internal/handlers/terminal_diagnose_test.go b/workspace-server/internal/handlers/terminal_diagnose_test.go index 15b94945..1364c2c2 100644 --- a/workspace-server/internal/handlers/terminal_diagnose_test.go +++ b/workspace-server/internal/handlers/terminal_diagnose_test.go @@ -245,3 +245,50 @@ func TestDiagnoseRemote_StopsAtSSHProbe(t *testing.T) { } } +// TestUnwrapGoError pins the unwrapGoError helper that extracts subprocess +// stderr from the Go-wrapped error string produced by sendSSHPublicKey. +// Regression gate for mc#687: the E2E smoke now reads detail (not error), +// so detail MUST contain the actionable AWS permission signal. +func TestUnwrapGoError(t *testing.T) { + cases := []struct { + name string + input string + want string + }{ + { + name: "AWS permission denied", + input: "send-ssh-public-key: exec: exit status 1 (AccessDeniedException: User: arn:aws:iam::123456789012:role/TestRole is not authorized to perform: ec2-instance-connect:OpenTunnel)", + want: "AccessDeniedException: User: arn:aws:iam::123456789012:role/TestRole is not authorized to perform: ec2-instance-connect:OpenTunnel", + }, + { + name: "generic exec error no output", + input: "send-ssh-public-key: exec: exit status 1", + want: "", + }, + { + name: "empty string", + input: "", + want: "", + }, + { + name: "short string below threshold", + input: "err", + want: "", + }, + { + name: "no parentheses", + input: "send-ssh-public-key: something went wrong", + want: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := unwrapGoError(tc.input) + if got != tc.want { + t.Errorf("unwrapGoError(%q): got %q, want %q", tc.input, got, tc.want) + } + }) + } +} +