fix: post-merge audit remediation — §1.12 positional leak + §1.8 keychain fail-loud [INT-443]#95
Merged
Merged
Conversation
…hain fail-loud [INT-443] Post-merge Codex audit of INT-442 (#93) found defects the pre-merge daemon APPROVE missed (the codex-pass-batching loop-3 final pass was skipped): Blockers: - init / set-credential used cobra.NoArgs, which quotes args[0] in its error — a fat-fingered `nrq init NRAK-...` would echo the literal API key to stderr and logs. New root.NoPositionalArgs validator rejects with a static message that never contains the value. - keychainRead folded every `security` failure (timeout / locked / denied) into not-found, so a real legacy Keychain secret could be silently skipped, a divergence missed, and the original left behind. It is now tri-state and discover() fails loud on a non-not-found error so the one-time migration never runs on a partial view. Majors: - config clear --all now also scrubs the legacy macOS Keychain accounts (keychain.ScrubLegacyKeychain), closing the same restore-on-next-Open() gap the plaintext-file scrub closed. - the equal-multi-source idempotent path now records a change and signals once when legacy originals are scrubbed (deleting the user's plaintext file / Keychain item is itself a one-time migration side effect). - api.New() restored as a deprecated shim returning an actionable ErrNewRemoved (api/ stays pure) instead of a bare compile break. Minor/Nit: doc wording no longer overstates the file-backend posture ("never on disk" -> "never in plaintext, never in config.yml"); the init --overwrite example is no longer self-contradictory. Regression tests: positional-arg no-echo (both ingress commands), the scrub-only signal, and the updated discover() signature. Closes #94
…sage, security exec seam [INT-443] - root/init Long help no longer says "never on disk" (file backend is on disk, encrypted) — now "never plaintext, never in config.yml" - root.NoPositionalArgs message is backend-generic (shared by set-credential, which uses --stdin/--from-env, not --api-key-*) - extract a securityRun seam classifying exit-code/timeout so keychainRead tri-state and ScrubLegacyKeychain all-accounts behavior are unit-tested hermetically (the highest-risk §1.8 fail-loud paths) Codex re-review: blockers=0 majors=0 (endorsed the equal-source scrub signal as the correct §1.8 fix). These close the 2 minors + 1 nit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fast-follow for #93 (INT-442). A post-merge Codex audit (the codex-pass-batching loop-3 final pass that was skipped before merging #93) surfaced 2 blockers + 3 majors the pre-merge daemon APPROVE missed. Per maintainer directive this is a single quick PR — no TDD assessment, no daemon runs (build/test/lint gate + one Codex re-review only).
Tracking: INT-443 (child of INT-310), closes #94.
Blockers
initandset-credentialusedcobra.NoArgs, which formats its error asunknown command %q for %q—nrq init NRAK-…would echo the fat-fingered API key to stderr/logs. Newroot.NoPositionalArgsvalidator: static message, never contains the value. Regression test asserts no-echo on both commands.keychainReadtreated everysecurityfailure (timeout / locked Keychain / access denial) as "not found", so a real legacy secret could be silently skipped, a divergence missed, and the original left behind. Now tri-state;discover()propagates and the migration fails loud rather than running on a partial view.Majors
config clear --allnow also deletes the legacy macOS Keychain accounts (keychain.ScrubLegacyKeychain) — closes the same restore-on-next-Open()gap the run-3 plaintext-file scrub closed, for the Keychain vector.TestPlanMigration_EqualMultiSource_Idempotent(no deleters) still holds; a new test pins the deleters-present case.api.New()restored as a deprecated shim returning an actionableErrNewRemoved(keepsapi/import-pure) instead of a bare compile break for external importers of this pre-1.0 package.Minor / Nit
config.yml").init --overwritemigration-conflict example is no longer self-contradictory.Gate: gofmt · build · vet · 414
-racetests ·go mod tidyno-drift · golangci-lint v2.0.2 0 issues · go 1.24.0.