diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d1805c1a7..7e037db156 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,10 +136,12 @@ jobs: # Pull requests run a Windows smoke suite: the packages that actually # carry *_windows.go code, the startup/checkpoint packages with portable # filesystem contracts, plus cmd/. The full ./... sweep stays on pushes - # to main-v2; the Unix legs always run the full suite. + # to main-v2; the Unix legs always run the full suite. Keep the job-level + # budget above normal 7-9 minute runner variance; each package test binary + # remains independently bounded by Go's 3 minute timeout below. - name: test (Windows smoke) if: env.RUN_STEPS == 'true' && runner.os == 'Windows' && github.event_name == 'pull_request' - timeout-minutes: 10 + timeout-minutes: 15 env: REASONIX_RELEASE_CACHE_GUARD: "1" WINDOWS_SANDBOX_WAIT_MS: "20000" diff --git a/go.mod b/go.mod index 0bf694022f..080ff23a44 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/bmatcuk/doublestar/v4 v4.10.0 github.com/charmbracelet/colorprofile v0.4.3 github.com/charmbracelet/x/ansi v0.11.7 + github.com/godbus/dbus/v5 v5.2.2 github.com/joho/godotenv v1.5.1 github.com/kevinburke/ssh_config v1.6.0 github.com/larksuite/oapi-sdk-go/v3 v3.9.5 @@ -54,7 +55,6 @@ require ( github.com/danieljoos/wincred v1.2.3 // indirect github.com/dlclark/regexp2/v2 v2.2.1 // indirect github.com/go-ole/go-ole v1.3.0 // indirect - github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/gogo/protobuf v1.3.2 // indirect github.com/gorilla/websocket v1.5.0 // indirect github.com/kr/fs v0.1.0 // indirect diff --git a/internal/capdiag/collect.go b/internal/capdiag/collect.go index 94563409ab..ff99193396 100644 --- a/internal/capdiag/collect.go +++ b/internal/capdiag/collect.go @@ -57,19 +57,19 @@ func Collect(opts Options) Report { if cfgErr != nil { issues = append(issues, Issue{ Severity: "error", Code: "config.load_failed", Subsystem: "config", - Message: "failed to load configuration: " + sanitizeErrTextWithPaths(cfgErr.Error(), root, home), + Message: "failed to load configuration: " + sanitizeErrTextWithPaths(cfgErr.Error(), root, home, reasonixHome), Remediation: "Fix reasonix.toml / config.toml syntax, then re-run doctor capabilities", }) } - disp := func(p string) string { return displayPath(p, root, home) } + disp := func(p string) string { return displayPath(p, root, home, reasonixHome) } instr, instructionIssues := collectInstructions(root, home, disp) skillsR, skillIssues := collectSkills(root, home, reasonixHome, cfg, disp) cmdsR, cmdIssues := collectCommands(root, disp) hooksR, hookIssues := collectHooks(root, home, reasonixHome, cfg, disp) pluginsR, pluginIssues := collectPlugins(reasonixHome, disp) - mcpR, mcpIssues := collectMCP(cfg, root, home, disp) + mcpR, mcpIssues := collectMCP(cfg, root, home, reasonixHome, disp) issues = append(issues, instructionIssues...) issues = append(issues, skillIssues...) @@ -80,10 +80,10 @@ func Collect(opts Options) Report { // Runtime host merge (desktop) or live probe (CLI). if opts.Live { - liveIssues := probeLiveMCP(&mcpR, cfg, root, home, opts.LiveTimeout) + liveIssues := probeLiveMCP(&mcpR, cfg, root, home, reasonixHome, opts.LiveTimeout) issues = append(issues, liveIssues...) } else if opts.RuntimeHost != nil { - mergeRuntimeHost(&mcpR, opts.RuntimeHost, root, home, &issues) + mergeRuntimeHost(&mcpR, opts.RuntimeHost, root, home, reasonixHome, &issues) } sortIssues(issues) @@ -287,14 +287,10 @@ func collectCommands(root string, disp func(string) string) (AssetReport, []Issu func collectHooks(root, home, reasonixHome string, cfg *config.Config, disp func(string) string) (HookReport, []Issue) { var issues []Issue - // Prefer explicit home for settings when tests isolate HOME. - homeDir := home - if reasonixHome != "" && home == "" { - homeDir = filepath.Dir(reasonixHome) - } insp := hook.Inspect(hook.LoadOptions{ - ProjectRoot: root, - HomeDir: homeDir, + ProjectRoot: root, + HomeDir: home, + ReasonixHomeDir: reasonixHome, }) runtimeOptions := hook.RuntimeOptions{} if cfg != nil { @@ -325,7 +321,7 @@ func collectHooks(root, home, reasonixHome string, cfg *config.Config, disp func } for _, e := range insp.Entries { rep.Entries = append(rep.Entries, HookEntry{ - Event: string(e.Event), Match: e.Match, Command: redactCommandDisplay(e.Command, root, home), + Event: string(e.Event), Match: e.Match, Command: redactCommandDisplay(e.Command, root, home, reasonixHome), ContextFile: disp(e.ContextFile), Description: e.Description, TimeoutMS: e.Timeout, Scope: string(e.Scope), Source: disp(e.Source), Blocking: hook.IsBlocking(e.Event), }) @@ -465,7 +461,7 @@ func collectPlugins(reasonixHome string, disp func(string) string) (PluginPackag return rep, issues } -func collectMCP(cfg *config.Config, root, home string, disp func(string) string) (MCPReport, []Issue) { +func collectMCP(cfg *config.Config, root, home, reasonixHome string, disp func(string) string) (MCPReport, []Issue) { var issues []Issue rep := MCPReport{Servers: []MCPServerInfo{}} if cfg == nil { @@ -501,7 +497,7 @@ func collectMCP(cfg *config.Config, root, home string, disp func(string) string) } } if info.Transport == "stdio" { - info.Command = redactCommandDisplay(p.Command, root, home) + info.Command = redactCommandDisplay(p.Command, root, home, reasonixHome) } else { info.URLHost = urlHostOnly(p.URL) } @@ -581,7 +577,7 @@ func commandExists(cmd string) bool { return false } -func mergeRuntimeHost(rep *MCPReport, host *plugin.Host, root, home string, issues *[]Issue) { +func mergeRuntimeHost(rep *MCPReport, host *plugin.Host, root, home, reasonixHome string, issues *[]Issue) { if host == nil { return } @@ -615,13 +611,13 @@ func mergeRuntimeHost(rep *MCPReport, host *plugin.Host, root, home string, issu } } for _, f := range host.Failures() { - errText := sanitizeErrTextWithPaths(f.Error, root, home) + errText := sanitizeErrTextWithPaths(f.Error, root, home, reasonixHome) if i, ok := byName[f.Name]; ok { rep.Servers[i].RuntimeStatus = "failed" rep.Servers[i].Error = errText rep.Servers[i].StartupStage = f.Stage rep.Servers[i].StartupElapsedMS = f.Elapsed.Milliseconds() - rep.Servers[i].Stderr = sanitizeErrTextWithPaths(f.Stderr, root, home) + rep.Servers[i].Stderr = sanitizeErrTextWithPaths(f.Stderr, root, home, reasonixHome) } *issues = append(*issues, Issue{ Severity: "error", Code: "mcp.start_failed", Subsystem: "mcp", @@ -669,10 +665,10 @@ func sanitizeErr(err error) string { // sanitizeErrText redacts secrets and machine-local identity from diagnostic // strings. Prefer sanitizeErrTextWithPaths when workspace/home are known. func sanitizeErrText(s string) string { - return sanitizeErrTextWithPaths(s, "", "") + return sanitizeErrTextWithPaths(s, "", "", "") } -func sanitizeErrTextWithPaths(s, workspace, home string) string { +func sanitizeErrTextWithPaths(s, workspace, home, reasonixHome string) string { s = strings.TrimSpace(s) if s == "" { return s @@ -705,7 +701,7 @@ func sanitizeErrTextWithPaths(s, workspace, home string) string { s = redactBearer(s) // Absolute paths: rewrite with displayPath when possible. - s = redactAbsolutePaths(s, workspace, home) + s = redactAbsolutePaths(s, workspace, home, reasonixHome) // Cap length after redaction. const max = 400 @@ -757,7 +753,7 @@ func redactBearer(s string) string { } } -func redactAbsolutePaths(s, workspace, home string) string { +func redactAbsolutePaths(s, workspace, home, reasonixHome string) string { // Walk for POSIX and Windows absolute path-like tokens. var b strings.Builder i := 0 @@ -785,7 +781,7 @@ func redactAbsolutePaths(s, workspace, home string) string { token := s[start:j] // Only rewrite if it looks like a path with a directory separator beyond root. if strings.ContainsAny(token, `/\`) && len(token) > 1 { - b.WriteString(displayPath(token, workspace, home)) + b.WriteString(displayPath(token, workspace, home, reasonixHome)) } else { b.WriteString(token) } @@ -794,7 +790,7 @@ func redactAbsolutePaths(s, workspace, home string) string { return b.String() } -func redactCommandDisplay(cmd, root, home string) string { +func redactCommandDisplay(cmd, root, home, reasonixHome string) string { cmd = strings.TrimSpace(cmd) if cmd == "" { return "" @@ -804,7 +800,7 @@ func redactCommandDisplay(cmd, root, home string) string { if len(fields) == 0 { return "" } - return displayPath(fields[0], root, home) + return displayPath(fields[0], root, home, reasonixHome) } // ioDiscard avoids importing io in every call site for skill.Options.Stderr. diff --git a/internal/capdiag/collect_test.go b/internal/capdiag/collect_test.go index 866f202b57..f7f0d4e348 100644 --- a/internal/capdiag/collect_test.go +++ b/internal/capdiag/collect_test.go @@ -122,6 +122,41 @@ auto_start = false } } +func TestCollectUsesExactReasonixHomeForGlobalHooks(t *testing.T) { + root := t.TempDir() + home := t.TempDir() + reasonixHome := filepath.Join(home, "AppData", "Roaming", "reasonix") + write(t, filepath.Join(reasonixHome, "settings.json"), `{ + "hooks": { + "SessionStart": [{"command": "echo exact-home"}] + } +}`) + + report := capdiag.Collect(capdiag.Options{ + Root: root, + HomeDir: home, + ReasonixHomeDir: reasonixHome, + }) + if report.Summary.Hooks != 1 || len(report.Hooks.Entries) != 1 { + t.Fatalf("hooks = %+v, summary = %+v", report.Hooks, report.Summary) + } + for _, source := range report.Hooks.Sources { + if source.Scope == "global" { + if source.Status != "ok" || source.HookCount != 1 { + t.Fatalf("global hook source = %+v, want exact Reasonix home settings", source) + } + if source.Path != "/settings.json" && source.Path != "\\settings.json" { + // displayPath uses ToSlash + if !strings.Contains(source.Path, "") { + t.Fatalf("global path = %q, want prefix", source.Path) + } + } + return + } + } + t.Fatal("global hook source not reported") +} + func TestMissingConventionDirsNoWarning(t *testing.T) { root := t.TempDir() home := t.TempDir() diff --git a/internal/capdiag/live.go b/internal/capdiag/live.go index 6abc85ed73..88efa628c4 100644 --- a/internal/capdiag/live.go +++ b/internal/capdiag/live.go @@ -20,7 +20,7 @@ func lookPath(cmd string) (string, error) { // connection results, and always closes the Host (including stdio children). // Persistence (startup stats / schema cache) is disabled so --live stays // free of cache/state side effects under Reasonix home. -func probeLiveMCP(rep *MCPReport, cfg *config.Config, root, home string, timeout time.Duration) []Issue { +func probeLiveMCP(rep *MCPReport, cfg *config.Config, root, home, reasonixHome string, timeout time.Duration) []Issue { var issues []Issue if cfg == nil { return issues @@ -99,13 +99,13 @@ func probeLiveMCP(rep *MCPReport, cfg *config.Config, root, home string, timeout } } for _, f := range host.Failures() { - errText := sanitizeErrTextWithPaths(f.Error, root, home) + errText := sanitizeErrTextWithPaths(f.Error, root, home, reasonixHome) if i, ok := byName[f.Name]; ok { rep.Servers[i].RuntimeStatus = "failed" rep.Servers[i].Error = errText rep.Servers[i].StartupStage = f.Stage rep.Servers[i].StartupElapsedMS = f.Elapsed.Milliseconds() - rep.Servers[i].Stderr = sanitizeErrTextWithPaths(f.Stderr, root, home) + rep.Servers[i].Stderr = sanitizeErrTextWithPaths(f.Stderr, root, home, reasonixHome) } issues = append(issues, Issue{ Severity: "error", Code: "mcp.start_failed", Subsystem: "mcp", diff --git a/internal/capdiag/paths.go b/internal/capdiag/paths.go index 10aae51146..21249f35ba 100644 --- a/internal/capdiag/paths.go +++ b/internal/capdiag/paths.go @@ -9,9 +9,10 @@ import ( // displayPath rewrites absolute paths for safe reports: // - under workspace → /... -// - under home → ~/... +// - under Reasonix home → /... +// - under OS home → ~/... // - elsewhere → /basename (no full external path, no username) -func displayPath(p, workspace, home string) string { +func displayPath(p, workspace, home, reasonixHome string) string { p = strings.TrimSpace(p) if p == "" { return "" @@ -36,6 +37,18 @@ func displayPath(p, workspace, home string) string { return "/" + filepath.ToSlash(rel) } } + if rh := strings.TrimSpace(reasonixHome); rh != "" { + if abs, err := filepath.Abs(rh); err == nil { + rh = abs + } + rh = filepath.Clean(rh) + if clean == rh { + return "" + } + if rel, err := filepath.Rel(rh, clean); err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return "/" + filepath.ToSlash(rel) + } + } if home == "" { if h, err := os.UserHomeDir(); err == nil { home = h diff --git a/internal/capdiag/sanitize_test.go b/internal/capdiag/sanitize_test.go index de3eb45324..d2557151cb 100644 --- a/internal/capdiag/sanitize_test.go +++ b/internal/capdiag/sanitize_test.go @@ -10,7 +10,7 @@ func TestSanitizeErrTextRedactsSecretsAndPaths(t *testing.T) { ws := t.TempDir() in := "stdio plugin \"x\": command \"npx\" not found on PATH; PATH=\"" + home + "/bin:/usr/bin\" Bearer sk-secret-token " + ws + "/secret.env stderr: Authorization=Bearer abc.def" - out := sanitizeErrTextWithPaths(in, ws, home) + out := sanitizeErrTextWithPaths(in, ws, home, "") if strings.Contains(out, home) { t.Fatalf("home path leaked: %q", out) } diff --git a/internal/cli/chat_tui.go b/internal/cli/chat_tui.go index 85709f92cf..6589735951 100644 --- a/internal/cli/chat_tui.go +++ b/internal/cli/chat_tui.go @@ -48,6 +48,10 @@ type chatTUI struct { ctrl control.SessionAPI label string missing string // missing-key warning surfaced once in the banner, "" when ready + // diagnostics is the process-owned TUI log/watchdog started before terminal + // takeover. Nil in unit tests that construct chatTUI without chatREPL. + diagnostics *tuiDiagnostics + firstFrameLogged bool width int height int @@ -845,6 +849,17 @@ func suspendWithMouseReset() tea.Cmd { } func (m chatTUI) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + // Feed the startup/stall watchdog so freezes leave a goroutine dump and + // recover the terminal instead of a zero-byte log (#7435). + if m.diagnostics != nil { + m.diagnostics.markProgress() + } + logFirstFrame := false + if m.diagnostics != nil && !m.firstFrameLogged { + if _, ok := msg.(tea.WindowSizeMsg); ok { + logFirstFrame = true + } + } wasAtBottom := m.viewport.AtBottom() prevLines := len(m.transcript) prevWidth := m.width @@ -856,6 +871,12 @@ func (m chatTUI) Update(msg tea.Msg) (tea.Model, tea.Cmd) { next, cmd := m.update(msg) cm := next.(chatTUI) + if logFirstFrame { + cm.firstFrameLogged = true + if cm.diagnostics != nil { + cm.diagnostics.Milestone("first_frame") + } + } contentW := transcriptContentWidth(cm.width, cm.nativeScrollback) cm.viewport.SetWidth(contentW) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 933d44f4dc..c31c5e51cd 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -1049,16 +1049,18 @@ func chatREPL(args []string, version string) int { fmt.Fprintln(os.Stderr, i18n.M.ErrorPrefix, err) return 1 } + // Bubble Tea owns the terminal from the resume picker through controller + // shutdown. Start diagnostics before config/controller work so hangs leave a + // non-zero log with milestones (#7435, #7507). + diagnostics := startTUIDiagnostics(config.ReasonixHomeDir()) + defer diagnostics.Close() + diagnostics.Milestone("config_load_begin") cfg, err := config.Load() if err == nil { configureCLIThemeWithStyle(cfg.UITheme(), cfg.UIThemeStyle()) cliCursorShape = cfg.UICursorShape() } - // Bubble Tea owns the terminal from the resume picker through controller - // shutdown. Route process logs and plugin stderr to a private, bounded file - // for that whole lifetime; user-facing warnings arrive as typed TUI events. - diagnostics := startTUIDiagnostics(config.ReasonixHomeDir()) - defer diagnostics.Close() + diagnostics.Milestone("config_load_done") // Decide whether we're starting fresh or resuming. --resume opens an // interactive picker; --continue / -c jumps straight into the newest. @@ -1154,6 +1156,7 @@ func chatREPL(args []string, version string) int { Stderr: diagnostics.Writer(), OnSessionRecovered: cliSessionRecoveredHandler(leases), } + diagnostics.Milestone("controller_build_begin") ctrl, err := setupProfileWithOverrides(ctx, *model, *maxSteps, false, sink, profile, overrides) if err != nil && errors.Is(err, boot.ErrUnknownModel) && isInteractive() && config.SourcePath() == "" { // True first run whose default model can't resolve: guide setup, then retry. @@ -1169,6 +1172,7 @@ func chatREPL(args []string, version string) int { fmt.Fprintln(os.Stderr, i18n.M.ErrorPrefix, err) return 1 } + diagnostics.Milestone("controller_build_done") // Decide where this conversation's auto-save lands. A resume reuses the // file so closing/reopening keeps appending to the same history; a fresh @@ -1231,9 +1235,10 @@ func chatREPL(args []string, version string) int { } m := newChatTUI(ctrl, missing, eventCh, termW) + m.diagnostics = diagnostics m.planMode = permissions.plan m.leases = leases - if cfg, err := config.Load(); err == nil { + if cfg != nil { m.outputStyle = cfg.Agent.OutputStyle // shown as the active entry in /output-style m.statuslineCmd = cfg.Statusline.Command // custom status-line command, "" = built-in row m.showReasoning = cfg.UI.ShowReasoning // /verbose persistence: start with config default @@ -1310,7 +1315,9 @@ func chatREPL(args []string, version string) int { // Non-Termux terminals use an alt-screen transcript viewport. Termux stays // in the normal buffer so native touch scrollback and soft-keyboard focus // keep working; finalized transcript lines are emitted via tea.Println. + diagnostics.Milestone("terminal_takeover_begin") p := tea.NewProgram(m) + diagnostics.StartWatchdog(p) // SSH drop (SIGHUP) or service stop (SIGTERM): persist the conversation // before the terminal goes away, then unwind through the normal close path // so resume picks up the interrupted session (#3772). @@ -1323,6 +1330,7 @@ func chatREPL(args []string, version string) int { }() final, runErr := p.Run() signal.Stop(hangup) + diagnostics.Milestone("terminal_released") // Close the active controller plus any retired ones from /model switches. // Retired controllers were stashed rather than closed at switch time // because Controller.Close() runs SessionEnd hooks and kills plugin diff --git a/internal/cli/tui_diagnostics.go b/internal/cli/tui_diagnostics.go index c12ae59535..38491577ec 100644 --- a/internal/cli/tui_diagnostics.go +++ b/internal/cli/tui_diagnostics.go @@ -1,18 +1,25 @@ package cli import ( + "fmt" "io" "log/slog" "os" "path/filepath" + "runtime" "strings" "sync" + "sync/atomic" "time" + + tea "charm.land/bubbletea/v2" ) const ( tuiDiagnosticLogLimit = 4 << 20 tuiDiagnosticLogRetention = 7 * 24 * time.Hour + tuiWatchdogInterval = time.Second + tuiWatchdogStall = 10 * time.Second ) // tuiDiagnostics owns process-level diagnostics while an interactive terminal @@ -20,6 +27,9 @@ const ( // plugin stderr must go to a private file instead of bypassing its renderer. // Failure to create that file degrades to io.Discard: typed event.Notice values // still carry user-facing warnings through the TUI. +// +// Milestones and a 1s heartbeat keep the log non-empty during hangs so Windows +// ConPTY freezes (#7435) and stuck D-Bus startups leave recoverable evidence. type tuiDiagnostics struct { previous *slog.Logger logger *slog.Logger @@ -27,10 +37,16 @@ type tuiDiagnostics struct { file *os.File path string close sync.Once + + lastProgress atomic.Int64 // unix nano of last Update/View progress + stopWatch chan struct{} + watchOnce sync.Once + watchWG sync.WaitGroup + killed atomic.Bool } func startTUIDiagnostics(reasonixHome string) *tuiDiagnostics { - d := &tuiDiagnostics{previous: slog.Default(), writer: io.Discard} + d := &tuiDiagnostics{previous: slog.Default(), writer: io.Discard, stopWatch: make(chan struct{})} if logDir := tuiDiagnosticLogDir(reasonixHome); logDir != "" { if err := os.MkdirAll(logDir, 0o700); err == nil { pruneTUIDiagnosticLogs(logDir, time.Now()) @@ -43,9 +59,47 @@ func startTUIDiagnostics(reasonixHome string) *tuiDiagnostics { } d.logger = slog.New(slog.NewTextHandler(d.writer, &slog.HandlerOptions{Level: slog.LevelInfo})) slog.SetDefault(d.logger) + d.markProgress() + d.Milestone("diagnostics_started") return d } +// Milestone records a startup/runtime phase and flushes the log immediately so a +// subsequent hang still leaves a non-zero diagnostic file. +func (d *tuiDiagnostics) Milestone(name string) { + if d == nil { + return + } + d.markProgress() + msg := fmt.Sprintf("milestone=%s t=%s", strings.TrimSpace(name), time.Now().UTC().Format(time.RFC3339Nano)) + _, _ = fmt.Fprintln(d.Writer(), msg) + d.Sync() +} + +// Sync flushes the diagnostic file to disk when possible. +func (d *tuiDiagnostics) Sync() { + if d == nil || d.file == nil { + return + } + _ = d.file.Sync() +} + +// markProgress records that the TUI event loop is alive. +func (d *tuiDiagnostics) markProgress() { + if d == nil { + return + } + d.lastProgress.Store(time.Now().UnixNano()) +} + +// Path returns the diagnostic log path (empty when falling back to Discard). +func (d *tuiDiagnostics) Path() string { + if d == nil { + return "" + } + return d.path +} + func (d *tuiDiagnostics) Writer() io.Writer { if d == nil || d.writer == nil { return io.Discard @@ -53,17 +107,86 @@ func (d *tuiDiagnostics) Writer() io.Writer { return d.writer } +// StartWatchdog arms a 1s heartbeat. If the TUI event loop makes no progress for +// 10s, it dumps all goroutines, syncs the log, and kills the Bubble Tea program +// so the terminal is restored instead of remaining frozen (#7435). +func (d *tuiDiagnostics) StartWatchdog(p *tea.Program) { + if d == nil || p == nil { + return + } + d.watchOnce.Do(func() { + d.markProgress() + d.watchWG.Add(1) + go func() { + defer d.watchWG.Done() + d.watch(p) + }() + }) +} + +func (d *tuiDiagnostics) watch(p *tea.Program) { + ticker := time.NewTicker(tuiWatchdogInterval) + defer ticker.Stop() + for { + select { + case <-d.stopWatch: + return + case now := <-ticker.C: + last := time.Unix(0, d.lastProgress.Load()) + age := now.Sub(last) + _, _ = fmt.Fprintf(d.Writer(), "heartbeat t=%s last_progress_age=%s\n", + now.UTC().Format(time.RFC3339Nano), age.Round(time.Millisecond)) + d.Sync() + if age < tuiWatchdogStall || d.killed.Load() { + continue + } + d.killed.Store(true) + d.dumpGoroutines("watchdog_stall") + d.Sync() + // Kill restores the terminal; Quit alone can hang if Update is blocked. + p.Kill() + return + } + } +} + +func (d *tuiDiagnostics) dumpGoroutines(reason string) { + if d == nil { + return + } + buf := make([]byte, 1<<20) + for { + n := runtime.Stack(buf, true) + if n < len(buf) { + buf = buf[:n] + break + } + buf = make([]byte, len(buf)*2) + } + _, _ = fmt.Fprintf(d.Writer(), "goroutine_dump reason=%s bytes=%d\n%s\n", reason, len(buf), buf) +} + func (d *tuiDiagnostics) Close() { if d == nil { return } d.close.Do(func() { + select { + case <-d.stopWatch: + default: + close(d.stopWatch) + } + // Wait for the watchdog to fully exit before closing the log. A timed + // wait left a window where runtime.Stack / Sync / Kill could still write + // the file after Close returned. + d.watchWG.Wait() // Do not overwrite a logger deliberately installed by another owner // after the TUI started. if slog.Default() == d.logger && d.previous != nil { slog.SetDefault(d.previous) } if d.file != nil { + _ = d.file.Sync() _ = d.file.Close() } }) diff --git a/internal/cli/tui_diagnostics_test.go b/internal/cli/tui_diagnostics_test.go index 3211a40f00..45dcffa896 100644 --- a/internal/cli/tui_diagnostics_test.go +++ b/internal/cli/tui_diagnostics_test.go @@ -144,3 +144,26 @@ func TestCLIProfileBuildOptionsUseResolvedLocaleForAutoPricing(t *testing.T) { } } } + +func TestTUIDiagnosticsMilestoneFlushesNonEmptyLog(t *testing.T) { + home := t.TempDir() + d := startTUIDiagnostics(home) + t.Cleanup(d.Close) + d.Milestone("config_load_begin") + d.Milestone("controller_build_done") + if d.Path() == "" { + t.Fatal("expected diagnostic log path") + } + body, err := os.ReadFile(d.Path()) + if err != nil { + t.Fatal(err) + } + if len(body) == 0 { + t.Fatal("diagnostic log must not be empty after milestones") + } + for _, want := range []string{"diagnostics_started", "config_load_begin", "controller_build_done"} { + if !strings.Contains(string(body), want) { + t.Fatalf("log missing %q:\n%s", want, body) + } + } +} diff --git a/internal/config/credentials.go b/internal/config/credentials.go index 00e60fd513..542d3e1b6a 100644 --- a/internal/config/credentials.go +++ b/internal/config/credentials.go @@ -8,6 +8,7 @@ import ( "sort" "strings" "sync" + "time" "github.com/joho/godotenv" @@ -63,7 +64,13 @@ var credentialSourceTracker = struct { var userCredentialEditMu sync.Mutex var storedCredentialValueLookup = storedCredentialValue -var legacyKeyringCredentialValueLookup = legacyKeyringCredentialValue + +// legacyKeyringProbeLookup is the test-facing single-key hook returning the full +// four-state outcome under a caller-owned context (shared 1s migration budget). +var legacyKeyringProbeLookup = legacyKeyringProbe + +// legacyKeyringLookupTimeout is the shared budget for one legacy keyring scan. +var legacyKeyringLookupTimeout = time.Second // CredentialResolver resolves credentials repeatedly for one caller-owned view // build. It keeps expensive global credential-store lookups bounded to one per @@ -276,6 +283,34 @@ func StoreCredentialLines(lines []string) (string, error) { return storeCredentialAssignmentsLocked(assignments) } +// storeCredentialIfAbsentAndNotCleared writes key=value only when the current +// credential store still lacks a value and a cleared tombstone for key. The +// re-check and write share LockUserCredentialEdits so a concurrent settings +// save or tombstone cannot be overwritten by a stale keyring import. +// stored is false when the write was skipped because the key is already present +// or cleared; err is non-nil only on lock/IO failures. +func storeCredentialIfAbsentAndNotCleared(key, value string) (stored bool, err error) { + key = strings.TrimSpace(key) + if key == "" || !isCredentialKey(key) { + return false, nil + } + if strings.ContainsAny(value, "\r\n") { + return false, fmt.Errorf("credential value for %s contains a newline", key) + } + unlock, err := LockUserCredentialEdits() + if err != nil { + return false, err + } + defer unlock() + if credentialCurrentStoreHasKey(key) || credentialCurrentStoreClearedKey(key) { + return false, nil + } + if _, err := storeCredentialAssignmentsLocked(map[string]string{key: value}); err != nil { + return false, err + } + return true, nil +} + func storeCredentialAssignmentsLocked(assignments map[string]string) (string, error) { if err := storeCredentialsInFile(UserCredentialsPath(), assignments); err != nil { return "", err diff --git a/internal/config/credentials_keyring_default.go b/internal/config/credentials_keyring_default.go index 1002559f96..3c3a39e012 100644 --- a/internal/config/credentials_keyring_default.go +++ b/internal/config/credentials_keyring_default.go @@ -3,16 +3,37 @@ package config import ( + "context" + "errors" "strings" "github.com/zalando/go-keyring" ) -func legacyKeyringCredentialValue(key string) (string, bool) { +// legacyKeyringProbe reads one legacy credential from the platform keyring. +// keyring.ErrNotFound (and empty values) are absent; other errors stay error. +// ctx is checked before the call so a shared batch budget can stop the scan; +// platform Get is not context-aware on these OSes. +func legacyKeyringProbe(ctx context.Context, key string) legacyKeyringOutcome { key = strings.TrimSpace(key) if key == "" { - return "", false + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + if err := ctx.Err(); err != nil { + return legacyKeyringOutcome{Status: legacyKeyringTimeout} } value, err := keyring.Get(credentialsKeyringService, key) - return value, err == nil && value != "" + if err != nil { + if errors.Is(err, keyring.ErrNotFound) { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + if ctx.Err() != nil { + return legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + return legacyKeyringOutcome{Status: legacyKeyringError} + } + if strings.TrimSpace(value) == "" { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: value} } diff --git a/internal/config/credentials_keyring_helper.go b/internal/config/credentials_keyring_helper.go new file mode 100644 index 0000000000..5e266fb3d1 --- /dev/null +++ b/internal/config/credentials_keyring_helper.go @@ -0,0 +1,75 @@ +package config + +import ( + "context" + "strings" + "time" +) + +// legacyKeyringStatus classifies one keyring probe outcome for migration. +// Only absent may write a migration-done marker. +type legacyKeyringStatus string + +const ( + legacyKeyringFound legacyKeyringStatus = "found" + legacyKeyringAbsent legacyKeyringStatus = "absent" + legacyKeyringError legacyKeyringStatus = "error" + legacyKeyringTimeout legacyKeyringStatus = "timeout" +) + +// legacyKeyringOutcome is the four-state result for one env key. +// Value is populated only for found probes inside this process and is scrubbed +// before the outcome is returned to migration callers after store-if-absent. +type legacyKeyringOutcome struct { + Status legacyKeyringStatus + Value string +} + +// lookupLegacyKeyringBatch probes keys under a single shared deadline in-process. +// There is no external helper entrypoint: secrets never leave the process via +// stdout or a caller-controlled REASONIX_HOME dump path. +func lookupLegacyKeyringBatch(keys []string, budget time.Duration) map[string]legacyKeyringOutcome { + out := make(map[string]legacyKeyringOutcome, len(keys)) + if len(keys) == 0 { + return out + } + if budget <= 0 { + budget = time.Second + } + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + for _, key := range keys { + if err := ctx.Err(); err != nil { + out[key] = legacyKeyringOutcome{Status: legacyKeyringTimeout} + continue + } + o := legacyKeyringProbeLookup(ctx, key) + switch o.Status { + case legacyKeyringFound: + if strings.TrimSpace(o.Value) == "" { + out[key] = legacyKeyringOutcome{Status: legacyKeyringAbsent} + continue + } + stored, err := storeCredentialIfAbsentAndNotCleared(key, o.Value) + o.Value = "" // scrub before returning + if err != nil { + out[key] = legacyKeyringOutcome{Status: legacyKeyringError} + continue + } + if !stored { + // Current store already has a value or tombstone; do not apply + // the legacy keyring secret. Report found so migration does not + // re-probe endlessly without writing an absent marker. + out[key] = legacyKeyringOutcome{Status: legacyKeyringFound} + continue + } + out[key] = legacyKeyringOutcome{Status: legacyKeyringFound} + case legacyKeyringAbsent, legacyKeyringError, legacyKeyringTimeout: + out[key] = legacyKeyringOutcome{Status: o.Status} + default: + out[key] = legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + } + return out +} diff --git a/internal/config/credentials_keyring_timeout_test.go b/internal/config/credentials_keyring_timeout_test.go new file mode 100644 index 0000000000..c32c3d3ad4 --- /dev/null +++ b/internal/config/credentials_keyring_timeout_test.go @@ -0,0 +1,229 @@ +package config + +import ( + "context" + "encoding/base64" + "strings" + "sync" + "testing" + "time" +) + +func TestLookupLegacyKeyringBatchInProcessFourState(t *testing.T) { + oldLookup := legacyKeyringProbeLookup + t.Cleanup(func() { legacyKeyringProbeLookup = oldLookup }) + + legacyKeyringProbeLookup = func(_ context.Context, key string) legacyKeyringOutcome { + switch key { + case "FOUND": + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "secret"} + case "EMPTY": + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: ""} + case "ERR": + return legacyKeyringOutcome{Status: legacyKeyringError} + default: + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + } + got := lookupLegacyKeyringBatch([]string{"FOUND", "EMPTY", "MISSING", "ERR"}, time.Second) + if got["FOUND"].Status != legacyKeyringFound || got["FOUND"].Value != "" { + t.Fatalf("FOUND = %+v, want found with scrubbed value", got["FOUND"]) + } + if !credentialCurrentStoreHasKey("FOUND") { + t.Fatal("FOUND secret should have been stored via store-if-absent") + } + if got["EMPTY"].Status != legacyKeyringAbsent { + t.Fatalf("EMPTY = %+v, want absent", got["EMPTY"]) + } + if got["MISSING"].Status != legacyKeyringAbsent { + t.Fatalf("MISSING = %+v, want absent", got["MISSING"]) + } + if got["ERR"].Status != legacyKeyringError { + t.Fatalf("ERR = %+v, want error", got["ERR"]) + } +} + +func TestLegacyKeyringErrorDoesNotWriteMarker(t *testing.T) { + home := t.TempDir() + t.Setenv("REASONIX_HOME", home) + t.Setenv("REASONIX_CREDENTIALS_STORE", "file") + + oldLookup := legacyKeyringProbeLookup + t.Cleanup(func() { legacyKeyringProbeLookup = oldLookup }) + + legacyKeyringProbeLookup = func(context.Context, string) legacyKeyringOutcome { + return legacyKeyringOutcome{Status: legacyKeyringError} + } + outcomes := lookupLegacyKeyringBatch([]string{"DEEPSEEK_API_KEY"}, time.Second) + if outcomes["DEEPSEEK_API_KEY"].Status != legacyKeyringError { + t.Fatalf("status = %+v", outcomes["DEEPSEEK_API_KEY"]) + } + if outcomes["DEEPSEEK_API_KEY"].Status == legacyKeyringAbsent { + _ = markLegacyKeyringMigrationDone("DEEPSEEK_API_KEY") + } + if legacyKeyringMigrationDone("DEEPSEEK_API_KEY") { + t.Fatal("error outcome must not write a migration marker") + } +} + +func TestLookupLegacyKeyringBatchSharedContextTimeout(t *testing.T) { + oldLookup := legacyKeyringProbeLookup + oldTimeout := legacyKeyringLookupTimeout + legacyKeyringLookupTimeout = 40 * time.Millisecond + t.Cleanup(func() { + legacyKeyringProbeLookup = oldLookup + legacyKeyringLookupTimeout = oldTimeout + }) + + legacyKeyringProbeLookup = func(ctx context.Context, key string) legacyKeyringOutcome { + if key == "FAST" { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + // Block until the shared budget cancels; must not leave a hung goroutine + // after the batch returns (probe returns when ctx is done). + <-ctx.Done() + return legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + + start := time.Now() + got := lookupLegacyKeyringBatch([]string{"FAST", "SLOW"}, legacyKeyringLookupTimeout) + elapsed := time.Since(start) + if got["FAST"].Status != legacyKeyringAbsent { + t.Fatalf("FAST = %+v", got["FAST"]) + } + if got["SLOW"].Status != legacyKeyringTimeout { + t.Fatalf("SLOW = %+v, want timeout", got["SLOW"]) + } + if elapsed > 250*time.Millisecond { + t.Fatalf("elapsed %v, shared budget did not bound the scan", elapsed) + } +} + +// TestLookupLegacyKeyringBatchDoesNotClobberUserWrite forces the production +// probe→store window: the batch has already decided the key needs import +// (probe returns found), then the user writes a new value before store-if-absent. +func TestLookupLegacyKeyringBatchDoesNotClobberUserWrite(t *testing.T) { + home := t.TempDir() + t.Setenv("REASONIX_HOME", home) + t.Setenv("REASONIX_CREDENTIALS_STORE", "file") + + oldLookup := legacyKeyringProbeLookup + t.Cleanup(func() { legacyKeyringProbeLookup = oldLookup }) + + probeReached := make(chan struct{}) + releaseProbe := make(chan struct{}) + var once sync.Once + legacyKeyringProbeLookup = func(ctx context.Context, key string) legacyKeyringOutcome { + if key != "DEEPSEEK_API_KEY" { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + once.Do(func() { close(probeReached) }) + select { + case <-releaseProbe: + case <-ctx.Done(): + return legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "sk-old-keyring"} + } + + done := make(chan map[string]legacyKeyringOutcome, 1) + go func() { + done <- lookupLegacyKeyringBatch([]string{"DEEPSEEK_API_KEY"}, time.Second) + }() + + select { + case <-probeReached: + case <-time.After(2 * time.Second): + t.Fatal("probe did not reach the interleaving checkpoint") + } + if _, err := SetCredential("DEEPSEEK_API_KEY", "sk-user-new"); err != nil { + t.Fatal(err) + } + close(releaseProbe) + + got := <-done + if got["DEEPSEEK_API_KEY"].Status != legacyKeyringFound { + t.Fatalf("batch status = %+v, want found (store skipped)", got["DEEPSEEK_API_KEY"]) + } + val, ok := envFileValue(UserCredentialsPath(), "DEEPSEEK_API_KEY") + if !ok || val != "sk-user-new" { + t.Fatalf("credential = (%q, %v), want sk-user-new (keyring must not clobber)", val, ok) + } +} + +// TestLookupLegacyKeyringBatchDoesNotReviveTombstone forces the same production +// probe→store window against a cleared tombstone written after the probe starts. +func TestLookupLegacyKeyringBatchDoesNotReviveTombstone(t *testing.T) { + home := t.TempDir() + t.Setenv("REASONIX_HOME", home) + t.Setenv("REASONIX_CREDENTIALS_STORE", "file") + + // Start empty: no value and no tombstone so the batch will probe. + oldLookup := legacyKeyringProbeLookup + t.Cleanup(func() { legacyKeyringProbeLookup = oldLookup }) + + probeReached := make(chan struct{}) + releaseProbe := make(chan struct{}) + var once sync.Once + legacyKeyringProbeLookup = func(ctx context.Context, key string) legacyKeyringOutcome { + if key != "DEEPSEEK_API_KEY" { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + once.Do(func() { close(probeReached) }) + select { + case <-releaseProbe: + case <-ctx.Done(): + return legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "sk-old-keyring"} + } + + done := make(chan map[string]legacyKeyringOutcome, 1) + go func() { + done <- lookupLegacyKeyringBatch([]string{"DEEPSEEK_API_KEY"}, time.Second) + }() + + select { + case <-probeReached: + case <-time.After(2 * time.Second): + t.Fatal("probe did not reach the interleaving checkpoint") + } + // Write then clear while probe is blocked: tombstone must win. + if _, err := SetCredential("DEEPSEEK_API_KEY", "sk-temp"); err != nil { + t.Fatal(err) + } + if err := RemoveCredential("DEEPSEEK_API_KEY"); err != nil { + t.Fatal(err) + } + if !credentialCurrentStoreClearedKey("DEEPSEEK_API_KEY") { + t.Fatal("expected cleared tombstone before releasing probe") + } + close(releaseProbe) + + got := <-done + if got["DEEPSEEK_API_KEY"].Status != legacyKeyringFound { + // store-if-absent skipped → still reported found (already handled) + t.Fatalf("batch status = %+v", got["DEEPSEEK_API_KEY"]) + } + if credentialCurrentStoreHasKey("DEEPSEEK_API_KEY") { + t.Fatal("keyring import revived a tombstoned credential") + } + if !credentialCurrentStoreClearedKey("DEEPSEEK_API_KEY") { + t.Fatal("tombstone missing after batch") + } +} + +func TestLegacyKeyringMarkerUsesRawURLBase64(t *testing.T) { + home := t.TempDir() + t.Setenv("REASONIX_HOME", home) + key := "A/B+C=" + path := legacyKeyringMigrationMarkerPath(key) + wantName := base64.RawURLEncoding.EncodeToString([]byte(key)) + if !strings.HasSuffix(path, wantName) { + t.Fatalf("marker path = %q, want suffix %q", path, wantName) + } + other := legacyKeyringMigrationMarkerPath("A_B_C_") + if path == other { + t.Fatalf("marker collision between %q and A_B_C_", key) + } +} diff --git a/internal/config/credentials_keyring_unix.go b/internal/config/credentials_keyring_unix.go index 4a658c964b..afe5bbf7b0 100644 --- a/internal/config/credentials_keyring_unix.go +++ b/internal/config/credentials_keyring_unix.go @@ -3,48 +3,231 @@ package config import ( + "context" + "fmt" "strings" - ss "github.com/zalando/go-keyring/secret_service" + dbus "github.com/godbus/dbus/v5" ) -func legacyKeyringCredentialValue(key string) (string, bool) { +// Secret Service constants (same service Reasonix historically used via +// zalando/go-keyring). Every D-Bus call uses the shared migration context so a +// stuck bus cannot hang CLI startup past the batch deadline. +const ( + ssServiceName = "org.freedesktop.secrets" + ssServicePath = "/org/freedesktop/secrets" + ssServiceInterface = "org.freedesktop.Secret.Service" + ssCollectionInterface = "org.freedesktop.Secret.Collection" + ssItemInterface = "org.freedesktop.Secret.Item" + ssSessionInterface = "org.freedesktop.Secret.Session" + ssCollectionsIface = "org.freedesktop.Secret.Service" + ssCollectionsProp = "Collections" + ssLoginCollection = "/org/freedesktop/secrets/collection/login" + ssLoginAlias = "/org/freedesktop/secrets/aliases/default" + ssPropertiesInterface = "org.freedesktop.DBus.Properties" +) + +type ssSecret struct { + Session dbus.ObjectPath + Parameters []byte + Value []byte + ContentType string `dbus:"content_type"` +} + +// legacyKeyringProbe reads one legacy credential from Secret Service under ctx. +// It opens a private, caller-owned session-bus connection (never dbus.SessionBus), +// bounds Dial/Auth/Hello by ctx, routes every method/property call through +// CallWithContext(ctx), and always closes the connection before returning so +// godbus worker goroutines cannot leak into goleak-checked tests. +func legacyKeyringProbe(ctx context.Context, key string) legacyKeyringOutcome { key = strings.TrimSpace(key) if key == "" { - return "", false + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + if err := ctx.Err(); err != nil { + return legacyKeyringOutcome{Status: legacyKeyringTimeout} } - svc, err := ss.NewSecretService() + conn, err := openPrivateSessionBus(ctx) if err != nil { - return "", false + return mapKeyringCtxErr(ctx, err) } - defer svc.Conn.Close() + defer func() { _ = conn.Close() }() - collection := svc.GetLoginCollection() + svc := conn.Object(ssServiceName, ssServicePath) + collectionPath, err := ssResolveLoginCollection(ctx, svc) + if err != nil { + return mapKeyringCtxErr(ctx, err) + } + if err := ssUnlock(ctx, svc, collectionPath); err != nil { + return mapKeyringCtxErr(ctx, err) + } + + collection := conn.Object(ssServiceName, collectionPath) search := map[string]string{ "username": key, "service": credentialsKeyringService, } - if err := svc.Unlock(collection.Path()); err != nil { - return "", false + var results []dbus.ObjectPath + if err := collection.CallWithContext(ctx, ssCollectionInterface+".SearchItems", 0, search).Store(&results); err != nil { + return mapKeyringCtxErr(ctx, err) } - results, err := svc.SearchItems(collection, search) - if err != nil || len(results) == 0 { - return "", false + if len(results) == 0 { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} } - session, err := svc.OpenSession() - if err != nil { - return "", false + var disregard dbus.Variant + var sessionPath dbus.ObjectPath + if err := svc.CallWithContext(ctx, ssServiceInterface+".OpenSession", 0, "plain", dbus.MakeVariant("")).Store(&disregard, &sessionPath); err != nil { + return mapKeyringCtxErr(ctx, err) } - defer svc.Close(session) + // Always close the Secret Service session with the remaining budget (never + // context.Background) so a stuck Close still respects the migration deadline. + defer func() { + session := conn.Object(ssServiceName, sessionPath) + _ = session.CallWithContext(ctx, ssSessionInterface+".Close", 0).Err + }() - if err := svc.Unlock(results[0]); err != nil { - return "", false + if err := ssUnlock(ctx, svc, results[0]); err != nil { + return mapKeyringCtxErr(ctx, err) } - secret, err := svc.GetSecret(results[0], session.Path()) - if err != nil || secret == nil || len(secret.Value) == 0 { - return "", false + + var secret ssSecret + item := conn.Object(ssServiceName, results[0]) + if err := item.CallWithContext(ctx, ssItemInterface+".GetSecret", 0, sessionPath).Store(&secret); err != nil { + return mapKeyringCtxErr(ctx, err) } - return string(secret.Value), true + if len(secret.Value) == 0 { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: string(secret.Value)} +} + +// openPrivateSessionBus dials a private session-bus connection (Auth+Hello) +// without using the process-global SessionBus cache. +// +// Connection lifecycle is bound to ctx via dbus.WithContext(ctx): when the +// migration budget expires, godbus closes the transport so Auth/Hello that have +// already obtained a *Conn unblock instead of hanging forever. We never call +// dbus-launch (NoAutoStartup): missing session address fails closed as error, +// which is correct for headless/CI and avoids an uncancellable CombinedOutput. +// +// Dial itself is still not fully context-cancellable in godbus before newConn +// installs WithContext; the outer select bounds the caller's wait, and any late +// *Conn is always closed. +func openPrivateSessionBus(ctx context.Context) (*dbus.Conn, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + type result struct { + conn *dbus.Conn + err error + } + ch := make(chan result, 1) + go func() { + conn, err := connectPrivateSessionBus(ctx) + ch <- result{conn: conn, err: err} + }() + select { + case <-ctx.Done(): + // Drain so the connect goroutine is reaped when WithContext causes + // Auth/Hello to return; always Close a late *Conn. + go func() { + r := <-ch + if r.conn != nil { + _ = r.conn.Close() + } + }() + return nil, ctx.Err() + case r := <-ch: + if r.err != nil { + if r.conn != nil { + _ = r.conn.Close() + } + if ctx.Err() != nil { + return nil, ctx.Err() + } + return nil, r.err + } + if err := ctx.Err(); err != nil { + _ = r.conn.Close() + return nil, err + } + return r.conn, nil + } +} + +// connectPrivateSessionBus opens a private, context-bound session bus and +// completes Auth+Hello. Prefer NoAutoStartup so we never block in dbus-launch. +func connectPrivateSessionBus(ctx context.Context) (*dbus.Conn, error) { + // WithContext: parent cancel → conn.Close → unblocks Auth transport I/O and + // Hello Call waiters once the *Conn exists. + conn, err := dbus.SessionBusPrivateNoAutoStartup(dbus.WithContext(ctx)) + if err != nil { + return nil, err + } + if err := conn.Auth(nil); err != nil { + _ = conn.Close() + return nil, err + } + if err := conn.Hello(); err != nil { + _ = conn.Close() + return nil, err + } + return conn, nil +} + +func ssResolveLoginCollection(ctx context.Context, svc dbus.BusObject) (dbus.ObjectPath, error) { + path := dbus.ObjectPath(ssLoginCollection) + val, err := ssGetProperty(ctx, svc, ssCollectionsIface, ssCollectionsProp) + if err != nil { + // Fall back to the default alias when Collections is unavailable. + return dbus.ObjectPath(ssLoginAlias), nil + } + paths, _ := val.Value().([]dbus.ObjectPath) + for _, p := range paths { + if p == path { + return path, nil + } + } + return dbus.ObjectPath(ssLoginAlias), nil +} + +// ssGetProperty is CallWithContext-based Properties.Get. BusObject.GetProperty +// uses a non-context Call and would escape the migration deadline. +func ssGetProperty(ctx context.Context, obj dbus.BusObject, iface, name string) (dbus.Variant, error) { + var val dbus.Variant + err := obj.CallWithContext(ctx, ssPropertiesInterface+".Get", 0, iface, name).Store(&val) + if err != nil { + return dbus.Variant{}, err + } + return val, nil +} + +func ssUnlock(ctx context.Context, svc dbus.BusObject, target dbus.ObjectPath) error { + var unlocked []dbus.ObjectPath + var prompt dbus.ObjectPath + if err := svc.CallWithContext(ctx, ssServiceInterface+".Unlock", 0, []dbus.ObjectPath{target}).Store(&unlocked, &prompt); err != nil { + return err + } + // Migration must not wait on an interactive prompt (would hang CLI startup). + if prompt != "/" && prompt != "" { + for _, p := range unlocked { + if p == target || target == dbus.ObjectPath(ssLoginAlias) { + return nil + } + } + return fmt.Errorf("secret service unlock requires interactive prompt") + } + return nil +} + +func mapKeyringCtxErr(ctx context.Context, err error) legacyKeyringOutcome { + if err == nil { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + if ctx.Err() != nil { + return legacyKeyringOutcome{Status: legacyKeyringTimeout} + } + return legacyKeyringOutcome{Status: legacyKeyringError} } diff --git a/internal/config/main_test.go b/internal/config/main_test.go index 81fc20b785..f6637c8391 100644 --- a/internal/config/main_test.go +++ b/internal/config/main_test.go @@ -1,6 +1,7 @@ package config import ( + "context" "os" "testing" @@ -15,7 +16,9 @@ func TestMain(m *testing.M) { // keyring has no environment variable in front of it, so legacy migration // would read the credentials of whoever is running the tests. Cases that // exercise the lookup substitute their own. - legacyKeyringCredentialValueLookup = func(string) (string, bool) { return "", false } + legacyKeyringProbeLookup = func(context.Context, string) legacyKeyringOutcome { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } testenv.RunWithIsolatedUserState(m) } @@ -23,8 +26,9 @@ func TestMain(m *testing.M) { // stay green either way, so the substitution itself is what gets asserted. func TestKeyringLookupStaysOutOfTheRealStore(t *testing.T) { for _, key := range []string{"DEEPSEEK_API_KEY", "OPENAI_API_KEY", "ANTHROPIC_API_KEY"} { - if value, ok := legacyKeyringCredentialValueLookup(key); ok || value != "" { - t.Fatalf("%s resolved through the real keyring in tests (ok=%v, %d bytes)", key, ok, len(value)) + o := legacyKeyringProbeLookup(context.Background(), key) + if o.Status != legacyKeyringAbsent || o.Value != "" { + t.Fatalf("%s resolved through the real keyring in tests: %+v", key, o) } } } diff --git a/internal/config/migrate.go b/internal/config/migrate.go index d39ee48ae0..eee6f146b8 100644 --- a/internal/config/migrate.go +++ b/internal/config/migrate.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "encoding/base64" "encoding/json" "errors" "fmt" @@ -370,17 +371,13 @@ func normalizedMCPMigrationRoots(roots []string) []string { func migrateLegacyCredentialsIfNeededForRoot(root string) error { missing := map[string]string{} - skip := func(key string) bool { + // File import ignores keyring markers: a marker only means "do not re-probe + // keyring for this env name", never "skip legacy credential files". + skipStore := func(key string) bool { return credentialCurrentStoreHasKey(key) || credentialCurrentStoreClearedKey(key) } - for _, key := range credentialEnvNamesForRoot(root) { - if skip(key) { - continue - } - if value, ok := legacyKeyringCredentialValueLookup(key); ok { - missing[key] = value - } - } + // Prefer legacy credential files first so a healthy file import does not + // depend on Secret Service / D-Bus (#7507). for _, src := range legacyCredentialsPaths() { if src == "" { continue @@ -391,11 +388,44 @@ func migrateLegacyCredentialsIfNeededForRoot(root string) error { } assignments := parseCredentialLines(strings.Split(string(data), "\n")) for key, value := range assignments { - if _, exists := missing[key]; !exists && !skip(key) { + if _, exists := missing[key]; !exists && !skipStore(key) { missing[key] = value } } } + keys := credentialEnvNamesForRoot(root) + needKeyring := make([]string, 0, len(keys)) + for _, key := range keys { + if skipStore(key) { + continue + } + if _, exists := missing[key]; exists { + continue + } + // Marker only filters keyring probes. + if legacyKeyringMigrationDone(key) { + continue + } + needKeyring = append(needKeyring, key) + } + if len(needKeyring) > 0 { + outcomes := lookupLegacyKeyringBatch(needKeyring, legacyKeyringLookupTimeout) + for _, key := range needKeyring { + o := outcomes[key] + switch o.Status { + case legacyKeyringFound: + // Secret was stored by the probe path (helper or in-process). + // Do not trust Value from the parent-visible outcome map. + case legacyKeyringAbsent: + // Confirmed empty probe only — never on error/timeout. + _ = markLegacyKeyringMigrationDone(key) + case legacyKeyringError, legacyKeyringTimeout: + // Leave unmarked so the next launch retries. + default: + // Unknown status: treat as timeout (no marker). + } + } + } if len(missing) == 0 { return nil } @@ -403,6 +433,38 @@ func migrateLegacyCredentialsIfNeededForRoot(root string) error { return err } +func legacyKeyringMigrationMarkerPath(key string) string { + home := ReasonixHomeDir() + key = strings.TrimSpace(key) + if strings.TrimSpace(home) == "" || key == "" { + return "" + } + // Env var names are identifiers, not secrets. RawURL base64 is collision-free + // and filesystem-safe without hashing secret material. + name := base64.RawURLEncoding.EncodeToString([]byte(key)) + return filepath.Join(home, "state", "legacy-keyring-checked", name) +} + +func legacyKeyringMigrationDone(key string) bool { + path := legacyKeyringMigrationMarkerPath(key) + if path == "" { + return false + } + _, err := os.Stat(path) + return err == nil +} + +func markLegacyKeyringMigrationDone(key string) error { + path := legacyKeyringMigrationMarkerPath(key) + if path == "" { + return nil + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + return os.WriteFile(path, []byte("v1\n"), 0o644) +} + func credentialLines(assignments map[string]string) []string { keys := make([]string, 0, len(assignments)) for key := range assignments { diff --git a/internal/config/migrate_test.go b/internal/config/migrate_test.go index 4d35ec0afe..882a07c0f8 100644 --- a/internal/config/migrate_test.go +++ b/internal/config/migrate_test.go @@ -1,6 +1,7 @@ package config import ( + "context" "os" "path/filepath" "runtime" @@ -715,14 +716,14 @@ func TestMigrateImportsLegacyCredentialsEvenWhenPrimaryConfigExists(t *testing.T func TestMigrateImportsLegacyKeyringCredentials(t *testing.T) { legacyHome(t) - old := legacyKeyringCredentialValueLookup - legacyKeyringCredentialValueLookup = func(key string) (string, bool) { + old := legacyKeyringProbeLookup + legacyKeyringProbeLookup = func(_ context.Context, key string) legacyKeyringOutcome { if key == "DEEPSEEK_API_KEY" { - return "sk-old-keyring", true + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "sk-old-keyring"} } - return "", false + return legacyKeyringOutcome{Status: legacyKeyringAbsent} } - t.Cleanup(func() { legacyKeyringCredentialValueLookup = old }) + t.Cleanup(func() { legacyKeyringProbeLookup = old }) res, err := MigrateLegacyIfNeeded() if err != nil { @@ -760,14 +761,14 @@ api_key_env = "WORKSPACE_ONLY_KEY" `), 0o644); err != nil { t.Fatal(err) } - old := legacyKeyringCredentialValueLookup - legacyKeyringCredentialValueLookup = func(key string) (string, bool) { + old := legacyKeyringProbeLookup + legacyKeyringProbeLookup = func(_ context.Context, key string) legacyKeyringOutcome { if key == "WORKSPACE_ONLY_KEY" { - return "sk-workspace", true + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "sk-workspace"} } - return "", false + return legacyKeyringOutcome{Status: legacyKeyringAbsent} } - t.Cleanup(func() { legacyKeyringCredentialValueLookup = old }) + t.Cleanup(func() { legacyKeyringProbeLookup = old }) if err := MigrateLegacyCredentialsForRoot(project); err != nil { t.Fatalf("MigrateLegacyCredentialsForRoot: %v", err) @@ -791,14 +792,14 @@ func TestMigrateLegacyCredentialsSkipsKeyringWhenIsolated(t *testing.T) { t.Setenv("REASONIX_HOME", isolated) t.Setenv("REASONIX_CREDENTIALS_STORE", "file") - old := legacyKeyringCredentialValueLookup - legacyKeyringCredentialValueLookup = func(key string) (string, bool) { + old := legacyKeyringProbeLookup + legacyKeyringProbeLookup = func(_ context.Context, key string) legacyKeyringOutcome { if key == "DEEPSEEK_API_KEY" { - return "legacy-keyring-value", true + return legacyKeyringOutcome{Status: legacyKeyringFound, Value: "legacy-keyring-value"} } - return "", false + return legacyKeyringOutcome{Status: legacyKeyringAbsent} } - t.Cleanup(func() { legacyKeyringCredentialValueLookup = old }) + t.Cleanup(func() { legacyKeyringProbeLookup = old }) if err := MigrateLegacyCredentialsForRoot("."); err != nil { t.Fatalf("MigrateLegacyCredentialsForRoot: %v", err) @@ -1064,3 +1065,67 @@ func TestMigrateSupportData(t *testing.T) { } } } + +func TestMigrateLegacyCredentialsFileImportIgnoresKeyringMarker(t *testing.T) { + _, dest, _ := legacyHome(t) + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(dest, []byte(`default_model = "deepseek-flash/deepseek-chat"`), 0o644); err != nil { + t.Fatal(err) + } + if err := markLegacyKeyringMigrationDone("DEEPSEEK_API_KEY"); err != nil { + t.Fatal(err) + } + paths := legacyCredentialsPaths() + if len(paths) == 0 { + t.Skip("no legacy credentials paths on this platform") + } + target := paths[0] + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(target, []byte("DEEPSEEK_API_KEY=sk-from-file\n"), 0o600); err != nil { + t.Fatal(err) + } + + oldLookup := legacyKeyringProbeLookup + legacyKeyringProbeLookup = func(context.Context, string) legacyKeyringOutcome { + return legacyKeyringOutcome{Status: legacyKeyringAbsent} + } + t.Cleanup(func() { + legacyKeyringProbeLookup = oldLookup + }) + + if err := MigrateLegacyCredentialsForRoot("."); err != nil { + t.Fatalf("MigrateLegacyCredentialsForRoot: %v", err) + } + data, err := os.ReadFile(UserCredentialsPath()) + if err != nil { + t.Fatalf("read migrated credentials: %v", err) + } + if !strings.Contains(string(data), "sk-from-file") { + t.Fatalf("file import blocked by keyring marker: %q", data) + } +} + +func TestMigrateLegacyCredentialsErrorDoesNotWriteKeyringMarker(t *testing.T) { + legacyHome(t) + oldLookup := legacyKeyringProbeLookup + legacyKeyringProbeLookup = func(context.Context, string) legacyKeyringOutcome { + return legacyKeyringOutcome{Status: legacyKeyringError} + } + t.Cleanup(func() { + legacyKeyringProbeLookup = oldLookup + }) + + if err := MigrateLegacyCredentialsForRoot("."); err != nil { + t.Fatalf("MigrateLegacyCredentialsForRoot: %v", err) + } + if legacyKeyringMigrationDone("DEEPSEEK_API_KEY") { + t.Fatal("keyring error must not write a migration-done marker") + } + if credentialCurrentStoreHasKey("DEEPSEEK_API_KEY") { + t.Fatal("keyring error must not store a credential") + } +} diff --git a/internal/doctor/report.go b/internal/doctor/report.go index 42f3eb5729..ea4c0966f1 100644 --- a/internal/doctor/report.go +++ b/internal/doctor/report.go @@ -6,6 +6,7 @@ import ( "io" "net/url" "os" + "os/user" "path/filepath" "runtime" "strings" @@ -148,6 +149,12 @@ func Collect(opts Options) Report { if bashConfigIgnored { warnings = append(warnings, `config requests [sandbox] bash = "enforce", but Windows does not provide an OS-level Bash sandbox; the setting is fixed to "off" and bash runs unconfined`) } + // Supervised deployments sometimes override HOME onto a service config dir + // while Reasonix isolation should use REASONIX_HOME. Do not rewrite + // subprocess HOME automatically (#7600 rejected); surface the mismatch. + if warn := homeIsolationWarning(); warn != "" { + warnings = append(warnings, warn) + } report := Report{ Version: opts.Version, OS: runtime.GOOS, @@ -359,6 +366,46 @@ func valueOr(s, fallback string) string { return s } +// homeIsolationWarning detects a process HOME that differs from the OS account +// home while REASONIX_HOME is unset. Services should keep the real account HOME +// and isolate Reasonix state with REASONIX_HOME instead of rewriting HOME. +func homeIsolationWarning() string { + if strings.TrimSpace(os.Getenv("REASONIX_HOME")) != "" { + return "" + } + envHome := strings.TrimSpace(os.Getenv("HOME")) + if envHome == "" { + // Windows services often set USERPROFILE rather than HOME. + envHome = strings.TrimSpace(os.Getenv("USERPROFILE")) + } + if envHome == "" { + return "" + } + acct, err := user.Current() + if err != nil || acct == nil || strings.TrimSpace(acct.HomeDir) == "" { + return "" + } + envClean := filepath.Clean(envHome) + acctClean := filepath.Clean(acct.HomeDir) + if samePathFold(envClean, acctClean) { + return "" + } + // Do not embed either absolute path: when HOME is overridden, redactHome + // cannot mask the account home, and shareable doctor output must stay free + // of machine-local identity. + return "process HOME differs from the OS account home; keep the real account HOME for services and isolate Reasonix with REASONIX_HOME" +} + +func samePathFold(a, b string) bool { + if a == b { + return true + } + if runtime.GOOS == "windows" { + return strings.EqualFold(a, b) + } + return false +} + // redactHome rewrites a path under the user's home directory to start with "~", // so a shared diagnostics report doesn't carry the account name. Paths outside // home are returned unchanged. diff --git a/internal/doctor/report_test.go b/internal/doctor/report_test.go index 1a260b89f7..b484f9e554 100644 --- a/internal/doctor/report_test.go +++ b/internal/doctor/report_test.go @@ -3,6 +3,7 @@ package doctor import ( "encoding/json" "os" + "os/user" "path/filepath" "strings" "testing" @@ -155,3 +156,30 @@ func TestCollectFlagsIgnoredEnforceConfig(t *testing.T) { t.Fatalf("plain off must not claim the config was ignored:\n%s", plain) } } + +func TestHomeIsolationWarningDetectsMismatch(t *testing.T) { + // Prefer a synthetic mismatch without requiring root privileges. + acct, err := user.Current() + if err != nil || acct == nil || strings.TrimSpace(acct.HomeDir) == "" { + t.Skip("account home unavailable") + } + t.Setenv("REASONIX_HOME", "") + serviceHome := filepath.Join(t.TempDir(), "service-home") + t.Setenv("HOME", serviceHome) + t.Setenv("USERPROFILE", serviceHome) + got := homeIsolationWarning() + if got == "" { + t.Fatal("expected HOME mismatch warning") + } + if !strings.Contains(got, "REASONIX_HOME") { + t.Fatalf("warning = %q, want REASONIX_HOME guidance", got) + } + // Shareable output must not embed either absolute home path. + if strings.Contains(got, serviceHome) || strings.Contains(got, acct.HomeDir) { + t.Fatalf("warning leaked a home path: %q", got) + } + t.Setenv("REASONIX_HOME", t.TempDir()) + if got := homeIsolationWarning(); got != "" { + t.Fatalf("REASONIX_HOME set should silence warning, got %q", got) + } +} diff --git a/internal/hook/hook.go b/internal/hook/hook.go index c3d8c04249..25d573c264 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -214,7 +214,14 @@ func ContextFileUsable(path string) bool { // LoadOptions configure Load. type LoadOptions struct { ProjectRoot string - HomeDir string + // HomeDir overrides the OS user home used by legacy callers and tests. The + // derived global path is /.reasonix unless ReasonixHomeDir is set. + HomeDir string + // ReasonixHomeDir is the exact current Reasonix home (settings.json lives + // directly under it). When set, it takes precedence over HomeDir for global + // settings and plugin hooks so Windows %APPDATA%/reasonix and REASONIX_HOME + // isolation stay consistent across hook/doctor/capdiag (#7411, #7331). + ReasonixHomeDir string // Trusted is retained for source compatibility. Project hooks are enabled // automatically now, so callers no longer need to set it. Trusted bool @@ -231,8 +238,12 @@ func Load(opts LoadOptions) []ResolvedHook { appendResolved(&out, s, ScopeProject, p) } } - appendPluginHooks(&out, reasonixHome(opts.HomeDir), opts.ProjectRoot) - g := GlobalSettingsPath(opts.HomeDir) + reasonixHomeDir := reasonixHomeForOptions(opts) + appendPluginHooks(&out, reasonixHomeDir, opts.ProjectRoot) + g := filepath.Join(reasonixHomeDir, SettingsFilename) + if reasonixHomeDir == "" { + g = GlobalSettingsPath(opts.HomeDir) + } if s := readSettings(g); s != nil { appendResolved(&out, s, ScopeGlobal, g) } else if !pathExists(g) { @@ -1580,6 +1591,13 @@ func reasonixHome(override string) string { return "" } +func reasonixHomeForOptions(opts LoadOptions) string { + if dir := strings.TrimSpace(opts.ReasonixHomeDir); dir != "" { + return filepath.Clean(dir) + } + return reasonixHome(opts.HomeDir) +} + func legacyGlobalSettingsPath(homeDir string) string { dir := legacyReasonixHome(homeDir) if dir == "" { diff --git a/internal/hook/hook_test.go b/internal/hook/hook_test.go index b982a20f31..e23c9f0f56 100644 --- a/internal/hook/hook_test.go +++ b/internal/hook/hook_test.go @@ -1024,6 +1024,23 @@ func TestReasonixHomeOverridesGlobalHookPaths(t *testing.T) { } } +func TestLoadOptionsReasonixHomeDirUsesExactGlobalHookPath(t *testing.T) { + home := t.TempDir() + reasonixHome := filepath.Join(home, "AppData", "Roaming", "reasonix") + settingsPath := filepath.Join(reasonixHome, SettingsFilename) + if err := os.MkdirAll(reasonixHome, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(settingsPath, []byte(`{"hooks":{"Stop":[{"command":"echo exact"}]}}`), 0o644); err != nil { + t.Fatal(err) + } + + hooks := Load(LoadOptions{HomeDir: home, ReasonixHomeDir: reasonixHome}) + if len(hooks) != 1 || hooks[0].Command != "echo exact" || hooks[0].Source != settingsPath { + t.Fatalf("Load hooks = %+v, want exact Reasonix home hook", hooks) + } +} + func TestReasonixHomeDoesNotFallBackToLegacyWhenIsolated(t *testing.T) { home := t.TempDir() reasonixHome := filepath.Join(t.TempDir(), "rx-home") diff --git a/internal/hook/inspect.go b/internal/hook/inspect.go index 7000de8d45..7aba66546b 100644 --- a/internal/hook/inspect.go +++ b/internal/hook/inspect.go @@ -67,9 +67,13 @@ func Inspect(opts LoadOptions) Inspection { } // Plugin hooks (enabled packages only — same as Load). - appendPluginInspect(&out, reasonixHome(opts.HomeDir), opts.ProjectRoot) + reasonixHomeDir := reasonixHomeForOptions(opts) + appendPluginInspect(&out, reasonixHomeDir, opts.ProjectRoot) - g := GlobalSettingsPath(opts.HomeDir) + g := filepath.Join(reasonixHomeDir, SettingsFilename) + if reasonixHomeDir == "" { + g = GlobalSettingsPath(opts.HomeDir) + } st := inspectSettingsFile(g, ScopeGlobal) if st.Status == "missing" { if legacy := legacyGlobalSettingsPath(opts.HomeDir); legacy != "" {