Repository navigation
Add: SecureState secret Keychain dogfood - #27
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
馃挕 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ea3794f84
鈩癸笍 About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 馃憤.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0xLeif
left a comment
0xLeif
left a comment
There was a problem hiding this comment.
Review from local verification (detached worktree at 9ea3794). Code is approve-worthy; merge is blocked on mechanics, not implementation.
What verified green locally
swift buildclean, 30/30 tests (incl. real-Keychain round-trip/reset/persistence),smoke.shpass (Darwin secret branch),fledge lanes run verifypassspecsync check --force(45/45 exports) andspecsync change check(CHG-0007 evidence exact) pass- AppState API conformance verified against the checkout:
secureState(initial:feature:id:), scope keydev.leif.aps/secretmatches the read-back check,reset(secureState:)deletes the item, all Security symbols#if canImport(Security)-gated withAPSError.keychainUnavailableon Linux. The Linux error path looks correct by inspection (set throws -> ValidationError -> non-zero; get/reset degrade cleanly).
Merge blockers (mechanics)
- CONFLICTING on 5 SpecSync artifacts vs current main:
.specsync/change-sequence.json(branch seq 7 vs main seq 9+) and the CHG-0004 ceremony JSONs (approvals/state/verification/verification-attempts). Fix: rebase onto main; take main'schange-sequence.jsonand CHG-0004 files as the resolution base, then re-run the ceremony to regenerate (renumber to the next free CHG id; do not hand-merge approval digests). - No CI evidence: zero check-runs on the head commit. All three workflows must report green.
Risks and nits
- Keychain on the self-hosted runner is unproven: the new smoke step
set secret "smoke-secret"fails the whole job if the runner's login Keychain is locked/inaccessible (read-backpersistenceFailed). Worth confirming the runner state before trusting the first green. - Test Keychain hygiene: Darwin tests write the real login Keychain item
dev.leif.aps/secret;setUpresets but there is notearDown, and the last secret test leaves"rotated"behind. Add atearDownreset for symmetry with the UserDefaults isolation that just landed in #22. - Process: reopening CHG-0003/CHG-0004 to amend GOAL.md scope is what inflates this diff and guarantees conflicts; GOAL scope edits would have been cheaper inside CHG-0007 alone.
Once rebased with CI green, this merges.
Wire demo key secret through AppState SecureState (Darwin Keychain), document Linux keychainUnavailable, and land SpecSync CHG-0012. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
9ea3794 to
991352d
Compare
There was a problem hiding this comment.
馃挕 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 991352df19
鈩癸笍 About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 馃憤.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "$bin" set secret "smoke-secret" >/dev/null | ||
| test "$("$bin" get secret)" = "smoke-secret" | ||
| "$bin" get secret --json | grep -q '"storage" : "SecureState"' | ||
| "$bin" reset secret >/dev/null |
There was a problem hiding this comment.
When this smoke path runs on Darwin and the fixed dev.leif.aps/secret Keychain account already contains a real value, the script overwrites it with smoke-secret and then deletes it. The surrounding smoke setup isolates FileState with a temp APS_HOME, but SecureState ignores that directory and uses the global account, so running ./Scripts/smoke.sh or the macOS CI smoke can erase a developer/runner's existing aps set secret value; save and restore the prior item or use an isolated test account.
Useful? React with 馃憤聽/ 馃憥.
1 participant
Summary
secretbacked by AppStateSecureState(Keychain accountdev.leif.aps/secret);resetdeletes the itemGOAL.mdand documents macOS / Linux / headless CI Keychain availability in READMETest plan
fledge lanes run verify(build + test + smoke)specsync checkpassessecretlisted inkeys;set secretfails with Keychain-unavailable (smoke branch)Made with Cursor