Address review feedback on secret store (PR #531)

[GregKinne]

Review remediation for PR mpfaffenberger#531 — secret store

This branch (fix/pr531-review) addresses every review finding on PR #531 as a series of focused, individually reviewable commits. All 11 findings are resolved; pytest tests/test_secret_store.py passes (71 passed) and ruff is clean.

Findings and responses

#	Severity	Finding	Commit	Our response
F1	Blocker	Windows fallback is plaintext; chmod(0o600) doesn't give owner-only NTFS DACL	c4c7bfd5	The original wording overpromised on Windows. The hardening now branches by platform: POSIX uses 0o600, and Windows applies an explicit owner-only NTFS DACL via icacls (/inheritance:r then /grant:r <user>:F, no new dependency), otherwise it warns that the fallback is plaintext. The warning copy is platform-aware, and the Windows permission test no longer skips; it asserts the DACL, or the explicit plaintext warning when ACLs are unavailable.
F2	Blocker	Fallback lookup bypasses the configured service namespace	13a18b03	Fixed. The fallback document is now nested by service name, so configure_service_name() isolates the file tier the same way it isolates keyring entries. Added a cross-service isolation test showing distribution B cannot read, overwrite, or delete distribution A's fallback secret.
F4	Blocker	set_secret() ignores _write_fallback() failure and returns normally after losing the credential	9e91e9ad	Fixed. set_secret() now checks the _write_fallback() result and raises a SecretStoreError when persistence fails, so a full or read-only filesystem can no longer report success. Added a test that forces the write to fail and asserts the error propagates.
F5	High	Fallback read-modify-write is unlocked — concurrent writers lose updates	202b31f7	Fixed. Fallback mutations now run under a cross-platform advisory lock (fcntl/msvcrt) covering the full read-modify-write, so concurrent processes no longer overwrite each other's updates. Added a concurrent-writers test that spawns competing writers and asserts no updates are lost.
F6	High	Successful keyring write never scrubs fallback — stale plaintext resurrection	202b31f7	Fixed. A successful keyring set now scrubs any stale fallback entry (under the F5 lock), so rotated secrets are not left in plaintext or read back from the fallback. Added a test asserting the fallback entry is gone after a healthy keyring set.
F7	High	Chunked overwrite isn't crash-safe and has no cross-process lock on Windows	117a47f1	Reworked. Chunked writes now use generation-numbered entries (name:cp::) and commit by flipping a single pointer key, then GC the old generation. The old value stays readable until the atomic flip, so a crash mid-overwrite cannot produce a partial value. Added crash-mid-overwrite and shrink tests.
F9	High	':cp:' reserved namespace isn't enforced — secret names can shadow/destroy chunk metadata	e1e2dfcd	Fixed. Secret names are now validated through a single helper that rejects the reserved ':cp:' substring with a ValueError, closing the shadow and destroy collision paths. Added tests for the count-marker shadow, the delete-a-different-secret case, and the chunk-0 overwrite.
F10	Medium	delete_secret() also ignores _write_fallback() return (worse: 'delete' lies on read-only FS)	9e91e9ad	Fixed. delete_secret() now honors the _write_fallback() return and raises when the scrub fails, so a read-only or full filesystem can no longer report a successful delete while the plaintext secret survives. Added a delete-failure-propagation test.
F3	Medium	Inconsistent fallback failure contract (mkstemp OSError escapes; other failures return False)	9e91e9ad	Tightened. mkstemp() is now inside the try/except so _write_fallback has a single, consistent False-on-failure contract. Call sites now honor that return (see F4/F10).
F8	Medium	Empty value = false-alarm warning + silent no-op; .strip() silently mutates secrets	9de46228	Fixed. Empty and whitespace-only values now raise ValueError at the API boundary instead of being reported as a backend failure. We also stopped .strip()-ing secret payloads, so credentials with leading or trailing whitespace round-trip byte-for-byte; stripping is confined to internal metadata (the chunk-count marker).
F11	Low	warnings.warn is invisible in the TUI and deduped by the default filter	e6df57d5	Fixed. User-facing notices (headless-fallback active, keyring-write-failed) now go through the messaging bus so they render in the TUI instead of being suppressed by the warnings filter. The once-per-process dedupe is preserved.

Commit series

117a47f1 fix(secret-store): generation-numbered crash-safe chunking (F7)
e6df57d5 fix(secret-store): route user-facing notices through the messaging bus (F11)
c4c7bfd5 fix(secret-store): honest owner-only hardening on Windows, not false chmod (F1)
13a18b03 fix(secret-store): namespace the fallback file by service name (F2)
202b31f7 fix(secret-store): lock fallback RMW + scrub stale plaintext on healthy set (F5/F6)
9e91e9ad fix(secret-store): uniform fallback failure contract, surfaced to callers (F3/F4/F10)
9de46228 fix(secret-store): stop stripping secrets; reject empty values loudly (F8)
e1e2dfcd fix(secret-store): enforce reserved ':cp:' namespace on secret names (F9)