fix: avoid keychain secret reads for exists checks#65
Conversation
rianjs
force-pushed
the
fix/keychain-exists-metadata-probe
branch
from
9ab6162 to
d94ebb0
Compare
rianjs
force-pushed
the
fix/keychain-exists-metadata-probe
branch
from
d94ebb0 to
3d3f982
Compare
Automated PR ReviewReviewed commit: Summary
0 PR discussion threads considered. 0 summarized; 0 resolved. Completed in 25s | unavailable | gpt-5.4 | cr 0.10.196
Per-workstream usage
|
Automated PR ReviewReviewed commit: Summary
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 23s | unavailable | gpt-5.4 | cr 0.10.196
Per-workstream usage
|
Automated PR ReviewReviewed commit: Summary
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 28s | unavailable | gpt-5.4 | cr 0.10.196
Per-workstream usage
|
rianjs
force-pushed
the
fix/keychain-exists-metadata-probe
branch
from
b7b6229 to
ec3fd5f
Compare
Automated PR ReviewReviewed commit: Summary
Reviewer Coverage
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 51s | unavailable | gpt-5.4 | cr 0.10.197
Per-workstream usage
|
Automated PR ReviewReviewed commit: Summary
go:implementation-tests (1 finding)Major -
|
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | credstore/filebackend_nocgo_unix.go, credstore/osbackend_byteness.go, credstore/osbackend_byteness_test.go, credstore/osbackend_core.go, credstore/osbackend_test.go, credstore/passbackend_nocgo_unix.go, credstore/secretservice_core.go | unavailable | Review scope limited to the assigned credstore/ Go files and nearby package context.; Verified with go test ./credstore after redirecting GOCACHE, TMPDIR, and CLANG_MODULE_CACHE_PATH because the sandbox blocked the default cache locations. |
| unassigned | incomplete_unassigned | unavailable | .codereview/agents/go/implementation-tests/prompt.md | changed files were not assigned to a selected reviewer |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Note: This PR modifies reviewer definitions under
.codereview/agents/. The review was conducted using base-branch versions; changes will affect future reviews after merge.
Completed in 3m 25s | unavailable | gpt-5.4 | cr 0.10.197
| Field | Value |
|---|---|
| Model | gpt-5.4 |
| Reviewers | go:implementation-tests |
| Engine | codex_cli · gpt-5.4 |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 25s wall · 3m 17s compute |
| Cost | unavailable |
| Tokens | 712.0k in / 8.4k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.4 | 26.5k | 737 | 20.2k | unavailable | unavailable | 17s |
| go:implementation-tests | gpt-5.4 | 674.0k | 7.6k | 618.9k | unavailable | unavailable | 2m 55s |
| orchestrator-rollup | gpt-5.4 | 11.5k | 123 | 7.6k | unavailable | unavailable | 4s |
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
Automated PR ReviewReviewed commit: Summary
Reviewer Coverage
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 5m 43s | unavailable | gpt-5.4 | cr 0.10.197
Per-workstream usage
|
Automated PR ReviewReviewed commit: Summary
go:implementation-tests (1 finding)Minor -
|
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | credstore/filebackend_nocgo_unix.go, credstore/osbackend_byteness.go, credstore/osbackend_byteness_test.go, credstore/osbackend_core.go, credstore/osbackend_test.go, credstore/passbackend_nocgo_unix.go, credstore/secretservice_core.go | unavailable | Sandbox prevented executing go test because the configured Go build cache path was not writable (operation not permitted), so the review relied on source inspection and existing test code. |
| policies:conventions | complete_broad | credstore/filebackend_nocgo_unix.go, credstore/osbackend_byteness.go, credstore/osbackend_byteness_test.go, credstore/osbackend_core.go, credstore/osbackend_test.go, credstore/passbackend_nocgo_unix.go, credstore/secretservice_core.go | unavailable | Review limited to the assigned changed files and the visible repo-local/shared standards docs; this review focused on convention drift, copied policy, and missing durable docs/checks rather than general implementation correctness. |
| structure:repo-health | complete_broad | .codereview/agents/go/implementation-tests/prompt.md | unavailable | Scope limited to the assigned review surface .codereview/agents/go/implementation-tests/prompt.md; broader PR code changes were intentionally not reviewed by this agent. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Note: This PR modifies reviewer definitions under
.codereview/agents/. The review was conducted using base-branch versions; changes will affect future reviews after merge.
Completed in 4m 12s | unavailable | gpt-5.4 | cr 0.10.197
| Field | Value |
|---|---|
| Model | gpt-5.4 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.4 |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 4m 12s wall · 6m 32s compute |
| Cost | unavailable |
| Tokens | 968.8k in / 17.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | gpt-5.4 | 26.4k | 1.1k | 20.2k | unavailable | unavailable | 24s |
| go:implementation-tests | gpt-5.4 | 533.0k | 9.3k | 443.0k | unavailable | unavailable | 3m 10s |
| policies:conventions | gpt-5.4 | 308.5k | 4.8k | 263.2k | unavailable | unavailable | 1m 54s |
| structure:repo-health | gpt-5.4 | 77.8k | 2.2k | 59.9k | unavailable | unavailable | 46s |
| orchestrator-rollup | gpt-5.4 | 23.0k | 254 | 15.1k | unavailable | unavailable | 16s |
| return keyringItem{}, err | ||
| } | ||
| } | ||
| if md.Item == nil { |
There was a problem hiding this comment.
The new md.Item == nil branch is a backend-specific contract edge and the added tests never exercise its successful path. byteness/keyring's file backend returns Metadata{ModificationTime: ...} with a nil Item for existing entries, so if this branch regresses to errKeyringMetadataUnsupported the cgo/Windows file backend will silently fall back to Get, reintroducing a secret read/passphrase prompt on Exists without failing the current suite. Add a focused test that returns metadata with Item:nil and a non-zero ModificationTime, then assert exists returns true and Get is not called.
Reply inline to this comment.
1 participant
Summary
Store.Existsbefore falling back to full secret readsCloses #68
Release tag: v0.4.1
Verification
go test ./credstoreExistspath took about 6.3s, patched path about 10ms on the same stale keychain item