Keyring adjustments

cmd/sherlock/main.go

@@ -57,19 +57,19 @@ func main() {
 		return
 	}
 
-	if err := keyring.Probe(); err != nil {
+	store, err := keyring.Open()
+	if err != nil {
 		fmt.Fprintln(os.Stderr, "sherlock:", err)
-		fmt.Fprintln(os.Stderr, "Hint:", keyring.RemediationHint())
 		os.Exit(exitKeyring)
 	}
 
 	switch sub {
 	case "login":
-		runLogin(rest)
+		runLogin(store, rest)
 	case "logout":
-		runLogout(rest)
+		runLogout(store, rest)
 	case "status":
-		runStatus(rest)
+		runStatus(store, rest)
 	case "run":
 		if len(rest) == 0 {
 			fmt.Fprintln(os.Stderr, "sherlock: run requires an agent name")
@@ -122,7 +122,7 @@ func loadAuthnConfig() authn.Config {
 	}
 }
 
-func runLogin(args []string) {
+func runLogin(store keyring.Store, args []string) {
 	fs := flag.NewFlagSet("login", flag.ExitOnError)
 	timeout := fs.Duration("timeout", 5*time.Minute, "how long to wait for the browser callback")
 	_ = fs.Parse(args)
@@ -154,23 +154,22 @@ func runLogin(args []string) {
 		fmt.Fprintln(os.Stderr, "sherlock: login:", err)
 		os.Exit(exitAuth)
 	}
-	if err := keyring.NewStore().SetAuthentikTokens(res.Tokens); err != nil {
+	if err := store.SetAuthentikTokens(res.Tokens); err != nil {
 		fmt.Fprintln(os.Stderr, "sherlock: persist tokens:", err)
 		os.Exit(exitKeyring)
 	}
 	fmt.Printf("Logged in as %s <%s>.\n", res.Tokens.Name, res.Tokens.Email)
 }
 
-func runLogout(_ []string) {
-	if err := keyring.NewStore().ClearAuthentikTokens(); err != nil {
+func runLogout(store keyring.Store, _ []string) {
+	if err := store.ClearAuthentikTokens(); err != nil {
 		fmt.Fprintln(os.Stderr, "sherlock: logout:", err)
 		os.Exit(exitKeyring)
 	}
 	fmt.Println("Logged out.")
 }
 
-func runStatus(_ []string) {
-	store := keyring.NewStore()
+func runStatus(store keyring.Store, _ []string) {
 	ts, err := store.GetAuthentikTokens()
 	switch {
 	case errors.Is(err, keyring.ErrNoTokens):

docs/agents.md

@@ -25,7 +25,7 @@ bare alias is the daily-driver form.
 
 ## What sherlock does per spawn
 
-1. `keyring.Probe()` — fail fast if the OS keyring isn't available.
+1. `keyring.Open()` — fail fast if the OS keyring isn't available (returns `*UnavailableError` with a remediation `Hint` field).
 2. Resolve the agent binary on `$PATH`. Friendly error if missing.
 3. Render the per-agent MCP config to
    `$XDG_RUNTIME_DIR/sherlock/<agent>.mcp.json` (0600). In Phase 1 the

docs/storage.md

@@ -7,7 +7,7 @@ How sherlock persists secrets and runtime state.
 | Topic | Decision |
 |---|---|
 | Token storage | OS keyring via [`github.com/zalando/go-keyring`](https://github.com/zalando/go-keyring). No on-disk credential files, no `age` blobs, no plaintext. |
-| Pre-flight | `sherlock` calls `keyring.Probe()` at startup of every subcommand. A missing/locked keyring fails fast with a platform-appropriate hint and exit code `3`. |
+| Pre-flight | The only constructor is `keyring.Open()`, which probes the keyring before returning a Store. There is no probe-less escape hatch. A missing/locked keyring returns `*UnavailableError` (with a populated `Hint` field) and exits 3. |
 | Service name | `sherlock` for real tokens; `sherlock-preflight` for the probe sentinel. |
 | Account key  | The Authentik `sub` claim once logged in, `default` before. |
 | Runtime files | `$XDG_RUNTIME_DIR/sherlock.sock`, `sherlock.pid`, `sherlock.log`, `sherlock/<agent>.mcp.json`. All `0600` (files) / `0700` (dirs). |
@@ -37,19 +37,19 @@ secrets.
 
 ## Pre-flight semantics
 
-`keyring.Probe()`:
+`keyring.Open()`:
 
-1. Writes a fixed sentinel value under service `sherlock-preflight`,
-   account `probe`.
-2. Reads it back, verifies the round-trip.
-3. Deletes it.
+1. Probes the keyring: writes a fixed sentinel value under service
+   `sherlock-preflight` / account `probe`, reads it back, deletes it.
+2. On success: returns a live `Store` backed by the OS keyring.
+3. On failure: returns `*keyring.UnavailableError` whose `Cause` field
+   wraps the underlying error and whose `Hint` field carries a
+   per-OS one-line remediation. `Error()` includes both, so callers
+   normally just print the error and move on; `keyring.IsUnavailable(err)`
+   is the type-predicate for branching.
 
-If any step fails the call returns `keyring.ErrUnavailable` wrapping
-the underlying cause. `keyring.IsUnavailable(err)` is the test;
-`keyring.RemediationHint()` returns a per-OS one-liner.
-
-CLI behaviour: any failure prints the wrapped error and the hint, then
-exits with code `3`.
+CLI behaviour: any failure prints the error (which already includes
+the hint) and exits with code `3`.
 
 ## Platform notes
 

internal/keyring/keyring.go

@@ -4,10 +4,11 @@
 // Linux, Keychain on macOS, Credential Manager on Windows). No
 // plaintext or age-encrypted blobs on disk.
 //
-// The CLI MUST call Probe at startup and exit early (with
-// platform-specific guidance) if the keyring is unavailable. Without
-// the keyring, refresh tokens would be lost on every command — better
-// to fail loudly.
+// Open is the only public way to obtain a Store; it probes the OS
+// keyring and fails fast with platform-specific guidance if the
+// keyring is missing or locked. There is deliberately no escape hatch
+// that constructs a Store without probing — without the keyring,
+// sherlock would silently lose refresh tokens on every command.
 package keyring
 
 import (
@@ -21,7 +22,7 @@ import (
 )
 
 // Service is the keyring "service" namespace under which sherlock
-// stores secrets. Per-key labels live in TokenSet's helpers.
+// stores secrets. Per-key labels live as untyped constants below.
 const Service = "sherlock"
 
 const (
@@ -32,56 +33,84 @@ const (
 	keyAuthentikTokens = "authentik-tokens"
 )
 
-// ErrUnavailable is returned by Probe when the OS keyring is missing
-// or non-responsive. Treat it as fatal; the CLI surfaces an actionable
-// hint per platform.
-type ErrUnavailable struct{ Cause error }
-
-func (e *ErrUnavailable) Error() string {
-	return fmt.Sprintf("os keyring unavailable: %v", e.Cause)
+// UnavailableError is returned by Open when the OS keyring is missing
+// or non-responsive. Hint is platform-specific remediation text,
+// populated by the constructor — callers don't need to look it up
+// themselves, just read e.Hint or print e.Error() (which includes it).
+type UnavailableError struct {
+	Cause error
+	Hint  string
 }
 
-func (e *ErrUnavailable) Unwrap() error { return e.Cause }
+func (e *UnavailableError) Error() string {
+	if e.Hint == "" {
+		return fmt.Sprintf("os keyring unavailable: %v", e.Cause)
+	}
+	return fmt.Sprintf("os keyring unavailable: %v\n  hint: %s", e.Cause, e.Hint)
+}
 
-// IsUnavailable is the convenience predicate over ErrUnavailable.
+func (e *UnavailableError) Unwrap() error { return e.Cause }
+
+// IsUnavailable reports whether err wraps an *UnavailableError.
 func IsUnavailable(err error) bool {
-	var u *ErrUnavailable
+	var u *UnavailableError
 	return errors.As(err, &u)
 }
 
-// RemediationHint returns a single-line, platform-aware hint that the
-// CLI prints when Probe fails.
-func RemediationHint() string {
+// newUnavailable wraps cause with the per-OS remediation hint.
+func newUnavailable(cause error) *UnavailableError {
+	return &UnavailableError{Cause: cause, Hint: hintForGOOS()}
+}
+
+// hintForGOOS returns a single-line, platform-aware remediation hint.
+// Internal — exposed only through UnavailableError.Hint.
+func hintForGOOS() string {
 	switch runtime.GOOS {
 	case "linux":
-		return "Linux: install/start a Secret Service provider (gnome-keyring, KWallet, or keepassxc with Secret Service enabled). See https://specifications.freedesktop.org/secret-service/latest/."
+		return "install/start a Secret Service provider (gnome-keyring, KWallet, or keepassxc with Secret Service enabled). See https://specifications.freedesktop.org/secret-service/latest/."
 	case "darwin":
-		return "macOS: the system Keychain must be unlocked. Check `security list-keychains`."
+		return "the system Keychain must be unlocked. Check `security list-keychains`."
 	case "windows":
-		return "Windows: the Credential Manager service must be running."
+		return "the Credential Manager service must be running."
 	default:
-		return runtime.GOOS + ": no Secret Service implementation is wired in sherlock for this OS."
+		return "no Secret Service implementation is wired in sherlock for " + runtime.GOOS
 	}
 }
 
-// Probe asserts that the OS keyring is reachable by performing a
-// write+read+delete of a sentinel value. Returns *ErrUnavailable on
-// any failure. Safe to call repeatedly.
-func Probe() error {
+// Open is the only way to obtain a Store. It probes the OS keyring
+// (write+read+delete of a sentinel value) and returns the live Store
+// on success, or *UnavailableError on any failure.
+//
+// We deliberately do not expose a constructor that skips the probe.
+// Every production code path needs the fail-fast guarantee, and an
+// "I know what I'm doing" escape hatch would invite real bugs where
+// refresh tokens silently vanish into a dead keyring. Tests that need
+// a Store without a real keyring use the in-memory keyring/fake.Store.
+//
+// Safe to call repeatedly. Each call performs a fresh probe; the
+// returned Store carries no per-instance state.
+func Open() (Store, error) {
+	if err := probe(); err != nil {
+		return nil, err
+	}
+	return realStore{}, nil
+}
+
+func probe() error {
 	if err := gokeyring.Set(probeService, probeAccount, probeValue); err != nil {
-		return &ErrUnavailable{Cause: fmt.Errorf("set: %w", err)}
+		return newUnavailable(fmt.Errorf("set: %w", err))
 	}
 	got, err := gokeyring.Get(probeService, probeAccount)
 	if err != nil {
 		_ = gokeyring.Delete(probeService, probeAccount)
-		return &ErrUnavailable{Cause: fmt.Errorf("get: %w", err)}
+		return newUnavailable(fmt.Errorf("get: %w", err))
 	}
 	if got != probeValue {
 		_ = gokeyring.Delete(probeService, probeAccount)
-		return &ErrUnavailable{Cause: fmt.Errorf("readback mismatch: %q", got)}
+		return newUnavailable(fmt.Errorf("readback mismatch: %q", got))
 	}
 	if err := gokeyring.Delete(probeService, probeAccount); err != nil {
-		return &ErrUnavailable{Cause: fmt.Errorf("delete: %w", err)}
+		return newUnavailable(fmt.Errorf("delete: %w", err))
 	}
 	return nil
 }
@@ -95,7 +124,7 @@ type TokenSet struct {
 	AccessToken  string    `json:"access_token,omitempty"`
 	RefreshToken string    `json:"refresh_token,omitempty"`
 	IDExpiresAt  time.Time `json:"id_expires_at"`
-	RefreshExpAt time.Time `json:"refresh_expires_at,omitempty"`
+	RefreshExpAt time.Time `json:"refresh_expires_at"`
 	Issuer       string    `json:"issuer"`
 	ClientID     string    `json:"client_id,omitempty"`
 	Scopes       []string  `json:"scopes,omitempty"`
@@ -107,25 +136,22 @@ type TokenSet struct {
 // Empty reports whether the TokenSet carries no usable session.
 func (t TokenSet) Empty() bool { return t.IDToken == "" && t.RefreshToken == "" }
 
-// Store is the typed wrapper sherlock uses. Implementations are
-// expected to be safe for concurrent use.
+// Store is the typed wrapper sherlock uses. Implementations must be
+// safe for concurrent use.
 type Store interface {
 	GetAuthentikTokens() (TokenSet, error)
 	SetAuthentikTokens(TokenSet) error
 	ClearAuthentikTokens() error
 }
 
-// realStore is the production implementation backed by zalando/go-keyring.
-type realStore struct{}
-
-// NewStore returns the real keyring-backed Store. Call Probe before
-// constructing a Store in production code paths.
-func NewStore() Store { return &realStore{} }
-
 // ErrNoTokens is returned by GetAuthentikTokens when nothing is stored.
 var ErrNoTokens = errors.New("keyring: no authentik tokens stored")
 
-func (r *realStore) GetAuthentikTokens() (TokenSet, error) {
+// realStore is the production Store backed by zalando/go-keyring.
+// Unexported on purpose — callers go through Open.
+type realStore struct{}
+
+func (realStore) GetAuthentikTokens() (TokenSet, error) {
 	raw, err := gokeyring.Get(Service, keyAuthentikTokens)
 	if err != nil {
 		if errors.Is(err, gokeyring.ErrNotFound) {
@@ -140,7 +166,7 @@ func (r *realStore) GetAuthentikTokens() (TokenSet, error) {
 	return ts, nil
 }
 
-func (r *realStore) SetAuthentikTokens(ts TokenSet) error {
+func (realStore) SetAuthentikTokens(ts TokenSet) error {
 	b, err := json.Marshal(ts)
 	if err != nil {
 		return fmt.Errorf("keyring: encode tokens: %w", err)
@@ -151,7 +177,7 @@ func (r *realStore) SetAuthentikTokens(ts TokenSet) error {
 	return nil
 }
 
-func (r *realStore) ClearAuthentikTokens() error {
+func (realStore) ClearAuthentikTokens() error {
 	if err := gokeyring.Delete(Service, keyAuthentikTokens); err != nil {
 		if errors.Is(err, gokeyring.ErrNotFound) {
 			return nil

internal/keyring/keyring_test.go

@@ -3,33 +3,41 @@ package keyring
 import (
 	"errors"
 	"os"
+	"strings"
 	"testing"
 )
 
-func TestRemediationHint_NonEmpty(t *testing.T) {
-	if RemediationHint() == "" {
-		t.Fatal("empty hint")
+func TestUnavailableError_HintPopulated(t *testing.T) {
+	err := newUnavailable(errors.New("boom"))
+	if err.Hint == "" {
+		t.Fatal("Hint should be populated by constructor")
+	}
+	if !strings.Contains(err.Error(), err.Hint) {
+		t.Fatalf("Error() should include Hint:\n  %s\n  hint=%q", err.Error(), err.Hint)
 	}
 }
 
 func TestIsUnavailable(t *testing.T) {
-	var err error = &ErrUnavailable{Cause: errors.New("boom")}
-	if !IsUnavailable(err) {
-		t.Fatal("IsUnavailable false for *ErrUnavailable")
+	if !IsUnavailable(newUnavailable(errors.New("boom"))) {
+		t.Fatal("IsUnavailable false for *UnavailableError")
 	}
 	if IsUnavailable(errors.New("other")) {
 		t.Fatal("IsUnavailable true for plain error")
 	}
 }
 
-// TestProbe_HappyPath runs against the real OS keyring. It's noisy on
+// TestOpen_HappyPath runs against the real OS keyring. It's noisy on
 // machines without a session keyring (e.g. CI), so we gate it on an
 // explicit opt-in env var.
-func TestProbe_HappyPath(t *testing.T) {
+func TestOpen_HappyPath(t *testing.T) {
 	if os.Getenv("TEST_KEYRING") == "" {
-		t.Skip("set TEST_KEYRING=1 to run the live keyring probe")
+		t.Skip("set TEST_KEYRING=1 to run the live keyring open")
 	}
-	if err := Probe(); err != nil {
-		t.Fatalf("Probe failed on a machine that opted into TEST_KEYRING=1: %v", err)
+	store, err := Open()
+	if err != nil {
+		t.Fatalf("Open failed on a machine that opted into TEST_KEYRING=1: %v", err)
+	}
+	if store == nil {
+		t.Fatal("Open returned a nil Store on success")
 	}
 }

plan.md

@@ -234,7 +234,7 @@ Reuses the existing gssh server (`gssh.alexandru.macocian.me`) — sherlock does
 - **SSH backend:** reuse the existing gssh server. `gssh-mcp` is a thin JWT-authenticated HTTP+WS client; no local SSH-cert work in sherlock. See [docs/gssh-integration.md](docs/gssh-integration.md).
 - **Wrapper extensibility:** agent-agnostic. Each supported agent CLI (Copilot, Claude Code, Aider, …) is a single Go file under `internal/agent/` that registers itself with the package via `init()`. `sherlock copilot` and `sherlock claude` are sugar for `sherlock run copilot` / `sherlock run claude`. Adding a new agent is a small code change, by design — per-CLI quirks (auth subcommands, MCP config schema, env-var quirks) need a real code home. The TOML-overlay design from the Phase 0 plan was tried in Phase 1 and dropped. See [docs/agents.md](docs/agents.md).
 - **Docs convention:** `README.md` is TOC only; every topic lives as its own file under `docs/`. New concerns get new files, never appended sections.
-- **Token persistence:** OS keyring via `zalando/go-keyring`. Strict pre-flight (`keyring.Probe()`) at startup of every CLI invocation; fail fast with exit code 3 and a per-OS hint if Secret Service / Keychain / Credential Manager is unavailable. See [docs/storage.md](docs/storage.md).
+- **Token persistence:** OS keyring via `zalando/go-keyring`. The only public constructor is `keyring.Open()` — it probes the keyring (write+read+delete of a sentinel) and returns the live Store or an `*UnavailableError` whose `Hint` field carries platform-specific remediation. Fails fast with exit code 3 if Secret Service / Keychain / Credential Manager is unavailable. There is no probe-less escape hatch — the API itself enforces the fail-fast invariant. See [docs/storage.md](docs/storage.md).
 - **Loopback redirect port:** `127.0.0.1:6990` (unassigned per IANA). Bound only for the duration of `sherlock login`. The actual Authentik OAuth2 provider config is deferred — sherlock ships with empty defaults and `sherlock login` returns `not_configured` until `SHERLOCK_AUTHENTIK_ISSUER` / `SHERLOCK_AUTHENTIK_CLIENT_ID` are set. End-to-end flow is exercised by an integration test against a stub Authentik (`internal/authn/login_test.go`).
 - **No daemon (reverses Phase 1 #9 + #10):** the Phase 1 design had a `sherlock-broker` daemon with UDS RPC (`docs/rpc.md`), JSON-over-newline framing, fork+setsid lifecycle, PID-file flock, and idle-timeout watcher. It was scrapped in the post-Phase-1 refactor — see [docs/architecture.md#why-there-is-no-daemon](docs/architecture.md#why-there-is-no-daemon). Multi-process refresh races are serialised via an exclusive `flock()` on `$XDG_RUNTIME_DIR/sherlock.refresh.lock` in `internal/authn/refresh.go`. Net: -800 LoC, no `internal/{broker,socket,rpc}/` packages, no `cmd/sherlock-broker/` binary.