feat(keyring): wire --backend flag and keyring.backend config key by rianjs · Pull Request #140 · open-cli-collective/google-readonly

rianjs · 2026-05-26T23:25:22Z

Summary

Adds --backend persistent flag on gro root and wires it through credstore.BindBackendFlag together with the existing keyring.backend config key. Precedence: --backend > GOOGLE_READONLY_KEYRING_BACKEND > keyring.backend > auto.
Bumps cli-common v0.1.0 → v0.2.0 (Go directive auto-bumps to 1.26), CI Go 1.24/1.22 → 1.26, golangci-lint v2.0.2 → v2.12.2.
Cobra-layer wiring is flag-only; config-side binding lives at the single credstore.Open call site in internal/keychain/keychain.go. WireBackendSelection is exported so any future shadowing subcommand can call it explicitly (cobra does NOT chain PersistentPreRunE).

Test plan

go build ./..., go test ./... (1392 passed), go vet ./..., golangci-lint run clean
gro --help shows --backend with the full precedence usage text
No-direct-env-read invariant: implementation files do not reference GOOGLE_READONLY_KEYRING_BACKEND (verified by grep)
Six new wiring tests in internal/cmd/root/backend_wire_test.go cover: flag-set on subcommand path, invalid flag, config-side passthrough via BindBackendFlag, deferred-validation of invalid config, shadowing-subcommand regression, and a real-tree walk asserting every leaf resolves --backend to the same *pflag.Flag pointer as the root (catches local shadowing).

Closes #139

Surface the cli-common credstore backend selector as a first-class user control: --backend persistent flag on the gro root command, and the existing keyring.backend config key now flows through the same BindBackendFlag helper. Precedence is flag > env > config > auto-detect, matching credstore's internal ordering. Bumps cli-common to v0.2.0 (which transitively bumps the Go directive to 1.26), bumps CI runners to Go 1.26 across ci.yml, release.yml, and test-chocolatey.yml, and bumps golangci-lint to v2.12.2 (v2.0.2 rejects Go 1.26 targets). Wiring is cobra-layer-only: WireBackendSelection validates the flag and records it in a package-level override; openWith binds the flag pair against cfg.Keyring.Backend at the single credstore.Open call site. WireBackendSelection is exported so any future shadowing subcommand can call it explicitly (cobra does NOT chain PersistentPreRunE); gro has no shadowers today, guarded by a regression test. Closes #139

rianjs · 2026-05-26T23:27:48Z

Findings

Minor: gro config show still does not surface the selector name keyring.backend.
internal/cmd/config/config.go:157-182 only reports the resolved backend and source, e.g. Backend: file (config). The converged plan said to add the selector name when the output only shows the resolved backend. This matters for discoverability of the new config key. Add a small human/JSON field or wording that explicitly names keyring.backend.

Minor: The docs sweep missed stale narrow backend wording.
README.md:210-211 still says set-credential lands in “the keyring.backend: file encrypted file,” which omits flag/env selection. CLAUDE.md:79-83 lists config fields but still omits keyring.backend. Update both so future users/agents see the three-knob model consistently.

Minor: The central openWith binding path is not directly tested.
The new tests cover root flag capture and credstore.BindBackendFlag in isolation, but none prove internal/keychain/keychain.go:104-115 actually consumes the stored flag pair and applies flag > config at the credstore.Open site. Add a small internal/keychain test using memory to assert config-only and flag-over-config behavior without platform/passphrase dependencies.

Nit: resetState has a stale cleanup comment and does not reset rootCmd.SetArgs or persistent flag state. I ran go test ./internal/cmd/root -count=20 and did not reproduce a failure, so this is not blocking, but tightening cleanup would make future root tests less fragile.

…s; add openWith binding tests Addresses PR review minors: - gro config show now emits keyring.backend (selector name) alongside the resolved Backend, with the config.yml value when set. - README and the agent-guidance doc sweep: set-credential prose and config-field list now name keyring.backend / --backend / env var consistently with the keyring section. - New internal/keychain/wire_test.go: TestOpenWith_ConfigOnlyMemoryBackend and TestOpenWith_FlagOverridesConfig prove openWith actually consumes cfg.Keyring.Backend via BindBackendFlag and that the flag override wins over config. Memory backend keeps the tests platform- and passphrase-free.

rianjs · 2026-05-26T23:31:20Z

Verdict: Gaps found (minor) — coverage is solid on the wiring, thin on the surfaced output.

Strong coverage on the new --backend wiring:

Flag-set, flag-invalid (with errors.Is(ErrBackendNotImplemented)), config passthrough, invalid-config-deferred, shadowing-subcommand regression, and a real-command-tree walk asserting pointer-identity of the persistent flag at every leaf.
openWith is exercised with config-only (asserts SourceConfig) and flag-overrides-config (asserts SourceExplicit with a config value that would otherwise fail — a nice trap).
1394 tests pass.

Concrete gaps:

Env-var precedence (GOOGLE_READONLY_KEYRING_BACKEND) is never positively asserted. Every test does t.Setenv(..., "") to neutralize it. The docs/CLAUDE.md/README assert "flag > env > config > auto" — env > config and flag > env are unproven in this repo. (May be covered upstream in cli-common/credstore, but the gro-side claim isn't verified.)
config show text output for keyring.backend is untested. TestRunShowReportsState doesn't seed cfg.Keyring.Backend, so the new fmt.Printf("keyring.backend: %s (config.yml)\n", ...) line and its != "" guard are not exercised — neither the present nor the omitted case.
showStatus.KeyringBackend JSON field is unasserted. The omitempty behavior and value passthrough are not covered.

Nothing structural; the secret-handling contract is well-tested. Add a small config show seed-and-assert and an env-precedence test and the docs claims are fully proved.

Addresses TDD-review gap: TestRunShowReportsKeyringBackendSelector seeds keyring.backend=file in config.yml and asserts both the text 'keyring.backend: file (config.yml)' line and the JSON 'keyring_backend' field are emitted. TestRunShowOmitsKeyringBackendWhenUnset locks in the omitempty path so unset configs produce neither output.

monit-reviewer

Automated PR Review

Reviewed commit: 7e8dcc6

Summary

Reviewer	Findings
documentation:docs-reviewer	1
harness-engineering:harness-architecture-reviewer	3
harness-engineering:harness-enforcement-reviewer	1
harness-engineering:harness-knowledge-reviewer	1

documentation:docs-reviewer (1 findings)

💡 Suggestion - README.md:165

Counting inconsistency: text states 'The backend can be selected three ways' but then lists four options (--backend flag, GOOGLE_READONLY_KEYRING_BACKEND env var, keyring.backend config, and auto-detect). Should say 'four ways' or clarify that three are user-configurable options with auto-detect as the fallback.

harness-engineering:harness-architecture-reviewer (3 findings)

⚠️ Should Fix - internal/keychain/wire.go:6

Package-level mutable globals (backendFlagValue, backendFlagWasSet, backendMu) create a hidden side-channel between the cobra PersistentPreRunE layer and openWith(). This bypasses normal Go scoping, demands careful cleanup in every test (resetState + t.Cleanup), and would silently misbehave if two goroutines concurrently called Execute(). Any code path that calls openWith without first going through WireBackendSelection will silently consume whatever backend override was last written. A dependency-injection approach — passing the resolved backend into openWith as a parameter, or threading it through cmd.Context() — would eliminate the ambient state without adding API complexity.

💡 Suggestion - internal/cmd/root/root.go:62

WireBackendSelection calls cccredstore.BindBackendFlag with a throwaway &cccredstore.Options{} (result immediately discarded) solely to validate the flag early, then separately calls keychain.SetBackendFlagOverride. openWith later calls BindBackendFlag a second time. If BindBackendFlag's semantics change (e.g. normalization of backend name, or context-sensitive cross-validation), the cobra-layer validation and the keychain-layer binding could diverge silently. Consider removing the throwaway validation call, or making validation the responsibility of the credstore package's own API.

💡 Suggestion - internal/keychain/keychain.go:105

Validation asymmetry: --backend flag validation is eager (fails at startup), but keyring.backend config validation is deferred until first credential access. A typo in keyring.backend in config.yml is silent until the user runs a command that touches the keyring. This is user-visible and worth surfacing via a startup warning or in gro config show output.

harness-engineering:harness-enforcement-reviewer (1 findings)

💡 Suggestion - internal/cmd/root/backend_wire_test.go:21

resetState's t.Cleanup closure has a misleading comment implying it strips added child commands, but the body only resets the flag override. Child cleanup is delegated to per-test defer removeChild() calls. If a test's AddCommand panics before the defer is registered, or a future test omits the defer, rootCmd is permanently mutated for subsequent tests. TestWireBackendSelection_RealCommandTreeInheritsFlag — which walks all children — would then walk leaked probe commands. Prefer subtest-scoped command trees or reset rootCmd's commands inside the cleanup.

harness-engineering:harness-knowledge-reviewer (1 findings)

⚠️ Should Fix - go.mod:24

The bump from cli-common v0.1.0 → v0.2.0 pulls in unexpected indirect dependencies including: forks of well-known packages under an unknown 'byteness' GitHub org (replacing 99designs/keyring, 99designs/go-keychain, gsterjov/go-libsecret), a beta-tagged 1Password SDK (onepassword-sdk-go v0.4.1-beta.1), a WebAssembly runtime (tetratelabs/wazero + extism/go-sdk + dylibso/observe-sdk), and Jaeger tracing. The WASM runtime stack executing inside a credential-storage operation is a particularly high-impact attack surface. The 'byteness/*' forks replacing established packages are a supply-chain risk. This dependency graph should be audited and confirmed as expected from the cli-common v0.2.0 release before merging.

4 info-level observations excluded. Run with --verbose to include.

2 PR discussion threads considered.

Completed in 3m 10s | $1.37 | 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	3m 10s wall · 10m 21s compute (Reviewers: 2m 13s · Synthesis: 54s)
Cost	$1.37
Tokens	266.1k in / 39.7k out
Turns	10

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	23.8k	2.7k	0	23.8k (1h)	$0.14
documentation:docs-reviewer	haiku	6.5k	6.2k	0	6.5k (1h)	$0.04
harness-engineering:harness-architecture-reviewer	sonnet	69.1k	5.8k	32.3k	36.8k (1h)	$0.26
harness-engineering:harness-enforcement-reviewer	sonnet	32.3k	4.9k	0	32.3k (1h)	$0.22
harness-engineering:harness-knowledge-reviewer	sonnet	78.3k	7.1k	6.9k	71.4k (1h)	$0.40
harness-engineering:harness-self-documenting-code-reviewer	sonnet	35.8k	6.9k	2.2k	33.6k (1h)	$0.24
security:security-code-auditor	haiku	20.4k	5.9k	0	20.3k (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.}

Superseded by updated review

monit-reviewer

Automated PR Review

Reviewed commit: e75258e | Previous: 7e8dcc6 (incremental)

Summary

Reviewer	Findings
harness-engineering:harness-enforcement-reviewer	1
harness-engineering:harness-knowledge-reviewer	2

harness-engineering:harness-enforcement-reviewer (1 findings)

💡 Suggestion - internal/cmd/root/backend_wire_test.go:57

rootCmd.SetArgs(...) is called in multiple tests but never reset via t.Cleanup. Cobra's SetArgs mutates the package-level rootCmd persistently; with go test -shuffle=on a stale args slice from a prior test could bleed into TestWireBackendSelection_RealCommandTreeInheritsFlag or other tests that call Execute(). Add t.Cleanup(func() { rootCmd.SetArgs(nil) }) immediately after each rootCmd.SetArgs(...) call.

harness-engineering:harness-knowledge-reviewer (2 findings)

⚠️ Should Fix - go.mod:24

The cli-common v0.1.0→v0.2.0 bump replaces the community-standard 99designs/keyring family with lesser-known byteness/* forks, and pulls in a large set of unexpected transitive dependencies for a security-critical credential-store library: a full WebAssembly runtime (tetratelabs/wazero ~5 MB), extism/go-sdk, pre-release 1Password SDKs (onepassword-sdk-go v0.4.1-beta.1, connect-sdk-go at a pre-release commit), Uber Jaeger tracing, and dylibso/observe-sdk. The byteness/go-libsecret fork has a timestamp of 20260108, suggesting very recent creation. Per the settled thread (go.mod:24), the author acknowledged the tradeoff was deliberate and documented in cli-common#23, with the WASM/1Password deps gated by AllowedBackends init() registration. However, the original thread settlement covered supply-chain risk in general — the specific concern about the byteness/* forks replacing 99designs/* (not previously called out with this specificity) and the beta SDK in a security-critical path remain worth a one-time explicit sign-off. This finding is not a re-raise of the settled thread but a more specific residual concern about the fork provenance.

💡 Suggestion - internal/keychain/keychain.go:106

When credstore.BindBackendFlag returns an error inside openWith, the error is always prefixed with '--backend:' regardless of whether the failure originated from the CLI flag or the keyring.backend config key. If a user sets an invalid keyring.backend in config.yml and never passes --backend on the command line, the error message will mislead them into thinking the problem is a CLI flag they didn't use. Consider using a context-aware prefix or surfacing both sources in the error text.

2 info-level observations excluded. Run with --verbose to include.

8 PR discussion threads considered.

Completed in 3m 51s | $1.18 | 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	3m 51s wall · 10m 54s compute (Reviewers: 2m 36s · Synthesis: 53s)
Cost	$1.18
Tokens	206.4k in / 44.6k out
Turns	8

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	32.7k	2.6k	7.5k	25.3k (1h)	$0.14
documentation:docs-reviewer	haiku	6.5k	8.6k	0	6.5k (1h)	$0.05
harness-engineering:harness-architecture-reviewer	sonnet	32.3k	5.0k	2.2k	30.2k (1h)	$0.21
harness-engineering:harness-enforcement-reviewer	sonnet	32.3k	6.3k	2.2k	30.2k (1h)	$0.23
harness-engineering:harness-knowledge-reviewer	sonnet	32.3k	5.9k	2.2k	30.2k (1h)	$0.23
harness-engineering:harness-self-documenting-code-reviewer	sonnet	35.8k	8.3k	17.7k	18.1k (1h)	$0.21
security:security-code-auditor	haiku	20.4k	7.1k	0	20.4k (1h)	$0.07
discussion-summarizer	—	14.0k	713	7.5k	6.5k (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.}

monit-reviewer · 2026-05-26T23:40:46Z

 	cloud.google.com/go/compute/metadata v0.9.0 // indirect
-	github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect
-	github.com/99designs/keyring v1.2.2 // indirect
+	github.com/1Password/connect-sdk-go v1.5.4-0.20250417152128-c154b387248b // indirect


🟡 Medium (harness-engineering:harness-knowledge-reviewer): The cli-common v0.1.0→v0.2.0 bump replaces the community-standard 99designs/keyring family with lesser-known byteness/* forks, and pulls in a large set of unexpected transitive dependencies for a security-critical credential-store library: a full WebAssembly runtime (tetratelabs/wazero ~5 MB), extism/go-sdk, pre-release 1Password SDKs (onepassword-sdk-go v0.4.1-beta.1, connect-sdk-go at a pre-release commit), Uber Jaeger tracing, and dylibso/observe-sdk. The byteness/go-libsecret fork has a timestamp of 20260108, suggesting very recent creation. Per the settled thread (go.mod:24), the author acknowledged the tradeoff was deliberate and documented in cli-common#23, with the WASM/1Password deps gated by AllowedBackends init() registration. However, the original thread settlement covered supply-chain risk in general — the specific concern about the byteness/* forks replacing 99designs/* (not previously called out with this specificity) and the beta SDK in a security-critical path remain worth a one-time explicit sign-off. This finding is not a re-raise of the settled thread but a more specific residual concern about the fork provenance.

Reply to this thread when addressed.

See thread on the earlier instance of this finding (#140 (comment)). The cli-common v0.2.0 dep graph is accepted as a deliberate tradeoff, gated by AllowedBackends so the WASM/1Password stacks never execute in gro. No new CGO.

monit-reviewer · 2026-05-26T23:40:46Z

+	rootCmd.SetArgs([]string{"probe-flagset", "--backend", "memory"})
+
+	if err := rootCmd.Execute(); err != nil {
+		t.Fatalf("Execute: %v", err)


🔵 Low (harness-engineering:harness-enforcement-reviewer): rootCmd.SetArgs(...) is called in multiple tests but never reset via t.Cleanup. Cobra's SetArgs mutates the package-level rootCmd persistently; with go test -shuffle=on a stale args slice from a prior test could bleed into TestWireBackendSelection_RealCommandTreeInheritsFlag or other tests that call Execute(). Add t.Cleanup(func() { rootCmd.SetArgs(nil) }) immediately after each rootCmd.SetArgs(...) call.

Reply to this thread when addressed.

Fixed — resetState's t.Cleanup now also calls rootCmd.SetArgs(nil) so a panicking test or shuffled execution order can't leak stale args into the next test.

monit-reviewer · 2026-05-26T23:40:46Z

-		// auto-selection and store credentials somewhere unintended.
-		return nil, fmt.Errorf("invalid keyring.backend %q in config (only \"file\" is supported)", b)
+	// Bind the --backend flag (from the cobra layer, recorded by
+	// root.WireBackendSelection) and cfg.Keyring.Backend together at one


🔵 Low (harness-engineering:harness-knowledge-reviewer): When credstore.BindBackendFlag returns an error inside openWith, the error is always prefixed with '--backend:' regardless of whether the failure originated from the CLI flag or the keyring.backend config key. If a user sets an invalid keyring.backend in config.yml and never passes --backend on the command line, the error message will mislead them into thinking the problem is a CLI flag they didn't use. Consider using a context-aware prefix or surfacing both sources in the error text.

Reply to this thread when addressed.

Fixed — openWith now attributes the error to whichever knob produced the invalid value: '--backend:' when flagSet=true, 'keyring.backend:' otherwise. The current contract is that BindBackendFlag only fails on the flag side (config passes through to credstore.Open), but the branch is in place for the day that broadens.

… SetArgs in tests - openWith now prefixes BindBackendFlag errors with 'keyring.backend:' when the flag is absent (instead of always '--backend:'), so a config-side typo surfaces a message that names the right knob. - resetState now resets rootCmd.SetArgs on cleanup to keep package-level state clean across tests under go test -shuffle=on.

rianjs · 2026-05-26T23:41:49Z

These are low-value, please approve the PR

Superseded by updated review

monit-reviewer

Automated PR Review

Reviewed commit: 9020bf9

Human Override

Approved at the request of @rianjs (comment).

These are low-value, please approve the PR

No automated reviewers were run.

rianjs · 2026-05-26T23:42:56Z

No architectural drift from the daemon edits. The added config show selector field is consistent with the plan, the keychain openWith tests now cover the actual flag/config binding site, and the root test cleanup is tighter without changing the runtime contract. I also ran the targeted packages: go test ./internal/cmd/root ./internal/cmd/config ./internal/keychain -count=1 passed.

One nit: internal/keychain/wire_test.go has a stale comment in TestOpenWith_FlagOverridesConfig saying the config is left at memory and expecting “SourceFlag”; the code correctly sets config to file and asserts credstore.SourceExplicit. Just update the comment.

monit-reviewer previously requested changes May 26, 2026

View reviewed changes

Comment thread go.mod

Comment thread internal/keychain/wire.go

Comment thread internal/cmd/root/root.go

Comment thread internal/keychain/keychain.go

Comment thread internal/cmd/root/backend_wire_test.go

Comment thread README.md Outdated

doc/test cleanup: clarify resetState contract, fix 'three knobs' wording

e75258e

monit-reviewer previously requested changes May 26, 2026

View reviewed changes

monit-reviewer approved these changes May 26, 2026

View reviewed changes

doc: correct stale comment on TestOpenWith_FlagOverridesConfig

6beadc0

rianjs merged commit 8c4ba9e into main May 26, 2026
3 checks passed

rianjs deleted the feat/139-backend-flag-wiring branch May 26, 2026 23:46

Uh oh!

Conversation

rianjs commented May 26, 2026

Summary

Test plan

Uh oh!

rianjs commented May 26, 2026

Uh oh!

rianjs commented May 26, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer May 26, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs May 26, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer May 26, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs May 26, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer May 26, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs May 26, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs commented May 26, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Human Override

Uh oh!

rianjs commented May 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants