Repository navigation
feat(credstore): recognize the pass(1) backend - #26
Conversation
Adds BackendPass to the recognized backend set so consumers wired
against the --backend flag/env/config knobs can pick pass(1) as their
keyring. pass shells out to the `pass` CLI binary with no Go SDK
dependency; the canonical headless-Linux answer noted in the original
ticket.
ByteNess's pass backend ignores Config.ServiceName and stores items at
filepath.Join(PassDir, PassPrefix, key)+".gpg", so without a per-service
PassPrefix every cli-common consumer would share a flat ~/.password-store
namespace and collide on identical key names ("api_token", etc.).
openOSBackend now sets PassPrefix = service for the pass backend,
mirroring the existing per-service carve-out for the file backend.
The keyring.Config construction is extracted into a small
buildKeyringConfig helper so the per-service-prefix wiring is unit
testable without opening a real backend.
Out of scope, deferred to follow-ups: op / op-connect / op-desktop are
all SDK-backed via github.com/1password/onepassword-sdk-go (still
pre-1.0); kwallet, keyctl, passage are real but unrequested. Documented
in §1.10.
Closes #24
The architect's PR review flagged that ByteNess's keyring.Open loops through AllowedBackends and 'continue's past any opener that returns an error — so the actionable 'The pass program is not available' message from byteness/keyring/pass.go init() is swallowed, and the user sees the generic 'Specified keyring backend not available' wrapped by our own error string. Adds preflightOSBackend with a single arm for BackendPass that does an exec.LookPath before calling keyring.Open. The error names the install hint (apt/brew) and the `pass init <gpg-key-id>` follow-up step so the user has a path forward without reading source. Also tightens the BackendMemory comment in the buildKeyringConfig exhaustive switch: memory short-circuits in store.Open and never reaches openOSBackend; the arm exists only to satisfy the exhaustive linter rather than 'no extras needed because ByteNess uses ServiceName.'
|
One meaningful gap: the preflight's actionable error message isn't pinned.
Non-gaps (coverage is fine):
TDD assessment, sub-agent run. |
… contract The TDD assessment on #26 caught a coverage gap: the actionable preflight error message and the assertion that non-pass backends are unaffected by the preflight were both unpinned. The existing TestOpenOSBackend_PassOnWindows_FailsGracefully only runs on Windows, so Mac/Linux CI had no preflight coverage at all. Adds: - TestPreflightOSBackend_PassNotOnPath: forces an empty PATH for the test (t.Setenv restores it) and asserts the error mentions "pass", "PATH", "install", and "pass init" so a future edit that drops the actionable text fails the suite. - TestPreflightOSBackend_NonPassBackendsSkipPreflight: iterates every non-pass backend in the recognized set and asserts the preflight is a no-op even with an empty PATH, so a future change cannot accidentally gate an OS keyring on a CLI binary it does not need.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 62027f9
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs-reviewer | 1 |
| harness-engineering:harness-architecture-reviewer | 1 |
| harness-engineering:harness-enforcement-reviewer | 1 |
| harness-engineering:harness-knowledge-reviewer | 3 |
documentation:docs-reviewer (1 findings)
💡 Suggestion - docs/working-with-secrets.md:161
Component attribution is incorrect: the phrase 'cli-common scopes each consumer's entries' attributes the scoping action to the wrong component. Since credstore sets PassPrefix, the sentence should reference credstore specifically, e.g. 'credstore scopes each consumer's entries under a per-service subtree of the password-store (by setting
PassPrefix = <service>)'.
harness-engineering:harness-architecture-reviewer (1 findings)
💡 Suggestion - credstore/osbackend_test.go:401
TestOpenOSBackend_PassOnWindows_FailsGracefully has an inaccurate comment claiming it pins the ByteNess 'not-registered' path (pass.go is !windows). In practice, preflightOSBackend's exec.LookPath check fires first on any system where pass is not on PATH — which includes virtually every Windows system. The test assertions remain correct (error contains 'pass'), but the comment misrepresents which code path is being tested. Update to reflect that the error originates from the preflight LookPath check, and note that the ByteNess registration path is not independently exercised by this test.
harness-engineering:harness-enforcement-reviewer (1 findings)
💡 Suggestion - credstore/osbackend.go:60
preflightOSBackend's BackendPass branch performs exec.LookPath on all platforms without a runtime.GOOS == "windows" short-circuit. On Windows systems with a WSL bridge script or Git-for-Windows shim that places 'pass' on PATH, the preflight succeeds but keyring.Open subsequently returns a generic ByteNess 'backend not available' error rather than a clear 'pass is unsupported on Windows' message. TestOpenOSBackend_PassOnWindows_FailsGracefully only exercises the no-pass-in-PATH path, leaving this edge case uncovered.
harness-engineering:harness-knowledge-reviewer (3 findings)
credstore/osbackend.go:54
The preflight error message for BackendPass hardcodes apt/brew install hints ('install it (e.g.
apt install pass/brew install pass)') without guarding on runtime.GOOS. On Windows — where the byteness/keyring pass backend is excluded via !windows build tags — a user hitting this path sees Linux/macOS-specific install instructions that are not actionable. Consider adding an explicit runtime.GOOS == "windows" guard that either returns 'pass backend is not supported on Windows' or omits the platform-specific hints.
💡 Suggestion - credstore/select_test.go:102
There is a test asserting BackendMemory is never auto-selected (TestSelectBackendMemoryNeverAuto) but no equivalent test asserting BackendPass is never auto-selected. BackendPass is now in allBackends, and on systems where pass is installed, a future selectBackend refactor could silently enable pass auto-selection. A TestSelectBackendPassNeverAuto test mirroring the Memory pattern would pin the 'pass is opt-in only' contract.
💡 Suggestion - credstore/osbackend.go:52
The preflightOSBackend switch is not covered by the exhaustive linter in the same way as buildKeyringConfig's switch. A developer adding a new Backend constant may add it to the combined no-op case without considering whether it needs a preflight check, and no linter would flag the omission. Consider adding a doc comment explicitly noting that new backends must be evaluated for preflight requirements, or splitting the combined case to mirror the pattern used in buildKeyringConfig.
5 info-level observations excluded. Run with --verbose to include.
1 PR discussion thread considered.
Completed in 6m 01s | $1.17 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 6m 01s wall · 15m 48s compute (Reviewers: 3m 45s · Synthesis: 2m 14s) |
| Cost | $1.17 |
| Tokens | 161.4k in / 63.7k out |
| Turns | 11 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 19.3k | 9.1k | 7.5k | 11.8k (1h) | $0.19 |
| documentation:docs-reviewer | haiku | 5.0k | 9.4k | 0 | 5.0k (1h) | $0.06 |
| harness-engineering:harness-architecture-reviewer | sonnet | 30.6k | 11.1k | 15.1k | 15.5k (1h) | $0.24 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 30.3k | 7.0k | 15.1k | 15.2k (1h) | $0.17 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 31.1k | 13.1k | 15.1k | 16.0k (1h) | $0.27 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 28.7k | 8.2k | 13.8k | 14.8k (1h) | $0.19 |
| security:security-code-auditor | haiku | 16.5k | 5.7k | 0 | 16.5k (1h) | $0.06 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| - **Secret Service is present but locked, returns an auth failure, or otherwise rejects the request.** **Fail closed** with an actionable error that names the backend, the operation that failed, and how to unlock or grant access (`gnome-keyring-daemon`, `seahorse`, `kwalletmanager`, etc.). Do *not* silently fall back to the file backend. A user with a working desktop keyring that happens to be locked must not have their secrets silently downgraded to an encrypted file in a different location — that is a stealth security regression and a likely source of "where did my credentials go?" support tickets. | ||
| - **Ambiguous failure → fail closed.** If the wrapper cannot confidently distinguish "unavailable" from "denied/locked" — D-Bus answered but returned an opaque error, the backend timed out, the response shape was unexpected — treat the case as denied/locked and fail closed. The fallback path is opt-in only when we are sure no working keyring is present. | ||
| - **Explicit user opt-in to the file backend** is supported via a config flag (working name `keyring.backend: file` in `config.yml`) or an env var (`<SERVICE>_KEYRING_BACKEND=file`). With that set, the CLI uses the file backend unconditionally and never attempts Secret Service. This is the supported path for users who genuinely prefer the file backend. The backend-selector env var is non-secret runtime configuration (it controls *where* the CLI looks, not *what* it finds); it is not the runtime-env-var exception described below. | ||
| - **Explicit user opt-in to the `pass` backend** is supported via the same knobs (`--backend pass`, `<SERVICE>_KEYRING_BACKEND=pass`, or `keyring.backend: pass`). `pass` is the canonical headless-Linux answer for users who already run a `pass` password-store; it requires the `pass` CLI binary on `$PATH` and an initialized password-store (`pass init <gpg-key-id>`). cli-common scopes each consumer's entries under a per-service subtree of the password-store (the credstore sets `PassPrefix = <service>`), so two CLIs storing the same key name do not collide. KeePassXC users do not need a separate backend — KeePassXC integrates with Secret Service over D-Bus, so `--backend secret-service` (the default) already covers it. The `pass` backend is `!windows`: requesting it on Windows fails closed with a "no such backend" error rather than silently falling back. |
There was a problem hiding this comment.
🔵 Low (documentation:docs-reviewer): Component attribution is incorrect: the phrase 'cli-common scopes each consumer's entries' attributes the scoping action to the wrong component. Since credstore sets PassPrefix, the sentence should reference credstore specifically, e.g. 'credstore scopes each consumer's entries under a per-service subtree of the password-store (by setting PassPrefix = <service>)'.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed — §1.4 now says 'credstore scopes each consumer's entries... (by setting PassPrefix = <service>)'.
| // TestOpenOSBackend_PassOnWindows_FailsGracefully pins the Windows | ||
| // behavior: byteness/keyring's pass.go is `//go:build !windows`, so a | ||
| // user reading our docs and trying `--backend pass` on Windows will hit | ||
| // the not-registered path inside ByteNess. We want a sensible error |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestOpenOSBackend_PassOnWindows_FailsGracefully has an inaccurate comment claiming it pins the ByteNess 'not-registered' path (pass.go is !windows). In practice, preflightOSBackend's exec.LookPath check fires first on any system where pass is not on PATH — which includes virtually every Windows system. The test assertions remain correct (error contains 'pass'), but the comment misrepresents which code path is being tested. Update to reflect that the error originates from the preflight LookPath check, and note that the ByteNess registration path is not independently exercised by this test.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed — comment now reflects that the preflight's runtime.GOOS guard fires first (the ByteNess !windows registration is not independently exercised by this test).
| // user sees a generic "Specified keyring backend not available". Catch | ||
| // the common cases here so users get actionable messages. | ||
| func preflightOSBackend(kind Backend) error { | ||
| switch kind { |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-knowledge-reviewer): The preflight error message for BackendPass hardcodes apt/brew install hints ('install it (e.g. apt install pass / brew install pass)') without guarding on runtime.GOOS. On Windows — where the byteness/keyring pass backend is excluded via !windows build tags — a user hitting this path sees Linux/macOS-specific install instructions that are not actionable. Consider adding an explicit runtime.GOOS == "windows" guard that either returns 'pass backend is not supported on Windows' or omits the platform-specific hints.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed — added a runtime.GOOS == "windows" short-circuit ahead of the LookPath. Windows users now get a clear 'not supported on Windows' message with no Linux/macOS install hints. Mooted both this and the Windows-shimmed-pass concern below.
| return fmt.Errorf("the pass(1) CLI is not on $PATH; install it (e.g. `apt install pass` / `brew install pass`) and run `pass init <gpg-key-id>` before selecting --backend pass") | ||
| } | ||
| case BackendKeychain, BackendWinCred, BackendSecretService, BackendFile, BackendMemory: | ||
| // No preflight check — ByteNess's openers (or store.Open's |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): preflightOSBackend's BackendPass branch performs exec.LookPath on all platforms without a runtime.GOOS == "windows" short-circuit. On Windows systems with a WSL bridge script or Git-for-Windows shim that places 'pass' on PATH, the preflight succeeds but keyring.Open subsequently returns a generic ByteNess 'backend not available' error rather than a clear 'pass is unsupported on Windows' message. TestOpenOSBackend_PassOnWindows_FailsGracefully only exercises the no-pass-in-PATH path, leaving this edge case uncovered.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed by the same runtime.GOOS guard in the sibling thread — preflight now fails closed on Windows before LookPath, so a shimmed pass on PATH cannot pass preflight only to die inside ByteNess.
| @@ -102,3 +102,18 @@ func TestSelectBackendMemoryNeverAuto(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): There is a test asserting BackendMemory is never auto-selected (TestSelectBackendMemoryNeverAuto) but no equivalent test asserting BackendPass is never auto-selected. BackendPass is now in allBackends, and on systems where pass is installed, a future selectBackend refactor could silently enable pass auto-selection. A TestSelectBackendPassNeverAuto test mirroring the Memory pattern would pin the 'pass is opt-in only' contract.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed — added TestSelectBackendPassNeverAuto mirroring the BackendMemory pattern. Asserts explicit pass selection works (SourceExplicit) and that auto selection on darwin/windows/linux never returns pass.
| // and `continue`s past any opener that returns an error, so a useful | ||
| // "pass program is not available" from the pass opener is lost and the | ||
| // user sees a generic "Specified keyring backend not available". Catch | ||
| // the common cases here so users get actionable messages. |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): The preflightOSBackend switch is not covered by the exhaustive linter in the same way as buildKeyringConfig's switch. A developer adding a new Backend constant may add it to the combined no-op case without considering whether it needs a preflight check, and no linter would flag the omission. Consider adding a doc comment explicitly noting that new backends must be evaluated for preflight requirements, or splitting the combined case to mirror the pattern used in buildKeyringConfig.
Reply to this thread when addressed.
There was a problem hiding this comment.
Added a doc comment to preflightOSBackend explicitly directing future authors of new Backend constants to evaluate whether a preflight is warranted. The exhaustive linter does cover this switch (an unhandled constant fails the build), but the doc hint helps an author think about preflight semantics rather than just adding their new case to the no-op arm.
…nt fixes Daemon review on #26 caught six items: - (Medium) preflight error hardcoded apt/brew hints regardless of OS. A Windows user with a `pass` shim on PATH (Git-for-Windows, WSL bridge) would otherwise pass preflight and then hit the generic ByteNess "backend not available" from keyring.Open. Added a runtime.GOOS == "windows" guard ahead of the LookPath that returns a clear "not supported on Windows" message with no Linux/macOS install hints. - (Low) TestSelectBackendPassNeverAuto mirrors the BackendMemory contract so a future selectBackend refactor cannot silently start auto-picking pass on systems that happen to have it installed. - (Low) Doc attribution: "cli-common scopes" → "credstore scopes" in §1.4. The PassPrefix wiring lives in credstore/osbackend.go, not the umbrella module. - (Low) Windows test comment claimed it pinned the ByteNess not-registered path; in practice the preflight's runtime.GOOS guard fires first. Comment now reflects that. - (Low) Doc comment on preflightOSBackend telling future authors of new Backend constants to evaluate whether a preflight is warranted.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: a9acb4a | Previous: 62027f9 (incremental)
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (2 findings)
credstore/osbackend_test.go:406
TestPreflightOSBackend_PassNotOnPath is missing a Windows skip guard and will fail on Windows CI. On Windows, preflightOSBackend(BackendPass) now returns the 'not supported on Windows' message before reaching the exec.LookPath branch, so the assertions for 'PATH', 'install', and 'pass init' — substrings that only appear in the POSIX message — will all fail. Add
if runtime.GOOS == "windows" { t.Skip("PATH-empty preflight test is POSIX-only; see TestOpenOSBackend_PassOnWindows_FailsGracefully") }at the top, mirroring the pattern already used in TestOpenOSBackend_PassOnWindows_FailsGracefully.
💡 Suggestion - credstore/osbackend.go:104
cfg.PassPrefix is set to the raw service string with no validation. The pass backend stores items at filepath.Join(PassDir, PassPrefix, key)+".gpg", so a service name containing path separators (e.g. "../sibling") would silently escape the intended password-store subtree. Service names are supplied by downstream library callers rather than end-users, so the practical risk is low, but a short guard (e.g. rejecting names containing "/" or "\") would prevent a surprising filesystem layout if a downstream CLI ever passes a directory-style service name.
4 info-level observations excluded. Run with --verbose to include.
7 PR discussion threads considered.
Completed in 5m 51s | $1.33 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 5m 51s wall · 15m 34s compute (Reviewers: 4m 05s · Synthesis: 1m 28s) |
| Cost | $1.33 |
| Tokens | 204.0k in / 63.2k out |
| Turns | 11 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 32.6k | 5.0k | 7.5k | 25.2k (1h) | $0.17 |
| documentation:docs-reviewer | haiku | 5.0k | 6.2k | 0 | 5.0k (1h) | $0.04 |
| harness-engineering:harness-architecture-reviewer | sonnet | 33.3k | 9.9k | 15.7k | 17.6k (1h) | $0.23 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 35.7k | 12.3k | 15.7k | 19.9k (1h) | $0.27 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 32.7k | 9.0k | 15.7k | 17.0k (1h) | $0.21 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 33.9k | 15.6k | 14.5k | 19.5k (1h) | $0.32 |
| security:security-code-auditor | haiku | 17.2k | 4.5k | 0 | 17.1k (1h) | $0.05 |
| discussion-summarizer | — | 13.6k | 736 | 7.5k | 6.1k (1h) | $0.04 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| // not from ByteNess's !windows opener registration — the preflight | ||
| // fires first so the platform constraint is the user-visible reason. | ||
| // Skipped on non-Windows. | ||
| func TestOpenOSBackend_PassOnWindows_FailsGracefully(t *testing.T) { |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-architecture-reviewer): TestPreflightOSBackend_PassNotOnPath is missing a Windows skip guard and will fail on Windows CI. On Windows, preflightOSBackend(BackendPass) now returns the 'not supported on Windows' message before reaching the exec.LookPath branch, so the assertions for 'PATH', 'install', and 'pass init' — substrings that only appear in the POSIX message — will all fail. Add if runtime.GOOS == "windows" { t.Skip("PATH-empty preflight test is POSIX-only; see TestOpenOSBackend_PassOnWindows_FailsGracefully") } at the top, mirroring the pattern already used in TestOpenOSBackend_PassOnWindows_FailsGracefully.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed — added runtime.GOOS == "windows" skip at the top, mirroring TestOpenOSBackend_PassOnWindows_FailsGracefully. The Windows branch is independently covered by that test.
| cfg.FilePasswordFunc = pwFunc | ||
| case BackendPass: | ||
| // ByteNess's pass backend ignores ServiceName — items are | ||
| // stored at filepath.Join(PassDir, PassPrefix, key) + ".gpg" |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): cfg.PassPrefix is set to the raw service string with no validation. The pass backend stores items at filepath.Join(PassDir, PassPrefix, key)+".gpg", so a service name containing path separators (e.g. "../sibling") would silently escape the intended password-store subtree. Service names are supplied by downstream library callers rather than end-users, so the practical risk is low, but a short guard (e.g. rejecting names containing "/" or "\") would prevent a surprising filesystem layout if a downstream CLI ever passes a directory-style service name.
Reply to this thread when addressed.
There was a problem hiding this comment.
Service names are already validated upstream by validSegment in credstore/ref.go, which restricts segments to [A-Za-z0-9_-] (no ., no /, no \\). store.Open enforces it on every entry path, so a "../sibling" service would be rejected long before reaching buildKeyringConfig. No additional guard needed.
Daemon review on #26 (run 2) caught that TestPreflightOSBackend_PassNotOnPath would fail on Windows CI: the Windows guard added in the previous commit returns the 'not supported on Windows' message before reaching LookPath, so the POSIX-specific substring assertions ('PATH', 'install', 'pass init') no longer match. Adds runtime.GOOS skip mirroring the existing TestOpenOSBackend_PassOnWindows_FailsGracefully pattern.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: ca022c2 | Previous: a9acb4a (incremental)
Approved with 4 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs-reviewer | 1 |
| harness-engineering:harness-architecture-reviewer | 2 |
| harness-engineering:harness-enforcement-reviewer | 1 |
documentation:docs-reviewer (1 findings)
💡 Suggestion - docs/working-with-secrets.md:160
The new
passbackend paragraph lists three configuration mechanisms (--backend pass,<SERVICE>_KEYRING_BACKEND=pass,keyring.backend: pass), but the immediately precedingfilebackend paragraph documents only two (env var and config file) with no mention of--backend file. This PR-introduced asymmetry will confuse readers comparing the two sections. Either update thefilebackend paragraph to document--backend file, or add a brief note explaining why the CLI flag applies to both backends equally.
harness-engineering:harness-architecture-reviewer (2 findings)
💡 Suggestion - credstore/osbackend_test.go:386
TestPreflightOSBackend_PassNotOnPath covers the 'pass absent from PATH' failure path, and TestOpenOSBackend_PassOnWindows_FailsGracefully covers the Windows guard, but there is no test for the success path: preflightOSBackend(BackendPass) returning nil when
passIS on PATH on a non-Windows host. If the LookPath error-check sense were accidentally inverted, or the early-return logic rearranged, the positive path would silently break and only manual testing would catch it.
💡 Suggestion - credstore/select_test.go:127
TestSelectBackendPassNeverAuto uses
continuewhen selectBackend returns an error, silently skipping the 'pass was not auto-selected' assertion for that platform. The comment frames this as expected for 'unknown' GOOS values, but all three enumerated platforms (darwin, windows, linux) are known. A future selectBackend change that returns an error for linux before evaluating the backend would make the loop a no-op for that platform without failing the test. Assertb != BackendPassin the error branch as well, or log a t.Logf noting the skip so coverage gaps are visible.
harness-engineering:harness-enforcement-reviewer (1 findings)
💡 Suggestion - credstore/osbackend.go:48
preflightOSBackend's switch has a BackendMemory arm that silently does nothing. If store.Open's short-circuit for BackendMemory is ever removed or bypassed, the preflight passes, buildKeyringConfig's BackendMemory arm also does nothing, and execution reaches keyring.Open with AllowedBackends={memory} — which is unlikely to work at the OS keyring layer. The equivalent risk in buildKeyringConfig carries a comment, but preflightOSBackend has no corresponding note explaining why silently passing is correct here.
4 info-level observations excluded. Run with --verbose to include.
9 PR discussion threads considered.
Completed in 7m 33s | $1.17 | sonnet | daemon 0.2.120 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 3 · Session resumed |
| Reviewers | hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 7m 33s wall · 14m 25s compute (Reviewers: 4m 36s · Synthesis: 2m 33s) |
| Cost | $1.17 |
| Tokens | 186.6k in / 55.1k out |
| Turns | 10 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 43.4k | 8.1k | 7.5k | 35.9k (1h) | $0.26 |
| documentation:docs-reviewer | haiku | 5.0k | 9.4k | 5.0k | 0 | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 31.2k | 7.9k | 15.8k | 15.4k (1h) | $0.19 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 27.8k | 2.2k | 15.8k | 12.0k (1h) | $0.09 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 13.6k | 7.1k | 2.2k | 11.4k (1h) | $0.16 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 34.2k | 16.8k | 14.5k | 19.7k (1h) | $0.34 |
| security:security-code-auditor | haiku | 17.2k | 2.7k | 0 | 17.2k (1h) | $0.04 |
| discussion-summarizer | — | 14.1k | 951 | 7.5k | 6.7k (1h) | $0.04 |
Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.
| @@ -158,6 +158,7 @@ The shared credstore package selects backends in this fixed order: | |||
| - **Secret Service is present but locked, returns an auth failure, or otherwise rejects the request.** **Fail closed** with an actionable error that names the backend, the operation that failed, and how to unlock or grant access (`gnome-keyring-daemon`, `seahorse`, `kwalletmanager`, etc.). Do *not* silently fall back to the file backend. A user with a working desktop keyring that happens to be locked must not have their secrets silently downgraded to an encrypted file in a different location — that is a stealth security regression and a likely source of "where did my credentials go?" support tickets. | |||
| - **Ambiguous failure → fail closed.** If the wrapper cannot confidently distinguish "unavailable" from "denied/locked" — D-Bus answered but returned an opaque error, the backend timed out, the response shape was unexpected — treat the case as denied/locked and fail closed. The fallback path is opt-in only when we are sure no working keyring is present. | |||
| - **Explicit user opt-in to the file backend** is supported via a config flag (working name `keyring.backend: file` in `config.yml`) or an env var (`<SERVICE>_KEYRING_BACKEND=file`). With that set, the CLI uses the file backend unconditionally and never attempts Secret Service. This is the supported path for users who genuinely prefer the file backend. The backend-selector env var is non-secret runtime configuration (it controls *where* the CLI looks, not *what* it finds); it is not the runtime-env-var exception described below. | |||
There was a problem hiding this comment.
🔵 Low (documentation:docs-reviewer): The new pass backend paragraph lists three configuration mechanisms (--backend pass, <SERVICE>_KEYRING_BACKEND=pass, keyring.backend: pass), but the immediately preceding file backend paragraph documents only two (env var and config file) with no mention of --backend file. This PR-introduced asymmetry will confuse readers comparing the two sections. Either update the file backend paragraph to document --backend file, or add a brief note explaining why the CLI flag applies to both backends equally.
Reply to this thread when addressed.
| return "" | ||
| } | ||
| cfg, err := buildKeyringConfig(BackendFile, "atlassian-cli", &Options{}, getenv) | ||
| if err != nil { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestPreflightOSBackend_PassNotOnPath covers the 'pass absent from PATH' failure path, and TestOpenOSBackend_PassOnWindows_FailsGracefully covers the Windows guard, but there is no test for the success path: preflightOSBackend(BackendPass) returning nil when pass IS on PATH on a non-Windows host. If the LookPath error-check sense were accidentally inverted, or the early-return logic rearranged, the positive path would silently break and only manual testing would catch it.
Reply to this thread when addressed.
| // on systems that happen to have the binary installed would be a stealth | ||
| // security regression (silent backend change), so pin the contract. | ||
| func TestSelectBackendPassNeverAuto(t *testing.T) { | ||
| if b, src, err := selectBackend("svc", &Options{Backend: BackendPass}, envFrom(nil), "linux"); err != nil || b != BackendPass || src != SourceExplicit { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestSelectBackendPassNeverAuto uses continue when selectBackend returns an error, silently skipping the 'pass was not auto-selected' assertion for that platform. The comment frames this as expected for 'unknown' GOOS values, but all three enumerated platforms (darwin, windows, linux) are known. A future selectBackend change that returns an error for linux before evaluating the backend would make the loop a no-op for that platform without failing the test. Assert b != BackendPass in the error branch as well, or log a t.Logf noting the skip so coverage gaps are visible.
Reply to this thread when addressed.
| } | ||
|
|
||
| // preflightOSBackend runs cheap, actionable pre-construction checks for | ||
| // backends whose ByteNess opener returns a useful error message that |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): preflightOSBackend's switch has a BackendMemory arm that silently does nothing. If store.Open's short-circuit for BackendMemory is ever removed or bypassed, the preflight passes, buildKeyringConfig's BackendMemory arm also does nothing, and execution reaches keyring.Open with AllowedBackends={memory} — which is unlikely to work at the OS keyring layer. The equivalent risk in buildKeyringConfig carries a comment, but preflightOSBackend has no corresponding note explaining why silently passing is correct here.
Reply to this thread when addressed.
|
No architectural drift found in the post-daemon diff. The new The added never-auto test and corrected |
Picks up open-cli-collective/cli-common#26: the pass(1) backend is now recognized by credstore. No code changes required in any downstream module — the existing --backend flag wiring passes the new name through to credstore.BindBackendFlag as an opaque string. After this lands, users can run e.g.: jtk issues list --backend pass cfl pages list --backend pass (both require the pass(1) CLI on $PATH and a `pass init`-initialized password-store).
Picks up open-cli-collective/cli-common#26: the `pass(1)` backend is now in credstore's recognized backend set. No code changes — the existing `--backend` wiring passes `pass` through to credstore as an opaque string. After merge: `nrq --backend pass <cmd>` works for users with the `pass(1)` CLI on `$PATH` and an initialized password-store.
Picks up open-cli-collective/cli-common#26: the `pass(1)` backend is now in credstore's recognized backend set. No code changes — the existing `--backend` wiring passes `pass` through to credstore as an opaque string. After merge: `slck --backend pass <cmd>` works for users with the `pass(1)` CLI on `$PATH` and an initialized password-store.
Summary
BackendPassto the recognized backend set so consumers wired against--backend/<SVC>_KEYRING_BACKEND/keyring.backendcan pickpass(1)as their keyring.buildKeyringConfigfromopenOSBackendand setscfg.PassPrefix = servicefor thepassbackend — critical because ByteNess's pass backend ignoresServiceNameand would otherwise let two CLIs collide in~/.password-storeon identical key names.working-with-secrets.md: §1.4 explicit-opt-in paragraph + §1.10 "what credstore exposes" rewrite. KeePassXC works today via Secret Service (no separate backend); 1Password is explicitly deferred because all three of ByteNess'sop*openers depend on the still-pre-1.0onepassword-sdk-go.Why
pass"the canonical headless-Linux answer" and noted it pairs with the existingloginctl enable-lingerguidance. cli-common#23 deliberately keptparseBackendat the original five names to keep that diff tight; this is the follow-up that puts the new name into circulation.--backend <whatever>through tocredstore.BindBackendFlagas an opaque string (atlassian-cli#386, google-readonly#140, newrelic-cli#105, slack-chat-api#170). After this PR releases as v0.2.1, they pick up--backend passautomatically the next time they bump cli-common — no per-CLI PR required.Architectural notes
PassPrefix = serviceis the load-bearing line. The pass backend's item path isfilepath.Join(PassDir, PassPrefix, key)+".gpg"(byteness/keyring/pass.go:129). WithoutPassPrefixevery consumer would share a flat namespace andcfl+slckstoring keys with identical names would collide silently.TestBuildKeyringConfig_PassSetsPrefixToServicepins this with two distinct services.buildKeyringConfigextraction. Moving the per-backend wiring into a pure helper makes the prefix assertion testable without opening a real keyring. The existingBackendFilebranch moves into the helper unchanged.TestAllBackends_MatchesConstantsis the drift guard: it forces a constant addition to land inallBackendsAND in the expected-slice literal. Addingpassexercised it.BackendPasstriggered theexhaustivelinter on the switch inbuildKeyringConfig. Added adefault-stylecase enumerating the no-extra-wiring backends rather than a baredefault:so a future addition forces the author to think about whether the new backend needs construction extras.passopener is!windowsand has no SDK imports.Out of scope (explicitly deferred)
op/op-connect/op-desktop— all SDK-backed viagithub.com/1password/onepassword-sdk-go(pre-1.0). Gated on the SDK going GA; will file a follow-up.kwallet,keyctl,passage— real but unrequested. Will file a single tracking issue for "expose remaining ByteNess backends on user ask".PassDir/PassCmdconfig options —PASSWORD_STORE_DIRenv var already covers the override case; addingOptionsfields without a user ask would be premature configurability.Test plan
go test ./... -race— all packages passgo vet ./...— cleangolangci-lint run— clean (after adding the exhaustive switch case)TestBuildKeyringConfig_PassSetsPrefixToServicecovers the prefix scoping with two distinct services + the file-backend negative caseTestAllBackends_MatchesConstantsexercises the lockstep drift guardTestOpenOSBackend_PassOnWindows_FailsGracefullyskipped on macOS; runs on Windows CIpass --version,pass init <gpg-key>, then a downstream CLI built against this commit with--backend passCloses #24