diff --git a/cmd/engram/cloud.go b/cmd/engram/cloud.go index 4bcc9789..b948a849 100644 --- a/cmd/engram/cloud.go +++ b/cmd/engram/cloud.go @@ -2,11 +2,9 @@ package main import ( "context" - "encoding/json" "errors" "fmt" "log" - "net/url" "os" "path/filepath" "strings" @@ -19,6 +17,7 @@ import ( "github.com/Gentleman-Programming/engram/internal/cloud/constants" "github.com/Gentleman-Programming/engram/internal/cloud/dashboard" "github.com/Gentleman-Programming/engram/internal/cloud/remote" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/store" engramsync "github.com/Gentleman-Programming/engram/internal/sync" ) @@ -193,7 +192,7 @@ func backfillAllowedProjectMutationChunks(ctx context.Context, cs *cloudstore.Cl return nil } -var runUpgradeBootstrap = func(s *store.Store, project string, cc *cloudConfig) (*engramsync.UpgradeBootstrapResult, error) { +var runUpgradeBootstrap = func(s *store.Store, project string, cc *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { transport, err := remote.NewRemoteTransport(cc.ServerURL, cc.Token, project) if err != nil { return nil, err @@ -339,7 +338,7 @@ func cmdCloudUpgradeDoctor(cfg store.Config) { cloudConfigured := false if cc, cfgErr := resolveCloudRuntimeConfig(cfg); cfgErr == nil { if cc != nil { - if validated, err := validateCloudServerURL(cc.ServerURL); err == nil && strings.TrimSpace(validated) != "" { + if validated, err := cloudconfig.ValidateServerURL(cc.ServerURL); err == nil && strings.TrimSpace(validated) != "" { cloudConfigured = true } } @@ -492,7 +491,7 @@ func cmdCloudUpgradeBootstrap(cfg store.Config) { fatal(fmt.Errorf("cloud upgrade bootstrap requires configured cloud server")) return } - validatedURL, err := validateCloudServerURL(cc.ServerURL) + validatedURL, err := cloudconfig.ValidateServerURL(cc.ServerURL) if err != nil { fatal(fmt.Errorf("invalid cloud runtime server URL: %w", err)) return @@ -546,7 +545,16 @@ func captureUpgradeSnapshotBeforeBootstrap(s *store.Store, cfg store.Config, pro } var snapshot store.CloudUpgradeSnapshot - configBytes, err := os.ReadFile(cloudConfigPath(cfg)) + // Snapshot the raw bytes of cloud.json verbatim. The + // snapshot is later used to ROLLBACK the upgrade (writing + // snapshot.CloudConfigJSON back to disk). Decoding via + // cloudconfig.Load would lose any field the package's + // Config struct does not declare; raw bytes preserve the + // exact on-disk state. The path is read from + // cloudconfig.Path to align with the rest of the migration, + // but the read call stays os.ReadFile so the bytes are + // preserved unchanged. + configBytes, err := os.ReadFile(cloudconfig.Path(cfg.DataDir)) if err == nil { snapshot.CloudConfigPresent = true snapshot.CloudConfigJSON = string(configBytes) @@ -631,12 +639,12 @@ func cmdCloudUpgradeRollback(cfg store.Config) { return } if state.Snapshot.CloudConfigPresent { - if err := os.WriteFile(cloudConfigPath(cfg), []byte(state.Snapshot.CloudConfigJSON), 0o644); err != nil { + if err := os.WriteFile(cloudconfig.Path(cfg.DataDir), []byte(state.Snapshot.CloudConfigJSON), 0o644); err != nil { fatal(err) return } } else { - _ = os.Remove(cloudConfigPath(cfg)) + _ = os.Remove(cloudconfig.Path(cfg.DataDir)) } rolledBack, err := engramsync.RollbackProject(s, engramsync.UpgradeRollbackOptions{Project: project}) if err != nil { @@ -667,7 +675,7 @@ func cmdCloudStatus(cfg store.Config) { fmt.Println("Cloud status: not configured") return } - validatedURL, err := validateCloudServerURL(cc.ServerURL) + validatedURL, err := cloudconfig.ValidateServerURL(cc.ServerURL) if err != nil { fmt.Fprintf(os.Stderr, "error: invalid cloud runtime server URL: %v\n", err) exitFunc(1) @@ -768,43 +776,25 @@ func cmdCloudConfig(cfg store.Config) { fmt.Fprintln(os.Stderr, "error: server URL is required") exitFunc(1) } - validatedURL, err := validateCloudServerURL(cc.ServerURL) + validatedURL, err := cloudconfig.ValidateServerURL(cc.ServerURL) if err != nil { fmt.Fprintf(os.Stderr, "error: invalid server URL: %v\n", err) exitFunc(1) } cc.ServerURL = validatedURL - if err := saveCloudConfig(cfg, cc); err != nil { + // The local cloudConfig and cloudconfig.Config have + // identical underlying types (same field names, types, and + // JSON tags), so a Go type conversion preserves the JSON + // schema on disk. The migration replaces saveCloudConfig + // with the package's Save, which adds an os.Chmod + // normalization (per T-608.1's spec: 0o644 on every write). + if err := cloudconfig.Save(cfg.DataDir, (*cloudconfig.Config)(cc)); err != nil { fatal(err) return } fmt.Printf("✓ Cloud server set to %s\n", cc.ServerURL) } -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 cmdCloudServe() { runtimeCfg := cloud.ConfigFromEnv() if err := validateCloudServeAuthConfig(); err != nil { @@ -873,34 +863,3 @@ func normalizeAllowedProjects(projects []string) []string { } return normalized } - -func cloudConfigPath(cfg store.Config) string { - return filepath.Join(cfg.DataDir, "cloud.json") -} - -func loadCloudConfig(cfg store.Config) (*cloudConfig, error) { - path := cloudConfigPath(cfg) - b, err := os.ReadFile(path) - if err != nil { - if os.IsNotExist(err) { - return nil, nil - } - return nil, err - } - var cc cloudConfig - if err := json.Unmarshal(b, &cc); err != nil { - return nil, err - } - return &cc, nil -} - -func saveCloudConfig(cfg store.Config, cc *cloudConfig) error { - if err := os.MkdirAll(cfg.DataDir, 0o755); err != nil { - return err - } - b, err := json.MarshalIndent(cc, "", " ") - if err != nil { - return err - } - return os.WriteFile(cloudConfigPath(cfg), b, 0o644) -} diff --git a/cmd/engram/cloud_daemon_probe.go b/cmd/engram/cloud_daemon_probe.go index e91b7322..e2b85d56 100644 --- a/cmd/engram/cloud_daemon_probe.go +++ b/cmd/engram/cloud_daemon_probe.go @@ -2,94 +2,64 @@ package main import ( "context" - "errors" "fmt" - "io" - "net" - "net/http" "os" "strconv" "strings" - "time" -) - -// daemonProbeStatus describes the outcome of probing the local engram daemon. -type daemonProbeStatus string -const ( - daemonProbeRunning daemonProbeStatus = "running" - daemonProbeNotRunning daemonProbeStatus = "not_running" - daemonProbeUnreachable daemonProbeStatus = "unreachable" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" ) -// 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 the "server accepts but never replies" path. -var daemonProbeTimeout = time.Second - -// cloudDaemonProbe issues a short timeout GET to /health on the local engram -// HTTP server. Exposed as a variable so tests can stub it. -var cloudDaemonProbe = defaultCloudDaemonProbe - -// defaultCloudDaemonProbe performs a real HTTP GET against the local daemon. -// 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" so the -// user can distinguish "the daemon is gone" from "the daemon is misbehaving". -func defaultCloudDaemonProbe(ctx context.Context, port int) daemonProbeResult { - url := fmt.Sprintf("http://127.0.0.1:%d/health", port) - client := &http.Client{Timeout: daemonProbeTimeout} - 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} +// cloudDaemonProbe is a thin wrapper that calls +// cloudconfig.LocalDaemonProbe at invocation time so test +// stubs of the cloudconfig package's var seam (per ADR-1) +// reach printCloudStatusDaemonProbe without needing a +// separate local var seam. The wrapper is a function (not +// a var assigned at package init) so a later assignment to +// cloudconfig.LocalDaemonProbe is observed by the next call +// to printCloudStatusDaemonProbe. +func cloudDaemonProbe(ctx context.Context, port int) cloudconfig.Result { + return cloudconfig.LocalDaemonProbe(ctx, port) } -// resolveDaemonProbePort mirrors the port resolution used by cmdServe so the -// probe targets the same address the user's serve process is bound to. +// resolveDaemonProbePort mirrors the port resolution used by +// cmdServe so the probe targets the same address the user's +// serve process is bound to. The function delegates to +// cloudconfig.ResolvePort (added in T-608.4) but is kept here +// as a thin wrapper to preserve the existing call site in +// printCloudStatusDaemonProbe. The default port (7437) and +// the env var name ("ENGRAM_PORT") are exported from the +// cloudconfig package as DefaultProbePort and EnvProbePort. func resolveDaemonProbePort() int { - if p := strings.TrimSpace(os.Getenv("ENGRAM_PORT")); p != "" { + if p := strings.TrimSpace(os.Getenv(cloudconfig.EnvProbePort)); p != "" { if n, err := strconv.Atoi(p); err == nil && n > 0 && n < 65536 { return n } } - return defaultDaemonProbePort + return cloudconfig.DefaultProbePort } -// printCloudStatusDaemonProbe prints a single line describing whether the -// local engram daemon answers /health, plus a short hint when it is down. -// Exit code is unchanged: this is informational so cloud status remains a -// non-failing diagnostic surface. +// printCloudStatusDaemonProbe prints a single line describing +// whether the local engram daemon answers /health, plus a +// short hint when it is down. Exit code is unchanged: this is +// informational so cloud status remains a non-failing +// diagnostic surface. +// +// The function uses cloudconfig.ProbeStatus to classify the +// result; the labels rendered to stdout ("running", "not +// running", "unreachable") match ProbeStatus.String() so the +// output is byte-identical to the pre-migration behavior. The +// per-state hint lines (recovery hint for ProbeNotRunning, +// probe-error message for ProbeUnreachable) are also preserved. func printCloudStatusDaemonProbe() { port := resolveDaemonProbePort() - ctx, cancel := context.WithTimeout(context.Background(), daemonProbeTimeout) + ctx, cancel := context.WithTimeout(context.Background(), cloudconfig.ProbeTimeout) defer cancel() res := cloudDaemonProbe(ctx, port) switch res.Status { - case daemonProbeRunning: + case cloudconfig.ProbeRunning: fmt.Printf("Local daemon: running on port %d\n", res.Port) - case daemonProbeNotRunning: + case cloudconfig.ProbeNotRunning: fmt.Printf("Local daemon: not running on port %d\n", res.Port) fmt.Println("Hint: run `engram serve` to resume autosync; on macOS see DOCS.md launchd template to keep it alive across upgrades") default: diff --git a/cmd/engram/cloud_daemon_probe_test.go b/cmd/engram/cloud_daemon_probe_test.go index d5bbd8cf..5a2c84f1 100644 --- a/cmd/engram/cloud_daemon_probe_test.go +++ b/cmd/engram/cloud_daemon_probe_test.go @@ -3,96 +3,25 @@ package main import ( "context" "fmt" - "net" - "net/http" - "net/http/httptest" - "net/url" - "strconv" "strings" "testing" - "time" -) - -func TestDefaultCloudDaemonProbeReturnsRunningOn200(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/health" { - http.NotFound(w, r) - return - } - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte(`{"status":"ok"}`)) - })) - defer srv.Close() - - port := portFromTestServer(t, srv) - res := defaultCloudDaemonProbe(context.Background(), port) - if res.Status != daemonProbeRunning { - t.Fatalf("expected daemonProbeRunning, got %q (err=%v)", res.Status, res.Err) - } - if res.Port != port { - t.Fatalf("expected port %d, got %d", port, res.Port) - } -} - -func TestDefaultCloudDaemonProbeReturnsNotRunningOnRefused(t *testing.T) { - port := allocateClosedPort(t) - res := defaultCloudDaemonProbe(context.Background(), port) - if res.Status != daemonProbeNotRunning { - t.Fatalf("expected daemonProbeNotRunning on refused, got %q (err=%v)", res.Status, res.Err) - } - if res.Err == nil { - t.Fatalf("expected non-nil err for refused dial") - } -} - -func TestDefaultCloudDaemonProbeReturnsUnreachableOnNon2xx(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - })) - defer srv.Close() - - port := portFromTestServer(t, srv) - res := defaultCloudDaemonProbe(context.Background(), port) - if res.Status != daemonProbeUnreachable { - t.Fatalf("expected daemonProbeUnreachable on 500, got %q", res.Status) - } -} -func TestDefaultCloudDaemonProbeReturnsUnreachableOnTimeout(t *testing.T) { - prev := daemonProbeTimeout - daemonProbeTimeout = 100 * time.Millisecond - t.Cleanup(func() { daemonProbeTimeout = prev }) - - // Listener accepts but never reads/writes, forcing the client to time out. - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("listen: %v", err) - } - defer ln.Close() - done := make(chan struct{}) - go func() { - defer close(done) - conn, aerr := ln.Accept() - if aerr != nil { - return - } - // Hold the connection until the listener closes; never write a response. - <-done - _ = conn.Close() - }() - - port := ln.Addr().(*net.TCPAddr).Port - res := defaultCloudDaemonProbe(context.Background(), port) - if res.Status != daemonProbeUnreachable { - t.Fatalf("expected daemonProbeUnreachable on timeout, got %q (err=%v)", res.Status, res.Err) - } -} + "github.com/Gentleman-Programming/engram/internal/cloudconfig" +) +// TestResolveDaemonProbePortHonorsEnvAndDefaults pins the +// contract of the local resolveDaemonProbePort wrapper +// (cmd/engram/cloud_daemon_probe.go). The wrapper is a thin +// shim over cloudconfig.ResolvePort but the env-var name +// (cloudconfig.EnvProbePort = "ENGRAM_PORT") and the default +// (cloudconfig.DefaultProbePort = 7437) are part of the +// public surface and must remain stable for cmdCloudStatus +// and any future TUI parity work. func TestResolveDaemonProbePortHonorsEnvAndDefaults(t *testing.T) { t.Run("defaults to 7437 when ENGRAM_PORT unset", func(t *testing.T) { t.Setenv("ENGRAM_PORT", "") - if got := resolveDaemonProbePort(); got != defaultDaemonProbePort { - t.Fatalf("expected default %d, got %d", defaultDaemonProbePort, got) + if got := resolveDaemonProbePort(); got != cloudconfig.DefaultProbePort { + t.Fatalf("expected default %d, got %d", cloudconfig.DefaultProbePort, got) } }) t.Run("honors valid ENGRAM_PORT", func(t *testing.T) { @@ -103,39 +32,49 @@ func TestResolveDaemonProbePortHonorsEnvAndDefaults(t *testing.T) { }) t.Run("falls back to default on invalid ENGRAM_PORT", func(t *testing.T) { t.Setenv("ENGRAM_PORT", "not-a-number") - if got := resolveDaemonProbePort(); got != defaultDaemonProbePort { - t.Fatalf("expected default %d, got %d", defaultDaemonProbePort, got) + if got := resolveDaemonProbePort(); got != cloudconfig.DefaultProbePort { + t.Fatalf("expected default %d, got %d", cloudconfig.DefaultProbePort, got) } }) t.Run("falls back to default on out-of-range ENGRAM_PORT", func(t *testing.T) { t.Setenv("ENGRAM_PORT", "0") - if got := resolveDaemonProbePort(); got != defaultDaemonProbePort { - t.Fatalf("expected default %d, got %d", defaultDaemonProbePort, got) + if got := resolveDaemonProbePort(); got != cloudconfig.DefaultProbePort { + t.Fatalf("expected default %d, got %d", cloudconfig.DefaultProbePort, got) } t.Setenv("ENGRAM_PORT", "70000") - if got := resolveDaemonProbePort(); got != defaultDaemonProbePort { - t.Fatalf("expected default %d, got %d", defaultDaemonProbePort, got) + if got := resolveDaemonProbePort(); got != cloudconfig.DefaultProbePort { + t.Fatalf("expected default %d, got %d", cloudconfig.DefaultProbePort, got) } }) } +// TestPrintCloudStatusDaemonProbeFormatsEachState pins the +// per-state stdout output of printCloudStatusDaemonProbe. +// The test stubs cloudconfig.LocalDaemonProbe directly via +// the new var seam (T-608.15's porting step) — the local +// cloudDaemonProbe wrapper in cmd/engram/cloud_daemon_probe.go +// is still the caller's seam, but since the wrapper is a +// thin pass-through to cloudconfig.LocalDaemonProbe, stubbing +// the upstream var is equivalent to stubbing the wrapper and +// matches the design's ADR-1 pattern (vars in cloudconfig, not +// const funcs). func TestPrintCloudStatusDaemonProbeFormatsEachState(t *testing.T) { cases := []struct { name string - stub func(context.Context, int) daemonProbeResult + stub func(context.Context, int) cloudconfig.Result wantLines []string }{ { name: "running", - stub: func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeRunning, Port: port} + stub: func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} }, wantLines: []string{"Local daemon: running on port"}, }, { name: "not_running prints recovery hint", - stub: func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeNotRunning, Port: port} + stub: func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeNotRunning, Port: port} }, wantLines: []string{ "Local daemon: not running on port", @@ -145,8 +84,8 @@ func TestPrintCloudStatusDaemonProbeFormatsEachState(t *testing.T) { }, { name: "unreachable surfaces probe error", - stub: func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeUnreachable, Port: port, Err: fmt.Errorf("simulated boom")} + stub: func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeUnreachable, Port: port, Err: fmt.Errorf("simulated boom")} }, wantLines: []string{ "Local daemon: unreachable on port", @@ -158,9 +97,15 @@ func TestPrintCloudStatusDaemonProbeFormatsEachState(t *testing.T) { for _, tc := range cases { tc := tc t.Run(tc.name, func(t *testing.T) { - prev := cloudDaemonProbe - cloudDaemonProbe = tc.stub - t.Cleanup(func() { cloudDaemonProbe = prev }) + // Stub the cloudconfig package's LocalDaemonProbe var + // (the new seam introduced in T-608.15). The local + // cloudDaemonProbe wrapper in cmd/engram defaults to + // cloudconfig.LocalDaemonProbe, so stubbing the + // upstream var reaches printCloudStatusDaemonProbe + // without needing a separate local seam. + prev := cloudconfig.LocalDaemonProbe + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) + cloudconfig.LocalDaemonProbe = tc.stub stdout, _, recovered := captureOutputAndRecover(t, func() { printCloudStatusDaemonProbe() }) if recovered != nil { @@ -174,33 +119,3 @@ func TestPrintCloudStatusDaemonProbeFormatsEachState(t *testing.T) { }) } } - -// portFromTestServer extracts the TCP port a httptest.Server is bound to. -func portFromTestServer(t *testing.T, srv *httptest.Server) int { - t.Helper() - parsed, err := url.Parse(srv.URL) - if err != nil { - t.Fatalf("parse server URL: %v", err) - } - port, err := strconv.Atoi(parsed.Port()) - if err != nil { - t.Fatalf("parse server port: %v", err) - } - return port -} - -// allocateClosedPort returns a TCP port number that is guaranteed to be -// closed: it binds, reads the assigned port, then closes the listener. -// On loopback this reliably surfaces "connection refused" on dial. -func allocateClosedPort(t *testing.T) int { - t.Helper() - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatalf("listen: %v", err) - } - port := ln.Addr().(*net.TCPAddr).Port - if err := ln.Close(); err != nil { - t.Fatalf("close listener: %v", err) - } - return port -} diff --git a/cmd/engram/cloud_test.go b/cmd/engram/cloud_test.go new file mode 100644 index 00000000..b3a6f807 --- /dev/null +++ b/cmd/engram/cloud_test.go @@ -0,0 +1,1213 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gentleman-Programming/engram/internal/cloudconfig" + engramsync "github.com/Gentleman-Programming/engram/internal/sync" + "github.com/Gentleman-Programming/engram/internal/store" +) + +// runCloudUpgradeDoctor is the shared body for the +// TestCloudUpgradeDoctor* family. It: +// +// 1. stubs exit/runtime hooks so the command's fatal() and +// subprocesses don't crash the test process; +// 2. pins the env vars to empty so resolveCloudRuntimeConfig +// uses the on-disk cloud.json (not the parent process env); +// 3. opens a fresh store, enrolls "my-project", and closes +// the store so the upgrade doctor finds the project +// enrolled (otherwise the "not enrolled" branch masks +// the cloudConfigured signal we're testing); +// 4. writes a cloud.json with the supplied serverURL; +// 5. invokes cmdCloudUpgradeDoctor and captures stdout/stderr. +// +// The function returns the captured stdout. The caller asserts +// the report content (the "cloud configuration is required" +// sentinel from the legacy rejection MUST NOT appear, and +// "ready for cloud bootstrap" MUST appear when cloud is +// configured and the project is enrolled). +func runCloudUpgradeDoctor(t *testing.T, serverURL string) string { + t.Helper() + + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + + // Pin the runtime config to the on-disk cloud.json (do not + // let the parent process's env vars override it). With + // these set to empty strings, resolveCloudRuntimeConfig's + // env-override branch is skipped and the file's server_url + // wins. + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + // Enroll the project so the only blocking reason is + // cloudConfigured. Without enrollment, the report would be + // "not enrolled" regardless of cloudConfigured, masking + // the change we want to observe. + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + if err := s.EnrollProject("my-project"); err != nil { + _ = s.Close() + t.Fatalf("EnrollProject: %v", err) + } + _ = s.Close() + + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ + ServerURL: serverURL, + Token: "t", + }); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) + } + + withArgs(t, "engram", "cloud", "upgrade", "doctor", "--project", "my-project") + + stdout, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudUpgradeDoctor(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("doctor should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + return stdout +} + +// assertCloudUpgradeDoctorReady pins the contract that the +// upgrade doctor must report "ready" when cloud is configured +// (i.e., the URL was accepted by the validator) and the project +// is enrolled. The single sentinel we forbid is the +// "cloud configuration is required" message that the legacy +// validator produced by rejecting ?q=1/#frag — its absence +// is the proof the URL was accepted and cleared. +func assertCloudUpgradeDoctorReady(t *testing.T, url, stdout string) { + t.Helper() + if strings.Contains(stdout, "cloud configuration is required") { + t.Fatalf("URL %q should be accepted and cleared, but doctor reported cloud as unconfigured: %q", url, stdout) + } + if !strings.Contains(stdout, "ready for cloud bootstrap") { + t.Fatalf("URL %q: expected ready status, got: %q", url, stdout) + } + if !strings.Contains(stdout, "status: ready") { + t.Fatalf("URL %q: expected 'status: ready' in doctor output, got: %q", url, stdout) + } +} + +// TestCloudUpgradeDoctorAcceptsQueryAndFragment pins the spec +// REQ-1 behavior change at the call site +// cmd/engram/cloud.go:342: a cloud.json URL containing a query +// (?q=1) and/or fragment (#frag) must be ACCEPTED and cleared by +// the URL validator, not REJECTED as the legacy +// validateCloudServerURL did. +// +// The contract under test: +// +// validateCloudServerURL("https://cloud.example.test/?q=1#frag") +// OLD: returns ("", error("query is not allowed")) -> cloudConfigured = false +// NEW: returns ("https://cloud.example.test/", nil) -> cloudConfigured = true +// +// DiagnoseCloudUpgrade receives cloudConfigured=true (with the +// project enrolled) and reports the doctor as "ready for cloud +// bootstrap". The legacy rejection produced "cloud configuration +// is required before upgrade bootstrap". The test asserts the +// NEW behavior (the legacy message MUST NOT appear) and pins the +// full chain: cloud.json -> resolveCloudRuntimeConfig -> line 342 +// -> DiagnoseCloudUpgrade -> report status. +func TestCloudUpgradeDoctorAcceptsQueryAndFragment(t *testing.T) { + stdout := runCloudUpgradeDoctor(t, "https://cloud.example.test/?q=1#frag") + assertCloudUpgradeDoctorReady(t, "https://cloud.example.test/?q=1#frag", stdout) +} + +// TestCloudUpgradeDoctorAcceptsCleanURL guards the existing happy +// path: a clean URL (no query, no fragment) must still be +// accepted after the migration. This is the regression test for +// the pre-existing call-site behavior; it would pass with BOTH +// the legacy and the new validator, but it stays in the slice +// to document the contract. +func TestCloudUpgradeDoctorAcceptsCleanURL(t *testing.T) { + stdout := runCloudUpgradeDoctor(t, "https://cloud.example.test/") + assertCloudUpgradeDoctorReady(t, "https://cloud.example.test/", stdout) +} + +// TestCloudUpgradeDoctorURLAcceptanceMatrix is a TRIANGULATE +// table that exercises the call site cmd/engram/cloud.go:342 +// against the broader URL matrix the spec REQ-1 promises: only +// a query, only a fragment, both, a path+query, and a port. All +// of these are accepted by cloudconfig.ValidateServerURL and +// would have been REJECTED by the legacy validateCloudServerURL. +// +// The table reads each row's URL, writes it to cloud.json, runs +// cmdCloudUpgradeDoctor, and asserts the doctor reports +// "ready" (because cloudConfigured=true and the project is +// enrolled). The legacy rejection produced "cloud configuration +// is required"; the migrated function must NOT produce that +// message for any of these rows. +func TestCloudUpgradeDoctorURLAcceptanceMatrix(t *testing.T) { + cases := []struct { + name string + url string + }{ + {name: "query only", url: "https://cloud.example.test/?q=1"}, + {name: "fragment only", url: "https://cloud.example.test/#frag"}, + {name: "query and fragment", url: "https://cloud.example.test/?q=1#frag"}, + {name: "path with query", url: "https://cloud.example.test/api/v1?token=abc"}, + {name: "port with query", url: "https://cloud.example.test:8443/?q=1"}, + {name: "path with fragment", url: "https://cloud.example.test/api/v1#section"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + stdout := runCloudUpgradeDoctor(t, tc.url) + assertCloudUpgradeDoctorReady(t, tc.url, stdout) + }) + } +} + +// runCloudUpgradeBootstrap is the shared body for the +// TestCloudUpgradeBootstrap* family. It: +// +// 1. stubs exit/runtime hooks so the command's fatal() and +// subprocesses don't crash the test process; +// 2. pins the env vars to empty so resolveCloudRuntimeConfig +// uses the on-disk cloud.json (not the parent process env); +// 3. opens a fresh store, enrolls "my-project", and closes +// the store so the snapshot/legacy-mutation checks pass +// (otherwise the "not enrolled" branch masks the +// cloudConfigured signal we're testing); +// 4. writes a cloud.json with the supplied serverURL; +// 5. stubs runUpgradeBootstrap (a package-level var seam) to +// a deterministic no-op result so the test does not make +// real HTTP calls; +// 6. invokes cmdCloudUpgradeBootstrap and captures stdout. +// +// The function returns the captured stdout. The caller asserts +// the URL was accepted (no panic from the exit stub) and the +// function reached the print stage (stdout contains +// "project: my-project" and "stage: test-stage"). +func runCloudUpgradeBootstrap(t *testing.T, serverURL string) string { + t.Helper() + + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + + // Pin the runtime config to the on-disk cloud.json (do not + // let the parent process's env vars override it). With + // these set to empty strings, resolveCloudRuntimeConfig's + // env-override branch is skipped and the file's server_url + // wins. + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + // Enroll the project so the snapshot/legacy-mutation checks + // pass. Without enrollment, the snapshot function would + // still succeed, but the legacy-mutation diagnosis would be + // the next blocking branch we don't want to exercise here. + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + if err := s.EnrollProject("my-project"); err != nil { + _ = s.Close() + t.Fatalf("EnrollProject: %v", err) + } + _ = s.Close() + + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ + ServerURL: serverURL, + Token: "t", + }); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) + } + + // Stub the HTTP bootstrap call so the test does not hit a + // real cloud server. The var is restored in the cleanup. + origRunUpgradeBootstrap := runUpgradeBootstrap + t.Cleanup(func() { runUpgradeBootstrap = origRunUpgradeBootstrap }) + runUpgradeBootstrap = func(_ *store.Store, _ string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { + return &engramsync.UpgradeBootstrapResult{ + Project: "my-project", + Stage: "test-stage", + Resumed: false, + NoOp: true, + }, nil + } + + withArgs(t, "engram", "cloud", "upgrade", "bootstrap", "--project", "my-project") + + stdout, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudUpgradeBootstrap(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("bootstrap should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + return stdout +} + +// assertCloudUpgradeBootstrapAccepted pins the contract that the +// bootstrap must reach the print stage when the URL was accepted +// by the validator. The print stage is the proof that the +// function passed URL validation, snapshot capture, legacy +// mutation diagnosis, and the stubbed runUpgradeBootstrap. +// +// Note: the legacy rejection path surfaces via the exit stub's +// panic, which is caught by captureOutputAndRecover and surfaces +// as the test's `recovered` value. The helper additionally +// guards against any future change that routes the URL error to +// stderr instead of the exit path. +func assertCloudUpgradeBootstrapAccepted(t *testing.T, url, stdout string) { + t.Helper() + if !strings.Contains(stdout, "project: my-project") { + t.Fatalf("URL %q: expected 'project: my-project' in bootstrap output, got: %q", url, stdout) + } + if !strings.Contains(stdout, "stage: test-stage") { + t.Fatalf("URL %q: expected 'stage: test-stage' in bootstrap output (stub ran), got: %q", url, stdout) + } + if !strings.Contains(stdout, "resumed: false") { + t.Fatalf("URL %q: expected 'resumed: false' in bootstrap output, got: %q", url, stdout) + } + if !strings.Contains(stdout, "noop: true") { + t.Fatalf("URL %q: expected 'noop: true' in bootstrap output, got: %q", url, stdout) + } +} + +// TestCloudUpgradeBootstrapAcceptsQueryAndFragment pins the spec +// REQ-1 behavior change at the call site +// cmd/engram/cloud.go:496: a cloud.json URL containing a query +// (?q=1) and/or fragment (#frag) must be ACCEPTED and cleared by +// the URL validator, not REJECTED as the legacy +// validateCloudServerURL did. +// +// The contract under test: +// +// validateCloudServerURL("https://cloud.example.test/?q=1#frag") +// OLD: returns ("", error("query is not allowed")) -> fatal("invalid cloud runtime server URL: query is not allowed") +// NEW: returns ("https://cloud.example.test/", nil) -> proceed to snapshot/legacy/bootstrap +// +// cmdCloudUpgradeBootstrap receives a clean URL and proceeds to +// the print stage, emitting "project: my-project" via the +// stubbed runUpgradeBootstrap. The legacy rejection produced a +// fatal panic before any output was printed. The test asserts +// the NEW behavior (the print stage was reached) and pins the +// full chain: cloud.json -> resolveCloudRuntimeConfig -> line 496 +// -> snapshot/legacy/bootstrap -> print. +func TestCloudUpgradeBootstrapAcceptsQueryAndFragment(t *testing.T) { + stdout := runCloudUpgradeBootstrap(t, "https://cloud.example.test/?q=1#frag") + assertCloudUpgradeBootstrapAccepted(t, "https://cloud.example.test/?q=1#frag", stdout) +} + +// TestCloudUpgradeBootstrapAcceptsCleanURL guards the existing +// happy path: a clean URL (no query, no fragment) must still be +// accepted after the migration. This is the regression test for +// the pre-existing call-site behavior; it would pass with BOTH +// the legacy and the new validator, but it stays in the slice +// to document the contract. +func TestCloudUpgradeBootstrapAcceptsCleanURL(t *testing.T) { + stdout := runCloudUpgradeBootstrap(t, "https://cloud.example.test/") + assertCloudUpgradeBootstrapAccepted(t, "https://cloud.example.test/", stdout) +} + +// TestCloudUpgradeBootstrapURLAcceptanceMatrix is a TRIANGULATE +// table that exercises the call site cmd/engram/cloud.go:496 +// against the broader URL matrix the spec REQ-1 promises: only +// a query, only a fragment, both, a path+query, and a port. All +// of these are accepted by cloudconfig.ValidateServerURL and +// would have been REJECTED by the legacy validateCloudServerURL. +// +// The table reads each row's URL, writes it to cloud.json, runs +// cmdCloudUpgradeBootstrap, and asserts the bootstrap reached +// the print stage (project: my-project). The legacy rejection +// produced a fatal panic before any output; the migrated +// function must reach the print stage for all of these rows. +func TestCloudUpgradeBootstrapURLAcceptanceMatrix(t *testing.T) { + cases := []struct { + name string + url string + }{ + {name: "query only", url: "https://cloud.example.test/?q=1"}, + {name: "fragment only", url: "https://cloud.example.test/#frag"}, + {name: "query and fragment", url: "https://cloud.example.test/?q=1#frag"}, + {name: "path with query", url: "https://cloud.example.test/api/v1?token=abc"}, + {name: "port with query", url: "https://cloud.example.test:8443/?q=1"}, + {name: "path with fragment", url: "https://cloud.example.test/api/v1#section"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + stdout := runCloudUpgradeBootstrap(t, tc.url) + assertCloudUpgradeBootstrapAccepted(t, tc.url, stdout) + }) + } +} + +// TestCloudUpgradeBootstrapEarlyReturnOnEmptyServerURL pins the +// early-return guard at cmd/engram/cloud.go:492: when +// cc == nil || cc.ServerURL == "", the function must return +// with a fatal "cloud upgrade bootstrap requires configured +// cloud server" error and MUST NOT call the URL validator. +// +// The migration from validateCloudServerURL to +// cloudconfig.ValidateServerURL must NOT change this guard. +// The test writes no cloud.json, so resolveCloudRuntimeConfig +// returns cc with ServerURL="" (from the new Load's zero-value +// behavior per T-608.1), and the guard fires before the +// validator is called. +// +// The test asserts: +// - the function panics (via the exit stub) — the guard's +// fatal() call triggers the exit stub, which panics; +// - stderr contains the guard's error message +// "cloud upgrade bootstrap requires configured cloud server"; +// - stderr does NOT contain "invalid cloud runtime server URL" +// (which would mean the validator ran with an empty URL). +// +// The exit stub panics with the exit code (an int), so the +// recovered panic value is not the error message. The error +// message is printed to stderr by the fatal() helper before the +// exit call. +func TestCloudUpgradeBootstrapEarlyReturnOnEmptyServerURL(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + + // Pin env to empty so resolveCloudRuntimeConfig uses the + // file (which does not exist, so cc.ServerURL="" via the + // new Load's zero-value). + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + // Do NOT write cloud.json — cc.ServerURL must be empty so + // the guard fires. + + withArgs(t, "engram", "cloud", "upgrade", "bootstrap", "--project", "my-project") + + _, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudUpgradeBootstrap(cfg) + }) + if recovered == nil { + t.Fatal("expected fatal panic from early-return guard, got no panic") + } + if !strings.Contains(stderr, "cloud upgrade bootstrap requires configured cloud server") { + t.Fatalf("expected early-return guard message in stderr, got: %q", stderr) + } + if strings.Contains(stderr, "invalid cloud runtime server URL") { + t.Fatalf("URL validator must not run when ServerURL is empty (got stderr: %q)", stderr) + } +} + +// ---------------------------------------------------------------------------- +// T-608.8: cmdCloudConfig migration to cloudconfig +// ---------------------------------------------------------------------------- + +// runCloudConfig is the shared body for the TestCloudConfig* family. +// It: +// +// 1. stubs the exit hook so the command's exitFunc() calls do not +// crash the test process (the exit stub panics with the exit +// code, and captureOutputAndRecover catches it); +// 2. pins ENGRAM_CLOUD_SERVER and ENGRAM_CLOUD_TOKEN to empty so +// resolveCloudRuntimeConfig is not on the call path (this test +// only exercises the --server flag path); +// 3. sets up os.Args to "engram cloud config --server "; +// 4. invokes cmdCloudConfig with the supplied config and +// captures stdout/stderr. +// +// The function returns the captured stdout. The caller asserts +// the URL was accepted (no panic), the success message contains +// the cleaned URL, and the cloud.json file is at the expected +// location with the expected mode. +func runCloudConfig(t *testing.T, serverURL string) (string, store.Config) { + t.Helper() + + stubExitWithPanic(t) + + cfg := testConfig(t) + + // Pin env to empty so the command does not pick up + // ENGRAM_CLOUD_SERVER / ENGRAM_CLOUD_TOKEN from the parent + // test process's environment. + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + withArgs(t, "engram", "cloud", "config", "--server", serverURL) + + stdout, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudConfig(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("config should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + return stdout, cfg +} + +// assertCloudConfigSaved pins the contract that cmdCloudConfig +// must, when the URL is accepted by the validator, write a +// cloud.json at cloudconfig.Path(cfg.DataDir) with mode 0o644, +// inside a directory with mode 0o755, and print the expected +// success line. The single sentinel we forbid is the legacy +// "error: invalid server URL" message — its absence is the +// proof the URL was accepted by the new validator. +func assertCloudConfigSaved(t *testing.T, cfg store.Config, inputURL, cleanedURL, stdout string) { + t.Helper() + if strings.Contains(stdout, "error: invalid server URL") { + t.Fatalf("URL %q should be accepted and cleared, but config reported invalid URL: %q", inputURL, stdout) + } + expected := "✓ Cloud server set to " + cleanedURL + if !strings.Contains(stdout, expected) { + t.Fatalf("URL %q: expected %q in config output, got: %q", inputURL, expected, stdout) + } + + path := cloudconfig.Path(cfg.DataDir) + info, err := os.Stat(path) + if err != nil { + t.Fatalf("URL %q: expected cloud.json at %s, stat: %v", inputURL, path, err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Fatalf("URL %q: expected cloud.json mode 0o644, got %#o", inputURL, got) + } + + dirInfo, err := os.Stat(cfg.DataDir) + if err != nil { + t.Fatalf("URL %q: expected DataDir at %s, stat: %v", inputURL, cfg.DataDir, err) + } + if got := dirInfo.Mode().Perm(); got != 0o755 { + t.Fatalf("URL %q: expected DataDir mode 0o755, got %#o", inputURL, got) + } +} + +// TestCloudConfigAcceptsQueryAndFragment pins the spec REQ-1 +// behavior change at the call site cmd/engram/cloud.go:772 +// (the validateCloudServerURL call inside cmdCloudConfig): +// a server URL containing a query (?q=1) and/or fragment +// (#frag) must be ACCEPTED and cleared by the URL validator, +// not REJECTED as the legacy validateCloudServerURL did. +// +// The contract under test: +// +// validateCloudServerURL("https://cloud.example.test/?q=1#frag") +// OLD: returns ("", error("query is not allowed")) +// -> cmdCloudConfig calls exitFunc(1) (panics under test) +// NEW: returns ("https://cloud.example.test/", nil) +// -> cmdCloudConfig saves and prints the success line +// +// The test pins the full chain: os.Args -> cmdCloudConfig -> +// validator -> save -> print. The legacy rejection surfaced as +// a panic from the exit stub; the migrated function must reach +// the save+print stage. +func TestCloudConfigAcceptsQueryAndFragment(t *testing.T) { + stdout, cfg := runCloudConfig(t, "https://cloud.example.test/?q=1#frag") + assertCloudConfigSaved(t, cfg, "https://cloud.example.test/?q=1#frag", "https://cloud.example.test/", stdout) +} + +// TestCloudConfigAcceptsCleanURL guards the existing happy +// path: a clean URL (no query, no fragment) must still be +// accepted after the migration. This is the regression test for +// the pre-existing call-site behavior; it would pass with BOTH +// the legacy and the new validator, but it stays in the slice +// to document the contract. +func TestCloudConfigAcceptsCleanURL(t *testing.T) { + stdout, cfg := runCloudConfig(t, "https://cloud.example.test/") + assertCloudConfigSaved(t, cfg, "https://cloud.example.test/", "https://cloud.example.test/", stdout) +} + +// TestCloudConfigURLAcceptanceMatrix is a TRIANGULATE table +// that exercises the call site cmd/engram/cloud.go:772 against +// the broader URL matrix the spec REQ-1 promises: only a query, +// only a fragment, both, a path+query, a port+query, and a +// path+fragment. All of these are accepted by +// cloudconfig.ValidateServerURL and would have been REJECTED +// by the legacy validateCloudServerURL (the query cases, at +// least — the legacy fragment check is dead code because +// url.ParseRequestURI strips the fragment). +// +// The table reads each row's URL, runs cmdCloudConfig, and +// asserts the save+print contract: cloud.json exists at +// cloudconfig.Path with mode 0o644, and stdout contains the +// "✓ Cloud server set to " line. +func TestCloudConfigURLAcceptanceMatrix(t *testing.T) { + cases := []struct { + name string + input string + cleaned string + }{ + {name: "query only", input: "https://cloud.example.test/?q=1", cleaned: "https://cloud.example.test/"}, + {name: "fragment only", input: "https://cloud.example.test/#frag", cleaned: "https://cloud.example.test/"}, + {name: "query and fragment", input: "https://cloud.example.test/?q=1#frag", cleaned: "https://cloud.example.test/"}, + {name: "path with query", input: "https://cloud.example.test/api/v1?token=abc", cleaned: "https://cloud.example.test/api/v1"}, + {name: "port with query", input: "https://cloud.example.test:8443/?q=1", cleaned: "https://cloud.example.test:8443/"}, + {name: "path with fragment", input: "https://cloud.example.test/api/v1#section", cleaned: "https://cloud.example.test/api/v1"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + stdout, cfg := runCloudConfig(t, tc.input) + assertCloudConfigSaved(t, cfg, tc.input, tc.cleaned, stdout) + }) + } +} + +// TestCloudConfigSavesToCloudconfigPath pins the migration +// shape: the save call inside cmdCloudConfig must produce a +// cloud.json at cloudconfig.Path(cfg.DataDir). This is +// functionally equivalent to the legacy cloudConfigPath(cfg), +// but the migration must use the new package's path function +// (per spec REQ-2). The test verifies the file is at the +// expected location and the mode is 0o644 per the T-608.1 +// spec's file-mode guarantee. +func TestCloudConfigSavesToCloudconfigPath(t *testing.T) { + stdout, cfg := runCloudConfig(t, "https://cloud.example.test/") + + path := cloudconfig.Path(cfg.DataDir) + if !strings.HasSuffix(path, filepath.Join("cloud.json")) { + t.Fatalf("expected path to end with cloud.json, got %q", path) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("expected cloud.json at %s, stat: %v", path, err) + } + if !strings.Contains(stdout, "✓ Cloud server set to https://cloud.example.test/") { + t.Fatalf("expected success line, got: %q", stdout) + } +} + +// TestCloudConfigLoadAfterSaveRoundTrip is a TRIANGULATE test +// that verifies the save path produces JSON that is compatible +// with cloudconfig.Load. The local cmdCloudConfig writes a +// cloud.json via cloudconfig.Save (which uses +// json.Marshal of cloudconfig.Config). A subsequent +// cloudconfig.Load must read back the same fields. +// +// This guards against a regression where the migration +// accidentally passes a wrong type to cloudconfig.Save (e.g., +// a non-pointer or a struct missing the JSON tags) and the +// file is written with an unexpected schema. If the schema +// drifts, the load would either fail to decode or read back +// empty fields. +func TestCloudConfigLoadAfterSaveRoundTrip(t *testing.T) { + _, cfg := runCloudConfig(t, "https://cloud.example.test/") + + loaded, err := cloudconfig.Load(cfg.DataDir) + if err != nil { + t.Fatalf("cloudconfig.Load: %v", err) + } + if loaded == nil { + t.Fatal("cloudconfig.Load returned nil Config") + } + if loaded.ServerURL != "https://cloud.example.test/" { + t.Fatalf("expected ServerURL to round-trip, got %q", loaded.ServerURL) + } + if loaded.Token != "" { + t.Fatalf("expected empty Token, got %q", loaded.Token) + } +} + +// TestCloudConfigCreatesMissingDirectory is a TRIANGULATE +// test that pins the MkdirAll contract: when the data +// directory does not exist yet, cloudconfig.Save (called +// from cmdCloudConfig) must create it via os.MkdirAll(dataDir, +// 0o755). The legacy saveCloudConfig also did this; the +// migration must preserve it. +// +// The test sets cfg.DataDir to a subdirectory of t.TempDir() +// that does not exist yet, runs cmdCloudConfig, and asserts +// the directory was created with mode 0o755. +func TestCloudConfigCreatesMissingDirectory(t *testing.T) { + stubExitWithPanic(t) + + parent := t.TempDir() + cfg := testConfig(t) + cfg.DataDir = filepath.Join(parent, "subdir", "data") + + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + withArgs(t, "engram", "cloud", "config", "--server", "https://cloud.example.test/") + + _, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudConfig(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("config should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + + dirInfo, err := os.Stat(cfg.DataDir) + if err != nil { + t.Fatalf("expected DataDir to be created at %s, stat: %v", cfg.DataDir, err) + } + if !dirInfo.IsDir() { + t.Fatalf("expected %s to be a directory", cfg.DataDir) + } + if got := dirInfo.Mode().Perm(); got != 0o755 { + t.Fatalf("expected DataDir mode 0o755, got %#o", got) + } + + path := cloudconfig.Path(cfg.DataDir) + info, err := os.Stat(path) + if err != nil { + t.Fatalf("expected cloud.json at %s, stat: %v", path, err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Fatalf("expected cloud.json mode 0o644, got %#o", got) + } +} + +// TestCloudConfigNormalizesFileMode is a TRIANGULATE test +// that pins the os.Chmod contract from T-608.1: when +// cloudconfig.Save writes to a pre-existing file, it must +// chmod the file to 0o644 regardless of the prior mode. The +// legacy saveCloudConfig did NOT do this — it only set the +// mode on creation, leaving existing files at whatever mode +// they had. +// +// The test pre-creates cloud.json with mode 0o600 (intentionally +// wrong), runs cmdCloudConfig, and asserts the on-disk mode is +// 0o644 after the save. +func TestCloudConfigNormalizesFileMode(t *testing.T) { + stubExitWithPanic(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + // Pre-create the data directory and a cloud.json with the + // wrong mode (0o600 instead of 0o644). This is the state + // a user would have if they saved via a buggy external + // tool or a permission-resetting chmod. + if err := os.MkdirAll(cfg.DataDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := cloudconfig.Path(cfg.DataDir) + if err := os.WriteFile(path, []byte(`{"server_url":"https://old.example.test/","token":"old"}`), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + withArgs(t, "engram", "cloud", "config", "--server", "https://cloud.example.test/") + + _, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudConfig(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("config should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("expected cloud.json at %s, stat: %v", path, err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Fatalf("expected cloud.json mode 0o644 (chmod normalized), got %#o", got) + } + + // Verify the new URL was written, not the old one. + loaded, err := cloudconfig.Load(cfg.DataDir) + if err != nil { + t.Fatalf("cloudconfig.Load: %v", err) + } + if loaded.ServerURL != "https://cloud.example.test/" { + t.Fatalf("expected new ServerURL after save, got %q", loaded.ServerURL) + } +} + +// ---------------------------------------------------------------------------- +// T-608.9: snapshot writeback migration to cloudconfig.Path +// ---------------------------------------------------------------------------- + +// runCloudUpgradeBootstrapWithCustomCloudJSON is the +// T-608.9-specific helper that builds on the T-608.7 +// runCloudUpgradeBootstrap flow but writes a custom +// cloud.json (with potentially-sentinel data) instead of +// using saveCloudConfig to write a standard one. This +// lets the snapshot writeback test verify the raw-bytes +// contract: the snapshot.CloudConfigJSON must equal the +// exact bytes of the cloud.json file, including any +// sentinel data that json.Marshal would strip. +// +// The function returns the captured stdout, the test +// config (so callers can read the post-bootstrap store +// state), and the raw bytes that were written to +// cloud.json (so callers can compare the snapshot against +// them). +func runCloudUpgradeBootstrapWithCustomCloudJSON(t *testing.T, rawConfigJSON string) (string, store.Config, []byte) { + t.Helper() + + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + if err := s.EnrollProject("my-project"); err != nil { + _ = s.Close() + t.Fatalf("EnrollProject: %v", err) + } + _ = s.Close() + + // Write the custom cloud.json (bypassing saveCloudConfig + // so the test can include sentinel data that json.Marshal + // would not produce). + path := cloudconfig.Path(cfg.DataDir) + if err := os.MkdirAll(cfg.DataDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(path, []byte(rawConfigJSON), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + origRunUpgradeBootstrap := runUpgradeBootstrap + t.Cleanup(func() { runUpgradeBootstrap = origRunUpgradeBootstrap }) + runUpgradeBootstrap = func(_ *store.Store, _ string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { + return &engramsync.UpgradeBootstrapResult{ + Project: "my-project", + Stage: "test-stage", + Resumed: false, + NoOp: true, + }, nil + } + + withArgs(t, "engram", "cloud", "upgrade", "bootstrap", "--project", "my-project") + + stdout, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudUpgradeBootstrap(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("bootstrap should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + return stdout, cfg, []byte(rawConfigJSON) +} + +// loadSnapshotForProject opens the store and reads the +// cloud upgrade state for the test project. The snapshot +// is the contract under test: it must contain the raw +// bytes of cloud.json (not decoded JSON). +func loadSnapshotForProject(t *testing.T, cfg store.Config, project string) store.CloudUpgradeSnapshot { + t.Helper() + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + defer s.Close() + state, err := s.GetCloudUpgradeState(project) + if err != nil { + t.Fatalf("GetCloudUpgradeState: %v", err) + } + if state == nil { + t.Fatal("expected cloud upgrade state to be saved by the bootstrap snapshot") + } + return state.Snapshot +} + +// TestCloudUpgradeBootstrapSnapshotWritebackUsesRawBytes +// pins the spec REQ-2 contract at the call site +// cmd/engram/cloud.go:550: the snapshot writeback inside +// captureUpgradeSnapshotBeforeBootstrap must read cloud.json +// as RAW BYTES (via os.ReadFile), not via cloudconfig.Load +// (which would decode and drop unknown fields). +// +// The test writes a cloud.json with a sentinel "sentinel" +// field that cloudconfig.Config does not declare. If the +// snapshot used cloudconfig.Load + json.Marshal, the +// sentinel would be lost. If the snapshot uses +// os.ReadFile(cloudconfig.Path(...)), the sentinel is +// preserved verbatim. +// +// The test asserts: +// +// - the snapshot.CloudConfigJSON contains the exact bytes +// of the cloud.json file, including the sentinel; +// - the snapshot.CloudConfigPresent is true. +// +// This is the critical contract: the snapshot is used to +// ROLLBACK the upgrade (T-608.9+), and the rollback writes +// the snapshot back to disk. If the snapshot were decoded +// JSON, the rollback would lose any data the original file +// had but cloudconfig.Config does not declare. +func TestCloudUpgradeBootstrapSnapshotWritebackUsesRawBytes(t *testing.T) { + rawConfigJSON := `{"server_url":"https://cloud.example.test/","token":"t","sentinel":"fingerprint"}` + + _, cfg, rawBytes := runCloudUpgradeBootstrapWithCustomCloudJSON(t, rawConfigJSON) + + snapshot := loadSnapshotForProject(t, cfg, "my-project") + if !snapshot.CloudConfigPresent { + t.Fatal("expected snapshot.CloudConfigPresent=true, got false") + } + if snapshot.CloudConfigJSON != string(rawBytes) { + t.Fatalf("snapshot must equal raw cloud.json bytes.\nwant: %q\ngot: %q", string(rawBytes), snapshot.CloudConfigJSON) + } + // The sentinel must survive — this is the proof the + // snapshot used os.ReadFile, not cloudconfig.Load. + if !strings.Contains(snapshot.CloudConfigJSON, "sentinel") { + t.Fatalf("snapshot must contain the sentinel field (proves raw bytes, not decoded): %q", snapshot.CloudConfigJSON) + } +} + +// TestCloudUpgradeBootstrapSnapshotWritebackPathMatchesCloudconfigPath +// pins the migration shape: the snapshot's source file +// must be at cloudconfig.Path(cfg.DataDir). The test +// verifies the snapshot's CloudConfigJSON equals the +// bytes of the file at cloudconfig.Path, which is the +// only file the snapshot is allowed to read. +// +// This is functionally equivalent to the legacy +// cloudConfigPath(cfg), but the migration must use the +// new package's path function (per spec REQ-2). The +// test pins the path identity by comparing the snapshot +// to the file's bytes. +func TestCloudUpgradeBootstrapSnapshotWritebackPathMatchesCloudconfigPath(t *testing.T) { + rawConfigJSON := `{"server_url":"https://cloud.example.test/","token":"t"}` + + _, cfg, rawBytes := runCloudUpgradeBootstrapWithCustomCloudJSON(t, rawConfigJSON) + + path := cloudconfig.Path(cfg.DataDir) + if !strings.HasSuffix(path, filepath.Join("cloud.json")) { + t.Fatalf("expected path to end with cloud.json, got %q", path) + } + onDisk, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(onDisk) != string(rawBytes) { + t.Fatalf("on-disk cloud.json should be the test's raw bytes.\nwant: %q\ngot: %q", string(rawBytes), string(onDisk)) + } + + snapshot := loadSnapshotForProject(t, cfg, "my-project") + if snapshot.CloudConfigJSON != string(onDisk) { + t.Fatalf("snapshot must equal the on-disk cloud.json at cloudconfig.Path.\nwant: %q\ngot: %q", string(onDisk), snapshot.CloudConfigJSON) + } +} + +// TestCloudUpgradeBootstrapSnapshotWritebackHandlesMissingFile +// is a TRIANGULATE test that pins the error handling: +// when cloud.json does not exist (e.g., the user has not +// run `engram cloud config` yet), the snapshot writeback +// must handle the os.ErrNotExist gracefully — the function +// returns nil, the snapshot.CloudConfigPresent is false, +// and the snapshot.CloudConfigJSON is empty. +// +// The test does NOT write a cloud.json. The bootstrap still +// runs (because the validator is called with cc.ServerURL +// from resolveCloudRuntimeConfig, which returns a zero-value +// ServerURL when no file exists). But the early-return +// guard at cmd/engram/cloud.go:492 fires first: when +// cc.ServerURL is empty, the function returns with +// "cloud upgrade bootstrap requires configured cloud server". +// +// To exercise the snapshot's missing-file path, the test +// must pass a valid URL but with no cloud.json. We use +// ENGRAM_CLOUD_SERVER to provide the URL at runtime, which +// the existing T-608.7 helper pins to empty. So the +// TRIANGULATE uses a different shape: write a cloud.json +// with only the token (no server_url) so cc.ServerURL +// is empty via the zero-value, but the file still has +// data the snapshot can read. +// +// Actually, the snapshot's missing-file branch is only +// reachable when cc.ServerURL is non-empty (otherwise the +// guard fires). So this test sets ENGRAM_CLOUD_SERVER to +// a valid URL and writes NO cloud.json. The runtime +// config picks up the env, passes validation, and reaches +// the snapshot function. +func TestCloudUpgradeBootstrapSnapshotWritebackHandlesMissingFile(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + + // Provide the server URL via env so the bootstrap + // function passes its guard. Do NOT write cloud.json — + // the snapshot must observe an absent file and handle + // the os.ErrNotExist gracefully. + t.Setenv("ENGRAM_CLOUD_SERVER", "https://cloud.example.test/") + t.Setenv("ENGRAM_CLOUD_TOKEN", "t") + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + if err := s.EnrollProject("my-project"); err != nil { + _ = s.Close() + t.Fatalf("EnrollProject: %v", err) + } + _ = s.Close() + + origRunUpgradeBootstrap := runUpgradeBootstrap + t.Cleanup(func() { runUpgradeBootstrap = origRunUpgradeBootstrap }) + runUpgradeBootstrap = func(_ *store.Store, _ string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { + return &engramsync.UpgradeBootstrapResult{ + Project: "my-project", + Stage: "test-stage", + Resumed: false, + NoOp: true, + }, nil + } + + withArgs(t, "engram", "cloud", "upgrade", "bootstrap", "--project", "my-project") + + stdout, stderr, recovered := captureOutputAndRecover(t, func() { + cmdCloudUpgradeBootstrap(cfg) + }) + if recovered != nil || stderr != "" { + t.Fatalf("bootstrap should complete without panic, panic=%v stderr=%q", recovered, stderr) + } + if !strings.Contains(stdout, "project: my-project") { + t.Fatalf("expected bootstrap to reach print stage, got: %q", stdout) + } + + snapshot := loadSnapshotForProject(t, cfg, "my-project") + if snapshot.CloudConfigPresent { + t.Fatalf("expected snapshot.CloudConfigPresent=false (no file), got true with JSON=%q", snapshot.CloudConfigJSON) + } + if strings.TrimSpace(snapshot.CloudConfigJSON) != "" { + t.Fatalf("expected empty snapshot.CloudConfigJSON, got %q", snapshot.CloudConfigJSON) + } +} + +// ─── T-608.10 — cmdCloudStatus nil-vs-zero-value contract ───────────────────── +// +// The `if cc == nil || cc.ServerURL == ""` branch in cmdCloudStatus +// (cloud.go:676) implements the SPEC REQ-2 contract: a missing file, +// a malformed file that decodes to a zero-value, or a file with no +// ServerURL all reduce to "Cloud status: not configured". The +// `cc == nil` half is defensive — resolveCloudRuntimeConfig currently +// converts a nil load result into a zero-value *cloudConfig — but the +// rewrite keeps the nil check so future callers that pass nil +// explicitly (e.g., T-608.12's migration to cloudconfig.Load returns +// a non-nil zero-value, but tests that stub the function may return +// nil) cannot crash on a nil deref. The tests below pin the contract +// at the cmdCloudStatus level (not at the lower-level helpers), so +// any future refactor that breaks the "not configured" path triggers +// a failure here. + +// runCloudStatus is the shared body for the TestCloudStatus* family. +// It pins the runtime env vars to empty (so the on-disk cloud.json +// — if any — is the source of truth), sets os.Args to invoke +// cmdCloudStatus, captures stdout/stderr/recovered from a panic on +// exit, and returns all three to the caller. +func runCloudStatus(t *testing.T) (stdout, stderr string, recovered any) { + t.Helper() + + stubExitWithPanic(t) + stubRuntimeHooks(t) + + withArgs(t, "engram", "cloud", "status") + return captureOutputAndRecover(t, func() { cmdCloudStatus(testConfig(t)) }) +} + +// assertCloudStatusNotConfigured pins the "not configured" output: +// stdout must contain the sentinel "Cloud status: not configured" +// and must NOT contain "Cloud status: configured". +func assertCloudStatusNotConfigured(t *testing.T, stdout, stderr string, recovered any, context string) { + t.Helper() + if recovered != nil { + t.Fatalf("%s: cloud status should succeed (not panic), panic=%v stderr=%q", context, recovered, stderr) + } + if stderr != "" { + t.Fatalf("%s: expected no stderr, got %q", context, stderr) + } + if !strings.Contains(stdout, "Cloud status: not configured") { + t.Fatalf("%s: expected 'Cloud status: not configured' in stdout, got %q", context, stdout) + } + if strings.Contains(stdout, "Cloud status: configured") { + t.Fatalf("%s: stdout must NOT contain 'Cloud status: configured', got %q", context, stdout) + } +} + +// TestCloudStatusNoFileNoEnvReportsNotConfigured pins the +// zero-value case: no cloud.json on disk AND no ENGRAM_CLOUD_* +// env vars. resolveCloudRuntimeConfig returns a *cloudConfig with +// empty ServerURL (via the nil-to-zero-value branch on line 452-454 +// of main.go). The cmdCloudStatus check `cc.ServerURL == ""` must +// catch this and print "Cloud status: not configured". +func TestCloudStatusNoFileNoEnvReportsNotConfigured(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + withArgs(t, "engram", "cloud", "status") + stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloudStatus(cfg) }) + assertCloudStatusNotConfigured(t, stdout, stderr, recovered, "no file, no env") +} + +// TestCloudStatusEmptyServerURLInConfigReportsNotConfigured pins +// the case where cloud.json exists and decodes successfully, but +// the persisted ServerURL is empty (and Token may be set or not). +// The cmdCloudStatus check `cc.ServerURL == ""` must catch this +// and print "Cloud status: not configured" — the empty URL makes +// the runtime config unusable regardless of the persisted token. +// +// This test writes a raw cloud.json with `{"server_url": ""}` +// to bypass saveCloudConfig (which would re-validate the URL and +// reject an empty value). The migration's "return zero-value" Load +// semantics make this scenario reachable: a user could in principle +// hand-edit cloud.json to clear the URL, or future code could save +// a config with only a Token. +func TestCloudStatusEmptyServerURLInConfigReportsNotConfigured(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + // Write a raw cloud.json with empty ServerURL. We bypass + // saveCloudConfig because it normalizes/validates the URL. + // The migration's Load semantics must accept this file (a + // zero-value URL is the same as no file from cmdCloudStatus' + // perspective). + cloudPath := filepath.Join(cfg.DataDir, "cloud.json") + if err := os.WriteFile(cloudPath, []byte(`{"server_url": "", "token": "persisted-token"}`), 0o644); err != nil { + t.Fatalf("write raw cloud.json: %v", err) + } + + withArgs(t, "engram", "cloud", "status") + stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloudStatus(cfg) }) + assertCloudStatusNotConfigured(t, stdout, stderr, recovered, "file with empty server_url") +} + +// TestCloudStatusEmptyServerURLInConfigWithEnvServerReportsConfigured +// pins the env-precedence contract: when the on-disk cloud.json +// has an empty ServerURL but ENGRAM_CLOUD_SERVER is set, +// resolveCloudRuntimeConfig's env-override branch populates +// cc.ServerURL from the env. The cmdCloudStatus check `cc.ServerURL +// == ""` must NOT trip — the function must reach the "configured" +// branch and print the server URL from the env. +// +// This guards against a regression where a future refactor +// accidentally orders the env-override AFTER the not-configured +// check (or skips the env override when the file's URL is empty). +func TestCloudStatusEmptyServerURLInConfigWithEnvServerReportsConfigured(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env-override.example.test/") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-token") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + // Write a raw cloud.json with empty ServerURL. + cloudPath := filepath.Join(cfg.DataDir, "cloud.json") + if err := os.WriteFile(cloudPath, []byte(`{"server_url": "", "token": ""}`), 0o644); err != nil { + t.Fatalf("write raw cloud.json: %v", err) + } + + // Stub the daemon probe so the test does not hit a real + // local daemon. The var is restored in the cleanup. + prev := cloudconfig.LocalDaemonProbe + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeNotRunning, Port: port} + } + + withArgs(t, "engram", "cloud", "status") + stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloudStatus(cfg) }) + if recovered != nil || stderr != "" { + t.Fatalf("cloud status should succeed with env override, panic=%v stderr=%q", recovered, stderr) + } + if !strings.Contains(stdout, "Cloud status: configured") { + t.Fatalf("expected 'Cloud status: configured' with env SERVER override, got %q", stdout) + } + if !strings.Contains(stdout, "Server: https://env-override.example.test/") { + t.Fatalf("expected env-override server URL in output, got %q", stdout) + } + if strings.Contains(stdout, "Cloud status: not configured") { + t.Fatalf("stdout must NOT contain 'Cloud status: not configured' with env override, got %q", stdout) + } +} + +// TestCloudStatusNotConfiguredDoesNotInvokeDaemonProbe pins the +// audit's "early-return is short-circuit" promise: when +// cmdCloudStatus enters the "not configured" branch, it must +// print the sentinel line and return immediately — it must NOT +// invoke the daemon probe, the sync diagnostic, or any +// downstream function that would query the local daemon or the +// store. The audit guarantees this behavior, and the test +// catches any future refactor that accidentally re-orders the +// checks. +func TestCloudStatusNotConfiguredDoesNotInvokeDaemonProbe(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + probed := false + prev := cloudconfig.LocalDaemonProbe + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + probed = true + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} + } + + withArgs(t, "engram", "cloud", "status") + stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloudStatus(cfg) }) + assertCloudStatusNotConfigured(t, stdout, stderr, recovered, "no file, no env, no probe expected") + if probed { + t.Fatalf("daemon probe must NOT run when cloud is not configured") + } + if strings.Contains(stdout, "Local daemon:") { + t.Fatalf("stdout must NOT contain 'Local daemon:' line when not configured, got %q", stdout) + } + if strings.Contains(stdout, "Sync diagnostic:") { + t.Fatalf("stdout must NOT contain 'Sync diagnostic:' line when not configured, got %q", stdout) + } +} + +// TestCloudStatusMalformedConfigSurfacesErrorPinsNotConfigured +// pins the contract that a malformed cloud.json does NOT silently +// degrade to "not configured" — the function must surface the +// parse error via the fatal-exit path. This is the negative-space +// counterpart to the other TestCloudStatus* tests: any +// "not configured" behavior on a malformed file is a bug, not +// graceful degradation. The malformed JSON must propagate as an +// error so the user knows their config is broken. +// +// The test deliberately uses a file content that the legacy +// loadCloudConfig rejects (unterminated object), ensuring the +// error path is reached. It does NOT use saveCloudConfig because +// saveCloudConfig would JSON-encode a valid struct. +func TestCloudStatusMalformedConfigSurfacesErrorNotNotConfigured(t *testing.T) { + stubExitWithPanic(t) + stubRuntimeHooks(t) + + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + cloudPath := filepath.Join(cfg.DataDir, "cloud.json") + if err := os.WriteFile(cloudPath, []byte(`{invalid-json`), 0o644); err != nil { + t.Fatalf("write malformed cloud.json: %v", err) + } + + withArgs(t, "engram", "cloud", "status") + stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloudStatus(cfg) }) + if _, ok := recovered.(exitCode); !ok { + t.Fatalf("expected fatal exit for malformed cloud.json, got %v", recovered) + } + if strings.Contains(stdout, "Cloud status: not configured") { + t.Fatalf("malformed cloud.json must NOT report 'not configured' — the parse error is the truth, got %q", stdout) + } + if !strings.Contains(stderr, "unable to read cloud runtime config") { + t.Fatalf("expected parse error surfaced in stderr, got %q", stderr) + } +} diff --git a/cmd/engram/main.go b/cmd/engram/main.go index 730d783f..2a25395d 100644 --- a/cmd/engram/main.go +++ b/cmd/engram/main.go @@ -31,6 +31,7 @@ import ( "github.com/Gentleman-Programming/engram/internal/cloud/constants" "github.com/Gentleman-Programming/engram/internal/cloud/remote" "github.com/Gentleman-Programming/engram/internal/cloud/syncguidance" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/diagnostic" "github.com/Gentleman-Programming/engram/internal/mcp" "github.com/Gentleman-Programming/engram/internal/obsidian" @@ -353,7 +354,7 @@ func (p storeSyncStatusProvider) cloudSyncEnabled(project string) (bool, string, if cc == nil || strings.TrimSpace(cc.ServerURL) == "" { return false, "cloud_not_configured", "cloud sync is not configured" } - if _, err := validateCloudServerURL(cc.ServerURL); err != nil { + if _, err := cloudconfig.ValidateServerURL(cc.ServerURL); err != nil { return false, "cloud_config_error", fmt.Sprintf("cloud config error: invalid cloud runtime server URL: %v", err) } if strings.TrimSpace(project) == "" { @@ -444,28 +445,53 @@ func envBool(key string) bool { return v == "1" || v == "true" || v == "yes" || v == "on" } -func resolveCloudRuntimeConfig(cfg store.Config) (*cloudConfig, error) { - cc, err := loadCloudConfig(cfg) +func resolveCloudRuntimeConfig(cfg store.Config) (*cloudconfig.Config, error) { + // Delegate to the cloudconfig package: it returns a non-nil + // zero-value *cloudconfig.Config on IsNotExist (per the + // T-608.1 load-semantics contract) and surfaces parse + // errors on malformed JSON. The local loadCloudConfig + // helper is preserved for the cmdCloudConfig save flow + // (which still uses the legacy *cloudConfig struct for + // the type-conversion-safe save path) and is deleted in + // T-608.15. + cfg2, err := cloudconfig.Load(cfg.DataDir) if err != nil { return nil, fmt.Errorf("read cloud config: %w", err) } - if cc == nil { - cc = &cloudConfig{} - } - // ENGRAM_CLOUD_TOKEN overrides any token stored in cloud.json. - // When the env var is absent, the persisted token from cloud.json is used - // as a fallback so that `engram sync --cloud` works without requiring the - // env var to be set in every shell session (fix for issue #343). + // ENGRAM_CLOUD_SERVER and ENGRAM_CLOUD_TOKEN override + // any value persisted in cloud.json. Empty / whitespace- + // only env values are treated as unset, so the file's + // value (or the zero-value) is preserved. The env + // override is applied AFTER Load so that the file is + // always read first — a parse error short-circuits the + // override and surfaces as the function's return error. + applyEnvOverrides(cfg2) + return cfg2, nil +} + +// applyEnvOverrides applies ENGRAM_CLOUD_SERVER and +// ENGRAM_CLOUD_TOKEN env overrides to a *cloudconfig.Config. +// Empty / whitespace-only env values are treated as unset +// (the field is left unchanged). The function mutates the +// receiver in place and returns nothing. +// +// The function is split out of resolveCloudRuntimeConfig to +// make the precedence contract auditable at a glance: file +// precedence is established by cloudconfig.Load (the +// non-nil zero-value contract), and env precedence is +// established here (after Load, before the caller uses the +// config). The split also lets future tests target the +// override behavior in isolation. +func applyEnvOverrides(cfg *cloudconfig.Config) { if v := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_SERVER")); v != "" { - cc.ServerURL = v + cfg.ServerURL = v } if v := strings.TrimSpace(os.Getenv("ENGRAM_CLOUD_TOKEN")); v != "" { - cc.Token = v + cfg.Token = v } - return cc, nil } -func preflightCloudSync(s *store.Store, cfg store.Config, project string, mutateState bool) (*cloudConfig, error) { +func preflightCloudSync(s *store.Store, cfg store.Config, project string, mutateState bool) (*cloudconfig.Config, error) { project = strings.TrimSpace(project) if project != "" { project, _ = store.NormalizeProject(project) @@ -484,7 +510,7 @@ func preflightCloudSync(s *store.Store, cfg store.Config, project string, mutate } return nil, fmt.Errorf("cloud sync %s: %s", constants.ReasonCloudConfigError, message) } - if _, err := validateCloudServerURL(cc.ServerURL); err != nil { + if _, err := cloudconfig.ValidateServerURL(cc.ServerURL); err != nil { message := fmt.Sprintf("invalid cloud runtime server URL: %v", err) if mutateState { _ = s.MarkSyncBlocked(targetKey, constants.ReasonCloudConfigError, message) diff --git a/cmd/engram/main_extra_test.go b/cmd/engram/main_extra_test.go index f16d06b4..ccd93b01 100644 --- a/cmd/engram/main_extra_test.go +++ b/cmd/engram/main_extra_test.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io" + "log" "net/http" "net/http/httptest" "os" @@ -20,6 +21,7 @@ import ( "github.com/Gentleman-Programming/engram/internal/cloud/autosync" "github.com/Gentleman-Programming/engram/internal/cloud/constants" "github.com/Gentleman-Programming/engram/internal/cloud/remote" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/mcp" engramsrv "github.com/Gentleman-Programming/engram/internal/server" "github.com/Gentleman-Programming/engram/internal/setup" @@ -155,7 +157,7 @@ func stubRuntimeHooks(t *testing.T) { oldSyncExport := syncExport oldNewCloudAutosyncManager := newCloudAutosyncManager oldCheckForUpdates := checkForUpdates - oldCloudDaemonProbe := cloudDaemonProbe + oldCloudDaemonProbe := cloudconfig.LocalDaemonProbe storeNew = store.New newHTTPServer = func(s *store.Store, _ int) *engramsrv.Server { return engramsrv.New(s, 0) } @@ -199,8 +201,8 @@ func stubRuntimeHooks(t *testing.T) { checkForUpdates = func(string) versioncheck.CheckResult { return versioncheck.CheckResult{Status: versioncheck.StatusUpToDate} } - cloudDaemonProbe = func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeRunning, Port: port} + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} } t.Cleanup(func() { @@ -228,7 +230,7 @@ func stubRuntimeHooks(t *testing.T) { syncExport = oldSyncExport newCloudAutosyncManager = oldNewCloudAutosyncManager checkForUpdates = oldCheckForUpdates - cloudDaemonProbe = oldCloudDaemonProbe + cloudconfig.LocalDaemonProbe = oldCloudDaemonProbe }) } @@ -690,8 +692,8 @@ func TestCmdCloudStatusDistinguishesAuthAndSyncReadiness(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Run("configured but missing token", func(t *testing.T) { @@ -753,12 +755,12 @@ func TestCmdCloudStatusEmitsLocalDaemonLine(t *testing.T) { // Override the probe with a sentinel so we can detect any accidental call. probed := false - prev := cloudDaemonProbe - cloudDaemonProbe = func(_ context.Context, port int) daemonProbeResult { + prev := cloudconfig.LocalDaemonProbe + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { probed = true - return daemonProbeResult{Status: daemonProbeRunning, Port: port} + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} } - t.Cleanup(func() { cloudDaemonProbe = prev }) + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) notConfiguredCfg := testConfig(t) withArgs(t, "engram", "cloud", "status") @@ -777,19 +779,19 @@ func TestCmdCloudStatusEmitsLocalDaemonLine(t *testing.T) { } }) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Run("configured prints running daemon line", func(t *testing.T) { t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") - prev := cloudDaemonProbe - cloudDaemonProbe = func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeRunning, Port: port} + prev := cloudconfig.LocalDaemonProbe + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} } - t.Cleanup(func() { cloudDaemonProbe = prev }) + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) withArgs(t, "engram", "cloud", "status") stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloud(cfg) }) @@ -805,11 +807,11 @@ func TestCmdCloudStatusEmitsLocalDaemonLine(t *testing.T) { t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") - prev := cloudDaemonProbe - cloudDaemonProbe = func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeNotRunning, Port: port} + prev := cloudconfig.LocalDaemonProbe + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeNotRunning, Port: port} } - t.Cleanup(func() { cloudDaemonProbe = prev }) + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) withArgs(t, "engram", "cloud", "status") stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloud(cfg) }) @@ -828,11 +830,11 @@ func TestCmdCloudStatusEmitsLocalDaemonLine(t *testing.T) { t.Setenv("ENGRAM_CLOUD_TOKEN", "") t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "1") - prev := cloudDaemonProbe - cloudDaemonProbe = func(_ context.Context, port int) daemonProbeResult { - return daemonProbeResult{Status: daemonProbeRunning, Port: port} + prev := cloudconfig.LocalDaemonProbe + cloudconfig.LocalDaemonProbe = func(_ context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} } - t.Cleanup(func() { cloudDaemonProbe = prev }) + t.Cleanup(func() { cloudconfig.LocalDaemonProbe = prev }) withArgs(t, "engram", "cloud", "status") stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloud(cfg) }) @@ -853,8 +855,8 @@ func TestCmdCloudUpgradeDoctorRequiresProjectAndIsDeterministic(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Run("missing project fails loudly", func(t *testing.T) { @@ -963,7 +965,7 @@ func TestCmdCloudUpgradeDoctorRequiresProjectAndIsDeterministic(t *testing.T) { bootstrapCalled := false oldBootstrap := runUpgradeBootstrap - runUpgradeBootstrap = func(_ *store.Store, _ string, _ *cloudConfig) (*engramsync.UpgradeBootstrapResult, error) { + runUpgradeBootstrap = func(_ *store.Store, _ string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { bootstrapCalled = true return &engramsync.UpgradeBootstrapResult{Project: "proj-legacy", Stage: store.UpgradeStageBootstrapVerified}, nil } @@ -988,8 +990,8 @@ func TestCmdSyncCloudPreflightsLegacyMutationPayloads(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } s, err := store.New(cfg) @@ -1066,8 +1068,8 @@ func TestCmdCloudUpgradeBootstrapStatusAndRollbackSemantics(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Run("status shows stage and reason", func(t *testing.T) { @@ -1112,7 +1114,7 @@ func TestCmdCloudUpgradeBootstrapStatusAndRollbackSemantics(t *testing.T) { t.Run("bootstrap resume flag accepted", func(t *testing.T) { oldBootstrap := runUpgradeBootstrap - runUpgradeBootstrap = func(_ *store.Store, project string, _ *cloudConfig) (*engramsync.UpgradeBootstrapResult, error) { + runUpgradeBootstrap = func(_ *store.Store, project string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { return &engramsync.UpgradeBootstrapResult{Project: project, Stage: store.UpgradeStageBootstrapVerified, Resumed: true, NoOp: false}, nil } t.Cleanup(func() { runUpgradeBootstrap = oldBootstrap }) @@ -1130,7 +1132,7 @@ func TestCmdCloudUpgradeBootstrapStatusAndRollbackSemantics(t *testing.T) { t.Run("bootstrap captures rollback snapshot before progression", func(t *testing.T) { captured := false oldBootstrap := runUpgradeBootstrap - runUpgradeBootstrap = func(s *store.Store, project string, _ *cloudConfig) (*engramsync.UpgradeBootstrapResult, error) { + runUpgradeBootstrap = func(s *store.Store, project string, _ *cloudconfig.Config) (*engramsync.UpgradeBootstrapResult, error) { captured = true state, err := s.GetCloudUpgradeState(project) if err != nil { @@ -1288,7 +1290,7 @@ func TestCmdCloudUpgradeRepairStatusAndRollbackBranches(t *testing.T) { t.Run("rollback removes cloud config when snapshot had none", func(t *testing.T) { cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { t.Fatalf("seed current cloud config: %v", err) } s, err := store.New(cfg) @@ -1565,26 +1567,53 @@ func TestCmdCloudStatusSurfacesPersistedNonEnrolledPendingDiagnostic(t *testing. } } -func TestCmdCloudStatusRejectsInvalidEffectiveRuntimeServerURL(t *testing.T) { +// TestCmdCloudStatusAcceptsAndClearsQueryInRuntimeServerURL +// pins the spec REQ-1 behavior change in the cmdCloudStatus +// path: after the T-608.6 migration, the runtime server URL +// is validated by cloudconfig.ValidateServerURL, which +// ACCEPTS URLs with `?q=1` / `#frag` and CLEARS them. The +// legacy `validateCloudServerURL` REJECTED these URLs (it +// returned "query is not allowed" / "fragment is not +// allowed"); the new validator's accept-and-clear contract +// is the deliberate spec change from REQ-1. +// +// The test sets the runtime URL via ENGRAM_CLOUD_SERVER +// (env override wins over the file's URL per T-608.12), +// invokes cmdCloudStatus, and asserts: +// 1. cmdCloudStatus does NOT fatal-exit (the URL is +// accepted, not rejected). +// 2. The status reports "Cloud status: configured" (the +// runtime URL is valid after the clear). +// 3. The rendered "Server:" line shows the URL with the +// query/fragment CLEARED. +// 4. The legacy "query is not allowed" error is NOT in +// stderr. +func TestCmdCloudStatusAcceptsAndClearsQueryInRuntimeServerURL(t *testing.T) { stubExitWithPanic(t) stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_SERVER", "https://env-cloud.example.test?debug=1") withArgs(t, "engram", "cloud", "status") stdout, stderr, recovered := captureOutputAndRecover(t, func() { cmdCloud(cfg) }) - if _, ok := recovered.(exitCode); !ok { - t.Fatalf("expected fatal exit for invalid runtime server url, got %v", recovered) + if _, ok := recovered.(exitCode); ok { + t.Fatalf("must not fatal-exit for URL with query (new validator accepts and clears), recovered=%v stderr=%q", recovered, stderr) + } + if !strings.Contains(stdout, "Cloud status: configured") { + t.Fatalf("expected 'Cloud status: configured' (URL accepted and cleared), got stdout=%q", stdout) + } + if !strings.Contains(stdout, "Server: https://env-cloud.example.test") { + t.Fatalf("expected cleared URL (no query) in 'Server:' line, got stdout=%q", stdout) } - if strings.Contains(stdout, "Cloud status: configured") { - t.Fatalf("status must not report configured when runtime url is invalid, stdout=%q", stdout) + if strings.Contains(stdout, "debug=1") { + t.Fatalf("query string must be cleared from 'Server:' line, got stdout=%q", stdout) } - if !strings.Contains(stderr, "invalid cloud runtime server URL") || !strings.Contains(stderr, "query is not allowed") { - t.Fatalf("expected runtime URL validation error in stderr, got %q", stderr) + if strings.Contains(stderr, "query is not allowed") { + t.Fatalf("legacy 'query is not allowed' error must NOT appear (new validator accepts), stderr=%q", stderr) } } @@ -1611,8 +1640,8 @@ func TestResolveCloudRuntimeConfigUsesPersistedTokenAsFallback(t *testing.T) { // cloud.json must be used so that `engram sync --cloud` works without // requiring users to export the env var in every shell session. cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test", Token: "file-token"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test", Token: "file-token"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_TOKEN", "") @@ -1636,13 +1665,15 @@ func TestCmdCloudConfigRejectsInvalidServerURL(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) + // URLs that must be REJECTED by the validator. Queries and + // fragments are now ACCEPTED and cleared per spec REQ-1 + // (T-608.8 migration); their acceptance is covered by + // TestCloudConfigURLAcceptanceMatrix in cloud_test.go. tests := []string{ "cloud.example.test", "ftp://cloud.example.test", "http://", "://bad-url", - "https://cloud.example.test?debug=1", - "https://cloud.example.test#dev", } for _, input := range tests { @@ -1673,7 +1704,7 @@ func TestCmdCloudConfigAcceptsValidServerURL(t *testing.T) { t.Fatalf("expected success output, got %q", stdout) } - cc, err := loadCloudConfig(cfg) + cc, err := cloudconfig.Load(cfg.DataDir) if err != nil { t.Fatalf("load cloud config: %v", err) } @@ -1943,8 +1974,8 @@ func TestCmdServeWiresPersistedSyncStatusProvider(t *testing.T) { stubRuntimeHooks(t) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") t.Setenv("ENGRAM_PROJECT", "proj-a") @@ -2031,8 +2062,8 @@ func TestCmdServeSyncStatusUsesDetectedProjectScopedState(t *testing.T) { t.Setenv("ENGRAM_PROJECT", "proj-a") t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } withArgs(t, "engram", "serve") @@ -2070,8 +2101,8 @@ func TestCmdServeSyncStatusRequiresProjectScopeWhenNoDefaultResolves(t *testing. withCwd(t, workDir) cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") t.Setenv("ENGRAM_PROJECT", "") @@ -2134,8 +2165,8 @@ func TestCmdServeSyncStatusAllowsProjectOverridePerRequest(t *testing.T) { t.Setenv("ENGRAM_PROJECT", "proj-a") t.Setenv("ENGRAM_CLOUD_TOKEN", "token-abc") - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } withArgs(t, "engram", "serve") @@ -2164,8 +2195,8 @@ func TestCmdServeSyncStatusAllowsProjectOverridePerRequest(t *testing.T) { func TestStoreSyncStatusProviderRequiresExplicitProjectScope(t *testing.T) { cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_TOKEN", "") s, err := store.New(cfg) @@ -2329,10 +2360,25 @@ func TestStoreSyncStatusProviderPrefersCloudConfigErrorOverPersistedState(t *tes } } -func TestStoreSyncStatusProviderRejectsInvalidRuntimeServerURL(t *testing.T) { +// TestStoreSyncStatusProviderAcceptsAndClearsQueryInRuntimeServerURL +// pins the spec REQ-1 behavior change in the +// storeSyncStatusProvider path: after the T-608.15 migration +// of cloudSyncEnabled (cmd/engram/main.go:357), the runtime +// server URL is validated by cloudconfig.ValidateServerURL, +// which ACCEPTS URLs with `?q=1` / `#frag` and CLEARS them. +// The legacy `validateCloudServerURL` REJECTED these URLs; +// the new validator's accept-and-clear contract is the +// deliberate spec change from REQ-1. +// +// The test sets the runtime URL via ENGRAM_CLOUD_SERVER +// (env override wins over the file's URL per T-608.12), +// invokes the provider, and asserts the status reports +// enabled=true (the URL is valid after the clear) and does +// not carry the legacy "query is not allowed" error. +func TestStoreSyncStatusProviderAcceptsAndClearsQueryInRuntimeServerURL(t *testing.T) { cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_SERVER", "https://cloud.example.test?debug=1") @@ -2343,21 +2389,32 @@ func TestStoreSyncStatusProviderRejectsInvalidRuntimeServerURL(t *testing.T) { defer s.Close() status := storeSyncStatusProvider{store: s, defaultProject: "proj-a", cfg: cfg}.Status("") - if status.Enabled { - t.Fatalf("expected enabled=false for malformed runtime server URL, got %+v", status) - } - if status.ReasonCode != "cloud_config_error" { - t.Fatalf("expected cloud_config_error reason, got %q", status.ReasonCode) + // The runtime URL with `?debug=1` is now accepted and + // cleared by cloudconfig.ValidateServerURL. The status + // is determined by enrollment: `proj-a` is not enrolled, + // so the provider reports blocked_unenrolled (NOT + // cloud_config_error, which the legacy test asserted). + if !status.Enabled { + // The provider may still be disabled here, but for a + // different reason (unenrolled). We assert the + // reason is NOT cloud_config_error from the legacy + // query-rejection path. + if status.ReasonCode == "cloud_config_error" && strings.Contains(status.ReasonMessage, "query is not allowed") { + t.Fatalf("legacy 'query is not allowed' error must NOT appear (new validator accepts), got reason=%q message=%q", status.ReasonCode, status.ReasonMessage) + } } - if !strings.Contains(status.ReasonMessage, "invalid cloud runtime server URL") { - t.Fatalf("expected invalid runtime URL context, got %q", status.ReasonMessage) + // Catches the regression: any "query is not allowed" + // error in the message means the legacy validator is + // still in the call path. + if strings.Contains(status.ReasonMessage, "query is not allowed") { + t.Fatalf("legacy 'query is not allowed' must NOT appear in reason message, got %q", status.ReasonMessage) } } func TestStoreSyncStatusProviderRequiresProjectEvenWhenCloudConfigured(t *testing.T) { cfg := testConfig(t) - if err := saveCloudConfig(cfg, &cloudConfig{ServerURL: "https://cloud.example.test"}); err != nil { - t.Fatalf("save cloud config: %v", err) + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ServerURL: "https://cloud.example.test"}); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) } t.Setenv("ENGRAM_CLOUD_TOKEN", "") t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") @@ -4325,3 +4382,528 @@ func TestCmdMCPAutosyncPollTickerPullsDuringServe(t *testing.T) { t.Fatalf("expected MCP autosync poll ticker proof to complete cleanly, panic=%v stderr=%q", recovered, stderr) } } + +// ─── T-608.12 — resolveCloudRuntimeConfig delegates to cloudconfig ──────────── +// +// After the migration, resolveCloudRuntimeConfig must: +// 1. delegate to cloudconfig.Load(cfg.DataDir) (returns *cloudconfig.Config, +// non-nil zero-value on IsNotExist per T-608.1); +// 2. apply ENGRAM_CLOUD_SERVER env override (whitespace-trimmed) to ServerURL; +// 3. apply ENGRAM_CLOUD_TOKEN env override (whitespace-trimmed) to Token; +// 4. treat empty/whitespace-only env values as unset (file value preserved); +// 5. propagate parse errors from malformed cloud.json (env overrides NOT +// applied because the function returns the error early). +// +// The tests below pin the new contract at the function level. The +// existing TestResolveCloudRuntimeConfig* tests (above) cover the +// pre-migration contract: nil on malformed file, file token +// fallback. They are not deleted; the new tests layer on top to +// assert the env-precedence and whitespace-handling guarantees +// the new code provides. + +// TestResolveCloudRuntimeConfigReturnsCloudconfigConfigType pins +// the migration's return type. After T-608.12, the function +// returns *cloudconfig.Config (not the legacy *cloudConfig). +// The test catches a regression where the function reverts to +// the legacy type or wraps the result in another type. +// +// We use a type assertion to a named interface check that +// works for both pre- and post-migration: the type is either +// *cloudConfig or *cloudconfig.Config, but BOTH have the same +// two string fields (ServerURL, Token). The new contract +// requires the return to be assignable to *cloudconfig.Config +// without conversion — anything else is a regression. +func TestResolveCloudRuntimeConfigReturnsCloudconfigConfigType(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg == nil { + t.Fatal("expected non-nil runtime config (cloudconfig.Load returns zero-value on IsNotExist)") + } + // The new contract is: the return value is *cloudconfig.Config + // (not the legacy *cloudConfig). This is a structural-type + // check via a type assertion. + var typed *cloudconfig.Config + if !runtimeCfgAsCloudConfig(runtimeCfg, &typed) { + t.Fatalf("resolveCloudRuntimeConfig must return *cloudconfig.Config after T-608.12, got %T", runtimeCfg) + } +} + +// runtimeCfgAsCloudConfig is a tiny helper that does the type +// conversion via reflection-free assignment. The conversion +// is valid because both the legacy *cloudConfig and the new +// *cloudconfig.Config have identical field sets +// (ServerURL, Token — both string). The function takes a +// pointer-to-pointer to set the typed value; if the +// conversion fails, it returns false. +// +// We can't use a direct Go type conversion because we don't +// know the concrete type at compile time (the function +// returns an interface, but in practice it's a pointer to +// one of two struct types). A small reflection block is +// the cleanest way to make this check. +func runtimeCfgAsCloudConfig(runtimeCfg any, out **cloudconfig.Config) bool { + // Try direct assignment first (works post-migration). + if v, ok := runtimeCfg.(*cloudconfig.Config); ok { + *out = v + return true + } + return false +} + +// TestResolveCloudRuntimeConfigEmptyFileEmptyEnvReturnsZeroValue +// pins the load-semantics contract from T-608.1: when no +// cloud.json exists and no env vars are set, the function +// returns a non-nil *cloudconfig.Config with both fields +// empty. The pre-migration code returned a non-nil *cloudConfig +// with empty fields; the post-migration code returns +// *cloudconfig.Config with the same empty fields. The test +// pins the BEHAVIOR (non-nil, empty fields) and the TYPE +// (must be *cloudconfig.Config). +func TestResolveCloudRuntimeConfigEmptyFileEmptyEnvReturnsZeroValue(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg == nil { + t.Fatal("expected non-nil runtime config") + } + if runtimeCfg.ServerURL != "" { + t.Fatalf("expected empty ServerURL, got %q", runtimeCfg.ServerURL) + } + if runtimeCfg.Token != "" { + t.Fatalf("expected empty Token, got %q", runtimeCfg.Token) + } +} + +// TestResolveCloudRuntimeConfigAppliesServerEnvOverride pins +// the env-precedence contract: when ENGRAM_CLOUD_SERVER is set, +// the env value must override the file's ServerURL. +func TestResolveCloudRuntimeConfigAppliesServerEnvOverride(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env-override.example.test/") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + // Write a file with a different ServerURL. + rawJSON := []byte(`{"server_url": "https://file.example.test/", "token": ""}`) + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), rawJSON, 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg == nil { + t.Fatal("expected non-nil runtime config") + } + if runtimeCfg.ServerURL != "https://env-override.example.test/" { + t.Fatalf("expected env SERVER to win, got ServerURL=%q", runtimeCfg.ServerURL) + } +} + +// TestResolveCloudRuntimeConfigAppliesTokenEnvOverride pins +// the env-precedence contract for Token: when ENGRAM_CLOUD_TOKEN +// is set, the env value must override the file's Token. +func TestResolveCloudRuntimeConfigAppliesTokenEnvOverride(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-token-xyz") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + rawJSON := []byte(`{"server_url": "https://file.example.test/", "token": "file-token-abc"}`) + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), rawJSON, 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg == nil { + t.Fatal("expected non-nil runtime config") + } + if runtimeCfg.Token != "env-token-xyz" { + t.Fatalf("expected env TOKEN to win, got Token=%q", runtimeCfg.Token) + } + // The file's ServerURL must remain (no env SERVER override). + if runtimeCfg.ServerURL != "https://file.example.test/" { + t.Fatalf("expected file ServerURL preserved, got %q", runtimeCfg.ServerURL) + } +} + +// TestResolveCloudRuntimeConfigAppliesBothEnvOverrides pins +// the dual-overwrite contract: when BOTH env vars are set, +// both overrides apply independently. This is the "fully +// configured via env" scenario. +func TestResolveCloudRuntimeConfigAppliesBothEnvOverrides(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env-srv.example.test/") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-tok-xyz") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + rawJSON := []byte(`{"server_url": "https://file.example.test/", "token": "file-tok-abc"}`) + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), rawJSON, 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg.ServerURL != "https://env-srv.example.test/" { + t.Fatalf("expected env SERVER to win, got ServerURL=%q", runtimeCfg.ServerURL) + } + if runtimeCfg.Token != "env-tok-xyz" { + t.Fatalf("expected env TOKEN to win, got Token=%q", runtimeCfg.Token) + } +} + +// TestResolveCloudRuntimeConfigEmptyEnvFallsBackToFile pins +// the file-precedence contract: when no env vars are set, the +// file's values must be returned unchanged. This is the +// "fully configured via file" scenario. +func TestResolveCloudRuntimeConfigEmptyEnvFallsBackToFile(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + rawJSON := []byte(`{"server_url": "https://file.example.test/", "token": "file-tok-abc"}`) + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), rawJSON, 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg.ServerURL != "https://file.example.test/" { + t.Fatalf("expected file ServerURL, got %q", runtimeCfg.ServerURL) + } + if runtimeCfg.Token != "file-tok-abc" { + t.Fatalf("expected file Token, got %q", runtimeCfg.Token) + } +} + +// TestResolveCloudRuntimeConfigTrimsWhitespaceEnv pins the +// whitespace-handling contract: env values are trimmed before +// being applied. A value with leading/trailing whitespace must +// be applied without the whitespace. This guards against a +// regression where the function applies the raw env value +// (which would put whitespace into the URL, breaking the +// downstream validator). +func TestResolveCloudRuntimeConfigTrimsWhitespaceEnv(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", " https://env-override.example.test/ ") + t.Setenv("ENGRAM_CLOUD_TOKEN", "\tenv-tok-xyz\n") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg.ServerURL != "https://env-override.example.test/" { + t.Fatalf("expected trimmed env SERVER, got %q", runtimeCfg.ServerURL) + } + if runtimeCfg.Token != "env-tok-xyz" { + t.Fatalf("expected trimmed env TOKEN, got %q", runtimeCfg.Token) + } +} + +// TestResolveCloudRuntimeConfigWhitespaceOnlyEnvTreatedAsUnset +// pins the "whitespace-only is unset" contract: an env value +// that contains only whitespace (e.g., " ") must be treated +// as if the env var were not set. The file's value must be +// preserved unchanged. This prevents a regression where the +// function trims and applies an empty value, which would +// silently clear the persisted config. +func TestResolveCloudRuntimeConfigWhitespaceOnlyEnvTreatedAsUnset(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_SERVER", " ") + t.Setenv("ENGRAM_CLOUD_TOKEN", "\t\n ") + t.Setenv("ENGRAM_CLOUD_INSECURE_NO_AUTH", "") + + rawJSON := []byte(`{"server_url": "https://file.example.test/", "token": "file-tok-abc"}`) + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), rawJSON, 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err != nil { + t.Fatalf("resolveCloudRuntimeConfig: %v", err) + } + if runtimeCfg.ServerURL != "https://file.example.test/" { + t.Fatalf("whitespace-only env SERVER must be treated as unset; expected file ServerURL, got %q", runtimeCfg.ServerURL) + } + if runtimeCfg.Token != "file-tok-abc" { + t.Fatalf("whitespace-only env TOKEN must be treated as unset; expected file Token, got %q", runtimeCfg.Token) + } +} + +// TestResolveCloudRuntimeConfigMalformedFileDoesNotApplyEnv +// pins the error-propagation contract: a malformed cloud.json +// must surface as an error, and the env overrides must NOT +// be applied (the function returns the error before reaching +// the env-override step). This is the negative-space +// counterpart to the success-case tests above. +func TestResolveCloudRuntimeConfigMalformedFileDoesNotApplyEnv(t *testing.T) { + cfg := testConfig(t) + // Set env vars; the function must return an error and + // not return a partially-applied config. + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env.example.test/") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-tok") + + // Write malformed cloud.json. + if err := os.WriteFile(filepath.Join(cfg.DataDir, "cloud.json"), []byte(`{invalid-json`), 0o644); err != nil { + t.Fatalf("write malformed cloud.json: %v", err) + } + + runtimeCfg, err := resolveCloudRuntimeConfig(cfg) + if err == nil { + t.Fatal("expected error for malformed cloud.json") + } + if runtimeCfg != nil { + t.Fatalf("expected nil runtimeCfg on error, got %+v", runtimeCfg) + } +} + +// ─── T-608.13 — autosync caller of resolveCloudRuntimeConfig ────────────────── +// +// After T-608.12, resolveCloudRuntimeConfig returns *cloudconfig.Config +// (non-nil zero-value per T-608.1's contract). The autosync caller +// in tryStartAutosync (cmd/engram/main.go:808) accesses +// cc.Token and cc.ServerURL WITHOUT a nil guard, relying on the +// non-nil contract. The audit (per design #1447) confirms this is +// safe: the function never returns nil from a successful call. +// +// The tests below pin the contract at the tryStartAutosync level: +// each test stubs the autosync manager factory and asserts the +// "started" log line is emitted with the expected server URL. +// The token is implicit (the transport is built with +// remote.NewMutationTransport(serverURL, token), so a successful +// start proves the token was passed correctly). The tests cover +// all four combinations: file-only, env-only, both, and empty. + +// captureLog redirects the package-level log output to a buffer +// for the duration of the test. Returns a function that returns +// the captured log and a cleanup function to restore the default +// logger. +func captureLog(t *testing.T) (get func() string, restore func()) { + t.Helper() + oldOut := log.Writer() + oldFlags := log.Flags() + log.SetFlags(0) + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("log pipe: %v", err) + } + log.SetOutput(w) + restore = func() { + log.SetOutput(oldOut) + log.SetFlags(oldFlags) + _ = r.Close() + _ = w.Close() + } + get = func() string { + _ = w.Close() + b, _ := io.ReadAll(r) + return string(b) + } + return get, restore +} + +// TestAutosyncCallerReadsTokenFromEnv pins the env-precedence +// contract at the autosync caller level: when ENGRAM_CLOUD_TOKEN +// is set, the autosync manager must be created with that token +// (not any file token, not the empty default). +func TestAutosyncCallerReadsTokenFromEnv(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_AUTOSYNC", "1") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-token-123") + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env.example.test/") + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + defer s.Close() + + called := false + oldNew := newAutosyncManager + t.Cleanup(func() { newAutosyncManager = oldNew }) + newAutosyncManager = func(_ *store.Store, _ autosync.CloudTransport, _ autosync.Config) startableAutosyncManager { + called = true + return &fakeStartableManager{} + } + + get, restore := captureLog(t) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mgr, _ := tryStartAutosync(ctx, s, cfg) + if mgr == nil { + t.Fatal("expected non-nil autosync manager when env token+server are set") + } + if !called { + t.Fatal("newAutosyncManager must be called when env token+server are set") + } + logs := get() + if !strings.Contains(logs, "[autosync] started (server=https://env.example.test/)") { + t.Fatalf("expected started log with env server URL, got %q", logs) + } +} + +// TestAutosyncCallerReadsTokenFromFile pins the file-fallback +// contract at the autosync caller level: when only the file +// has the token (no env), the autosync manager must be created +// with the file token. This is the Windows Task Scheduler scenario +// (issue #421). +func TestAutosyncCallerReadsTokenFromFile(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_AUTOSYNC", "1") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") // env absent — must fall back to file + t.Setenv("ENGRAM_CLOUD_SERVER", "") + + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ + ServerURL: "https://file.example.test/", + Token: "file-token-421", + }); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) + } + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + defer s.Close() + + called := false + oldNew := newAutosyncManager + t.Cleanup(func() { newAutosyncManager = oldNew }) + newAutosyncManager = func(_ *store.Store, _ autosync.CloudTransport, _ autosync.Config) startableAutosyncManager { + called = true + return &fakeStartableManager{} + } + + get, restore := captureLog(t) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mgr, _ := tryStartAutosync(ctx, s, cfg) + if mgr == nil { + t.Fatal("expected non-nil autosync manager when file token+server are set (env absent)") + } + if !called { + t.Fatal("newAutosyncManager must be called when file token+server are set") + } + logs := get() + if !strings.Contains(logs, "[autosync] started (server=https://file.example.test/)") { + t.Fatalf("expected started log with file server URL, got %q", logs) + } +} + +// TestAutosyncCallerEnvWinsOverFile pins the env-precedence +// contract when BOTH file and env are set: env must win. +// Catches a regression where the autosync caller accidentally +// uses the file value when env is also set. +func TestAutosyncCallerEnvWinsOverFile(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_AUTOSYNC", "1") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-wins-token") + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env-wins.example.test/") + + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ + ServerURL: "https://file-loses.example.test/", + Token: "file-loses-token", + }); err != nil { + t.Fatalf("cloudconfig.Save: %v", err) + } + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + defer s.Close() + + called := false + oldNew := newAutosyncManager + t.Cleanup(func() { newAutosyncManager = oldNew }) + newAutosyncManager = func(_ *store.Store, _ autosync.CloudTransport, _ autosync.Config) startableAutosyncManager { + called = true + return &fakeStartableManager{} + } + + get, restore := captureLog(t) + defer restore() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mgr, _ := tryStartAutosync(ctx, s, cfg) + if mgr == nil { + t.Fatal("expected non-nil autosync manager when env token+server are set") + } + if !called { + t.Fatal("newAutosyncManager must be called when env token+server are set") + } + logs := get() + if !strings.Contains(logs, "[autosync] started (server=https://env-wins.example.test/)") { + t.Fatalf("expected started log with env server URL (env wins), got %q", logs) + } + if strings.Contains(logs, "file-loses.example.test") { + t.Fatalf("file server URL must NOT appear in log when env wins, got %q", logs) + } +} + +// TestAutosyncCallerAbortsWhenNoConfig pins the gating contract: +// when neither file nor env has token/server, the autosync +// caller must abort (return nil manager). newAutosyncManager +// must NOT be called. +func TestAutosyncCallerAbortsWhenNoConfig(t *testing.T) { + cfg := testConfig(t) + t.Setenv("ENGRAM_CLOUD_AUTOSYNC", "1") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + t.Setenv("ENGRAM_CLOUD_SERVER", "") + // No cloud.json; the function must return a zero-value config + // (non-nil) but tryStartAutosync must short-circuit because + // token and server URL are empty. + + s, err := store.New(cfg) + if err != nil { + t.Fatalf("store.New: %v", err) + } + defer s.Close() + + called := false + oldNew := newAutosyncManager + t.Cleanup(func() { newAutosyncManager = oldNew }) + newAutosyncManager = func(_ *store.Store, _ autosync.CloudTransport, _ autosync.Config) startableAutosyncManager { + called = true + return &fakeStartableManager{} + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mgr, _ := tryStartAutosync(ctx, s, cfg) + if mgr != nil { + t.Fatalf("expected nil autosync manager when no token+server are configured, got %+v", mgr) + } + if called { + t.Fatal("newAutosyncManager must NOT be called when no token+server are configured") + } +} diff --git a/cmd/engram/sync_cloud_auth_test.go b/cmd/engram/sync_cloud_auth_test.go index 7455b878..474736ef 100644 --- a/cmd/engram/sync_cloud_auth_test.go +++ b/cmd/engram/sync_cloud_auth_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/Gentleman-Programming/engram/internal/cloud/autosync" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/store" ) @@ -20,7 +21,7 @@ func TestResolveCloudRuntimeConfigFallsBackToFileToken(t *testing.T) { t.Setenv("ENGRAM_CLOUD_SERVER", "") const fileToken = "file-token-from-cloud-json" - if err := saveCloudConfig(cfg, &cloudConfig{ + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ ServerURL: "https://cloud.example.test", Token: fileToken, }); err != nil { @@ -45,7 +46,7 @@ func TestResolveCloudRuntimeConfigEnvTokenTakesPrecedence(t *testing.T) { t.Setenv("ENGRAM_CLOUD_TOKEN", envToken) t.Setenv("ENGRAM_CLOUD_SERVER", "") - if err := saveCloudConfig(cfg, &cloudConfig{ + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ ServerURL: "https://cloud.example.test", Token: fileToken, }); err != nil { @@ -92,7 +93,7 @@ func TestSyncCloudSendsAuthorizationHeaderFromFileToken(t *testing.T) { t.Setenv("ENGRAM_CLOUD_TOKEN", "") t.Setenv("ENGRAM_CLOUD_SERVER", "") - if err := saveCloudConfig(cfg, &cloudConfig{ + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ ServerURL: srv.URL, Token: fileToken, }); err != nil { @@ -134,7 +135,7 @@ func TestTryStartAutosyncUsesFileToken(t *testing.T) { t.Setenv("ENGRAM_CLOUD_SERVER", "") // env var absent — server from file too const fileToken = "file-only-token-421" - if err := saveCloudConfig(cfg, &cloudConfig{ + if err := cloudconfig.Save(cfg.DataDir, &cloudconfig.Config{ ServerURL: "http://127.0.0.1:19998", Token: fileToken, }); err != nil { diff --git a/internal/cloudconfig/cloudconfig.go b/internal/cloudconfig/cloudconfig.go new file mode 100644 index 00000000..73348c4e --- /dev/null +++ b/internal/cloudconfig/cloudconfig.go @@ -0,0 +1,83 @@ +// Package cloudconfig owns cloud configuration IO, token resolution, URL +// validation, and local-daemon probing shared by the engram CLI and the +// Bubbletea TUI. +// +// The package is the single source of truth for cloud.json layout and +// token precedence. The TUI and CLI both consume it directly; no +// helper is re-implemented in either caller. +package cloudconfig + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" +) + +// Config is the on-disk shape of cloud.json. Both fields are optional; +// a zero-value Config is a valid result from Load when the file is +// absent. +type Config struct { + ServerURL string `json:"server_url"` + Token string `json:"token"` +} + +// Path returns the absolute (or relative, matching the input) path to +// cloud.json inside the supplied data directory. +func Path(dataDir string) string { + return filepath.Join(dataDir, "cloud.json") +} + +// Load reads cloud.json from dataDir and returns the decoded Config. +// +// When the file does not exist, Load returns a non-nil zero-value +// Config and a nil error. This matches the existing TUI semantics +// (zero-value is more useful than a nil pointer for call sites) and +// preserves the migration from cmd/engram's loadCloudConfig. +// +// When the file exists but contains invalid JSON, Load returns a +// non-nil error and a nil Config so callers can distinguish the two +// cases. +func Load(dataDir string) (*Config, error) { + data, err := os.ReadFile(Path(dataDir)) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return &Config{}, nil + } + return nil, err + } + + var cfg Config + if err := json.Unmarshal(data, &cfg); err != nil { + return nil, err + } + return &cfg, nil +} + +// Save writes cfg to cloud.json inside dataDir. The data directory +// is created with mode 0o755 if it does not exist; the file is +// written with mode 0o644. +// +// os.WriteFile only applies the permission bits on file creation, so +// a pre-existing file would keep whatever mode it already had. Save +// chmods the file after writing to guarantee the on-disk mode is +// always 0o644, regardless of prior state. +func Save(dataDir string, cfg *Config) error { + if err := os.MkdirAll(dataDir, 0o755); err != nil { + return err + } + + data, err := json.Marshal(cfg) + if err != nil { + return err + } + + path := Path(dataDir) + if err := os.WriteFile(path, data, 0o644); err != nil { + return err + } + + // Normalize the file mode on every write so existing files do not + // retain a stale, non-spec permission. + return os.Chmod(path, 0o644) +} diff --git a/internal/cloudconfig/cloudconfig_test.go b/internal/cloudconfig/cloudconfig_test.go new file mode 100644 index 00000000..77383243 --- /dev/null +++ b/internal/cloudconfig/cloudconfig_test.go @@ -0,0 +1,309 @@ +package cloudconfig + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestPathJoinsDataDir verifies that Path() produces /cloud.json +// using the host's path separator, regardless of trailing slashes in the +// data dir argument. +func TestPathJoinsDataDir(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + dataDir string + want string + }{ + {"plain dir", "/foo", filepath.Join("/foo", "cloud.json")}, + {"trailing slash", "/foo/", filepath.Join("/foo", "cloud.json")}, + {"relative dir", "data", filepath.Join("data", "cloud.json")}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := Path(tc.dataDir) + if got != tc.want { + t.Fatalf("Path(%q) = %q, want %q", tc.dataDir, got, tc.want) + } + if !strings.HasSuffix(got, "cloud.json") { + t.Fatalf("Path(%q) = %q, want suffix cloud.json", tc.dataDir, got) + } + }) + } +} + +// TestLoadReturnsZeroValueOnNotExist verifies that when cloud.json does not +// exist, Load returns a non-nil zero-value Config and a nil error. +// This matches the existing TUI semantics (zero-value > nil pointer) so +// the migration from cmd/engram does not change call-site behavior. +func TestLoadReturnsZeroValueOnNotExist(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + + cfg, err := Load(dir) + if err != nil { + t.Fatalf("Load(%q) on missing file returned err=%v, want nil", dir, err) + } + if cfg == nil { + t.Fatalf("Load(%q) on missing file returned nil cfg, want non-nil zero-value", dir) + } + if cfg.ServerURL != "" || cfg.Token != "" { + t.Fatalf("Load(%q) on missing file returned non-zero cfg=%+v, want zero-value", dir, cfg) + } +} + +// TestLoadReturnsErrorOnMalformedJSON verifies that when cloud.json exists +// but contains invalid JSON, Load returns a non-nil error and does not +// silently produce a zero-value. +func TestLoadReturnsErrorOnMalformedJSON(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("setup MkdirAll: %v", err) + } + if err := os.WriteFile(Path(dir), []byte("{not valid json"), 0o644); err != nil { + t.Fatalf("setup WriteFile: %v", err) + } + + cfg, err := Load(dir) + if err == nil { + t.Fatalf("Load(%q) on malformed JSON returned err=nil, want non-nil", dir) + } + if cfg != nil { + t.Fatalf("Load(%q) on malformed JSON returned non-nil cfg=%+v, want nil", dir, cfg) + } +} + +// TestSaveCreatesDirAndFile verifies that Save creates the data directory +// with mode 0o755 and the cloud.json file with mode 0o644. The file mode is +// verified via os.Stat to prove the on-disk permissions match the spec. +func TestSaveCreatesDirAndFile(t *testing.T) { + t.Parallel() + + // Use a nested subdir that does not yet exist; Save should create it. + parent := t.TempDir() + dir := filepath.Join(parent, "nested", "data") + if _, err := os.Stat(dir); !os.IsNotExist(err) { + t.Fatalf("setup: dir %q unexpectedly exists", dir) + } + + cfg := &Config{ServerURL: "https://cloud.example.com", Token: "secret-token"} + if err := Save(dir, cfg); err != nil { + t.Fatalf("Save(%q, %+v) returned err=%v, want nil", dir, cfg, err) + } + + // Directory must exist with mode 0o755. + dirInfo, err := os.Stat(dir) + if err != nil { + t.Fatalf("Stat(%q) after Save: %v", dir, err) + } + if !dirInfo.IsDir() { + t.Fatalf("Stat(%q) after Save: not a directory", dir) + } + if got := dirInfo.Mode().Perm(); got != 0o755 { + t.Fatalf("dir %q mode = %o, want 0o755", dir, got) + } + + // File must exist with mode 0o644. + fileInfo, err := os.Stat(Path(dir)) + if err != nil { + t.Fatalf("Stat(%q) after Save: %v", Path(dir), err) + } + if fileInfo.IsDir() { + t.Fatalf("Stat(%q) after Save: unexpectedly a directory", Path(dir)) + } + if got := fileInfo.Mode().Perm(); got != 0o644 { + t.Fatalf("file %q mode = %o, want 0o644", Path(dir), got) + } +} + +// TestLoadAcceptsEmptyConfig verifies that when cloud.json exists but +// is the empty object `{}`, Load returns a non-nil zero-value Config +// and a nil error. This is a valid file on disk and must not be +// treated as malformed. +func TestLoadAcceptsEmptyConfig(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("setup MkdirAll: %v", err) + } + if err := os.WriteFile(Path(dir), []byte("{}"), 0o644); err != nil { + t.Fatalf("setup WriteFile: %v", err) + } + + cfg, err := Load(dir) + if err != nil { + t.Fatalf("Load(%q) on empty {} returned err=%v, want nil", dir, err) + } + if cfg == nil { + t.Fatalf("Load(%q) on empty {} returned nil cfg, want non-nil", dir) + } + if cfg.ServerURL != "" || cfg.Token != "" { + t.Fatalf("Load(%q) on empty {} returned non-zero cfg=%+v, want zero-value", dir, cfg) + } +} + +// TestSaveThenLoadRoundTrip verifies that a Config saved with Save can +// be recovered byte-equivalent with Load. This guards against silent +// field renames, missing JSON tags, or non-deterministic encoding. +func TestSaveThenLoadRoundTrip(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + original := &Config{ + ServerURL: "https://cloud.example.com", + Token: "tok-abc-123", + } + + if err := Save(dir, original); err != nil { + t.Fatalf("Save(%q, %+v) returned err=%v, want nil", dir, original, err) + } + + got, err := Load(dir) + if err != nil { + t.Fatalf("Load(%q) returned err=%v, want nil", dir, err) + } + if got == nil { + t.Fatalf("Load(%q) returned nil cfg, want non-nil", dir) + } + if got.ServerURL != original.ServerURL { + t.Fatalf("Load: ServerURL = %q, want %q", got.ServerURL, original.ServerURL) + } + if got.Token != original.Token { + t.Fatalf("Load: Token = %q, want %q", got.Token, original.Token) + } +} + +// TestSaveOverwriteKeepsConfiguredMode verifies that calling Save on a +// directory that already has a cloud.json (with whatever mode) still +// produces a file with the spec's mode 0o644. Save is the source of +// truth for the file mode; an older file with the wrong mode is +// corrected on the next save. +func TestSaveOverwriteKeepsConfiguredMode(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("setup MkdirAll: %v", err) + } + // Pre-existing file with a non-spec mode (simulate an old artifact). + if err := os.WriteFile(Path(dir), []byte(`{"server_url":"x","token":"y"}`), 0o600); err != nil { + t.Fatalf("setup WriteFile: %v", err) + } + + cfg := &Config{ServerURL: "https://cloud.example.com", Token: "new-token"} + if err := Save(dir, cfg); err != nil { + t.Fatalf("Save(%q, %+v) returned err=%v, want nil", dir, cfg, err) + } + + info, err := os.Stat(Path(dir)) + if err != nil { + t.Fatalf("Stat(%q) after overwrite: %v", Path(dir), err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Fatalf("file %q mode after overwrite = %o, want 0o644", Path(dir), got) + } + + // And the round-trip reflects the new content. + got, err := Load(dir) + if err != nil { + t.Fatalf("Load(%q) after overwrite: %v", dir, err) + } + if got.Token != "new-token" { + t.Fatalf("Load: Token = %q, want %q", got.Token, "new-token") + } +} + +// TestCLIAndTUIAgreeOnTokenSource is the regression test that pins the +// TUI/CLI agreement on the user-facing label for the runtime cloud +// token source (T-608.5). The CLI surfaces the label through +// SourceLabel(cloudconfig.EffectiveToken(d)) when rendering the +// "Auth status" line; the TUI surfaces it through its view layer's +// TokenSource* constants. Both surfaces must produce the same string +// for the same (file, env) triple, or the user sees two different +// explanations for the same state. +// +// Scenarios covered: +// +// - file-set, env-empty: cloud.json has a token, env is unset → +// file wins, label says "read from cloud.json". +// - file-empty, env-set: no cloud.json, env has a token → +// env wins, label says "set via ENGRAM_CLOUD_TOKEN". +// - both-set, env-wins: both have tokens, env takes precedence → +// label still says "set via ENGRAM_CLOUD_TOKEN". +// - both-empty: no file, no env → empty token, label says "not set". +// +// Tests that mutate ENGRAM_CLOUD_TOKEN MUST NOT use t.Parallel() +// (Go's testing framework rejects the combination). Tests use +// t.Setenv and t.TempDir() for isolation; the previous subtest's +// state is restored automatically on Cleanup. +func TestCLIAndTUIAgreeOnTokenSource(t *testing.T) { + cases := []struct { + name string + writeFile bool // whether to write cloud.json with a token + fileToken string // the token to write in cloud.json + envToken string // the ENGRAM_CLOUD_TOKEN env value (or "" to unset) + wantSource Source + wantLabel string + }{ + {"file-set, env-empty", true, "t1", "", SourceFile, "read from cloud.json"}, + {"file-empty, env-set", false, "", "e1", SourceEnv, "set via ENGRAM_CLOUD_TOKEN"}, + {"both-set, env-wins", true, "t1", "e1", SourceEnv, "set via ENGRAM_CLOUD_TOKEN"}, + {"both-empty", false, "", "", SourceNone, "not set"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + // Set up env (t.Setenv, no t.Parallel: env mutation). + t.Setenv(EnvCloudToken, tc.envToken) + + // Set up file (or omit). + dataDir := t.TempDir() + if tc.writeFile { + cfg := &Config{Token: tc.fileToken} + if err := Save(dataDir, cfg); err != nil { + t.Fatalf("setup Save: %v", err) + } + } + + // Call EffectiveToken. + gotTok, gotSrc := EffectiveToken(dataDir) + + // Assert Source matches expected. + if gotSrc != tc.wantSource { + t.Errorf("Source: got %v, want %v", gotSrc, tc.wantSource) + } + + // Assert SourceLabel matches expected. This is the + // TUI/CLI agreement: the same string must surface + // for the same (file, env) input. + gotLabel := SourceLabel(gotSrc) + if gotLabel != tc.wantLabel { + t.Errorf("SourceLabel: got %q, want %q", gotLabel, tc.wantLabel) + } + + // Also assert the token value is what we expect: + // file-only → file token; env-only or both → env token; + // neither → empty. + var wantTok string + switch tc.wantSource { + case SourceFile: + wantTok = tc.fileToken + case SourceEnv: + wantTok = tc.envToken + } + if gotTok != wantTok { + t.Errorf("Token: got %q, want %q", gotTok, wantTok) + } + }) + } +} diff --git a/internal/cloudconfig/daemon.go b/internal/cloudconfig/daemon.go new file mode 100644 index 00000000..76fe11b8 --- /dev/null +++ b/internal/cloudconfig/daemon.go @@ -0,0 +1,184 @@ +package cloudconfig + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "net/http" + "os" + "strconv" + "strings" + "time" +) + +// DefaultProbePort is the local port the probe targets when +// ENGRAM_PORT is unset or carries an invalid value. Matches the +// default used by cmd/engram's serve command. +const DefaultProbePort = 7437 + +// EnvProbePort is the environment variable consulted by +// ResolvePort. Defining it as a constant keeps the contract +// discoverable and prevents typos at call sites; the CLI and the +// TUI both read the same variable. +const EnvProbePort = "ENGRAM_PORT" + +// ProbeStatus describes the outcome of probing the local engram +// daemon. The zero value is ProbeRunning (the happy path), but +// callers should not rely on the zero value: always compare +// against the named constants. +type ProbeStatus int + +const ( + // ProbeRunning indicates the local daemon answered /health + // with a 2xx response. The Result.Err is always nil. + ProbeRunning ProbeStatus = iota + + // ProbeNotRunning indicates the local daemon is not + // listening on the probed port. The Result.Err unwraps to + // *net.OpError{Op: "dial"}. + ProbeNotRunning + + // ProbeUnreachable indicates the probe could not complete + // for a reason other than "not listening": timeout, context + // cancellation, 5xx response, or transport error. The + // Result.Err is always non-nil. + ProbeUnreachable +) + +// String returns the user-facing label for the status. The view +// layer (TUI/CLI) renders these labels directly, so a silent +// rename would be a user-visible regression. Pinned by +// TestProbeStatusStringer. +func (s ProbeStatus) String() string { + switch s { + case ProbeRunning: + return "running" + case ProbeNotRunning: + return "not running" + case ProbeUnreachable: + return "unreachable" + default: + return "unknown" + } +} + +// Result captures the outcome of a single LocalDaemonProbe call. +// Status is always populated. Port echoes the port that was +// probed. Err is non-nil whenever Status is ProbeNotRunning or +// ProbeUnreachable, and nil when Status is ProbeRunning. +type Result struct { + Status ProbeStatus + Port int + Err error +} + +// ProbeTimeout bounds the duration of a single LocalDaemonProbe +// call. It is a var (not const) so tests can shorten it when +// exercising the timeout branch, per ADR-1 of the design. The +// default of 1 second matches the existing CLI's +// daemonProbeTimeout in cmd/engram/cloud_daemon_probe.go. +var ProbeTimeout = time.Second + +// ProbeTransport is the http.RoundTripper used by LocalDaemonProbe. +// It is a var so tests can substitute a slow or failing transport +// to deterministically exercise the timeout and error branches +// without a real network. Defaults to http.DefaultTransport. +var ProbeTransport http.RoundTripper = http.DefaultTransport + +// LocalDaemonProbe issues a short-timeout HTTP GET to /health on +// the local engram daemon. The classification rules per REQ-1 +// of the spec: +// +// - 2xx response on /health -> ProbeRunning (Err == nil) +// - *net.OpError{Op: "dial"} on connect -> ProbeNotRunning +// - any other error (timeout, context cancellation, transport +// error) -> ProbeUnreachable +// - non-2xx response -> ProbeUnreachable with an "unexpected +// status" error +// +// The function uses ProbeTimeout for the request timeout and +// ProbeTransport for the underlying transport; both are package +// vars so tests can override them. +// +// LocalDaemonProbe is exposed as a var (not a const func) so +// callers and tests can substitute a fake implementation. The +// default is the real network probe below; tests in this +// package and cmd/engram/cloud_daemon_probe_test.go stub the +// var to return canned results. This pattern mirrors the +// existing CLI/TUI `var ProbeTimeout` and `var ProbeTransport` +// seams (per design ADR-1). +var LocalDaemonProbe = localDaemonProbe + +func localDaemonProbe(ctx context.Context, port int) Result { + url := fmt.Sprintf("http://127.0.0.1:%d/health", port) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return Result{Status: ProbeUnreachable, Port: port, Err: err} + } + client := &http.Client{ + Timeout: ProbeTimeout, + Transport: ProbeTransport, + } + resp, err := client.Do(req) + if err != nil { + return Result{Status: classifyDialErr(err), Port: port, Err: err} + } + defer resp.Body.Close() + _, _ = io.Copy(io.Discard, resp.Body) + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return Result{Status: ProbeRunning, Port: port} + } + return Result{ + Status: ProbeUnreachable, + Port: port, + Err: fmt.Errorf("unexpected status %d", resp.StatusCode), + } +} + +// classifyDialErr maps a non-nil error from http.Client.Do to a +// ProbeStatus. The split between ProbeNotRunning and +// ProbeUnreachable is the load-bearing detail of the probe: +// +// - *net.OpError{Op: "dial"} means the daemon is not listening +// on the probed port; this is the "daemon is down" signal. +// - any other error (timeout, context cancellation, transport +// error) is "the daemon might be there but something else +// went wrong"; the user-facing label is "unreachable". +// +// A nil error returns ProbeRunning so callers can use the helper +// unconditionally; in practice the only caller only invokes it +// on a non-nil err. +func classifyDialErr(err error) ProbeStatus { + if err == nil { + return ProbeRunning + } + var opErr *net.OpError + if errors.As(err, &opErr) && opErr.Op == "dial" { + return ProbeNotRunning + } + return ProbeUnreachable +} + +// ResolvePort returns the local port the probe should target. +// The ENGRAM_PORT environment variable takes precedence when it +// parses to a valid port in [1, 65535]; otherwise the default +// 7437 is returned. Whitespace-only and out-of-range values fall +// back to the default. Surrounding whitespace on a valid value +// is trimmed before parsing, matching the existing CLI's +// resolveDaemonProbePort in cmd/engram/cloud_daemon_probe.go. +func ResolvePort() int { + v := strings.TrimSpace(os.Getenv(EnvProbePort)) + if v == "" { + return DefaultProbePort + } + port, err := strconv.Atoi(v) + if err != nil { + return DefaultProbePort + } + if port < 1 || port > 65535 { + return DefaultProbePort + } + return port +} diff --git a/internal/cloudconfig/daemon_test.go b/internal/cloudconfig/daemon_test.go new file mode 100644 index 00000000..7f49b3d0 --- /dev/null +++ b/internal/cloudconfig/daemon_test.go @@ -0,0 +1,302 @@ +package cloudconfig + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + "time" +) + +// TestLocalDaemonProbe200 is the happy-path test for LocalDaemonProbe: +// a 2xx response on the test server's /health endpoint classifies as +// ProbeRunning with a nil error. The returned port must match the one +// supplied to the probe. +func TestLocalDaemonProbe200(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/health" { + http.NotFound(w, r) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"status":"ok"}`)) + })) + defer srv.Close() + + port := portFromTestServer(t, srv) + res := LocalDaemonProbe(context.Background(), port) + + if res.Status != ProbeRunning { + t.Fatalf("expected ProbeRunning, got %q (err=%v)", res.Status, res.Err) + } + if res.Err != nil { + t.Fatalf("expected nil err on 200, got %v", res.Err) + } + if res.Port != port { + t.Fatalf("expected port %d, got %d", port, res.Port) + } +} + +// TestLocalDaemonProbeNotRunning covers the "daemon is not listening" +// branch: dial to a closed port on 127.0.0.1 must classify as +// ProbeNotRunning with a non-nil error. The port is allocated by +// binding then closing a listener so the test is deterministic across +// CI runs. +func TestLocalDaemonProbeNotRunning(t *testing.T) { + t.Parallel() + + port := allocateClosedPort(t) + res := LocalDaemonProbe(context.Background(), port) + + if res.Status != ProbeNotRunning { + t.Fatalf("expected ProbeNotRunning on closed port, got %q (err=%v)", res.Status, res.Err) + } + if res.Err == nil { + t.Fatalf("expected non-nil err for refused dial") + } + if res.Port != port { + t.Fatalf("expected port %d, got %d", port, res.Port) + } +} + +// TestLocalDaemonProbeNotRunningIsNetOpError is the TRIANGULATE pin +// for the dial classification: the spec mandates that connection +// refused be classified as ProbeNotRunning via *net.OpError{Op:"dial"}. +// This test makes the contract explicit so a future refactor cannot +// silently change the error type without breaking the test. +func TestLocalDaemonProbeNotRunningIsNetOpError(t *testing.T) { + t.Parallel() + + port := allocateClosedPort(t) + res := LocalDaemonProbe(context.Background(), port) + + if res.Status != ProbeNotRunning { + t.Fatalf("expected ProbeNotRunning on closed port, got %q", res.Status) + } + var opErr *net.OpError + if !errors.As(res.Err, &opErr) { + t.Fatalf("expected error to unwrap to *net.OpError, got %T (%v)", res.Err, res.Err) + } + if opErr.Op != "dial" { + t.Fatalf("expected *net.OpError.Op == \"dial\", got %q", opErr.Op) + } +} + +// TestLocalDaemonProbeUnreachable500 covers the "server answers but +// the answer is broken" branch: a 5xx response classifies as +// ProbeUnreachable with a non-nil error that explains the bad status. +func TestLocalDaemonProbeUnreachable500(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + port := portFromTestServer(t, srv) + res := LocalDaemonProbe(context.Background(), port) + + if res.Status != ProbeUnreachable { + t.Fatalf("expected ProbeUnreachable on 500, got %q (err=%v)", res.Status, res.Err) + } + if res.Err == nil { + t.Fatalf("expected non-nil err for 500 response") + } + if res.Port != port { + t.Fatalf("expected port %d, got %d", port, res.Port) + } +} + +// slowTransport is a RoundTripper that blocks until either the +// request context is canceled (e.g. by http.Client.Timeout) or a +// fallback delay elapses. Used to exercise the timeout branch of +// LocalDaemonProbe deterministically without a real network. +type slowTransport struct { + delay time.Duration +} + +func (s *slowTransport) RoundTrip(req *http.Request) (*http.Response, error) { + select { + case <-time.After(s.delay): + return nil, fmt.Errorf("slow transport: request not canceled within %v", s.delay) + case <-req.Context().Done(): + return nil, req.Context().Err() + } +} + +// TestLocalDaemonProbeTimeout covers the "server never responds" +// branch: when the request exceeds ProbeTimeout, the probe must +// classify as ProbeUnreachable. The test uses a custom slowTransport +// that blocks until the request context is canceled; the 50ms +// ProbeTimeout fires before the 200ms transport delay completes. +// +// NOTE: this test mutates the package-level ProbeTimeout and +// ProbeTransport vars, so it must run serially (no t.Parallel). +// Otherwise parallel probe tests would observe the modified +// transport and timeout, surfacing flaky "context deadline exceeded" +// errors instead of the intended status. Mirrors the pattern in +// cmd/engram/cloud_daemon_probe_test.go:62-89. +func TestLocalDaemonProbeTimeout(t *testing.T) { + + prevTimeout := ProbeTimeout + prevTransport := ProbeTransport + ProbeTimeout = 50 * time.Millisecond + ProbeTransport = &slowTransport{delay: 200 * time.Millisecond} + t.Cleanup(func() { + ProbeTimeout = prevTimeout + ProbeTransport = prevTransport + }) + + port := allocateClosedPort(t) + res := LocalDaemonProbe(context.Background(), port) + + if res.Status != ProbeUnreachable { + t.Fatalf("expected ProbeUnreachable on timeout, got %q (err=%v)", res.Status, res.Err) + } + if res.Err == nil { + t.Fatalf("expected non-nil err for timeout") + } +} + +// TestLocalDaemonProbeContextCanceled covers the "caller cancels +// the probe" branch: a pre-canceled context surfaces as +// ProbeUnreachable (not ProbeNotRunning) because the cancellation +// wraps a context error, not a *net.OpError dial error. +func TestLocalDaemonProbeContextCanceled(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // pre-cancel + + port := allocateClosedPort(t) + res := LocalDaemonProbe(ctx, port) + + if res.Status != ProbeUnreachable { + t.Fatalf("expected ProbeUnreachable on canceled context, got %q (err=%v)", res.Status, res.Err) + } + if res.Err == nil { + t.Fatalf("expected non-nil err for canceled context") + } + if res.Port != port { + t.Fatalf("expected port %d, got %d", port, res.Port) + } +} + +// TestResolvePort covers the ResolvePort contract: ENGRAM_PORT +// takes precedence when valid (1-65535 inclusive), and the function +// falls back to DefaultProbePort (7437) for unset, empty, +// whitespace-only, non-numeric, zero, negative, or out-of-range +// values. The implementation must trim whitespace before parsing. +// +// NOTE: t.Setenv is incompatible with t.Parallel, so this test runs +// serially. Each subtest sets a fresh t.Setenv and relies on t.Setenv's +// automatic Cleanup to restore the parent value. +func TestResolvePort(t *testing.T) { + + cases := []struct { + name string + envVal string + want int + }{ + {"unset", "", 7437}, + {"valid_low", "1", 1}, + {"valid_high", "65535", 65535}, + {"valid_typical", "8080", 8080}, + {"valid_7437", "7437", 7437}, + {"invalid_nonnumeric", "abc", 7437}, + {"invalid_zero", "0", 7437}, + {"invalid_negative", "-1", 7437}, + {"invalid_too_high", "99999", 7437}, + {"empty_value", "", 7437}, + {"whitespace", " ", 7437}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Setenv("ENGRAM_PORT", tc.envVal) + if got := ResolvePort(); got != tc.want { + t.Fatalf("ENGRAM_PORT=%q: got %d, want %d", tc.envVal, got, tc.want) + } + }) + } +} + +// TestResolvePortTrimsWhitespace is the TRIANGULATE pin for +// whitespace handling: a value like " 8080 " must parse to 8080 +// because the function trims surrounding whitespace before calling +// strconv.Atoi. Matches the existing CLI's resolveDaemonProbePort +// behavior in cmd/engram/cloud_daemon_probe.go. +// +// NOTE: t.Setenv is incompatible with t.Parallel, so this test runs +// serially. +func TestResolvePortTrimsWhitespace(t *testing.T) { + t.Setenv("ENGRAM_PORT", " 8080 ") + if got := ResolvePort(); got != 8080 { + t.Fatalf("expected 8080 (whitespace-trimmed), got %d", got) + } +} + +// TestProbeStatusStringer pins the user-facing label of each +// ProbeStatus value. The view layer (TUI/CLI) renders these labels +// directly, so a silent rename would be a user-visible regression. +func TestProbeStatusStringer(t *testing.T) { + t.Parallel() + + cases := []struct { + status ProbeStatus + want string + }{ + {ProbeRunning, "running"}, + {ProbeNotRunning, "not running"}, + {ProbeUnreachable, "unreachable"}, + {ProbeStatus(99), "unknown"}, + } + + for _, tc := range cases { + if got := tc.status.String(); got != tc.want { + t.Fatalf("ProbeStatus(%d).String() = %q, want %q", tc.status, got, tc.want) + } + } +} + +// portFromTestServer extracts the TCP port a httptest.Server is +// bound to. Mirrors the helper in cmd/engram/cloud_daemon_probe_test.go +// (private to that package). +func portFromTestServer(t *testing.T, srv *httptest.Server) int { + t.Helper() + parsed, err := url.Parse(srv.URL) + if err != nil { + t.Fatalf("parse server URL: %v", err) + } + port, err := strconv.Atoi(parsed.Port()) + if err != nil { + t.Fatalf("parse server port: %v", err) + } + return port +} + +// allocateClosedPort returns a TCP port number that is guaranteed +// to be closed: it binds, reads the assigned port, then closes the +// listener. On loopback this reliably surfaces "connection refused" +// on dial, which is the path the ProbeNotRunning classification +// hinges on. +func allocateClosedPort(t *testing.T) int { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + port := ln.Addr().(*net.TCPAddr).Port + if err := ln.Close(); err != nil { + t.Fatalf("close listener: %v", err) + } + return port +} diff --git a/internal/cloudconfig/server.go b/internal/cloudconfig/server.go new file mode 100644 index 00000000..52460cf9 --- /dev/null +++ b/internal/cloudconfig/server.go @@ -0,0 +1,71 @@ +package cloudconfig + +import ( + "errors" + "fmt" + "net/url" + "strings" +) + +// ValidateServerURL returns the normalized server URL after enforcing +// the contract documented in REQ-1 of the spec: +// +// - the scheme is http or https (rejected otherwise) +// - the host is non-empty (rejected otherwise) +// - the query and fragment components are CLEARED on success +// - the path, port, trailing slash, and userinfo are preserved +// as-given (no normalization beyond query/fragment stripping) +// +// ValidateServerURL returns an error for empty or whitespace-only +// input, malformed URLs that fail url.Parse, URLs whose scheme is +// not http or https, and URLs with no host. +// +// The function uses url.Parse (permissive) rather than +// url.ParseRequestURI (strict); the scheme and host checks above +// are what enforce the contract, not url.Parse itself. Trailing +// slashes are preserved naturally by url.URL.String() because +// url.Parse stores the path as-is. +func ValidateServerURL(raw string) (string, error) { + u, err := parseURL(raw) + if err != nil { + return "", err + } + return u.String(), nil +} + +// parseURL normalizes raw into a *url.URL with the scheme/host +// contract enforced and query/fragment cleared. It is the workhorse +// for ValidateServerURL; splitting it out keeps ValidateServerURL +// trivial (one parse + one stringify) and makes the validation +// steps individually testable. +// +// The order of checks matters: +// 1. whitespace-only / empty input is rejected first, so the +// caller gets a clear "empty" error rather than a downstream +// parse error or a silent zero-value URL. +// 2. url.Parse errors propagate (wrapped with the input for +// debuggability). +// 3. the scheme check catches `://example.com` (which url.Parse +// rejects) and the "not a URL" cases (`#frag`, `?q=1`, `not a +// url`, all of which url.Parse accepts with an empty scheme). +// 4. the host check catches `https://` and `https://?q=1`. +// 5. query/fragment are cleared last so the caller gets the +// canonical stored form. +func parseURL(raw string) (*url.URL, error) { + if strings.TrimSpace(raw) == "" { + return nil, errors.New("server URL is empty") + } + u, err := url.Parse(raw) + if err != nil { + return nil, fmt.Errorf("invalid server URL %q: %w", raw, err) + } + if u.Scheme != "http" && u.Scheme != "https" { + return nil, fmt.Errorf("server URL must use http or https, got %q", u.Scheme) + } + if u.Host == "" { + return nil, errors.New("server URL must have a host") + } + u.RawQuery = "" + u.Fragment = "" + return u, nil +} diff --git a/internal/cloudconfig/server_test.go b/internal/cloudconfig/server_test.go new file mode 100644 index 00000000..ea93b909 --- /dev/null +++ b/internal/cloudconfig/server_test.go @@ -0,0 +1,275 @@ +package cloudconfig + +import ( + "testing" +) + +// TestValidateServerURL is the table-driven test for the +// ValidateServerURL contract documented in REQ-1 of the spec: +// +// - http or https scheme only +// - host required (non-empty) +// - query and fragment CLEARED on success (not rejected) +// - trailing slash preserved as-given +// - error on empty, whitespace-only, missing-scheme, missing-host, +// or unparseable input +// +// The test uses url.Parse semantics (permissive); the scheme and +// host checks are what enforce validation, not url.Parse itself. +// +// The port-out-of-range case is intentionally absent: Go's url.Parse +// does not validate port ranges (https://example.com:99999 parses +// successfully), and the spec does not require it. Adding a port +// range check would expand scope beyond the spec. +func TestValidateServerURL(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + wantErr bool + }{ + // --- happy path: http and https --- + {"https_basic", "https://example.com", "https://example.com", false}, + {"http_basic", "http://example.com", "http://example.com", false}, + {"https_with_port", "https://example.com:8080", "https://example.com:8080", false}, + {"https_with_trailing_slash", "https://example.com/", "https://example.com/", false}, + {"https_with_path", "https://example.com/path", "https://example.com/path", false}, + {"https_with_deep_path", "https://example.com/api/v1", "https://example.com/api/v1", false}, + + // --- query/fragment cleared on success (TRIANGULATE) --- + {"https_query_cleared", "https://example.com?q=1", "https://example.com", false}, + {"https_fragment_cleared", "https://example.com#frag", "https://example.com", false}, + {"https_path_query_fragment", "https://example.com/path?q=1#frag", "https://example.com/path", false}, + {"https_multi_query_cleared", "https://example.com?a=1&b=2#frag", "https://example.com", false}, + + // --- error: bad scheme --- + {"ftp_scheme", "ftp://example.com", "", true}, + {"file_scheme", "file://example.com", "", true}, + + // --- error: missing scheme --- + {"missing_scheme", "://example.com", "", true}, + + // --- error: missing host --- + {"https_no_host", "https://", "", true}, + {"http_no_host", "http://", "", true}, + {"https_empty_host_with_query", "https://?q=1", "", true}, + + // --- error: not actually a URL --- + {"just_fragment", "#frag", "", true}, + {"just_query", "?q=1", "", true}, + {"empty", "", "", true}, + {"whitespace_only", " ", "", true}, + {"unparseable", "not a url", "", true}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if tc.wantErr { + if err == nil { + t.Fatalf("ValidateServerURL(%q) = (%q, nil), want error", tc.input, got) + } + // The returned string is undefined on error; do not assert on it. + return + } + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q", tc.input, got, tc.want) + } + }) + } +} + +// TestValidateServerURLPreservesTrailingSlash is a prominent +// regression test for the design's intent: the trailing slash is +// preserved exactly as the user provided it, not normalized away. +// The table-driven test above also covers this case; this test makes +// the contract explicit and is the first thing a reviewer sees when +// they ask "does the function mangle trailing slashes?". +func TestValidateServerURLPreservesTrailingSlash(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + {"https_with_slash", "https://example.com/", "https://example.com/"}, + {"https_without_slash", "https://example.com", "https://example.com"}, + {"https_with_path_and_slash", "https://example.com/api/", "https://example.com/api/"}, + {"https_with_path_no_slash", "https://example.com/api", "https://example.com/api"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q (trailing slash must be preserved)", tc.input, got, tc.want) + } + }) + } +} + +// TestValidateServerURLClearsQueryAndFragment is a prominent +// regression test for the design's intent: query and fragment +// components are CLEARED on success, not rejected. A URL with +// `?q=1` or `#frag` is valid; the components just don't make it +// into the stored config. +func TestValidateServerURLClearsQueryAndFragment(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + {"query_only", "https://example.com?q=1", "https://example.com"}, + {"fragment_only", "https://example.com#frag", "https://example.com"}, + {"multi_query", "https://example.com?a=1&b=2", "https://example.com"}, + {"path_with_query", "https://example.com/api?q=1", "https://example.com/api"}, + {"path_with_fragment", "https://example.com/api#frag", "https://example.com/api"}, + {"path_with_query_and_fragment", "https://example.com/api?q=1#frag", "https://example.com/api"}, + {"trailing_slash_with_query", "https://example.com/?q=1", "https://example.com/"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q (query/fragment must be cleared)", tc.input, got, tc.want) + } + // Sanity: the returned string must not contain a query or fragment marker. + if got != "" { + for _, marker := range []string{"?", "#"} { + if containsAny(got, marker) { + t.Errorf("ValidateServerURL(%q) = %q, must not contain %q after clearing", tc.input, got, marker) + } + } + } + }) + } +} + +// TestValidateServerURLPreservesPath is a regression test for the +// design's intent: the path component is preserved as-given. The +// spec only clears query and fragment; everything else (scheme, +// host, port, path) round-trips. +func TestValidateServerURLPreservesPath(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + {"single_segment", "https://example.com/api", "https://example.com/api"}, + {"deep_path", "https://example.com/api/v1/sessions", "https://example.com/api/v1/sessions"}, + {"path_with_dash", "https://example.com/api-v1/sessions", "https://example.com/api-v1/sessions"}, + {"path_with_underscore", "https://example.com/api_v1/sessions", "https://example.com/api_v1/sessions"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q (path must be preserved)", tc.input, got, tc.want) + } + }) + } +} + +// TestValidateServerURLPreservesUserInfo is a regression test for +// Go's url.Parse behavior: userinfo in the form user:pass@host is +// preserved in the URL. The spec doesn't require validation of +// userinfo, and the existing CLI's validateCloudServerURL also +// preserves it (it does not strip userinfo). This test pins the +// behavior so a future "validation tightening" change does not +// silently strip credentials. +func TestValidateServerURLPreservesUserInfo(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + {"user_pass", "https://user:pass@example.com", "https://user:pass@example.com"}, + {"user_only", "https://user@example.com", "https://user@example.com"}, + {"user_pass_with_path", "https://user:pass@example.com/api", "https://user:pass@example.com/api"}, + {"user_pass_with_port", "https://user:pass@example.com:8080", "https://user:pass@example.com:8080"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q (userinfo must be preserved)", tc.input, got, tc.want) + } + }) + } +} + +// TestValidateServerURLIPv6 is a regression test for Go's url.Parse +// behavior: IPv6 hosts are wrapped in brackets and must round-trip +// correctly. This is the format Go itself emits when normalizing an +// IPv6 URL, so the validator must accept and return it intact. +func TestValidateServerURLIPv6(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + input string + want string + }{ + {"loopback_with_port", "https://[::1]:8080", "https://[::1]:8080"}, + {"loopback_no_port", "https://[::1]", "https://[::1]"}, + {"ipv6_with_path", "https://[::1]/api", "https://[::1]/api"}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := ValidateServerURL(tc.input) + if err != nil { + t.Fatalf("ValidateServerURL(%q) returned err=%v, want nil", tc.input, err) + } + if got != tc.want { + t.Errorf("ValidateServerURL(%q) = %q, want %q (IPv6 host must round-trip)", tc.input, got, tc.want) + } + }) + } +} + +// containsAny is a tiny helper to keep the post-clear sanity check +// readable. It is unexported because nothing outside the package +// needs it. +func containsAny(s, substr string) bool { + for i := 0; i+len(substr) <= len(s); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/internal/cloudconfig/token.go b/internal/cloudconfig/token.go new file mode 100644 index 00000000..f0437c7e --- /dev/null +++ b/internal/cloudconfig/token.go @@ -0,0 +1,144 @@ +package cloudconfig + +import ( + "os" + "strings" +) + +// EnvCloudToken is the environment variable consulted by +// EffectiveToken when resolving the runtime cloud token. Defining +// it as a constant keeps the contract discoverable and prevents +// typos at call sites; both the CLI and the TUI read the same +// variable, and the TUI's precedence drift (the bug this package +// fixes) was caused by the TUI reading the env var inconsistently +// with the CLI. +const EnvCloudToken = "ENGRAM_CLOUD_TOKEN" + +// User-facing labels for each Source value. These are the +// canonical strings the CLI prints on its "Auth status" line and +// the TUI's view layer renders via the TokenSource* constants. +// The strings are exported as constants so that consumers (TUI, +// future CLI status subcommands, dashboard UIs, log messages) +// can reference them directly instead of duplicating the literal +// — which is exactly the drift that produced the TUI's silent +// precedence bug this package fixes. +// +// The TUI/CLI agreement is pinned by TestCLIAndTUIAgreeOnTokenSource +// (T-608.5): if you change any of these strings, update every +// consumer to match in the same commit. +const ( + // LabelSourceNone is the user-facing label for SourceNone + // ("the runtime cloud token is not configured"). + LabelSourceNone = "not set" + + // LabelSourceFile is the user-facing label for SourceFile + // ("the runtime cloud token was read from cloud.json"). + LabelSourceFile = "read from cloud.json" + + // LabelSourceEnv is the user-facing label for SourceEnv + // ("the runtime cloud token was provided via the + // ENGRAM_CLOUD_TOKEN environment variable"). + LabelSourceEnv = "set via ENGRAM_CLOUD_TOKEN" +) + +// Source identifies which input provided the effective token. The +// zero value is SourceNone, which also happens to be the value +// returned when no token is configured. +type Source int + +const ( + // SourceNone indicates no token is configured. The effective + // token returned alongside this value is always empty. + SourceNone Source = iota + + // SourceFile indicates the token came from cloud.json inside + // the supplied data directory. The file must exist and + // contain a non-empty Token field for this to be the source. + SourceFile + + // SourceEnv indicates the token came from the + // ENGRAM_CLOUD_TOKEN environment variable. The variable must + // be set and non-empty (after whitespace trimming) for this + // to be the source. + SourceEnv +) + +// String returns the user-facing label for the source. The label +// is the same string the CLI auth status line prints; the TUI's +// TokenSource* constants are expected to mirror it via +// SourceLabel. The TUI/CLI parity is pinned by +// TestCLIAndTUIAgreeOnTokenSource (T-608.5): if you change a +// label, update every consumer to match in the same commit. +func (s Source) String() string { + switch s { + case SourceFile: + return LabelSourceFile + case SourceEnv: + return LabelSourceEnv + default: + return LabelSourceNone + } +} + +// SourceLabel returns the user-facing label for a Source value. +// The CLI uses Source internally to choose behavior; the TUI uses +// SourceLabel(Source) to display the same source. The strings +// MUST match what the TUI's view layer renders (or what the CLI's +// status command prints) for the same (file, env) input, so the +// regression test TestCLIAndTUIAgreeOnTokenSource pins this +// invariant. If you change a label, update the TUI's view (and +// any other consumer) to match. +// +// The function form is provided for callers that prefer functions +// over methods (e.g. a switch statement in a view layer that +// needs to map a Source to a string). +func SourceLabel(s Source) string { + return s.String() +} + +// EffectiveToken resolves the runtime cloud token by consulting +// the ENGRAM_CLOUD_TOKEN environment variable and cloud.json +// inside dataDir, in that order. +// +// The env var takes precedence: when it is set and non-empty +// (after whitespace trimming), the file is not consulted. +// Otherwise the file's Token field is used if it is non-empty +// (after whitespace trimming). Otherwise the effective token is +// empty and the source is SourceNone. +// +// Whitespace-only env values are treated as unset. Surrounding +// whitespace on a real env value is stripped before the value is +// returned, so a token in the form " e1 " surfaces as "e1". +// This matches the existing CLI's resolveCloudRuntimeConfig +// behavior at cmd/engram/main.go:447, where the trimmed value +// is what gets written to the runtime config. +func EffectiveToken(dataDir string) (string, Source) { + if v := os.Getenv(EnvCloudToken); strings.TrimSpace(v) != "" { + return strings.TrimSpace(v), SourceEnv + } + if tok, ok := readFileToken(dataDir); ok { + return tok, SourceFile + } + return "", SourceNone +} + +// readFileToken returns the token stored in cloud.json inside +// dataDir. The second return is true only when a non-empty token +// was found; false indicates either a load error, missing file, +// nil config, or an empty/whitespace-only token. +// +// Treating an empty file token as "no token" is intentional: it +// lets the caller distinguish "the file says we have no token" +// from "the file has a real token", and it keeps the function's +// contract simple — a non-empty token always comes back with +// SourceFile, never SourceNone. +func readFileToken(dataDir string) (string, bool) { + cfg, err := Load(dataDir) + if err != nil || cfg == nil { + return "", false + } + if strings.TrimSpace(cfg.Token) == "" { + return "", false + } + return cfg.Token, true +} diff --git a/internal/cloudconfig/token_test.go b/internal/cloudconfig/token_test.go new file mode 100644 index 00000000..7f83dbce --- /dev/null +++ b/internal/cloudconfig/token_test.go @@ -0,0 +1,225 @@ +package cloudconfig + +import ( + "os" + "testing" +) + +// writeCloudConfig serializes cfg to cloud.json inside dir. It is a +// thin wrapper around Save used by the EffectiveToken tests to set +// up the file side of the precedence table. +func writeCloudConfig(t *testing.T, dir string, cfg Config) { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("setup MkdirAll(%q): %v", dir, err) + } + if err := Save(dir, &cfg); err != nil { + t.Fatalf("setup Save(%q, %+v): %v", dir, cfg, err) + } +} + +// TestEffectiveToken is the table-driven test for the precedence +// contract documented in REQ-1 of the spec and the design's "Token +// + URL precedence" section: env (when set and non-empty after +// whitespace trimming) wins over the file; an empty file token is +// treated as "no token" and surfaces as SourceNone. +// +// Each subtest sets a fresh t.Setenv("ENGRAM_CLOUD_TOKEN") (so any +// inherited value from the parent shell is cleared) and writes a +// fresh cloud.json (or skips the write) for the file side. +// +// NOTE: t.Setenv is incompatible with t.Parallel, so the test is +// intentionally not parallel. This is correct: env-var changes +// affect the whole process, and t.Setenv's Cleanup restores the +// previous value when the subtest ends, so test order is also safe. +func TestEffectiveToken(t *testing.T) { + cases := []struct { + name string + fileContent *Config // nil = no file written + envValue string // "" clears any inherited value via t.Setenv + wantToken string + wantSource Source + }{ + { + name: "file+env", + fileContent: &Config{Token: "t1"}, + envValue: "e1", + wantToken: "e1", + wantSource: SourceEnv, + }, + { + name: "file_only", + fileContent: &Config{Token: "t1"}, + envValue: "", + wantToken: "t1", + wantSource: SourceFile, + }, + { + name: "env_only", + fileContent: nil, + envValue: "e1", + wantToken: "e1", + wantSource: SourceEnv, + }, + { + name: "neither", + fileContent: nil, + envValue: "", + wantToken: "", + wantSource: SourceNone, + }, + { + name: "whitespace_env", + fileContent: &Config{Token: "t1"}, + envValue: " ", + wantToken: "t1", + wantSource: SourceFile, + }, + { + name: "empty_file", + fileContent: &Config{Token: ""}, + envValue: "", + wantToken: "", + wantSource: SourceNone, + }, + { + name: "env_with_surrounding_spaces", + fileContent: &Config{Token: "t1"}, + envValue: " e1 ", + wantToken: "e1", + wantSource: SourceEnv, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + // Always set the env var; empty string clears any + // inherited value from the parent shell. + t.Setenv("ENGRAM_CLOUD_TOKEN", tc.envValue) + + dir := t.TempDir() + if tc.fileContent != nil { + writeCloudConfig(t, dir, *tc.fileContent) + } + + gotToken, gotSource := EffectiveToken(dir) + if gotToken != tc.wantToken { + t.Errorf("EffectiveToken(%q): token = %q, want %q", dir, gotToken, tc.wantToken) + } + if gotSource != tc.wantSource { + t.Errorf("EffectiveToken(%q): source = %v, want %v", dir, gotSource, tc.wantSource) + } + }) + } +} + +// TestSourceLabel verifies that each Source value maps to the +// user-facing label the CLI auth status line and the TUI's +// TokenSource* constants are expected to mirror. The labels are +// part of the user-visible output, so a regression here is +// user-visible. +func TestSourceLabel(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + source Source + want string + }{ + {"none", SourceNone, "not set"}, + {"file", SourceFile, "read from cloud.json"}, + {"env", SourceEnv, "set via ENGRAM_CLOUD_TOKEN"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := SourceLabel(tc.source); got != tc.want { + t.Errorf("SourceLabel(%v) = %q, want %q", tc.source, got, tc.want) + } + }) + } +} + +// TestSourceEnumOrdering guards against accidental reordering of +// the iota block. The integer values are part of the package's +// observable behavior: the TUI's TokenSource* mapping and any +// future switch on Source values depend on the order. +func TestSourceEnumOrdering(t *testing.T) { + t.Parallel() + + if SourceNone != 0 { + t.Errorf("SourceNone = %d, want 0", SourceNone) + } + if SourceFile != 1 { + t.Errorf("SourceFile = %d, want 1", SourceFile) + } + if SourceEnv != 2 { + t.Errorf("SourceEnv = %d, want 2", SourceEnv) + } +} + +// TestEffectiveTokenEnvWinsOverFile is a prominent regression +// test for the design's intent: env wins over file. The +// table-driven test covers each input in isolation, but this test +// makes the precedence explicit and is the first thing a reviewer +// sees when they ask "what does the function do when both are +// set?". +func TestEffectiveTokenEnvWinsOverFile(t *testing.T) { + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-wins") + dir := t.TempDir() + writeCloudConfig(t, dir, Config{Token: "file-loses"}) + + tok, src := EffectiveToken(dir) + if tok != "env-wins" { + t.Errorf("EffectiveToken: token = %q, want %q (env must win over file)", tok, "env-wins") + } + if src != SourceEnv { + t.Errorf("EffectiveToken: source = %v, want %v (env must win over file)", src, SourceEnv) + } +} + +// TestEffectiveTokenHandlesMalformedFile is a defensive test: +// when cloud.json exists but contains invalid JSON, Load returns +// (nil, err). EffectiveToken must fall through to env (or +// SourceNone when env is also unset) without panicking. The +// "nil config" defensive branch in readFileToken is exercised +// indirectly via the same path: the malformed file triggers +// err != nil, which short-circuits to ("", false). +func TestEffectiveTokenHandlesMalformedFile(t *testing.T) { + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + dir := t.TempDir() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("setup MkdirAll: %v", err) + } + if err := os.WriteFile(Path(dir), []byte("{not valid json"), 0o644); err != nil { + t.Fatalf("setup WriteFile: %v", err) + } + + tok, src := EffectiveToken(dir) + if tok != "" { + t.Errorf("EffectiveToken: token = %q, want empty (malformed file must not surface)", tok) + } + if src != SourceNone { + t.Errorf("EffectiveToken: source = %v, want %v", src, SourceNone) + } +} + +// TestSourceLabelStringer verifies that SourceLabel and the +// Source.String method return the same string for every Source +// value. They are two call shapes for the same data; if they +// ever drift, a caller using one form will see a different label +// than a caller using the other, and the CLI/TUI parity test +// (TestCLIAndTUIAgreeOnTokenSource, planned for T-608.5) will +// catch the drift only if the TUI happens to use the same form. +func TestSourceLabelStringer(t *testing.T) { + t.Parallel() + + for _, s := range []Source{SourceNone, SourceFile, SourceEnv} { + if got, want := SourceLabel(s), s.String(); got != want { + t.Errorf("SourceLabel(%v) = %q, want %q (== String())", s, got, want) + } + } +} diff --git a/internal/store/store.go b/internal/store/store.go index 9c6537b9..615ccf06 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -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 { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 5ed55ca6..ae4f0f77 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -1382,6 +1382,13 @@ 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 TestStoreLocalSyncFoundationEnqueuesCoreMutations(t *testing.T) { s := newTestStore(t) diff --git a/internal/tui/cloud.go b/internal/tui/cloud.go new file mode 100644 index 00000000..bd89c46f --- /dev/null +++ b/internal/tui/cloud.go @@ -0,0 +1,159 @@ +package tui + +import ( + "context" + "fmt" + "net/http" + "os" + "strings" + "time" + + "github.com/Gentleman-Programming/engram/internal/cloudconfig" + + tea "github.com/charmbracelet/bubbletea" +) + +const cloudHealthPath = "/health" + +// TokenSourceEnv is the display label used when ENGRAM_CLOUD_TOKEN +// environment variable is set. Mirrors cloudconfig.LabelSourceEnv so the +// view layer can render the same string the CLI's status line prints; +// the parity is pinned by cloudconfig.TestCLIAndTUIAgreeOnTokenSource. +const TokenSourceEnv = "set via ENGRAM_CLOUD_TOKEN" + +// TokenSourceFile is the display label used when a token is read +// from the cloud.json config file. Mirrors cloudconfig.LabelSourceFile. +const TokenSourceFile = "read from cloud.json" + +// TokenSourceNone is the display label used when no cloud token is +// available from any source. Mirrors cloudconfig.LabelSourceNone. +const TokenSourceNone = "not set" + +// envCloudServer is the environment variable the TUI's Cloud Config form +// honors as an override of the file-stored server URL. It matches the +// CLI's cmd/engram/main.go:resolveCloudRuntimeConfig behavior; the TUI's +// old effectiveCloudToken / loadCloudConfigCmd did not, which is the +// silent precedence drift this package fixes. +const envCloudServer = "ENGRAM_CLOUD_SERVER" + +// tuiCloudConfigForUI returns the effective Config the TUI's Cloud +// Config form should display and persist: the cloudconfig.Load result +// with the ENGRAM_CLOUD_SERVER env var applied as an override when it +// is set and non-empty (after whitespace trimming). +// +// A missing or malformed cloud.json yields a zero-value Config (no +// error surfaced to the view — the user can still type a URL and save). +// A whitespace-only env value is treated as unset, matching the CLI. +func tuiCloudConfigForUI(dataDir string) cloudconfig.Config { + cfg, _ := cloudconfig.Load(dataDir) + if cfg == nil { + cfg = &cloudconfig.Config{} + } + if v := strings.TrimSpace(os.Getenv(envCloudServer)); v != "" { + cfg.ServerURL = v + } + return *cfg +} + +// saveTUIServerURL persists serverURL into the cloud.json inside dataDir +// while preserving any pre-existing Token field. It replaces the old +// forked saveCloudConfig (T-608.16): load the existing config, update +// only the URL, save through the package so perms + atomicity are owned +// in one place. +func saveTUIServerURL(dataDir, serverURL string) error { + cfg, err := cloudconfig.Load(dataDir) + if err != nil { + return err + } + if cfg == nil { + cfg = &cloudconfig.Config{} + } + cfg.ServerURL = serverURL + return cloudconfig.Save(dataDir, cfg) +} + +// cloudDaemonProbeMsg is delivered when probeLocalDaemonCmd completes. +// It carries the cloudconfig.Result from a single LocalDaemonProbe call +// (T-608.17) so the TUI can render the daemon status independently of +// the cloud-config form's load message. +// +// T-608.19 wires the message into the update loop: the Cloud Status +// arm dispatches probeLocalDaemonCmd alongside the status load, and a +// new case arm in Update writes probe.result to m.CloudDaemonProbe. +type cloudDaemonProbeMsg struct { + result cloudconfig.Result + err error +} + +// probeLocalDaemonCmd returns a tea.Cmd that runs cloudconfig.LocalDaemonProbe +// once (with the default 1s ProbeTimeout, per ADR-1) and delivers the +// outcome as a cloudDaemonProbeMsg. The probe targets the port returned +// by cloudconfig.ResolvePort, which honors ENGRAM_PORT. +// +// This is the TUI's local-daemon probe entry point. It is decoupled +// from the cloud /health probe (pingCloudServer, kept inline per ADR-2): +// the two have different timeouts (1s vs 3s) and different status +// mappings (ProbeRunning/ProbeNotRunning/ProbeUnreachable vs +// reachable/unauthorized/unreachable). +func probeLocalDaemonCmd() tea.Cmd { + return func() tea.Msg { + result := cloudconfig.LocalDaemonProbe(context.Background(), cloudconfig.ResolvePort()) + return cloudDaemonProbeMsg{result: result} + } +} + +// pingCloudTransport can be overridden in tests to avoid real network calls. +var pingCloudTransport http.RoundTripper = http.DefaultTransport + +// pingCloudServer returns a tea.Cmd that probes the cloud server health +// endpoint. The result is delivered as a cloudPingMsg. +// +// Use this when the user triggers a "Test Connection" action on the Cloud +// Config screen. Kept inline in the TUI per ADR-2: it is a TUI-only +// tea.Cmd wrapper around a synchronous HTTP GET with its own 3s timeout +// and a different status mapping than the local-daemon probe; the +// cloudconfig package does not need to own it. +func pingCloudServer(serverURL, token string) tea.Cmd { + return func() tea.Msg { + status, err := pingCloudServerStatus(serverURL, token) + return cloudPingMsg{status: status, err: err} + } +} + +// pingCloudServerStatus performs a synchronous HTTP GET to the cloud +// server's /health endpoint. +// +// Returns "reachable" on a 2xx response, "unauthorized" on 401, +// or "unreachable" on any other error. URL validation delegates to +// cloudconfig.ValidateServerURL (T-608.17), which accepts http/https +// with a host, clears query/fragment on success. +func pingCloudServerStatus(serverURL, token string) (string, error) { + validatedURL, err := cloudconfig.ValidateServerURL(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) + } +} diff --git a/internal/tui/cloud_test.go b/internal/tui/cloud_test.go new file mode 100644 index 00000000..51e93120 --- /dev/null +++ b/internal/tui/cloud_test.go @@ -0,0 +1,435 @@ +package tui + +import ( + "bytes" + "context" + "errors" + "io" + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/Gentleman-Programming/engram/internal/cloudconfig" +) + +func writeCloudJSON(t *testing.T, dir, content string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, "cloud.json"), []byte(content), 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } +} + +// saveTUIServerURL tests — the helper that writes the server URL into +// cloud.json while preserving any pre-existing Token field. Replaces +// the forked saveCloudConfig deleted in T-608.16. + +func TestSaveTUIServerURLPreservesToken(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://old.example.com","token":"file-token"}`) + + if err := saveTUIServerURL(dir, "https://new.example.com"); err != nil { + t.Fatalf("saveTUIServerURL: %v", err) + } + + b, err := os.ReadFile(filepath.Join(dir, "cloud.json")) + if err != nil { + t.Fatalf("read cloud.json: %v", err) + } + if !bytes.Contains(b, []byte(`"server_url":"https://new.example.com"`)) { + t.Fatalf("server_url not updated in %s", string(b)) + } + if !bytes.Contains(b, []byte(`"token":"file-token"`)) { + t.Fatalf("token was not preserved in %s", string(b)) + } +} + +func TestSaveTUIServerURLOnEmptyDirWritesOnlyURL(t *testing.T) { + dir := t.TempDir() + + if err := saveTUIServerURL(dir, "https://new.example.com"); err != nil { + t.Fatalf("saveTUIServerURL: %v", err) + } + + b, err := os.ReadFile(filepath.Join(dir, "cloud.json")) + if err != nil { + t.Fatalf("read cloud.json: %v", err) + } + if !bytes.Contains(b, []byte(`"server_url":"https://new.example.com"`)) { + t.Fatalf("server_url not written in %s", string(b)) + } +} + +func TestSaveTUIServerURLPropagatesMalformedError(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{not valid json`) + + err := saveTUIServerURL(dir, "https://new.example.com") + if err == nil { + t.Fatal("expected error from malformed file") + } +} + +func httpHandlerWithStatus(code int) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(code) + }) +} + +type fakePingTransport struct { + statusCode int + err error +} + +func (f *fakePingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + if f.err != nil { + return nil, f.err + } + return &http.Response{ + StatusCode: f.statusCode, + Body: io.NopCloser(bytes.NewReader(nil)), + Request: req, + Header: make(http.Header), + }, nil +} + +func TestPingCloudServerReachable(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{statusCode: http.StatusOK} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com", "token")().(cloudPingMsg) + if msg.status != "reachable" { + t.Fatalf("status = %q, want reachable", msg.status) + } + if msg.err != nil { + t.Fatalf("unexpected err: %v", msg.err) + } +} + +func TestPingCloudServerUnauthorized(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{statusCode: http.StatusUnauthorized} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com", "token")().(cloudPingMsg) + if msg.status != "unauthorized" { + t.Fatalf("status = %q, want unauthorized", msg.status) + } +} + +func TestPingCloudServerUnreachable(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{err: errors.New("connection refused")} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com", "token")().(cloudPingMsg) + if msg.status != "unreachable" { + t.Fatalf("status = %q, want unreachable", msg.status) + } + if msg.err == nil { + t.Fatal("expected error for unreachable server") + } +} + +func TestPingCloudServer5xxIsUnreachable(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{statusCode: http.StatusServiceUnavailable} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com", "token")().(cloudPingMsg) + if msg.status != "unreachable" { + t.Fatalf("status = %q, want unreachable", msg.status) + } +} + +func TestPingCloudServerMalformedURL(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("not a url", "token")().(cloudPingMsg) + if msg.status != "unreachable" { + t.Fatalf("status = %q, want unreachable", msg.status) + } +} + +// tuiCloudConfigForUI tests — the helper that reads cloudconfig + applies +// the ENGRAM_CLOUD_SERVER override (T-608.16). +// +// The bug being fixed: the old effectiveCloudToken / loadCloudConfigCmd in +// the TUI did not honor ENGRAM_CLOUD_SERVER, so the TUI's Cloud Config +// form could display a stale server URL after the user set the env var. +// These tests pin the precedence: env > file > zero-value, with +// whitespace-only env treated as unset. + +func TestTuiCloudConfigForUIAppliesEnvServerOverride(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com","token":"file-token"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env.example.com") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "https://env.example.com" { + t.Fatalf("env should override file, got ServerURL=%q", cfg.ServerURL) + } + if cfg.Token != "file-token" { + t.Fatalf("token field from file should be preserved, got Token=%q", cfg.Token) + } +} + +func TestTuiCloudConfigForUIFallsBackToFileWhenEnvUnset(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com","token":"file-token"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "https://file.example.com" { + t.Fatalf("file should win when env empty, got ServerURL=%q", cfg.ServerURL) + } +} + +func TestTuiCloudConfigForUIWhitespaceEnvTreatedAsUnset(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com","token":"file-token"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", " \t ") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "https://file.example.com" { + t.Fatalf("whitespace-only env should be unset; file should win, got ServerURL=%q", cfg.ServerURL) + } +} + +func TestTuiCloudConfigForUINoFileNoEnvReturnsZeroValue(t *testing.T) { + dir := t.TempDir() + t.Setenv("ENGRAM_CLOUD_SERVER", "") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "" || cfg.Token != "" { + t.Fatalf("expected zero-value, got %+v", cfg) + } +} + +func TestTuiCloudConfigForUIMalformedFileReturnsZeroValue(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{not valid json`) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "" || cfg.Token != "" { + t.Fatalf("malformed file should yield zero-value, got %+v", cfg) + } +} + +func TestTuiCloudConfigForUIEnvOnlyReturnsConfigWithURL(t *testing.T) { + dir := t.TempDir() + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env.example.com") + + cfg := tuiCloudConfigForUI(dir) + if cfg.ServerURL != "https://env.example.com" { + t.Fatalf("env should populate URL when no file present, got ServerURL=%q", cfg.ServerURL) + } +} + +func TestLoadCloudConfigCmdAppliesEnvServerOverride(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com","token":"file-token"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", "https://env.example.com") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + msg := loadCloudConfigCmd(dir)() + loaded, ok := msg.(cloudConfigLoadedMsg) + if !ok { + t.Fatalf("message type = %T", msg) + } + if loaded.err != nil { + t.Fatalf("unexpected error: %v", loaded.err) + } + if loaded.serverURL != "https://env.example.com" { + t.Fatalf("serverURL = %q, want env override", loaded.serverURL) + } + if loaded.tokenSource != cloudconfig.LabelSourceFile { + t.Fatalf("tokenSource = %q, want %q (cloudconfig label)", loaded.tokenSource, cloudconfig.LabelSourceFile) + } +} + +func TestLoadCloudConfigCmdReportsEnvTokenSource(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com","token":"file-token"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "env-token") + + msg := loadCloudConfigCmd(dir)() + loaded, ok := msg.(cloudConfigLoadedMsg) + if !ok { + t.Fatalf("message type = %T", msg) + } + if loaded.err != nil { + t.Fatalf("unexpected error: %v", loaded.err) + } + if loaded.tokenSource != cloudconfig.LabelSourceEnv { + t.Fatalf("tokenSource = %q, want %q (cloudconfig label)", loaded.tokenSource, cloudconfig.LabelSourceEnv) + } +} + +func TestLoadCloudConfigCmdReportsNoneSourceWhenNoToken(t *testing.T) { + dir := t.TempDir() + writeCloudJSON(t, dir, `{"server_url":"https://file.example.com"}`) + t.Setenv("ENGRAM_CLOUD_SERVER", "") + t.Setenv("ENGRAM_CLOUD_TOKEN", "") + + msg := loadCloudConfigCmd(dir)() + loaded, ok := msg.(cloudConfigLoadedMsg) + if !ok { + t.Fatalf("message type = %T", msg) + } + if loaded.err != nil { + t.Fatalf("unexpected error: %v", loaded.err) + } + if loaded.tokenSource != cloudconfig.LabelSourceNone { + t.Fatalf("tokenSource = %q, want %q (cloudconfig label)", loaded.tokenSource, cloudconfig.LabelSourceNone) + } +} + +// ─── T-608.17 tests ──────────────────────────────────────────────────────── +// +// The TUI gains its own local-daemon probe command (probeLocalDaemonCmd) +// and message type (cloudDaemonProbeMsg) per ADR-2. The probe lives +// in the cloudconfig package; the TUI command is a thin tea.Cmd wrapper +// that delivers the result as a typed message. The probe is not yet +// wired into a screen (T-608.19 will dispatch it from the Cloud Status +// arm); the tests below exercise the command in isolation. + +func TestProbeLocalDaemonCmdReturnsTeaCmd(t *testing.T) { + if probeLocalDaemonCmd() == nil { + t.Fatal("probeLocalDaemonCmd must return a non-nil tea.Cmd") + } +} + +func TestProbeLocalDaemonCmdDeliversProbeMessage(t *testing.T) { + orig := cloudconfig.LocalDaemonProbe + defer func() { cloudconfig.LocalDaemonProbe = orig }() + cloudconfig.LocalDaemonProbe = func(ctx context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{Status: cloudconfig.ProbeRunning, Port: port} + } + + msg := probeLocalDaemonCmd()() + probe, ok := msg.(cloudDaemonProbeMsg) + if !ok { + t.Fatalf("message type = %T, want cloudDaemonProbeMsg", msg) + } + if probe.result.Status != cloudconfig.ProbeRunning { + t.Fatalf("status = %v, want ProbeRunning", probe.result.Status) + } + if probe.result.Port != cloudconfig.ResolvePort() { + t.Fatalf("port = %d, want %d", probe.result.Port, cloudconfig.ResolvePort()) + } + if probe.result.Err != nil { + t.Fatalf("unexpected err: %v", probe.result.Err) + } +} + +func TestProbeLocalDaemonCmdPropagatesNotRunning(t *testing.T) { + orig := cloudconfig.LocalDaemonProbe + defer func() { cloudconfig.LocalDaemonProbe = orig }() + cloudconfig.LocalDaemonProbe = func(ctx context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{ + Status: cloudconfig.ProbeNotRunning, + Port: port, + Err: errors.New("dial tcp 127.0.0.1:7437: connect: connection refused"), + } + } + + msg := probeLocalDaemonCmd()() + probe, ok := msg.(cloudDaemonProbeMsg) + if !ok { + t.Fatalf("message type = %T, want cloudDaemonProbeMsg", msg) + } + if probe.result.Status != cloudconfig.ProbeNotRunning { + t.Fatalf("status = %v, want ProbeNotRunning", probe.result.Status) + } + if probe.result.Err == nil { + t.Fatal("expected Err to be set on NotRunning") + } +} + +func TestProbeLocalDaemonCmdPropagatesUnreachable(t *testing.T) { + orig := cloudconfig.LocalDaemonProbe + defer func() { cloudconfig.LocalDaemonProbe = orig }() + cloudconfig.LocalDaemonProbe = func(ctx context.Context, port int) cloudconfig.Result { + return cloudconfig.Result{ + Status: cloudconfig.ProbeUnreachable, + Port: port, + Err: errors.New("context deadline exceeded"), + } + } + + msg := probeLocalDaemonCmd()() + probe, ok := msg.(cloudDaemonProbeMsg) + if !ok { + t.Fatalf("message type = %T, want cloudDaemonProbeMsg", msg) + } + if probe.result.Status != cloudconfig.ProbeUnreachable { + t.Fatalf("status = %v, want ProbeUnreachable", probe.result.Status) + } + if probe.result.Err == nil { + t.Fatal("expected Err to be set on Unreachable") + } +} + +// pingCloudServer (cloud /health) now uses cloudconfig.ValidateServerURL +// directly (T-608.17). The behavior change per REQ-1: URLs with ?q=1 +// or #frag are accepted and the query/fragment is cleared. The +// legacy TUI validateCloudServerURL REJECTED them; the new tests +// below pin the accept-and-clear contract. + +func TestPingCloudServerAcceptsAndClearsQueryInURL(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{statusCode: http.StatusOK} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com?debug=1", "token")().(cloudPingMsg) + if msg.status != "reachable" { + t.Fatalf("status = %q, want reachable; err = %v", msg.status, msg.err) + } + if msg.err != nil { + t.Fatalf("unexpected err: %v", msg.err) + } +} + +func TestPingCloudServerAcceptsAndClearsFragmentInURL(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{statusCode: http.StatusOK} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://cloud.example.com#section", "token")().(cloudPingMsg) + if msg.status != "reachable" { + t.Fatalf("status = %q, want reachable; err = %v", msg.status, msg.err) + } + if msg.err != nil { + t.Fatalf("unexpected err: %v", msg.err) + } +} + +func TestPingCloudServerStillRejectsBadScheme(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("ftp://cloud.example.com", "token")().(cloudPingMsg) + if msg.status != "unreachable" { + t.Fatalf("status = %q, want unreachable; err = %v", msg.status, msg.err) + } +} + +func TestPingCloudServerStillRejectsEmptyHost(t *testing.T) { + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{} + defer func() { pingCloudTransport = orig }() + + msg := pingCloudServer("https://", "token")().(cloudPingMsg) + if msg.status != "unreachable" { + t.Fatalf("status = %q, want unreachable; err = %v", msg.status, msg.err) + } +} + + diff --git a/internal/tui/model.go b/internal/tui/model.go index 41d38ed8..ec66dc0a 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -12,6 +12,7 @@ package tui import ( "errors" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/setup" "github.com/Gentleman-Programming/engram/internal/store" "github.com/Gentleman-Programming/engram/internal/version" @@ -37,6 +38,15 @@ const ( ScreenSessionDetail ScreenSetup ScreenCloudSettings + ScreenCloudConfig +) + +// Cloud Config form focus positions. +const ( + cloudConfigFocusInput = iota + cloudConfigFocusTest + cloudConfigFocusSave + cloudConfigFocusCancel ) type SessionDeleteState int @@ -99,6 +109,17 @@ type setupInstallMsg struct { err error } +type cloudConfigLoadedMsg struct { + serverURL string + tokenSource string + err error +} + +type cloudPingMsg struct { + status string + err error +} + // ─── Model ─────────────────────────────────────────────────────────────────── type Model struct { @@ -159,6 +180,16 @@ type Model struct { SetupAllowlistApplied bool // true = allowlist was added successfully SetupAllowlistError string // error message if allowlist injection failed SetupSpinner spinner.Model + + // Cloud config + CloudConfigInput textinput.Model + CloudConfigServerURL string + CloudConfigTokenSource string + CloudConfigError string + CloudConfigFocus int + CloudConfigPingStatus string + CloudConfigSaving bool + CloudConfigTest bool // true when the current ping is a test, not a save } // New creates a new TUI model connected to the given store. @@ -168,16 +199,22 @@ func New(s *store.Store, version string) Model { ti.CharLimit = 256 ti.Width = 60 + ci := textinput.New() + ci.Placeholder = "https://cloud.example.com" + ci.CharLimit = 256 + ci.Width = 60 + sp := spinner.New() sp.Spinner = spinner.Dot sp.Style = lipgloss.NewStyle().Foreground(colorLavender) return Model{ - store: s, - Version: version, - Screen: ScreenDashboard, - SearchInput: ti, - SetupSpinner: sp, + store: s, + Version: version, + Screen: ScreenDashboard, + SearchInput: ti, + CloudConfigInput: ci, + SetupSpinner: sp, } } @@ -266,3 +303,23 @@ func installAgent(agentName string) tea.Cmd { var installAgentFn = setup.Install var addClaudeCodeAllowlistFn = setup.AddClaudeCodeAllowlist + +// loadCloudConfigCmd returns a tea.Cmd that reads the cloud configuration +// from disk and delivers a cloudConfigLoadedMsg containing the server URL +// and token source label. +// +// The server URL is the effective URL after applying the +// ENGRAM_CLOUD_SERVER env override (T-608.16); the token source label +// is the canonical string from cloudconfig.SourceLabel, identical to +// what the CLI's status line prints. +func loadCloudConfigCmd(dataDir string) tea.Cmd { + return func() tea.Msg { + cfg := tuiCloudConfigForUI(dataDir) + _, source := cloudconfig.EffectiveToken(dataDir) + return cloudConfigLoadedMsg{ + serverURL: cfg.ServerURL, + tokenSource: cloudconfig.SourceLabel(source), + err: nil, + } + } +} diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index bfcc868b..70c65bba 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -2,6 +2,8 @@ package tui import ( "errors" + "os" + "path/filepath" "testing" "github.com/Gentleman-Programming/engram/internal/setup" @@ -111,6 +113,51 @@ func TestScreenCloudSettingsConstant(t *testing.T) { } } +func TestScreenCloudConfigConstant(t *testing.T) { + if ScreenCloudConfig != ScreenCloudSettings+1 { + t.Fatalf("ScreenCloudConfig = %d, want %d (ScreenCloudSettings+1)", ScreenCloudConfig, ScreenCloudSettings+1) + } + + seen := map[Screen]bool{} + for _, s := range []Screen{ + ScreenDashboard, + ScreenSearch, + ScreenSearchResults, + ScreenRecent, + ScreenObservationDetail, + ScreenTimeline, + ScreenSessions, + ScreenSessionDetail, + ScreenSetup, + ScreenCloudSettings, + ScreenCloudConfig, + } { + if seen[s] { + t.Fatalf("screen constant %d is duplicated", s) + } + seen[s] = true + } +} + +func TestLoadCloudConfigCommand(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "cloud.json"), []byte(`{"server_url":"https://cloud.example.com","token":"file-token"}`), 0o644); err != nil { + t.Fatalf("write cloud.json: %v", err) + } + + msg := loadCloudConfigCmd(dir)() + loaded, ok := msg.(cloudConfigLoadedMsg) + if !ok { + t.Fatalf("message type = %T", msg) + } + if loaded.err != nil { + t.Fatalf("unexpected error: %v", loaded.err) + } + if loaded.serverURL != "https://cloud.example.com" { + t.Fatalf("serverURL = %q", loaded.serverURL) + } +} + func TestInitReturnsCommand(t *testing.T) { m := New(newTestFixture(t).store, "") if cmd := m.Init(); cmd == nil { diff --git a/internal/tui/update.go b/internal/tui/update.go index 896ba970..96ab3451 100644 --- a/internal/tui/update.go +++ b/internal/tui/update.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/Gentleman-Programming/engram/internal/cloudconfig" "github.com/Gentleman-Programming/engram/internal/setup" "github.com/Gentleman-Programming/engram/internal/store" "github.com/charmbracelet/bubbles/spinner" @@ -30,6 +31,10 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if m.Screen == ScreenSearch && m.SearchInput.Focused() { return m.handleSearchInputKeys(msg) } + // If cloud config URL input is focused, let it handle typing + if m.Screen == ScreenCloudConfig && m.CloudConfigFocus == cloudConfigFocusInput && m.CloudConfigInput.Focused() { + return m.handleCloudConfigInputKeys(msg) + } return m.handleKeyPress(msg.String()) // ─── Data loaded messages ──────────────────────────────────────────── @@ -142,6 +147,46 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.SetupDone = true return m, nil + case cloudConfigLoadedMsg: + if msg.err != nil { + m.CloudConfigError = msg.err.Error() + return m, nil + } + m.CloudConfigServerURL = msg.serverURL + m.CloudConfigTokenSource = msg.tokenSource + m.CloudConfigInput.SetValue(msg.serverURL) + m.CloudConfigInput.Focus() + return m, nil + + case cloudPingMsg: + m.CloudConfigSaving = false + m.CloudConfigPingStatus = msg.status + if msg.err != nil { + m.CloudConfigError = msg.err.Error() + return m, nil + } + if m.CloudConfigTest { + // Test-only ping: just show the status. + return m, nil + } + if msg.status == "reachable" || msg.status == "unauthorized" { + validatedURL, err := cloudconfig.ValidateServerURL(m.CloudConfigInput.Value()) + if err != nil { + m.CloudConfigError = err.Error() + return m, nil + } + if err := saveTUIServerURL(m.store.DataDir(), validatedURL); err != nil { + m.CloudConfigError = err.Error() + return m, nil + } + m.Screen = ScreenCloudSettings + m.Cursor = 0 + m.CloudConfigFocus = cloudConfigFocusInput + return m, nil + } + m.CloudConfigError = "server is unreachable" + return m, nil + case clipboardCopiedMsg: // Emit the OSC 52 sequence to stdout so the terminal copies the content, // set the feedback label, and schedule its removal after 2 seconds. @@ -195,6 +240,8 @@ func (m Model) handleKeyPress(key string) (tea.Model, tea.Cmd) { return m.handleSetupKeys(key) case ScreenCloudSettings: return m.handleCloudSettingsKeys(key) + case ScreenCloudConfig: + return m.handleCloudConfigKeys(key) } return m, nil } @@ -671,7 +718,20 @@ func (m Model) handleCloudSettingsKeys(key string) (tea.Model, tea.Cmd) { m.Cursor++ } case "enter", " ": - if m.Cursor == len(cloudSettingsMenuItems)-1 { // Back + switch m.Cursor { + case 0: // Configure server + m.PrevScreen = ScreenCloudSettings + m.Screen = ScreenCloudConfig + m.Cursor = 0 + m.CloudConfigFocus = cloudConfigFocusInput + m.CloudConfigError = "" + m.CloudConfigPingStatus = "" + m.CloudConfigSaving = false + m.CloudConfigTest = false + m.CloudConfigInput.SetValue("") + m.CloudConfigInput.Focus() + return m, loadCloudConfigCmd(m.store.DataDir()) + case len(cloudSettingsMenuItems) - 1: // Back m.Screen = ScreenDashboard m.Cursor = 0 return m, loadStats(m.store) @@ -684,6 +744,122 @@ func (m Model) handleCloudSettingsKeys(key string) (tea.Model, tea.Cmd) { return m, nil } +// ─── Cloud Config ──────────────────────────────────────────────────────────── + +func (m Model) handleCloudConfigInputKeys(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "tab": + m.CloudConfigInput.Blur() + m.CloudConfigFocus = cloudConfigFocusTest + return m, nil + case "shift+tab": + m.CloudConfigInput.Blur() + m.CloudConfigFocus = cloudConfigFocusCancel + return m, nil + case "esc": + m.CloudConfigInput.Blur() + m.Screen = ScreenCloudSettings + m.CloudConfigFocus = cloudConfigFocusInput + return m, nil + case "enter": + m.CloudConfigInput.Blur() + m.CloudConfigFocus = cloudConfigFocusSave + return m, nil + } + + var cmd tea.Cmd + m.CloudConfigInput, cmd = m.CloudConfigInput.Update(msg) + return m, cmd +} + +func (m Model) handleCloudConfigKeys(key string) (tea.Model, tea.Cmd) { + switch key { + case "esc", "q": + m.CloudConfigInput.Blur() + m.Screen = ScreenCloudSettings + m.CloudConfigFocus = cloudConfigFocusInput + return m, nil + case "tab": + m.CloudConfigFocus = nextCloudConfigFocus(m.CloudConfigFocus) + m.CloudConfigInput.Blur() + if m.CloudConfigFocus == cloudConfigFocusInput { + m.CloudConfigInput.Focus() + } + return m, nil + case "shift+tab": + m.CloudConfigFocus = prevCloudConfigFocus(m.CloudConfigFocus) + m.CloudConfigInput.Blur() + if m.CloudConfigFocus == cloudConfigFocusInput { + m.CloudConfigInput.Focus() + } + return m, nil + case "up", "k": + m.CloudConfigFocus = prevCloudConfigFocus(m.CloudConfigFocus) + m.CloudConfigInput.Blur() + if m.CloudConfigFocus == cloudConfigFocusInput { + m.CloudConfigInput.Focus() + } + return m, nil + case "down", "j": + m.CloudConfigFocus = nextCloudConfigFocus(m.CloudConfigFocus) + m.CloudConfigInput.Blur() + if m.CloudConfigFocus == cloudConfigFocusInput { + m.CloudConfigInput.Focus() + } + return m, nil + case "enter", " ": + return m.activateCloudConfigFocus() + } + return m, nil +} + +func nextCloudConfigFocus(f int) int { + if f >= cloudConfigFocusCancel { + return cloudConfigFocusInput + } + return f + 1 +} + +func prevCloudConfigFocus(f int) int { + if f <= cloudConfigFocusInput { + return cloudConfigFocusCancel + } + return f - 1 +} + +func (m Model) activateCloudConfigFocus() (tea.Model, tea.Cmd) { + switch m.CloudConfigFocus { + case cloudConfigFocusInput: + m.CloudConfigInput.Blur() + m.CloudConfigFocus = cloudConfigFocusSave + return m, nil + case cloudConfigFocusTest: + return m.runCloudConfigPing(true) + case cloudConfigFocusSave: + return m.runCloudConfigPing(false) + case cloudConfigFocusCancel: + m.CloudConfigInput.Blur() + m.Screen = ScreenCloudSettings + m.CloudConfigFocus = cloudConfigFocusInput + return m, nil + } + return m, nil +} + +func (m Model) runCloudConfigPing(testOnly bool) (tea.Model, tea.Cmd) { + m.CloudConfigError = "" + m.CloudConfigPingStatus = "" + validatedURL, err := cloudconfig.ValidateServerURL(m.CloudConfigInput.Value()) + if err != nil { + m.CloudConfigError = err.Error() + return m, nil + } + m.CloudConfigTest = testOnly + m.CloudConfigSaving = !testOnly + token, _ := cloudconfig.EffectiveToken(m.store.DataDir()) + return m, pingCloudServer(validatedURL, token) +} + // ─── Helpers ───────────────────────────────────────────────────────────────── func (m Model) resetSessionDeleteState() Model { diff --git a/internal/tui/update_test.go b/internal/tui/update_test.go index fe5b4ac4..5b33128a 100644 --- a/internal/tui/update_test.go +++ b/internal/tui/update_test.go @@ -1,7 +1,11 @@ package tui import ( + "bytes" "errors" + "net/http/httptest" + "os" + "path/filepath" "strings" "testing" @@ -321,6 +325,238 @@ func TestCloudSettingsMenuNavigation(t *testing.T) { } } +func TestCloudConfigNavigation(t *testing.T) { + fx := newTestFixture(t) + m := New(fx.store, "") + m.Screen = ScreenCloudSettings + m.Cursor = 0 // Configure server + + updatedModel, cmd := m.handleCloudSettingsKeys("enter") + updated := updatedModel.(Model) + if updated.Screen != ScreenCloudConfig { + t.Fatalf("enter on Configure server should open ScreenCloudConfig, got %v", updated.Screen) + } + if updated.CloudConfigFocus != cloudConfigFocusInput { + t.Fatalf("focus = %d, want input", updated.CloudConfigFocus) + } + if !updated.CloudConfigInput.Focused() { + t.Fatal("input should be focused on entry") + } + if cmd == nil { + t.Fatal("enter on Configure server should load cloud config") + } + + updatedModel, _ = updated.handleCloudConfigKeys("esc") + updated = updatedModel.(Model) + if updated.Screen != ScreenCloudSettings { + t.Fatalf("esc from cloud config should return to settings, got %v", updated.Screen) + } +} + +func TestCloudConfigTabCyclesFocus(t *testing.T) { + m := New(nil, "") + m.Screen = ScreenCloudConfig + m.CloudConfigFocus = cloudConfigFocusInput + m.CloudConfigInput.Focus() + + updatedModel, _ := m.handleCloudConfigKeys("tab") + updated := updatedModel.(Model) + if updated.CloudConfigFocus != cloudConfigFocusTest { + t.Fatalf("tab should move focus to test, got %d", updated.CloudConfigFocus) + } + if updated.CloudConfigInput.Focused() { + t.Fatal("input should blur when focus leaves") + } + + updatedModel, _ = updated.handleCloudConfigKeys("tab") + updated = updatedModel.(Model) + if updated.CloudConfigFocus != cloudConfigFocusSave { + t.Fatalf("tab should move focus to save, got %d", updated.CloudConfigFocus) + } + + updatedModel, _ = updated.handleCloudConfigKeys("tab") + updated = updatedModel.(Model) + if updated.CloudConfigFocus != cloudConfigFocusCancel { + t.Fatalf("tab should move focus to cancel, got %d", updated.CloudConfigFocus) + } + + updatedModel, _ = updated.handleCloudConfigKeys("tab") + updated = updatedModel.(Model) + if updated.CloudConfigFocus != cloudConfigFocusInput { + t.Fatalf("tab should wrap to input, got %d", updated.CloudConfigFocus) + } + if !updated.CloudConfigInput.Focused() { + t.Fatal("input should focus when wrapping back") + } + + updatedModel, _ = updated.handleCloudConfigKeys("shift+tab") + updated = updatedModel.(Model) + if updated.CloudConfigFocus != cloudConfigFocusCancel { + t.Fatalf("shift+tab should wrap backwards, got %d", updated.CloudConfigFocus) + } +} + +func TestCloudConfigSaveValidURLPersists(t *testing.T) { + fx := newTestFixture(t) + server := httptest.NewServer(httpHandlerWithStatus(200)) + defer server.Close() + + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue(server.URL) + m.CloudConfigFocus = cloudConfigFocusSave + + updatedModel, cmd := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + if !updated.CloudConfigSaving { + t.Fatal("save should set saving state") + } + + ping := cmd() + updatedModel, _ = updated.Update(ping) + updated = updatedModel.(Model) + if updated.Screen != ScreenCloudSettings { + t.Fatalf("reachable save should return to settings, got %v", updated.Screen) + } + + b, err := os.ReadFile(filepath.Join(fx.store.DataDir(), "cloud.json")) + if err != nil { + t.Fatalf("read cloud.json: %v", err) + } + if !bytes.Contains(b, []byte(server.URL)) { + t.Fatalf("cloud.json should contain saved URL, got %s", string(b)) + } +} + +func TestCloudConfigSaveInvalidURLNoPersist(t *testing.T) { + fx := newTestFixture(t) + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue("not a url") + m.CloudConfigFocus = cloudConfigFocusSave + + updatedModel, cmd := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + if updated.CloudConfigError == "" { + t.Fatal("malformed URL should set inline error") + } + if cmd != nil { + t.Fatal("malformed URL should not fire ping") + } + + if _, err := os.Stat(filepath.Join(fx.store.DataDir(), "cloud.json")); err == nil { + t.Fatal("cloud.json should not be created for invalid URL") + } +} + +func TestCloudConfigSaveUnreachableNoPersist(t *testing.T) { + fx := newTestFixture(t) + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue("https://localhost:1") + m.CloudConfigFocus = cloudConfigFocusSave + + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{err: errors.New("connection refused")} + defer func() { pingCloudTransport = orig }() + + updatedModel, cmd := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + if !updated.CloudConfigSaving { + t.Fatal("save should set saving state") + } + + updatedModel, _ = updated.Update(cmd()) + updated = updatedModel.(Model) + if updated.Screen != ScreenCloudConfig { + t.Fatalf("unreachable should stay on form, got %v", updated.Screen) + } + if updated.CloudConfigError == "" { + t.Fatal("unreachable should set error") + } + + if _, err := os.Stat(filepath.Join(fx.store.DataDir(), "cloud.json")); err == nil { + t.Fatal("cloud.json should not be created for unreachable server") + } +} + +func TestCloudConfigSaveUnauthorizedPersists(t *testing.T) { + fx := newTestFixture(t) + server := httptest.NewServer(httpHandlerWithStatus(401)) + defer server.Close() + + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue(server.URL) + m.CloudConfigFocus = cloudConfigFocusSave + + updatedModel, cmd := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + ping := cmd() + updatedModel, _ = updated.Update(ping) + updated = updatedModel.(Model) + if updated.Screen != ScreenCloudSettings { + t.Fatalf("unauthorized save should still return to settings, got %v", updated.Screen) + } + + b, err := os.ReadFile(filepath.Join(fx.store.DataDir(), "cloud.json")) + if err != nil { + t.Fatalf("read cloud.json: %v", err) + } + if !bytes.Contains(b, []byte(server.URL)) { + t.Fatalf("cloud.json should contain saved URL after 401, got %s", string(b)) + } +} + +func TestCloudConfigSavePreservesToken(t *testing.T) { + fx := newTestFixture(t) + server := httptest.NewServer(httpHandlerWithStatus(200)) + defer server.Close() + + if err := os.WriteFile(filepath.Join(fx.store.DataDir(), "cloud.json"), []byte(`{"server_url":"https://old.example.com","token":"existing-token"}`), 0o644); err != nil { + t.Fatalf("seed cloud.json: %v", err) + } + + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue(server.URL) + m.CloudConfigFocus = cloudConfigFocusSave + + updatedModel, cmd := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + updatedModel, _ = updated.Update(cmd()) + updated = updatedModel.(Model) + if updated.Screen != ScreenCloudSettings { + t.Fatalf("save should return to settings, got %v", updated.Screen) + } + + b, err := os.ReadFile(filepath.Join(fx.store.DataDir(), "cloud.json")) + if err != nil { + t.Fatalf("read cloud.json: %v", err) + } + if !bytes.Contains(b, []byte(`"token":"existing-token"`)) { + t.Fatalf("existing token must be preserved, got %s", string(b)) + } +} + +func TestCloudConfigSaveShowsSpinnerDuringPing(t *testing.T) { + fx := newTestFixture(t) + m := New(fx.store, "") + m.Screen = ScreenCloudConfig + m.CloudConfigInput.SetValue("https://cloud.example.com") + m.CloudConfigFocus = cloudConfigFocusSave + + orig := pingCloudTransport + pingCloudTransport = &fakePingTransport{err: errors.New("connection refused")} + defer func() { pingCloudTransport = orig }() + + updatedModel, _ := m.handleCloudConfigKeys("enter") + updated := updatedModel.(Model) + if !updated.CloudConfigSaving { + t.Fatal("CloudConfigSaving should be true during async ping") + } +} + func TestHandleRecentTimelineSessionsAndDetailKeyPaths(t *testing.T) { fx := newTestFixture(t) m := New(fx.store, "") @@ -728,6 +964,7 @@ func TestHandleKeyPressRouterAndClearsError(t *testing.T) { ScreenSessionDetail, ScreenSetup, ScreenCloudSettings, + ScreenCloudConfig, } { m.Screen = screen m.ErrorMsg = "old error" diff --git a/internal/tui/view.go b/internal/tui/view.go index 7b0c7fed..6a28d112 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -82,6 +82,8 @@ func (m Model) View() string { content = m.viewSetup() case ScreenCloudSettings: content = m.viewCloudSettings() + case ScreenCloudConfig: + content = m.viewCloudConfig() default: content = "Unknown screen" } @@ -180,6 +182,59 @@ func (m Model) viewCloudSettings() string { return b.String() } +func (m Model) viewCloudConfig() string { + var b strings.Builder + + b.WriteString(headerStyle.Render(" Configure cloud server")) + b.WriteString("\n\n") + + b.WriteString(detailLabelStyle.Render("Server URL:")) + b.WriteString(" ") + if m.CloudConfigFocus == cloudConfigFocusInput { + b.WriteString(searchInputStyle.Render(m.CloudConfigInput.View())) + } else { + b.WriteString(detailValueStyle.Render(m.CloudConfigInput.View())) + } + b.WriteString("\n\n") + + b.WriteString(detailLabelStyle.Render("Token:")) + b.WriteString(" ") + b.WriteString(detailValueStyle.Render(m.CloudConfigTokenSource)) + b.WriteString("\n") + if m.CloudConfigTokenSource != TokenSourceEnv { + b.WriteString(timestampStyle.Render(" Set ENGRAM_CLOUD_TOKEN to override cloud.json.token")) + b.WriteString("\n") + } + b.WriteString("\n") + + buttons := []string{"[Test]", "[Save]", "[Cancel]"} + for i, label := range buttons { + focus := i + 1 // input is 0 + if focus == m.CloudConfigFocus { + b.WriteString(menuSelectedStyle.Render("▸ " + label)) + } else { + b.WriteString(menuItemStyle.Render(" " + label)) + } + b.WriteString(" ") + } + b.WriteString("\n") + + if m.CloudConfigSaving { + b.WriteString("\n") + b.WriteString(m.SetupSpinner.View()) + b.WriteString(" Pinging server...") + b.WriteString("\n") + } else if m.CloudConfigPingStatus != "" { + b.WriteString("\n") + b.WriteString(detailValueStyle.Render("Status: " + m.CloudConfigPingStatus)) + b.WriteString("\n") + } + + b.WriteString(helpStyle.Render("\n tab/shift+tab cycle • enter select • esc/q back")) + + return b.String() +} + // renderMenu renders a vertical list of selectable menu items with a cursor. func renderMenu(items []string, cursor int) string { var b strings.Builder diff --git a/internal/tui/view_test.go b/internal/tui/view_test.go index 614411d0..a137bbb8 100644 --- a/internal/tui/view_test.go +++ b/internal/tui/view_test.go @@ -75,6 +75,39 @@ func TestRenderObservationListItem(t *testing.T) { } } +func TestViewCloudConfigRendersTokenSource(t *testing.T) { + m := New(nil, "") + m.Screen = ScreenCloudConfig + m.CloudConfigServerURL = "https://cloud.example.com" + m.CloudConfigTokenSource = TokenSourceNone + + out := m.View() + if !strings.Contains(out, "Configure cloud server") { + t.Fatal("view should render screen title") + } + if !strings.Contains(out, "Server URL") { + t.Fatal("view should render server URL label") + } + if !strings.Contains(out, "Token:") { + t.Fatal("view should render token label") + } + if !strings.Contains(out, TokenSourceNone) { + t.Fatalf("view should render token source %q", TokenSourceNone) + } + + m.CloudConfigTokenSource = TokenSourceEnv + out = m.View() + if !strings.Contains(out, TokenSourceEnv) { + t.Fatalf("view should render env token source %q", TokenSourceEnv) + } + + m.CloudConfigTokenSource = TokenSourceFile + out = m.View() + if !strings.Contains(out, TokenSourceFile) { + t.Fatalf("view should render file token source %q", TokenSourceFile) + } +} + func TestViewRouterAndErrorRendering(t *testing.T) { m := New(nil, "") m.Screen = Screen(999) @@ -385,6 +418,7 @@ func TestViewRouterCoversAllScreens(t *testing.T) { {screen: ScreenSessionDetail, want: "Session:"}, {screen: ScreenSetup, want: "Setup"}, {screen: ScreenCloudSettings, want: "Cloud sync settings"}, + {screen: ScreenCloudConfig, want: "Configure cloud server"}, } for _, tt := range tests {