fix(keyring): actionable error + documented bypass when the OS keyring is unavailable

[piekstra]

Closes #384

Summary

Root cause. Since the token was moved into the OS keyring (#366 / #369), runtime token resolution opens the keyring on every API command: keyring.ResolveToken → Open() → cccredstore.Open(...) (and, when a plaintext token still sits in a config file, the one-time §1.8 migration, which is a keyring write). On the reporter's machine (KDE Plasma / Debian, Secret Service reachable) this means the keyring is touched on every invocation. When the keyring is locked, or the CLI is driven headless under an agent, the OS unlock prompt never surfaces — so the command just fails. The error that did surface was the opaque backend error plus the unhelpful "run init" hint (which itself opens the keyring), and there was no signposted way to run keyring-free. That is the regression the reporter hit even though their config file was already populated.

Re-introducing a config-file token resolver would violate the Secret-Handling Standard (§1.1/§1.2: runtime secrets never come from the plaintext config file). The standard already defines the correct keyring-free fallbacks (§1.4) — they were simply undiscoverable at the moment of failure.

Fix. Wrap genuine keyring open/read failures in ResolveToken (the shared chokepoint for both cfl and jtk) with an actionable message naming the existing, standard-sanctioned escape hatches, in resolution order:

• the per-tool / shared API-token env vars (JIRA_API_TOKEN / CFL_API_TOKEN / ATLASSIAN_API_TOKEN) — resolved before the keyring is ever opened, so the keyring is never touched (the canonical headless answer);
• the encrypted-file backend (--backend file) + its passphrase env var, and the pass backend (--backend pass).

Properties preserved:

• The underlying error is kept via %w, so errors.Is classification (credstore.ErrSecretServiceFailClosed, ErrBackendNotImplemented, …) still works.
• The §1.8 corrupt-shared-config graceful-degradation path is untouched: a corrupt shared config with a working keyring still resolves the token with no error (regression-tested).
• No new plaintext path is added — the env var and file backend are the standard's own §1.4 fallbacks, merely surfaced.

Also documents these keyring-free options in README.md so the escape hatch is discoverable before a user hits the failure.

Files

• shared/keyring/resolve.go — keyringUnavailableError helper + wrapping in ResolveToken.
• shared/keyring/resolve_test.go — new tests (both paths + regression guard).
• README.md — new "Headless and Keyring-Free Environments" subsection.

Testing

All commands run with the repo's documented flags (GOFLAGS=-tags=keyring_no1password,keyring_nopassage, per the Makefile).

• go build -o bin/cfl ./tools/cfl/cmd/cfl and … bin/jtk … — pass (both binaries build).
• go test -race ./shared/... — pass.
• go test -race -count=3 ./shared/keyring/... — pass (new tests stable under the race detector).
• go test -race ./tools/jtk/... — pass.
• golangci-lint run in shared/, tools/cfl/, tools/jtk/ — 0 issues each.
• go mod tidy across all three modules — clean (no diff).

New tests:

• TestKeyringUnavailableError_WrapsAndNamesEscapeHatches — wraps underlying error (errors.Is), names env vars + --backend file/pass + passphrase var, no-op on nil.
• TestResolveToken_KeyringOpenFailure_WrappedActionable — forces a real backend-open failure (unknown backend) → actionable wrapped error, underlying ErrBackendNotImplemented preserved.
• TestResolveToken_EnvBypassesBrokenKeyring — with ATLASSIAN_API_TOKEN set, the keyring is never opened even when the backend is broken (the config-file-present user's fix).
• TestResolveToken_CorruptSharedConfig_NotWrappedWhenKeyringWorks — regression guard for the graceful-degradation contract.

Caveat (unrelated, pre-existing)

go test -race ./tools/cfl/... intermittently fails in TestConfig_LoadFromEnv/env_vars_override_existing_values (tools/cfl/internal/config/config_test.go). Two t.Parallel() subtests mutate the same process env vars via os.Setenv (not t.Setenv) and race each other. It reproduces on main without this change and is independent of shared/keyring (different module). Left untouched as out of scope; happy to file a separate issue.

[piekstra-dev]

Automated PR Review

Reviewed commit: 2b1b61a00b56
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer	Findings
documentation:docs	0
go:implementation-tests	2
policies:conventions	1

go:implementation-tests (2 findings)

Minor - shared/keyring/resolve.go:155

keyringUnavailableError is applied to every non-nil return from resolveFromStore, but the wrapper message and its suggested remedies ("no usable keyring", env vars, --backend file/pass) are accurate only for backend-unavailability failures. If resolveFromStore can surface non-backend errors — a credential-decoding failure, a format mismatch, or a sentinel from the store layer — those errors will arrive at the caller tagged with misleading "headless/no-keyring" advice. The comment above the function says the wrapper is "applied ONLY to real backend errors", but the implementation doesn't enforce that: it blindly wraps any error resolveFromStore returns.

Fix: either document (with a package-internal contract or a type guard) that resolveFromStore exclusively returns backend errors, or introduce a narrow predicate (e.g. isBackendError(err) bool that checks against credstore.ErrSecretServiceFailClosed and similar sentinels) and only wrap when the predicate matches, propagating other errors unwrapped. The same applies to the resolveFromStore(ns) call in the ErrCorruptStore branch.

Minor - shared/keyring/resolve_test.go

The new if rerr != nil { return "", SourceNone, keyringUnavailableError(tool, rerr) } branch inside ResolveToken — the branch reached when Open succeeds but resolveFromStore subsequently fails — has no test. TestResolveToken_KeyringOpenFailure_WrappedActionable covers the Open-fails path; TestResolveToken_CorruptSharedConfig_NotWrappedWhenKeyringWorks covers the OpenNoMigrate success path. Neither exercises a successful open followed by a resolveFromStore failure in the main (non-corrupt) flow.

The practical risk is low (the error is wrapped, not silenced), but the wrapping behaviour of this new branch is unproven. Add a test that seeds a backend which opens cleanly but fails on read (or injects a fake resolveFromStore that returns an error), asserts the error is non-nil, checks errors.Is still classifies it, and verifies the escape-hatch prose appears in the message — matching the pattern already established by TestResolveToken_KeyringOpenFailure_WrappedActionable.

policies:conventions (1 finding)

Minor - shared/keyring/resolve.go

The new keyringUnavailableError function in this shared package hardcodes both tool names in the error message tail (See \cfl config show` / `jtk config show` for the active backend), while the function already accepts a tool` parameter to derive tool-specific env vars. This breaks the parameterisation pattern established in the same function: a third tool using this shared package would silently produce the wrong hint.

Suggested fix — derive the config command from tool instead of hard-coding:

fmt.Sprintf("See `%s config show` for the active backend", tool)

or, if dual-tool display is intentional, pass the command name as a second string parameter alongside tool.

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 3m 43s | ~$0.27 (est.) | claude-sonnet-4-6 | cr 0.4.161

Field	Value
Model	claude-sonnet-4-6
Reviewers	documentation:docs, go:implementation-tests, policies:conventions
Engine	claude_cli · claude-sonnet-4-6
Reviewed by	cr · piekstra-dev
Duration	3m 43s wall · 3m 34s compute
Cost	~$0.27 (est.)
Tokens	18 in / 4.5k out

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost	Duration
orchestrator-selection	claude-sonnet-4-6	3	124	3.3k	4.8k	~$0.02 (est.)	21s
documentation:docs	claude-sonnet-4-6	4	1.2k	7.2k	12.3k	~$0.07 (est.)	24s
go:implementation-tests	claude-sonnet-4-6	3	124	3.3k	4.8k	~$0.02 (est.)	1m 29s
policies:conventions	claude-sonnet-4-6	4	2.6k	7.2k	13.4k	~$0.09 (est.)	51s
orchestrator-rollup	claude-sonnet-4-6	4	532	19.6k	14.3k	~$0.07 (est.)	28s

[piekstra-dev]

Automated PR review completed with outcome: approved.

[piekstra-dev]

Automated PR Review

Reviewed commit: 0ae8e85d520f
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer	Findings
documentation:docs	0
go:implementation-tests	1
policies:conventions	0

go:implementation-tests (1 finding)

Minor - shared/keyring/resolve_test.go:209

The new error-wrapping paths inside the corrupt-store branch of ResolveToken are not directly tested. The diff adds keyringUnavailableError wrapping to two sub-paths that only execute when Open() returns ErrCorruptStore: (1) OpenNoMigrate() returning a backend error, and (2) resolveFromStore() returning a backend error after OpenNoMigrate() succeeds. TestResolveToken_CorruptSharedConfig_NotWrappedWhenKeyringWorks covers the happy path only (corrupt config + working keyring → no error). The two new error sub-paths have no coverage.

The composition of independent unit tests (TestKeyringUnavailableError_WrapsAndNamesEscapeHatches, TestResolveToken_KeyringOpenFailure_WrappedActionable, TestResolveToken_StoreReadFailure_WrappedActionable) gives reasonable confidence the logic is correct, so this is not blocking. But if someone changes the corrupt-store branch in the future — e.g., removes a keyringUnavailableError call — no test would catch the regression.

Concrete fix: add one test for the corrupt-config + OpenNoMigrate() backend-failure path (set the backend env var to no-such-backend after writing a corrupt shared config and assert the error is wrapped with escape-hatch advice and classifies via errors.Is(err, cccredstore.ErrBackendNotImplemented)), and optionally one test for the post-open read failure in the same branch using the resolveFromStore seam.

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 3m 52s | ~$0.46 (est.) | claude-sonnet-4-6 | cr 0.4.161

Field	Value
Model	claude-sonnet-4-6
Reviewers	documentation:docs, go:implementation-tests, policies:conventions
Engine	claude_cli · claude-sonnet-4-6
Reviewed by	cr · piekstra-dev
Duration	3m 52s wall · 3m 50s compute
Cost	~$0.46 (est.)
Tokens	20 in / 11.7k out

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost	Duration
orchestrator-selection	claude-sonnet-4-6	5	2.1k	22.7k	18.4k	~$0.11 (est.)	44s
documentation:docs	claude-sonnet-4-6	4	1.4k	7.7k	12.3k	~$0.07 (est.)	29s
go:implementation-tests	claude-sonnet-4-6	4	5.4k	7.2k	18.3k	~$0.15 (est.)	1m 31s
policies:conventions	claude-sonnet-4-6	4	2.8k	7.2k	17.9k	~$0.11 (est.)	55s
orchestrator-rollup	claude-sonnet-4-6	3	124	3.3k	4.8k	~$0.02 (est.)	9s

[piekstra-dev]

Automated PR review completed with outcome: approved.