feat(credstore): expose 1Password backends

[rianjs]

No description provided.

[rianjs]

Major

• docs/working-with-secrets.md:181 and docs/working-with-secrets.md:343 still say <SERVICE>_KEYRING_PASSPHRASE is the only runtime env-var exception for secret/unlocking material. That conflicts with the new explicit op / op-connect backends, where ByteNess reads OP_SERVICE_ACCOUNT_TOKEN / OP_CONNECT_TOKEN at runtime to unlock the 1Password credential store. Update the exception/compliance text to allow backend-auth material for explicitly selected 1Password backends, while keeping the ban on env vars as primary API credentials.

Minor

• credstore/osbackend_1password_build_test.go:16 does not actually pin the disabled-build contract: it accepts nil, so a regression where keyring_no1password stops returning ErrBackendNotImplemented would still pass. Add a //go:build keyring_no1password test that requires the three 1Password backends to fail with ErrBackendNotImplemented in that build.

Nit

• docs/development.md:18 and docs/working-with-secrets.md:100 still say “OS-keyring/OS credential store” for the package/storage bucket. With pass and 1Password now first-class explicit backends, use “credential store” unless the paragraph is specifically about platform keyrings.

[rianjs]

No blockers or majors found. The prior standards contradiction is resolved: backend-auth env vars for explicit 1Password backends are now documented separately from upstream API credentials, and the disabled-build behavior is pinned with a keyring_no1password test.

[rianjs]

Findings

• Blocker: credstore/osbackend_1password_build_test.go:16-25 does not actually prove the enabled build supports 1Password. It passes when unsupportedOnePasswordBackendInBuild() returns either nil or ErrBackendNotImplemented, so the default CI lane would still go green if 1Password were accidentally compiled out everywhere. There is no Open(...BackendOP*) success-path test to catch that regression.
• Major: The no-1Password lane is still only helper-level coverage. credstore/osbackend_1password_disabled_test.go:10-16 checks unsupportedOnePasswordBackendInBuild() directly, but nothing exercises the public Open path under keyring_no1password. A break in openOSBackend() or the helper call site would still slip through, even though that is the behavior CI is meant to preserve.
• Minor: credstore/flag_test.go:229-234 is redundant and weak. strings.Contains(usage, "op") can be satisfied by op-connect or op-desktop, so it does not independently pin the standalone op entry. The existing TestAllBackends_MatchesConstants already covers the recognized set.

[monit-reviewer]

Automated PR Review

Reviewed commit: bc08bb7

Summary

Reviewer	Findings
documentation:docs-reviewer	2
harness-engineering:harness-knowledge-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	2

documentation:docs-reviewer (2 findings)

⚠️ Should Fix - docs/working-with-secrets.md:333

The revised 'Standard build configuration: keyring opt-out tags' subsection removes the explanation that 1Password SDKs (~10.6 MB, including WASM runtime wazero/extism) compile into every consumer that omits -tags keyring_no1password. The previous text mandated the opt-out for all consumers and explained the dependency consequence; the replacement characterizes keyring_no1password as merely 'an allowed opt-out for compliance-scoped or minimal-dependency builds' with no size or dependency impact explained. Non-1Password consumer teams lose the context needed to make an informed build decision and may unknowingly ship binaries bloated with a pre-1.0 SDK and a WASM runtime.

💡 Suggestion - docs/development.md:48

The 'Keyring opt-out tags' paragraph now only addresses CLIs that expose 1Password and mentions the optional opt-out path, but gives no guidance for CLIs that intentionally skip 1Password support. The previous text prescribed a MUST configuration for all consumers. Developers maintaining non-1Password consumer CLIs have no signal here that they should adopt -tags keyring_no1password,keyring_nopassage; the pointer to working-with-secrets.md §1.10 partially compensates but the summary paragraph itself is now incomplete.

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

💡 Suggestion - docs/working-with-secrets.md:178

The env-var exceptions paragraph lists OP_SERVICE_ACCOUNT_TOKEN and OP_CONNECT_TOKEN as the permitted 1Password backend-auth env vars but omits OP_DESKTOP_ACCOUNT_ID (introduced as DefaultOnePasswordDesktopAccountEnv in credstore/store.go). An op-desktop deployment setting OP_DESKTOP_ACCOUNT_ID would appear non-compliant under a strict reading of the acceptance criteria. Add OP_DESKTOP_ACCOUNT_ID (or 'caller-configured account-ID env var') to the exceptions list alongside the other two.

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

💡 Suggestion - credstore/osbackend_core.go:149

The condition if kind == BackendOP || kind == BackendOPDesktop silently excludes BackendOPConnect from the default timeout with no explanation. The distinction — Connect is HTTP-based and manages its own timeout at the transport layer, while OP and OPDesktop invoke a local CLI that can hang — is non-obvious from the code alone. A brief inline comment such as // OPConnect is HTTP-based; the HTTP client enforces its own timeout would prevent a future maintainer from treating this asymmetry as an accidental omission.

💡 Suggestion - credstore/osbackend_test.go:393

Timeout: 5 assigns a raw integer literal to a time.Duration field. The value 5 means 5 nanoseconds — clearly unrealistic for a timeout — but the unit is invisible at the call site, and the subsequent assertion message 'want 5' compounds the ambiguity. The analogous test in osbackend_byteness_test.go uses 5 * time.Second. Use an explicit unit (5 * time.Nanosecond if the intent is a non-zero sentinel for forwarding verification, or 5 * time.Second to match surrounding style) to eliminate the unit ambiguity.

3 PR discussion threads considered.

---

Completed in 4m 14s | $2.46 | sonnet | daemon 0.2.128 | 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	4m 14s wall · 4m 11s compute (Reviewers: 2m 37s · Synthesis: 1m 34s)
Cost	$2.46 (estimated)
Tokens	420.3k in / 32.8k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	41.4k	5.0k	25.0k	16.3k (1h)	$0.18
documentation:docs-reviewer	sonnet	54.7k	7.8k	13.8k	40.9k (1h)	$0.37
harness-engineering:harness-architecture-reviewer	sonnet	68.1k	1.0k	2.6k	65.5k (1h)	$0.41
harness-engineering:harness-enforcement-reviewer	sonnet	74.2k	6.8k	13.8k	60.4k (1h)	$0.47
harness-engineering:harness-knowledge-reviewer	sonnet	71.5k	4.3k	13.8k	57.7k (1h)	$0.41
harness-engineering:harness-self-documenting-code-reviewer	sonnet	57.9k	6.1k	13.8k	44.1k (1h)	$0.36
security:security-code-auditor	sonnet	52.7k	1.8k	13.8k	38.9k (1h)	$0.26

_{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: 410e43f | Previous: bc08bb7 (incremental)

Summary

No issues found.

8 PR discussion threads considered.

---

Completed in 1m 45s | $6.41 | sonnet | daemon 0.2.128 | 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	1m 45s wall · 1m 15s compute (Reviewers: 1m 00s · Synthesis: 16s)
Cost	$6.41 (estimated)
Tokens	1125.9k in / 39.4k out
Turns	28

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	96.4k	5.7k	39.5k	57.0k (1h)	$0.44
documentation:docs-reviewer	sonnet	140.3k	10.3k	28.2k	112.0k (1h)	$0.84
harness-engineering:harness-architecture-reviewer	sonnet	178.4k	1.2k	17.0k	161.4k (1h)	$0.99
harness-engineering:harness-enforcement-reviewer	sonnet	197.4k	7.7k	28.2k	169.2k (1h)	$1.14
harness-engineering:harness-knowledge-reviewer	sonnet	189.7k	5.3k	28.2k	161.4k (1h)	$1.06
harness-engineering:harness-self-documenting-code-reviewer	sonnet	147.2k	6.4k	28.2k	119.0k (1h)	$0.82
security:security-code-auditor	sonnet	131.5k	2.0k	28.2k	103.3k (1h)	$0.66
discussion-summarizer	—	45.0k	798	0	45.0k (1h)	$0.47

_{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.}

[rianjs]

No blockers or majors found. The daemon edits did not introduce architectural drift: they strengthen test precision around help text and build-tag behavior, keep 1Password opt-in semantics intact, and preserve the CI/docs alignment for standard vs no-1Password builds.