Skip to content

fix(cli): platform-specific keychain store hints after login (#235)#265

Open
adriannoes wants to merge 1 commit into
devfrom
fix/cli-macos-keychain-hint-235
Open

fix(cli): platform-specific keychain store hints after login (#235)#265
adriannoes wants to merge 1 commit into
devfrom
fix/cli-macos-keychain-hint-235

Conversation

@adriannoes

@adriannoes adriannoes commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add auth_keychain_hints.py with platform-specific remediation text when pipefy auth login succeeds at OAuth but store_session fails.
  • On macOS, surface errSecParam (-25244) and the Terminal.app + Always Allow ACL workaround instead of the headless Linux Secret Service hint.
  • Keep the existing Linux Secret Service / PIPEFY_KEYCHAIN_BACKEND=file hint and add a Windows Credential Manager variant.
  • Document the macOS first-run keychain ACL flow in docs/cli/auth.md.

Closes #235.

Branch error messages on macOS, Linux, and Windows when OAuth succeeds
but store_session fails. Document the macOS errSecParam (-25244) ACL
workaround in docs/cli/auth.md.
@adriannoes adriannoes requested a review from gbrlcustodio June 1, 2026 18:12
@adriannoes adriannoes self-assigned this Jun 1, 2026
Comment thread docs/cli/auth.md
The login worked but `keyring` couldn't write the entry.

When `PIPEFY_KEYCHAIN_BACKEND=file` is active the backend reports as `PlaintextKeyring` and the hint switches to a config-directory writability check (the file backend writes to `keyring.cfg` under the resolved config directory).
**macOS (Keychain / `Keyring` backend).** OAuth can succeed while persistence fails with `Can't store password on keychain: (-25244, 'Unknown Error')`. That code is `errSecParam` from Security.framework — common when the calling process cannot surface the new-item ACL dialog (IDE slash commands, agent hosts, and other non-TTY subprocesses that do not share an interactive Aqua session). The fix is independent of how `pipefy` was installed (`uvx`, `uv tool install`, or a wheel).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

suggestion (non-blocking): "Independent of how pipefy was installed" reads as "grant once, forever" to the reader who lands here after a failure. That holds for uv tool install and wheel installs (stable Python interpreter path, so the macOS keychain ACL persists), but uvx uses cached ephemeral environments whose binary path can change after uvx --refresh or a uv version bump. When the path changes, the existing Always Allow grant no longer applies and the next write surfaces the same errSecParam. Worth qualifying so uvx users don't conclude the workaround silently broke.

Suggested rewrite of the trailing sentence:

The remediation procedure is the same regardless of install method. macOS binds the keychain ACL to the calling Python binary's path, so uv tool install and a wheel install give you a stable grant; with uvx, the cached environment's binary path can change (for example after uvx --refresh or a uv version bump), and macOS will prompt again from Terminal.app.

Comment on lines +13 to +15
("Darwin", "errSecParam"),
("Darwin", "Terminal.app"),
("Darwin", "Always Allow"),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick (non-blocking): the three Darwin rows run the same call against the same patched platform.system three times to assert three substrings of one hint. The same shape leaves no symmetric check that the Linux or Windows hints don't accidentally pick up another platform's wording.

Consider collapsing to one row per platform with required/forbidden fragment lists:

@pytest.mark.parametrize(
    ("system", "required", "forbidden"),
    [
        ("Darwin", ["errSecParam", "Terminal.app", "Always Allow"], ["Secret Service"]),
        ("Linux", ["Secret Service"], ["Terminal.app", "Credential Manager"]),
        ("Windows", ["Credential Manager"], ["Secret Service", "Terminal.app"]),
        ("FreeBSD", ["pipefy auth status"], []),
    ],
)

The CLI test in test_auth_login.py already covers cross-platform leakage at the integration layer, so this is a readability cleanup, not a coverage gap.

@@ -0,0 +1,60 @@
"""Platform-specific hints when OS keychain session storage fails after OAuth."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nitpick (non-blocking): every other module in commands/ without a leading underscore registers a Typer app and is a public CLI surface; the one existing helper (_common.py) carries the underscore. This module is also a helper (one function, called only from auth.py), so the name is slightly inconsistent with the directory convention.

Consider renaming to _auth_keychain_hints.py and updating the two import sites (commands/auth.py and tests/test_auth_keychain_hints.py). The test filename can stay as-is; pytest discovery doesn't require name parity with the SUT.

Defer freely; this is a one-time naming nit, not a coverage or correctness concern.

@gbrlcustodio gbrlcustodio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Inline notes are all non-blocking; nothing here blocks merge.

Base automatically changed from dev to main June 2, 2026 14:39
@gbrlcustodio gbrlcustodio changed the base branch from main to dev June 2, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants