Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/engram-cloud/troubleshooting.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ Cloud auth token is runtime config:
export ENGRAM_CLOUD_TOKEN="your-token"
```

The local `~/.engram/cloud.json` stores the server URL. The token is intentionally read from the environment.
The local `~/.engram/cloud.json` stores the server URL and may also store a `token` fallback. `ENGRAM_CLOUD_TOKEN` takes precedence over any token in `cloud.json`; if the env var is unset, Engram falls back to `cloud.json.token`. This fallback is intentional (issue #343) for use cases such as background autosync where exporting the env var on every shell is not practical.

---

Expand Down
24 changes: 24 additions & 0 deletions internal/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,11 @@ func (s *Store) MaxObservationLength() int {
return s.cfg.MaxObservationLength
}

// DataDir returns the configured data directory for the store.
func (s *Store) DataDir() string {
return s.cfg.DataDir
}

// ─── Store ───────────────────────────────────────────────────────────────────

type Store struct {
Expand Down Expand Up @@ -3761,6 +3766,25 @@ func (s *Store) CountPendingNonEnrolledSyncMutations(targetKey string) ([]Pendin
return counts, rows.Err()
}

// CountPendingSyncMutations returns the number of unacknowledged mutations for
// the given target that are currently eligible to sync (global/project-empty or
// enrolled projects).
func (s *Store) CountPendingSyncMutations(targetKey string) (int64, error) {
targetKey = normalizeSyncTargetKey(targetKey)
var count int64
row := s.db.QueryRow(`
SELECT COUNT(*)
FROM sync_mutations sm
LEFT JOIN sync_enrolled_projects sep ON sm.project = sep.project
WHERE sm.target_key = ? AND sm.acked_at IS NULL
AND (sm.project = '' OR sep.project IS NOT NULL)
`, targetKey)
if err := row.Scan(&count); err != nil {
return 0, err
}
return count, nil
}
Comment on lines +3769 to +3786

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

CountPendingSyncMutations eligibility semantics for global and non-enrolled mutations are unverified. The query's (sm.project = '' OR sep.project IS NOT NULL) filter depends on global mutations being persisted with an actual empty string (not SQL NULL, which this codebase often uses via nullableString(...) for optional columns elsewhere), and the exclusion of non-enrolled projects' mutations is never asserted by a test — both are core, previously-changed behaviors per the PR summary.

  • internal/store/store.go#L3769-L3786: confirm enqueueSyncMutationTx/backfillProjectSyncMutationsTx store global mutations with project = '' (not NULL) so sm.project = '' actually matches them.
  • internal/store/store_test.go#L1392-L1436: add a case creating a mutation for a non-enrolled project and assert it's excluded from the count (and ideally a global/project-empty case asserting inclusion).
📍 Affects 2 files
  • internal/store/store.go#L3769-L3786 (this comment)
  • internal/store/store_test.go#L1392-L1436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/store.go` around lines 3769 - 3786, The
CountPendingSyncMutations eligibility behavior needs verification. In
internal/store/store.go:3769-3786, inspect enqueueSyncMutationTx and
backfillProjectSyncMutationsTx and ensure global mutations persist project as an
empty string rather than SQL NULL so the existing filter matches them; in
internal/store/store_test.go:1392-1436, add coverage proving non-enrolled
project mutations are excluded and global/project-empty mutations are included.


// SkipAckNonEnrolledMutations acks (marks as skipped) all pending mutations
// that belong to non-enrolled projects, preventing journal bloat. Empty-project
// mutations are never skipped — they always sync regardless of enrollment.
Expand Down
53 changes: 53 additions & 0 deletions internal/store/store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1382,6 +1382,59 @@ func TestSessionObservationsAddPromptImportAndSyncChunks(t *testing.T) {
}
}

func TestStoreDataDir(t *testing.T) {
s := newTestStore(t)
if got := s.DataDir(); got == "" {
t.Fatal("DataDir should not be empty")
}
}

func TestCountPendingSyncMutations(t *testing.T) {
s := newTestStore(t)

count, err := s.CountPendingSyncMutations(DefaultSyncTargetKey)
if err != nil {
t.Fatalf("CountPendingSyncMutations empty: %v", err)
}
if count != 0 {
t.Fatalf("count = %d, want 0", count)
}

if err := s.EnrollProject("engram"); err != nil {
t.Fatalf("enroll: %v", err)
}
if err := s.CreateSession("s1", "engram", "/tmp/engram"); err != nil {
t.Fatalf("create session: %v", err)
}

count, err = s.CountPendingSyncMutations(DefaultSyncTargetKey)
if err != nil {
t.Fatalf("CountPendingSyncMutations after session: %v", err)
}
if count != 1 {
t.Fatalf("count = %d, want 1", count)
}

if _, err := s.AddObservation(AddObservationParams{
SessionID: "s1",
Type: "bugfix",
Title: "one",
Content: "content",
Project: "engram",
Scope: "project",
}); err != nil {
t.Fatalf("add observation: %v", err)
}

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

func TestStoreLocalSyncFoundationEnqueuesCoreMutations(t *testing.T) {
s := newTestStore(t)

Expand Down
231 changes: 231 additions & 0 deletions internal/tui/cloud.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,231 @@
package tui

import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/url"
"os"
"path/filepath"
"strconv"
"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 +58 to +72

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Cloud config file/dir permissions are world-readable despite storing a token.

saveCloudConfig creates the data dir with 0o755 and writes cloud.json with 0o644. Per the updated docs, this file can carry a token value (preserved across saves), so other local users can read it. os.WriteFile's mode only takes effect on file creation, so this weak permission persists for the file's lifetime once created.

🔒 Proposed fix: restrict 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)
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/tui/cloud.go` around lines 58 - 72, Restrict cloud configuration
permissions in saveCloudConfig by creating dataDir with owner-only access (0700)
and writing cloud.json with owner read/write access (0600). Ensure existing
files are also secured, since os.WriteFile’s mode does not change permissions
when the file already exists.


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)
}
Comment on lines +74 to +94

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Duplicate token-precedence logic between tokenSourceMessage and effectiveCloudToken.

Both functions re-implement the same env-var → file-token precedence and each calls loadCloudConfig independently. Given how subtle this precedence already is (see the dedicated empty-env-var regression test), keeping two copies risks them drifting out of sync.

♻️ Proposed fix: single source of truth
 func tokenSourceMessage(dataDir string) string {
-	if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" {
+	if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" {
 		return TokenSourceEnv
 	}
-	cc, err := loadCloudConfig(dataDir)
-	if err == nil && strings.TrimSpace(cc.Token) != "" {
+	if effectiveCloudToken(dataDir) != "" {
 		return TokenSourceFile
 	}
 	return TokenSourceNone
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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)
}
func tokenSourceMessage(dataDir string) string {
if strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")) != "" {
return TokenSourceEnv
}
if effectiveCloudToken(dataDir) != "" {
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)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/tui/cloud.go` around lines 74 - 94, Consolidate the shared
token-precedence lookup used by tokenSourceMessage and effectiveCloudToken into
a single source-of-truth helper that evaluates the environment token before the
cloud config token and loads the config only once per lookup. Update both
callers to reuse that helper while preserving TokenSourceEnv, TokenSourceFile,
TokenSourceNone, and empty-token behavior, including whitespace-only environment
values.


// 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
}
Comment on lines +106 to +159

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Trailing slash in server URL produces a double-slash health-check request.

validateCloudServerURL doesn't strip a trailing path slash, so https://host/ becomes https://host/ + /health = https://host//health at Line 112. This can make the ping fail for a perfectly valid, commonly-pasted URL. Failure degrades gracefully to "unreachable" rather than crashing, but it's an easy fix. TestValidateCloudServerURL also has no case covering this.

🐛 Proposed fix
 	parsed.RawQuery = ""
 	parsed.Fragment = ""
+	parsed.Path = strings.TrimRight(parsed.Path, "/")
 	return parsed.String(), nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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
}
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 = ""
parsed.Path = strings.TrimRight(parsed.Path, "/")
return parsed.String(), nil
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/tui/cloud.go` around lines 106 - 159, Update validateCloudServerURL
to remove trailing slashes from the validated base URL before
pingCloudServerStatus appends cloudHealthPath, while preserving the root URL and
existing validation behavior. Add a TestValidateCloudServerURL case verifying a
URL such as https://host/ is normalized so the health-check request does not
contain a double slash.


// isInsecureNoAuth mirrors cmd/engram/main.go:envBool for the
// ENGRAM_CLOUD_INSECURE_NO_AUTH env var. Truthy values are "1", "true",
// "yes" and "on" (case-insensitive, whitespace-trimmed).
func isInsecureNoAuth() bool {
v := strings.TrimSpace(strings.ToLower(os.Getenv("ENGRAM_CLOUD_INSECURE_NO_AUTH")))
return v == "1" || v == "true" || v == "yes" || v == "on"
}

// daemonProbeStatus describes the outcome of probing the local engram daemon.
type daemonProbeStatus string

const (
daemonProbeRunning daemonProbeStatus = "running"
daemonProbeNotRunning daemonProbeStatus = "not_running"
daemonProbeUnreachable daemonProbeStatus = "unreachable"
)

// daemonProbeResult captures the outcome of a single probe.
type daemonProbeResult struct {
Status daemonProbeStatus
Port int
Err error
}

const defaultDaemonProbePort = 7437

// daemonProbeTimeout is a var (not const) so tests can shorten it when
// exercising slow paths.
var daemonProbeTimeout = time.Second

// daemonProbeTransport can be overridden in tests to avoid real network calls.
var daemonProbeTransport http.RoundTripper = http.DefaultTransport

// probeLocalDaemon mirrors cmd/engram/cloud_daemon_probe.go:defaultCloudDaemonProbe.
// A dial error to 127.0.0.1 is interpreted as "not running"; any other error
// (timeout, non-2xx response, malformed reply) maps to "unreachable".
func probeLocalDaemon(ctx context.Context, port int) daemonProbeResult {
url := fmt.Sprintf("http://127.0.0.1:%d/health", port)
client := &http.Client{Timeout: daemonProbeTimeout, Transport: daemonProbeTransport}
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return daemonProbeResult{Status: daemonProbeUnreachable, Port: port, Err: err}
}
resp, err := client.Do(req)
if err != nil {
var opErr *net.OpError
if errors.As(err, &opErr) && opErr.Op == "dial" {
return daemonProbeResult{Status: daemonProbeNotRunning, Port: port, Err: err}
}
return daemonProbeResult{Status: daemonProbeUnreachable, Port: port, Err: err}
}
defer resp.Body.Close()
_, _ = io.Copy(io.Discard, resp.Body)
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
return daemonProbeResult{Status: daemonProbeRunning, Port: port}
}
return daemonProbeResult{Status: daemonProbeUnreachable, Port: port}
}

// resolveDaemonProbePort mirrors cmd/engram/cloud_daemon_probe.go:resolveDaemonProbePort.
// It reads ENGRAM_PORT and falls back to 7437.
func resolveDaemonProbePort() int {
if p := strings.TrimSpace(os.Getenv("ENGRAM_PORT")); p != "" {
if n, err := strconv.Atoi(p); err == nil && n > 0 && n < 65536 {
return n
}
}
return defaultDaemonProbePort
}


Loading
Loading