feat(credstore): Store/Open core + in-memory backend (INT-430)#4
Conversation
Add the Store lifecycle (Open/Close/Backend) and single-key operations (Get/Set/Delete/Exists) per the Secret-Handling Standard 2.1, with the 1.3 <service>/<profile>/<key> -> ServiceName + <profile>/<key> mapping and 1.5/1.5.2 overwrite + allowed-key enforcement. - Internal backend interface (get/set+overwrite/delete/exists/kind/close) so the later real OS backends slot in without reshaping these methods. - In-memory backend: no disk side effects, concurrency-safe, the 2.1 test/CI backend. Conditional no-overwrite write is atomic under the backend lock (ErrExists). - Fail closed: unset/OS backend selection returns ErrBackendNotImplemented rather than silently using memory. - AllowedKeys normalized at Open (syntax-checked, deduped, sorted); enforced on Set/Delete only; read paths syntax-validated, not gated. - Close is idempotent, best-effort clears values (no false zeroization claim); post-close ops return ErrStoreClosed; Backend() stays metadata. Closes #3 [INT-430]
|
Blocker None. Confirmed current CI run Major None. Minor
Nit None. Code Quality Lens: the implementation matches the converged INT-430 design. Backend selection fails closed instead of silently using memory; |
TDD Coverage AssessmentOverall the test suite is solid. All of the meaningful behaviors called out in the checklist are covered, and the tests assert real sentinel errors and specific field values rather than tautologies. A couple of minor gaps are worth addressing before merge. Gap
Minor
Nit
If a goroutine returns an unexpected error it is silently dropped. Since Out of scope (explicitly not flagged): OS keyring backends, bundle operations, redaction, migration helpers. |
Address PR review: - KeyError.Allowed is now copied on construction so a caller mutating it cannot corrupt the Store's future error messages (Codex). - TestSyntaxErrors now exercises Get/Delete/Exists too, not just Set — each validates independently via joinItemKey (TDD gap). - Round-trip asserts Exists is false after Delete (TDD). - TestConcurrentSetGet asserts no unexpected goroutine errors instead of discarding them (TDD nit). - Add TestMemoryBackendClosedPaths covering the backend's defensive nil-map branches directly (TDD minor). [INT-430]
|
Addressed review feedback in Codex
TDD assessment
Local: build, |
|
Verified latest PR head Blocker Major Minor Nit The PR now matches the converged INT-430 architecture: fail-closed backend selection, explicit memory-only path, backend-owned atomic no-overwrite write, normalized allowlist, Set/Delete-only allowlist enforcement, ungated reads, |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 76598ec
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 4 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 2 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-architecture-reviewer (4 findings)
credstore/store.go:225
Store.Close() sets s.closed = true before calling s.be.close(). If a future OS backend's close() returns an error, the store is permanently marked closed while backend resources may be leaked or in an undefined state — subsequent calls short-circuit on ErrStoreClosed with no retry path. Harmless for the current in-memory backend (always returns nil), but this ordering creates a forward-compatibility hazard that should be resolved before OS backends are wired in. Consider setting s.closed = true only after a successful s.be.close(), or documenting that a failed close leaves the store in a terminal state.
💡 Suggestion - credstore/store.go:119
(*KeyError).Is matches any *KeyError target regardless of which key was disallowed, making it impossible for callers to distinguish between specific *KeyError instances using errors.Is. While intentional for the ErrKeyNotAllowed sentinel, this behavior may surprise library consumers who attempt to create their own *KeyError sentinels for finer-grained error handling.
💡 Suggestion - credstore/store_test.go:203
TestSyntaxErrors builds a map[string]func() error and iterates with for op, fn := range ops. Map iteration order in Go is randomized, so when a subtest fails the operation name in the failure message may differ between runs, making reproduction and triage harder. Using an ordered slice of {name string, fn func() error} structs would give deterministic output consistent with the outer test's table-driven style.
💡 Suggestion - credstore/store_test.go:78
TestOpenBackendSelection covers BackendKeychain and BackendFile as not-implemented sentinels but omits BackendWinCred and BackendSecretService. All four are handled by the same switch arm today, so the risk is low, but any future per-backend divergence in that case block would go undetected.
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - credstore/store_test.go:263
TestAtomicPreWriteUnderContention serializes all goroutines on s.mu before reaching the backend, so the backend's own b.mu is never under contention in this test. The test correctly validates observable behavior (exactly one success), but the name and comment imply it exercises backend-level atomicity. A future maintainer removing the backend's own lock could believe this test would catch the regression when it would not.
💡 Suggestion - credstore/store_test.go:67
TestOpenInvalidAllowedKey only covers a space character ("bad key") in AllowedKeys. The normalizeAllowedKeys function has a separate branch for the empty-string case ([]string{""}) that should produce a RefError{Segment: "key", Kind: RefErrorEmpty}, but there is no test asserting this reachable code path.
harness-engineering:harness-knowledge-reviewer (2 findings)
💡 Suggestion - credstore/memory.go:20
memoryBackend uses sync.Mutex for all methods including read-only get() and exists(). Upgrading to sync.RWMutex would allow concurrent readers with no semantic change. This matters most for direct backend access (as in TestMemoryBackendClosedPaths); calls through Store are already serialized by s.mu.
💡 Suggestion - credstore/store.go:213
Store.Get() and Store.Exists() acquire s.mu exclusively, serializing all reads behind all writes. Both methods are read-only at the Store level; switching Store.mu to sync.RWMutex and using RLock for Get/Exists would allow concurrent reads without correctness risk.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - credstore/memory.go:73
memoryBackend.close() relies on 'for k := range nil' being a no-op to achieve idempotency after the map is set to nil — this is valid Go but non-obvious. An explicit 'if b.m == nil { return nil }' guard at the top of close() would make the intent self-evident and protect against future changes that add state requiring one-time clearing.
7 info-level observations excluded. Run with --verbose to include.
4 PR discussion threads considered.
Completed in 4m 12s | $0.91 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Reviewers | hybrid-synthesis, 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 12s wall · 10m 55s compute (Reviewers: 2m 20s · Synthesis: 1m 49s) |
| Cost | $0.91 |
| Tokens | 117.5k in / 39.3k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 28.5k | 6.2k | 13.6k | 14.8k (1h) | $0.16 |
| harness-engineering:harness-architecture-reviewer | sonnet | 17.7k | 6.7k | 2.1k | 15.6k (1h) | $0.17 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 17.7k | 8.2k | 2.1k | 15.6k (1h) | $0.19 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 17.7k | 6.6k | 2.1k | 15.6k (1h) | $0.17 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 17.8k | 5.4k | 2.1k | 15.6k (1h) | $0.15 |
| security:security-code-auditor | haiku | 18.0k | 6.0k | 0 | 18.0k (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.
| // metadata only — no error, and it remains valid after Close. | ||
| func (s *Store) Backend() (Backend, Source) { return s.kind, s.src } | ||
|
|
||
| // Close releases the backend and best-effort clears stored values. It is |
There was a problem hiding this comment.
🟡 Medium (harness-engineering:harness-architecture-reviewer): Store.Close() sets s.closed = true before calling s.be.close(). If a future OS backend's close() returns an error, the store is permanently marked closed while backend resources may be leaked or in an undefined state — subsequent calls short-circuit on ErrStoreClosed with no retry path. Harmless for the current in-memory backend (always returns nil), but this ordering creates a forward-compatibility hazard that should be resolved before OS backends are wired in. Consider setting s.closed = true only after a successful s.be.close(), or documenting that a failed close leaves the store in a terminal state.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in 9f7f977. Close() now calls be.close() first and only sets s.closed on success; a failed backend close returns the error and leaves the store retryable rather than terminally closed with leaked resources. In-memory close always succeeds so current behavior is unchanged; this is forward-compat hardening for the OS-backend unit.
|
|
||
| // Open returns a service-scoped Store. The service segment must satisfy | ||
| // the §1.3 ref grammar. Backend selection fails closed: only | ||
| // BackendMemory is implemented in this unit. |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): (*KeyError).Is matches any *KeyError target regardless of which key was disallowed, making it impossible for callers to distinguish between specific *KeyError instances using errors.Is. While intentional for the ErrKeyNotAllowed sentinel, this behavior may surprise library consumers who attempt to create their own *KeyError sentinels for finer-grained error handling.
Reply to this thread when addressed.
There was a problem hiding this comment.
By design, won't change. (*KeyError).Is matching any *KeyError mirrors the (*RefError).Is sentinel pattern established in INT-429 and documented on the type — ErrKeyNotAllowed is a class sentinel. Callers needing the specific disallowed key use errors.As and read KeyError.Key (demonstrated in TestAllowlistEnforcement). Per-instance sentinels are not a supported pattern here.
| if err := s.Close(); err != nil { | ||
| t.Fatalf("Close: %v", err) | ||
| } | ||
| if err := s.Close(); err != nil { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestSyntaxErrors builds a map[string]func() error and iterates with for op, fn := range ops. Map iteration order in Go is randomized, so when a subtest fails the operation name in the failure message may differ between runs, making reproduction and triage harder. Using an ordered slice of {name string, fn func() error} structs would give deterministic output consistent with the outer test's table-driven style.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in 9f7f977. TestSyntaxErrors now iterates an ordered []struct{name,fn} instead of a map, so the failing operation name is deterministic and consistent with the table-driven style.
| } | ||
| if err := s.Delete("default", "api_token"); err != nil { | ||
| t.Fatalf("Delete: %v", err) | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestOpenBackendSelection covers BackendKeychain and BackendFile as not-implemented sentinels but omits BackendWinCred and BackendSecretService. All four are handled by the same switch arm today, so the risk is low, but any future per-backend divergence in that case block would go undetected.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in 9f7f977. TestOpenBackendSelection now also covers BackendWinCred and BackendSecretService so any future per-backend divergence in that switch arm is caught.
| var wg sync.WaitGroup | ||
| for i := 0; i < 50; i++ { | ||
| wg.Add(2) | ||
| go func() { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): TestAtomicPreWriteUnderContention serializes all goroutines on s.mu before reaching the backend, so the backend's own b.mu is never under contention in this test. The test correctly validates observable behavior (exactly one success), but the name and comment imply it exercises backend-level atomicity. A future maintainer removing the backend's own lock could believe this test would catch the regression when it would not.
Reply to this thread when addressed.
There was a problem hiding this comment.
Addressed in 9f7f977 by correcting the comment: the test validates the observable Store-level no-overwrite contract (exactly one success), and now explicitly notes Store.mu serializes ops so it does not exercise the backend's own lock. The backend's defensive layer is covered structurally, not by contention here.
| if ok, err := s.Exists("default", "api_token"); err != nil || ok { | ||
| t.Fatalf("Exists before set = (%v,%v), want (false,nil)", ok, err) | ||
| } | ||
| if err := s.Set("default", "api_token", "v1"); err != nil { |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): TestOpenInvalidAllowedKey only covers a space character ("bad key") in AllowedKeys. The normalizeAllowedKeys function has a separate branch for the empty-string case ([]string{""}) that should produce a RefError{Segment: "key", Kind: RefErrorEmpty}, but there is no test asserting this reachable code path.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in 9f7f977. TestOpenInvalidAllowedKey is now table-driven and asserts both RefErrorInvalidChar ("bad key") and the previously-untested RefErrorEmpty branch ([]string{""}), both with Segment=key.
| func (b *memoryBackend) kind() Backend { return BackendMemory } | ||
|
|
||
| func (b *memoryBackend) get(itemKey string) (string, error) { | ||
| b.mu.Lock() |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): memoryBackend uses sync.Mutex for all methods including read-only get() and exists(). Upgrading to sync.RWMutex would allow concurrent readers with no semantic change. This matters most for direct backend access (as in TestMemoryBackendClosedPaths); calls through Store are already serialized by s.mu.
Reply to this thread when addressed.
There was a problem hiding this comment.
Won't change, by rationale. Credential operations are infrequent (init/refresh/read at command start), not a concurrent hot path; sync.Mutex keeps the backend simple and correct. RWMutex adds complexity for no measurable benefit (Simple/surgical lens). Revisit only if a real read-contention need appears.
| } | ||
|
|
||
| type setOptions struct{ overwrite bool } | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): Store.Get() and Store.Exists() acquire s.mu exclusively, serializing all reads behind all writes. Both methods are read-only at the Store level; switching Store.mu to sync.RWMutex and using RLock for Get/Exists would allow concurrent reads without correctness risk.
Reply to this thread when addressed.
There was a problem hiding this comment.
Won't change, same rationale as the memoryBackend RWMutex thread: Store-level reads are not a contended hot path for a credential CLI; the coarse sync.Mutex is intentional simplicity. No correctness issue; deferring speculative optimization.
| return ok, nil | ||
| } | ||
|
|
||
| // close best-effort clears values then drops the map. Go string secrets |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): memoryBackend.close() relies on 'for k := range nil' being a no-op to achieve idempotency after the map is set to nil — this is valid Go but non-obvious. An explicit 'if b.m == nil { return nil }' guard at the top of close() would make the intent self-evident and protect against future changes that add state requiring one-time clearing.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in 9f7f977. Added an explicit 'if b.m == nil { return nil }' guard at the top of memoryBackend.close() so idempotency is self-evident and robust to future state added to close().
Address daemon review: - Close() now calls be.close() first and only marks the store closed on success, so a future OS backend whose close() fails is not left in a terminal state with leaked resources and no retry path (Medium). - memory close() has an explicit nil-map guard for self-evident idempotency. - Tests: TestSyntaxErrors uses an ordered slice (deterministic op name on failure); TestOpenBackendSelection covers wincred + secret-service; TestOpenInvalidAllowedKey covers the empty-string key branch; the contention test comment no longer overclaims backend-lock coverage. [INT-430]
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 9f7f977 | Previous: 76598ec (incremental)
Approved with 5 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 2 |
| harness-engineering:harness-self-documenting-code-reviewer | 1 |
harness-engineering:harness-architecture-reviewer (2 findings)
💡 Suggestion - credstore/store_test.go:305
TestAtomicPreWriteUnderContention validates Store-level serialization but — as the corrected comment now acknowledges — never exercises memoryBackend.set's own b.mu under true concurrent access. Store.mu serializes all callers before the backend is reached. A complementary test that calls memoryBackend.set concurrently from multiple goroutines (bypassing Store) would close this gap before OS backends are wired in and the backend interface gains direct callers.
💡 Suggestion - credstore/store.go:231
joinItemKey (profile+key syntax validation) and checkAllowed are pure string operations with no shared-state dependency, yet both are called while s.mu is held in Get, Set, Delete, and Exists. Moving them before mutex acquisition would reduce lock hold time and improve throughput for stores with many concurrent callers without any correctness impact.
harness-engineering:harness-knowledge-reviewer (2 findings)
💡 Suggestion - credstore/store.go:100
ErrKeyNotAllowed is declared as a mutable package-level *KeyError variable. External callers can modify its Key or Allowed fields (e.g., credstore.ErrKeyNotAllowed.Key = "x"), which would not affect errors.Is behavior (Is() only type-checks) but could corrupt error messages in downstream code that references the sentinel directly. Consider an unexported concrete type with a separate exported error value, or document that the sentinel fields must not be mutated.
💡 Suggestion - credstore/store.go:231
After Close() succeeds, s.be is retained rather than set to nil. All four operation methods guard on s.closed before reaching s.be, so there is no current bug. However, if a future subtype or test bypasses that guard it will silently call the already-closed backend rather than panicking loudly. Setting s.be = nil after be.close() returns nil would make misuse fail fast; Backend() is unaffected since it reads s.kind and s.src, not s.be.
harness-engineering:harness-self-documenting-code-reviewer (1 findings)
💡 Suggestion - credstore/store.go:116
ErrKeyNotAllowed is defined as &KeyError{} with an empty Key field. Its Error() method returns
credstore: key "" is not allowed, which is misleading if a caller ever logs or prints the sentinel directly rather than using it exclusively with errors.Is. A doc comment warning callers not to call ErrKeyNotAllowed.Error() directly, or an unexported zero-value struct type, would prevent the misleading output.
6 info-level observations excluded. Run with --verbose to include.
13 PR discussion threads considered.
Completed in 4m 12s | $1.30 | sonnet | daemon 0.2.116 | Glorfindel
| Field | Value |
|---|---|
| Model | sonnet |
| Mode | Re-review · Cycle 2 · Session resumed |
| Reviewers | hybrid-synthesis, 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 12s wall · 9m 08s compute (Reviewers: 1m 35s · Synthesis: 2m 10s) |
| Cost | $1.30 |
| Tokens | 229.1k in / 31.4k out |
| Turns | 6 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 40.1k | 7.3k | 13.6k | 26.5k (1h) | $0.21 |
| harness-engineering:harness-architecture-reviewer | sonnet | 18.2k | 4.6k | 2.1k | 16.0k (1h) | $0.14 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 18.2k | 5.0k | 2.1k | 16.0k (1h) | $0.15 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 18.2k | 4.4k | 2.1k | 16.0k (1h) | $0.14 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 18.2k | 4.7k | 2.1k | 16.1k (1h) | $0.14 |
| security:security-code-auditor | haiku | 18.5k | 3.8k | 0 | 18.5k (1h) | $0.05 |
| discussion-summarizer | — | 97.8k | 1.6k | 32.2k | 65.6k (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.
| s := openMem(t) | ||
| const n = 64 | ||
| var successes, exists int64 | ||
| var wg sync.WaitGroup |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): TestAtomicPreWriteUnderContention validates Store-level serialization but — as the corrected comment now acknowledges — never exercises memoryBackend.set's own b.mu under true concurrent access. Store.mu serializes all callers before the backend is reached. A complementary test that calls memoryBackend.set concurrently from multiple goroutines (bypassing Store) would close this gap before OS backends are wired in and the backend interface gains direct callers.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in a5ef968. Added TestMemoryBackendConcurrentSet: N goroutines call memoryBackend.set directly (bypassing Store.mu) so the backend's own b.mu is genuinely contended; asserts exactly one success and the rest ErrExists. Closes the gap before OS backends gain direct callers.
| // backing store, which is the best a Go library can do. | ||
| func (s *Store) Close() error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): joinItemKey (profile+key syntax validation) and checkAllowed are pure string operations with no shared-state dependency, yet both are called while s.mu is held in Get, Set, Delete, and Exists. Moving them before mutex acquisition would reduce lock hold time and improve throughput for stores with many concurrent callers without any correctness impact.
Reply to this thread when addressed.
There was a problem hiding this comment.
Won't change, by rationale (consistent with the other lock threads). Moving joinItemKey/checkAllowed outside s.mu is a throughput micro-opt for a non-contended path (credential ops run at command start, not in a request loop). Keeping the lock scope coarse and obvious is intentional simplicity; no correctness benefit to the change.
| delete(itemKey string) error | ||
| exists(itemKey string) (bool, error) | ||
| kind() Backend | ||
| close() error |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): ErrKeyNotAllowed is declared as a mutable package-level *KeyError variable. External callers can modify its Key or Allowed fields (e.g., credstore.ErrKeyNotAllowed.Key = "x"), which would not affect errors.Is behavior (Is() only type-checks) but could corrupt error messages in downstream code that references the sentinel directly. Consider an unexported concrete type with a separate exported error value, or document that the sentinel fields must not be mutated.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in a5ef968. Documented on ErrKeyNotAllowed that it is a type sentinel whose fields must not be mutated (the real key/allowed set live on the returned error). Mirrors the established RefError sentinel pattern from INT-429.
| // backing store, which is the best a Go library can do. | ||
| func (s *Store) Close() error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): After Close() succeeds, s.be is retained rather than set to nil. All four operation methods guard on s.closed before reaching s.be, so there is no current bug. However, if a future subtype or test bypasses that guard it will silently call the already-closed backend rather than panicking loudly. Setting s.be = nil after be.close() returns nil would make misuse fail fast; Backend() is unaffected since it reads s.kind and s.src, not s.be.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in a5ef968. Store.Close() now sets s.be = nil after a successful be.close(); future guard-bypass fails fast on nil deref instead of silently using a closed backend. Backend() is unaffected (reads s.kind/s.src).
| closed bool | ||
| be backend | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): ErrKeyNotAllowed is defined as &KeyError{} with an empty Key field. Its Error() method returns credstore: key "" is not allowed, which is misleading if a caller ever logs or prints the sentinel directly rather than using it exclusively with errors.Is. A doc comment warning callers not to call ErrKeyNotAllowed.Error() directly, or an unexported zero-value struct type, would prevent the misleading output.
Reply to this thread when addressed.
Address daemon run-2 review (all low): - Close() sets s.be = nil after a successful backend close so a future guard-bypass fails fast instead of using a closed backend. - Document that ErrKeyNotAllowed sentinel fields must not be mutated. - Add TestMemoryBackendConcurrentSet driving memoryBackend.set concurrently without Store.mu, so the backend's own lock is genuinely contended (the path later units exercise via direct callers). [INT-430]
|
Verified PR #4 latest head Blocker Major Minor Nit The final changes preserve the agreed architecture. |
…s doc Address pr-review-daemon Medium #4 (BackendFlagUsage mutable exported var) and Low #8 (allBackends mutability documentation): - BackendFlagUsage is now a function, not an exported var. External packages cannot accidentally overwrite the help text and silently corrupt every consumer's flag help. Downstream CLIs call it as credstore.BackendFlagUsage() instead of using it as a value. - allBackends doc now explicitly states the effective-const contract: never append, never reassign, never mutate. Helpers that need a mutable slice (ValidBackendNames) allocate their own copy.
…pers (#23) (#25) * feat(credstore): swap 99designs/keyring -> byteness/keyring + add --backend helpers Migrate cli-common/credstore from the unmaintained github.com/99designs/keyring v1.2.2 to the active fork github.com/byteness/keyring v1.9.3, and add a small public helper surface (BackendFlagName, BackendFlagUsage, ValidBackendNames, ParseBackend, BackendEnvVar, BindBackendFlag) that downstream CLIs use to wire a --backend flag and a keyring.backend config key with thin glue and consistent UX. Library swap is API-compatible (Config, BackendType strings, Item, PromptFunc, FixedStringPrompt, ErrKeyNotFound, Keyring interface methods all match). The swap also forces a godbus migration v0 -> v5: ByteNess v1.9.3 uses godbus/dbus/v5, and *dbus.Error is a different Go type between v0 and v5, so the errors.As plumbing in linuxfallback.go's dbusErrName had to be migrated to match or the Linux Secret-Service classification would have silently downgraded every dbus error to ssAmbiguous (fail-closed) and broken the headless-Linux file-fallback path. The Go directive auto-bumps 1.24 -> 1.26 because ByteNess v1.9.x requires it transitively (via 1password/onepassword-sdk-go, which ships as a WASM blob loaded via Extism). Downstream consumers (atlassian-cli, google-readonly, newrelic-cli, slack-chat-api) will need to bump their own go directive when they pick up this cli-common version. Backend name set is unchanged at the existing five (keychain, wincred, secret-service, file, memory); exposing pass / keepassxc / 1password as additional native backends is tracked in #24. The OP-family openers are registered in supportedBackends but never invoked because openOSBackend pins AllowedBackends to one of the five, and selectBackend rejects unknown names upstream. select.go now derives parseBackend, ValidBackendNames, and BackendFlagUsage from a single ordered slice (allBackends) so #24 only edits one list to expose new names. Closes #23 * fix(credstore): add flagSet to BindBackendFlag, fix §1.10 doc framing Address Codex PR review on #25: 1. BindBackendFlag signature gains a flagSet bool parameter so the helper can distinguish "flag never supplied" from "--backend=" (explicit empty). The latter now fails closed with ErrBackendNotImplemented rather than silently falling through to lower-precedence env/config/ auto selection. 2. docs/working-with-secrets.md §1.10: rephrase "The CLI itself does not know about 1Password" as a "default path" statement so #24's runtime 1Password backend exposure isn't a contradiction. The default recommendation stays installer-time translation via set-credential. 3. credstore/flag.go package comment qualifies which errors wrap ErrBackendNotImplemented (invalid backend values do; nil-opts programmer-error signal does not). Adds TestBindBackendFlag_FlagNotSet and TestBindBackendFlag_ExplicitEmptyFlag for the new flagSet contract. * test(credstore): add drift guard between Backend constants and allBackends TDD assessment on #25 flagged that flag.go's doc comment claims "the test that guards against drift" exists, but no such test does — all current tests iterate allBackends itself, so a future orphaned Backend constant would slip past CI. TestAllBackends_MatchesConstants pins both the length and the order of allBackends against the explicit constant list, and inversely asserts each expected constant parses via parseBackend. When #24 adds a new Backend, the test fails until both allBackends and the expected slice here are updated in lock-step. * ci: bump go to 1.26 and golangci-lint to v2.12.2 for ByteNess swap golangci-lint v2.0.2 was built with Go 1.24 and rejects any project whose go directive is >= 1.25. Since #23 bumps go.mod from 1.24 -> 1.26 (forced by byteness/keyring v1.9.3's transitive dep on onepassword-sdk-go which requires go 1.26), the lint job fails with: 'the Go language version (go1.24) used to build golangci-lint is lower than the targeted Go version (1.26)'. Bump both pins: go-version 1.24 -> 1.26 in actions/setup-go, golangci-lint v2.0.2 -> v2.12.2 (latest stable, built with Go 1.26). * refactor(credstore): BackendFlagUsage var -> func; tighten allBackends doc Address pr-review-daemon Medium #4 (BackendFlagUsage mutable exported var) and Low #8 (allBackends mutability documentation): - BackendFlagUsage is now a function, not an exported var. External packages cannot accidentally overwrite the help text and silently corrupt every consumer's flag help. Downstream CLIs call it as credstore.BackendFlagUsage() instead of using it as a value. - allBackends doc now explicitly states the effective-const contract: never append, never reassign, never mutate. Helpers that need a mutable slice (ValidBackendNames) allocate their own copy. * docs(credstore): update BackendFlagUsage callsite + tighten allBackends comment Address codex final-pass review on #25: - docs/working-with-secrets.md §1.1 now shows credstore.BackendFlagUsage() with parens so downstream CLIs (#385, #139, #104, #169) copy the correct call shape. - credstore/select.go allBackends comment now correctly notes that TestAllBackends_MatchesConstants has its own literal expected slice that must also be updated when adding a backend.
2 participants
Closes #3 · Jira INT-430 (child of epic INT-310). Builds on #2 (ref grammar).
Second discrete unit of
cli-common, per the Secret-Handling Standard §2.1 (API surface), §1.3 (mapping), §1.5/§1.5.2 (overwrite + allowlist).Surface
Store+Open(service, *Options)/Close()/Backend() (Backend, Source).Options{AllowedKeys, Backend};SetOptWithOverwrite(); single-keyGet/Set/Delete/Exists(profile, key).ErrNotFound/ErrExists/ErrStoreClosed/ErrBackendNotImplemented;*KeyError(ErrKeyNotAllowed);*RefErrorreused for profile/key/service syntax.Design
backendinterface (get/set+overwrite/delete/exists/kind/close) — these core methods don't reshape for the later real OS backends.ErrBackendNotImplemented(production must never silently use memory; real selection is a later unit).AllowedKeysnormalized atOpen(syntax-checked →*RefError{key}, deduped, sorted ⇒ deterministicKeyErrormessages); enforced onSet/Deleteonly — read paths syntax-validated, not gated (§1.5.2).Close: idempotent, best-effort value clearing (no false zeroization claim — Go strings); post-closeGet/Set/Delete/Exists→ErrStoreClosed;Backend()stays metadata.Tests (77 cases,
-race)Backend selection (incl. fail-closed + invalid service/allowed-key), round-trip, overwrite, delete/exists-missing, allowlist (sorted message, read paths ungated), syntax errors, Close contract, concurrent Set/Get, and the atomic pre-write contention contract (N goroutines → exactly one success, rest
ErrExists).Out of scope (later INT-310 units)
Real OS backends + auto/env/config selection,
ListBundle/DeleteBundle,SetBundleatomicity, Linux fail-closed, redaction, migration helpers,v0.1.0tag.Architect review
Codex (reused session) converged after one revision round (2 majors: widen backend iface with
close()+ atomic overwrite, tighten Close/Backend contract; 2 minors: normalize AllowedKeys, Exists-missing semantics) to blockers=0 majors=0; the remaining minor (contention test) and nit (interface-extension wording) were applied.