-
Notifications
You must be signed in to change notification settings - Fork 656
test(tui): close coverage gaps and harden env-var tests #611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
01d7d78
95b99ea
1e48b10
309da7e
19f7807
6200b80
84bfeff
fca5aa5
47f2b48
748f09f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,156 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package tui | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "encoding/json" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+5
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/url" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "path/filepath" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tea "github.com/charmbracelet/bubbletea" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cloudConfigFileName = "cloud.json" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cloudHealthPath = "/health" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Token source labels displayed on the Cloud Config screen. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TokenSourceEnv = "set via ENGRAM_CLOUD_TOKEN" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TokenSourceFile = "read from cloud.json" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| TokenSourceNone = "not set" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type tuiCloudConfig struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ServerURL string `json:"server_url"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Token string `json:"token,omitempty"` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func cloudConfigPath(dataDir string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return filepath.Join(dataDir, cloudConfigFileName) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func loadCloudConfig(dataDir string) (*tuiCloudConfig, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path := cloudConfigPath(dataDir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| b, err := os.ReadFile(path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if os.IsNotExist(err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &tuiCloudConfig{}, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var cc tuiCloudConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := json.Unmarshal(b, &cc); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &cc, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func saveCloudConfig(dataDir, serverURL string) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := os.MkdirAll(dataDir, 0o755); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc, err := loadCloudConfig(dataDir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc.ServerURL = serverURL | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| b, err := json.MarshalIndent(cc, "", " ") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return os.WriteFile(cloudConfigPath(dataDir), b, 0o644) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+53
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Cloud config file is written world/group-readable, exposing the auth token.
🔒 Proposed fix: restrict file permissions func saveCloudConfig(dataDir, serverURL string) error {
- if err := os.MkdirAll(dataDir, 0o755); err != nil {
+ if err := os.MkdirAll(dataDir, 0o700); err != nil {
return err
}
cc, err := loadCloudConfig(dataDir)
if err != nil {
return err
}
cc.ServerURL = serverURL
b, err := json.MarshalIndent(cc, "", " ")
if err != nil {
return err
}
- return os.WriteFile(cloudConfigPath(dataDir), b, 0o644)
+ return os.WriteFile(cloudConfigPath(dataDir), b, 0o600)
}Note: 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func tokenSourceMessage(dataDir string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return TokenSourceEnv | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc, err := loadCloudConfig(dataDir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err == nil && strings.TrimSpace(cc.Token) != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return TokenSourceFile | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return TokenSourceNone | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func effectiveCloudToken(dataDir string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if token := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")); token != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return token | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cc, err := loadCloudConfig(dataDir) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return strings.TrimSpace(cc.Token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // pingCloudTransport can be overridden in tests to avoid real network calls. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var pingCloudTransport http.RoundTripper = http.DefaultTransport | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func pingCloudServer(serverURL, token string) tea.Cmd { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return func() tea.Msg { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status, err := pingCloudServerStatus(serverURL, token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return cloudPingMsg{status: status, err: err} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func pingCloudServerStatus(serverURL, token string) (string, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| validatedURL, err := validateCloudServerURL(serverURL) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unreachable", err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req, err := http.NewRequest(http.MethodGet, validatedURL+cloudHealthPath, nil) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unreachable", err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if token != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| req.Header.Set("Authorization", "Bearer "+token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client := &http.Client{Timeout: 3 * time.Second, Transport: pingCloudTransport} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp, err := client.Do(req) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unreachable", err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer resp.Body.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| switch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case resp.StatusCode == http.StatusUnauthorized: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unauthorized", nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "reachable", nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unreachable", fmt.Errorf("unexpected status %d", resp.StatusCode) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func validateCloudServerURL(raw string) (string, error) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| trimmed := strings.TrimSpace(raw) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed, err := url.ParseRequestURI(trimmed) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "", err | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| scheme := strings.ToLower(strings.TrimSpace(parsed.Scheme)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if scheme != "http" && scheme != "https" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "", fmt.Errorf("scheme must be http or https") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if strings.TrimSpace(parsed.Host) == "" || strings.TrimSpace(parsed.Hostname()) == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "", fmt.Errorf("host is required") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if strings.TrimSpace(parsed.RawQuery) != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "", fmt.Errorf("query is not allowed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if strings.TrimSpace(parsed.Fragment) != "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "", fmt.Errorf("fragment is not allowed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed.RawQuery = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsed.Fragment = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return parsed.String(), nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add coverage for the non-enrolled-project exclusion branch.
The test only exercises mutations under an enrolled project. It never asserts that a mutation tied to a project that is not enrolled is excluded from the count — that's the actual new eligibility rule the
LEFT JOIN ... sep.project IS NOT NULLclause implements, and it's currently unverified.✅ Suggested additional assertion
count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) if err != nil { t.Fatalf("CountPendingSyncMutations after observation: %v", err) } if count != 2 { t.Fatalf("count = %d, want 2", count) } + + // A session/observation under a project that is NOT enrolled must not + // be counted as a pending, eligible-to-sync mutation. + if err := s.CreateSession("s2", "unenrolled-project", "/tmp/other"); err != nil { + t.Fatalf("create session for unenrolled project: %v", err) + } + count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey) + if err != nil { + t.Fatalf("CountPendingSyncMutations after unenrolled session: %v", err) + } + if count != 2 { + t.Fatalf("count = %d, want 2 (unenrolled project mutation should not count)", count) + } }As per path instructions,
**/*_test.go: "Verify coverage of happy path, error paths, and edge cases."📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions