Skip to content

feat: Fix keychain issues and add fs fallback#10

Merged
jpage-godaddy merged 29 commits into
mainfrom
fix-keyring
Jun 1, 2026
Merged

feat: Fix keychain issues and add fs fallback#10
jpage-godaddy merged 29 commits into
mainfrom
fix-keyring

Conversation

@jpage-godaddy
Copy link
Copy Markdown
Collaborator

@jpage-godaddy jpage-godaddy commented May 29, 2026

  • Add missing features for keyring to make it actually work instead of being a no-op (grr)
  • Allow option for storing token in a file as a fallback for keychain issues (disabled by default)
  • Add ability to get debug logging

@jpage-godaddy
feat: Fix keychain issues and add fs fallback
3bb3cf5 
- Add missing features for `keyring` to make it actually work instead of being a no-op (grr)
- Allow option for storing token in a file as a fallback for keychain issues (disabled by default)
- Add ability to get debug logging
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 18:13
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to make PKCE auth token persistence reliable by fixing keychain backend configuration and adding an opt-in file-based fallback for environments where the system keychain is unavailable, along with additional debug/warn logging around keychain operations.

Changes:

  • Add keyring OS-specific feature flags so keychain storage is functional on Linux/macOS/Windows.
  • Add an opt-in file fallback for token storage when keychain operations fail.
  • Add tracing logs around keychain read/write failures and fallback usage.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.

File Description
src/auth/pkce.rs Implements file fallback storage/removal and adds keychain error logging.
Cargo.toml Configures keyring with OS-specific backend features.
Cargo.lock Updates lockfile for new/changed transitive dependencies pulled in by keyring features.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 19:04
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.

Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs Outdated
Comment thread Cargo.toml Outdated
Comment thread Cargo.toml Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 19:24
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 19:59
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/auth/pkce.rs Outdated
Comment thread Cargo.toml
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 21:11
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.

Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 21:19
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 21:31
Copilot AI reviewed May 29, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot May 29, 2026 21:42
Copilot AI reviewed May 30, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/auth/pkce.rs Outdated
Comment thread src/auth/pkce.rs Outdated
Comment thread .github/workflows/ci.yml
@jpage-godaddy @claude
fix: harden delete path diagnostics, self-heal corrupt keychain entry…
41df082 
…, fix CI doc tests

- delete_token_from_keychain: clone service before closure so the
  JoinError warning includes the service name; log WARN when
  Entry::new fails (consistent with read/write helpers)
- load_token_from_keychain: best-effort delete the keychain entry
  when its JSON is corrupt, preventing repeated warnings on every run
- ci.yml: add --features pkce-auth to the Doc Tests step so the
  pkce module's no_run example is compile-checked in CI

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jpage-godaddy jpage-godaddy requested a review from Copilot May 30, 2026 02:07
Copilot AI reviewed May 30, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

@jgowdy-godaddy jgowdy-godaddy mentioned this pull request Jun 1, 2026
Merged
4 tasks
@jgowdy-godaddy
fix: address pkce auth review findings (#11)
f941638 
## Summary

Addresses all findings and test gaps from the deep review of #10.

- **Finding 1** — Log WARN when the best-effort self-heal delete of a
corrupt keychain entry fails, matching the diagnostic pattern in
`delete_token_from_keychain`. Previously the error was silently
discarded, causing the corrupt-entry warning to fire on every subsequent
load.
- **Finding 2** — Fix `SAFETY` comment in `EnvVarGuard::drop` to state
the invariant directly (`XDG_MUTEX is held`) without referencing
`with_xdg_config_home` specifically, since the mutex is also acquired
directly in some tests.
- **Finding 4** — Reject leading space and `DEL` byte (`0x7F`) in
`is_safe_path_component`, mirroring the existing trailing-space
rejection. A leading space produces an invisible path component in
directory listings.
- **Finding 5** — Move `json` into the keychain write closure and return
it back, so only one serialized copy of the token material is on the
heap at a time in the normal path (eliminates the redundant
`json.clone()`). On task panic/cancel, re-serialize as a fallback.
- **Finding 6** — Update `list_environments` doc comment to explicitly
note that file-fallback tokens from a prior session are not enumerated,
not just keychain tokens.
- **Test gap** — Add `is_safe_path_component_rejects_empty_string`,
`is_safe_path_component_rejects_leading_space_and_del`,
`list_environments_returns_only_cached_keys`, and
`list_environments_returns_empty_without_cache` tests.

## Test plan

- [ ] `cargo fmt --all --check` passes
- [ ] `cargo clippy --all-targets --features pkce-auth -- -D warnings`
passes
- [ ] `cargo test --all-targets --features pkce-auth` passes (229 tests)
- [ ] `RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --features
pkce-auth` passes
@jgowdy-godaddy
ci: lint docs with pkce-auth feature enabled (#12)
f14c6f5 
## Summary

- The `Docs` CI step ran `cargo doc --no-deps` without `--features
pkce-auth`. The `Doc Tests` step already ran both; the doc-build step
did not. This meant broken intra-doc links or missing-docs violations in
feature-gated items would not be caught.
- Run the doc build twice: once without and once with `--features
pkce-auth`.

## Test plan

- [ ] CI Docs step passes with both invocations green
@jgowdy-godaddy
Copy link
Copy Markdown
Collaborator

jgowdy-godaddy commented Jun 1, 2026

Two follow-up branches were pushed to address findings from a deep review:

Merged (#11): src/auth/pkce.rs corrections

  • Self-heal corrupt keychain entry now logs WARN if the delete fails, instead of silently dropping the error
  • save_token_to_keychain moves json into the keychain closure and returns it back — avoids a redundant clone so only one serialized copy of the token is on the heap at a time
  • is_safe_path_component now also rejects leading space and DEL (0x7F), mirroring the existing trailing-space rejection
  • list_environments doc comment updated to note that file-fallback tokens from a prior session are also not enumerated (not just keychain tokens)
  • SAFETY comment in EnvVarGuard::drop corrected to state the invariant directly rather than referencing with_xdg_config_home specifically
  • Added tests: empty-string rejection, leading-space/DEL rejection, and list_environments cache-only behavior

Open (#12): .github/workflows/ci.yml

  • Docs step now runs cargo doc twice — once without and once with --features pkce-auth — so feature-gated items are included in the doc-warning check (the Doc Tests step already did this; the build step did not)

@jpage-godaddy jpage-godaddy requested a review from Copilot June 1, 2026 17:00
Copilot AI reviewed Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

Comment thread src/auth/pkce.rs
@jpage-godaddy jpage-godaddy requested a review from Copilot June 1, 2026 17:23
Copilot AI reviewed Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot June 1, 2026 18:54
Copilot AI reviewed Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.

Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs
Comment thread src/auth/pkce.rs Outdated
@jpage-godaddy jpage-godaddy requested a review from Copilot June 1, 2026 19:20
Copilot AI reviewed Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

@jpage-godaddy jpage-godaddy merged commit 853a98a into main Jun 1, 2026
2 checks passed
@jpage-godaddy jpage-godaddy deleted the fix-keyring branch June 1, 2026 19:43
@github-actions github-actions Bot mentioned this pull request May 29, 2026
Merged
jpage-godaddy pushed a commit that referenced this pull request Jun 1, 2026
@github-actions
chore(main): release cli-engine 0.1.2 (#8)
bbcaaf1 
🤖 I have created a release *beep* *boop*
---


##
[0.1.2](cli-engine-v0.1.1...cli-engine-v0.1.2)
(2026-06-01)


### Features

* Allow hard-coded redirect URL
([#9](#9))
([e24dc24](e24dc24))
* Fix keychain issues and add fs fallback
([#10](#10))
([853a98a](853a98a))


### Bug Fixes

* Allow Ctrl+C to work while waiting on OAuth flow
([#7](#7))
([2b6d10e](2b6d10e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jgowdy-godaddy jgowdy-godaddy jgowdy-godaddy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jpage-godaddy @jgowdy-godaddy