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

[adriannoes]

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.

[gbrlcustodio]

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

[adriannoes]

@gbrlcustodio — thanks again for the earlier approval and the non-blocking notes.

I missed the review comments at the time; just caught up and addressed them after rebasing onto current dev:

• Docs: qualified the macOS ACL note so uvx path churn vs stable installs (uv tool install / wheel) is clear; aligned packages/mcp/README.md with the same diagnosis.
• Tests: one row per platform with required / forbidden fragments in test_auth_keychain_hints.py.
• Naming: renamed the helper to _auth_keychain_hints.py to match the commands/ convention.

Also confirmed the change is still needed on dev: the post-login store_session failure path still surfaces the Linux Secret Service hint on every OS, so #235 remains open.

CI is green after the rebase. Could you take another look when you have a moment? Happy to adjust anything.

[adriannoes]

Follow-up after rebase

Rebased onto current dev (clean). Unit tests for the keychain hints / login path still pass locally.

Coordination with sibling PRs

Holding further Windows-facing changes here until:

• fix(auth): add dpapi keychain backend for oversized Windows sessions #409 (dpapi / WinError 1783) — owns the Windows Credential Manager blob-cap diagnosis and the PIPEFY_KEYCHAIN_BACKEND=dpapi remediation. Review feedback on that PR already covers aligning the post-store_session hint (ideally into _auth_keychain_hints.py once this lands, or equivalent).
• fix(cli): don't render a non-expiring refresh token as expired #425 (refresh_expires_in: 0 status readout) — independent functionally; will resolve any doc/auth.py merge noise after it merges.

This PR stays scoped to macOS (errSecParam / Terminal.app / Always Allow) + Linux (Secret Service) platform hints and the _auth_keychain_hints.py extraction. After #409 and #425 land, we can soft-touch the Windows branch of the helper (or leave that entirely to #409) and re-request review.

[mocha06]

Verification first: this branch merges cleanly into current dev (it's behind, but no conflicts), 52 passed on the merged tree, ruff check / ruff format clean. The refactor itself — extracting the hint into a testable keychain_store_failure_hint(backend=...) helper — is the right call, and the docs correctly name Keyring as the macOS backend class. The new module's top-level from pipefy_infra.config import config_dir costs nothing (already loaded transitively via pipefy_cli.auth), and main.py imports commands explicitly, so the new _auth_keychain_hints.py can't disturb the CLI help golden.

The problem is the diagnosis the PR commits to.

1. -25244 is not errSecParam (blocking)

$ security error -25244
Error: 0xFFFF9D64 -25244 Invalid attempt to change the owner of this item.   # errSecInvalidOwnerEdit
$ security error -50
Error: 0xFFFFFFCE -50 One or more parameters passed to a function were not valid.  # errSecParam

The PR puts errSecParam, -25244 into a user-facing CLI string (_MACOS_HINT) and asserts it as fact in docs/cli/auth.md: "That code is errSecParam from Security.framework". Both are wrong.

The label is pre-existing elsewhere (README.md, docs/mcp/README.md, packages/mcp/README.md:33, skills/onboarding/pipefy-toolkit-setup/SKILL.md:93), so it isn't a regression introduced here — but this PR promotes it from a section heading into a stated diagnosis plus CLI output, and #235 is precisely "platform-aware error messaging", so this is the right place to correct it. A user who greps errSecInvalidOwnerEdit finds relevant material; errSecParam sends them nowhere.

2. The stated mechanism doesn't match the error code, and the remediation may not work

"Cannot surface the new-item ACL dialog … non-TTY subprocesses" is the semantics of errSecInteractionNotAllowed (-25308) — "User interaction is not allowed." Not -25244, which is about changing an item's owner.

That distinction matters because of how keyring actually writes (keyring/backends/macOS/api.py):

def set_generic_password(name, service, username, password):
    with contextlib.suppress(NotFound):
        delete_generic_password(name, service, username)   # <-- destroys the existing item
    ...
    status = SecItemAdd(q, None)                            # <-- creates a brand-new one

Every store_session deletes and re-adds the item. ACL trust is therefore per-item and cannot survive a rewrite by a different binary — and deleting an item created by another binary is exactly the situation "invalid attempt to change the owner" describes. Read that way, logging in once from Terminal.app plausibly creates the condition that makes the IDE's next write fail, rather than fixing it.

Worth weighing too: the packages/mcp/README.md text this PR replaces recorded repro evidence pointing away from the ACL theory — "direct keyring.set_password calls from the same uv-tool-installed Python succeed under repro testing, so this is likely a transient Security.framework condition rather than a deterministic per-binary ACL problem." The PR swaps that hedge for a confident diagnosis without presenting new evidence, and the discarded evidence contradicts it: a "non-TTY can't prompt" failure would fail on direct calls too, not succeed.

Suggestion: keep the platform branching, but stop asserting the mechanism. And add the remediation that actually fits a stale item owned by another binary — pipefy auth logout (delete_session → keyring.delete_password), or security delete-generic-password -s pipefy if the delete itself is what fails — then re-login. It's cheap, already exists in the CLI, and is absent from the PR entirely.

3. The macOS branch drops both escape hatches and the docs link

Today a macOS user hitting this gets the shared hint naming PIPEFY_KEYCHAIN_BACKEND=file and PIPEFY_TOKEN. After this PR, _MACOS_HINT offers only the Terminal.app procedure — no fallback, no docs pointer — on the single platform the PR exists to serve. Linux and Windows keep both. DOCS_CLI_AUTH_REF only lands on the _GENERIC_HINT branch, even though appending See {DOCS_CLI_AUTH_REF}. is the established convention in this exact module (commands/auth.py:270, :499, pipefy_cli/auth.py:183).

The PR's own test encodes the loss:

if system == "Linux":
    assert "PIPEFY_KEYCHAIN_BACKEND=file" in result.stderr

Put the docs ref on all four branches and keep the file-backend / PIPEFY_TOKEN fallback on macOS — especially since, per #2, the Terminal.app step may not resolve it.

4. Minor

• docs/cli/auth.md: "The remediation procedure is the same regardless of install method." is immediately followed by a paragraph on how uvx differs. Pick one. Separately, "macOS binds the keychain ACL to the calling Python binary's path" — ACL trust keys on the code signature / designated requirement (effectively path+hash for unsigned binaries), so "path" is a simplification. Both sentences likely disappear if #2 is addressed.
• No CHANGELOG entry. The repo maintains a detailed [Unreleased] section per change, and this alters user-facing CLI output plus three docs surfaces.
• Nit: import platform mid-function in test_auth_login.py (module-level in the sibling test file), and the if system == "Linux" conditional assert would read better as a third parametrize column.

Bottom line

The structure is good and should land. The blocker is that the PR replaces an honestly-hedged "not yet diagnosed" with a confident diagnosis built on a misidentified error code, and ships that into a user-facing string. Fix the code name, soften or re-ground the mechanism, add auth logout as the matching remedy, and restore macOS's escape hatches.

[adriannoes]

Thanks — verified locally with security error -25244 / -50 / -25308 and addressed the four points:

1. CLI + docs now name errSecInvalidOwnerEdit (-25244) (and stop calling it errSecParam).
2. Softened the mechanism: we no longer claim a non-TTY ACL-dialog failure. Hint + docs lead with clearing a stale item (pipefy auth logout, fallback security delete-generic-password -s pipefy), then Terminal.app / Always Allow as a follow-up step, with an explicit “not fully pinned” hedge in docs/cli/auth.md.
3. Restored PIPEFY_KEYCHAIN_BACKEND=file / PIPEFY_TOKEN and See docs/cli/auth.md on every platform branch (and PlaintextKeyring).
4. CHANGELOG [Unreleased] Fixed entry; docs contradiction cleaned up; login test uses module-level platform + a require_file_backend parametrize column.

Deferred (intentionally): no Windows dpapi / WinError 1783 diagnosis in this PR. That lives in #409; once this lands we’ll point _WINDOWS_HINT at that remediation there so we don’t fight the same auth.py / docs hunk twice.

Could you take another look?

[mocha06]

All four points addressed, and addressed properly rather than textually. Verified on the merged tree: 72 passed, ruff check / ruff format clean.

Specifically good:

• errSecInvalidOwnerEdit (-25244) propagated to all five surfaces (README.md, docs/mcp/README.md, packages/mcp/README.md, skills/onboarding/pipefy-toolkit-setup/SKILL.md, docs/cli/auth.md). The only two remaining errSecParam mentions are deliberate — the docs disambiguation (— not errSecParam (-50)) and a forbidden fragment in test_auth_keychain_hints.py, which is a nice regression guard.
• The mechanism paragraph now captures the delete-then-add behaviour accurately and carries an explicit hedge ("the root cause is not fully pinned; treat the steps below as remediation, not a proven mechanism").
• Dropping "binds the ACL to the calling Python binary's path" in favour of "stable Python binary path … cross-binary ownership mismatch" sidesteps the code-signature-vs-path overclaim without my having raised it.
• _ESCAPE_HATCH shared across all four platform branches and the PlaintextKeyring branch, with DOCS_CLI_AUTH_REF on every one, asserted per-platform in tests.

One new item, arising from the remediation that was added.

pipefy auth logout does not clear the entry in the case it's prescribed for

The new hint and docs/cli/auth.md step 1 both lead with pipefy auth logout to clear the stale item. That command early-returns before ever attempting the delete (commands/auth.py):

session = load_session(issuer=issuer, client_id=client_id)
if session is None:
    typer.echo("Not signed in. Nothing to do.")
    return                                   # <-- delete_session is never reached

load_session returns None on KeyringError and on ValidationError (packages/auth/src/pipefy_auth/storage.py:158-175) — that is, "an item exists but is unreadable, or holds a stale-schema blob," which is precisely the stale-entry case the hint targets. Confirmed against the real CLI with a keyring stub whose get_password raises KeyringError:

EXIT: 0
STDOUT: Not signed in. Nothing to do.
DELETE ATTEMPTED: []

Exit 0, no delete attempted, and a message that reads as though the keychain is already clean.

The existing hedge doesn't catch it. Both the CLI hint ("If logout cannot delete the item…") and docs step 2 ("If logout cannot delete the item…") trigger on logout failing — but here logout succeeds while doing nothing. A user follows step 1, sees "Nothing to do.", and never reaches security delete-generic-password.

Cheapest fix, in scope here — make the fallback trigger match the actual output:

Clear the entry with pipefy auth logout; if it reports Not signed in. Nothing to do. (or fails), remove it directly with security delete-generic-password -s pipefy.

The better fix — letting auth_logout still attempt delete_session when load_session returns None, since revocation is impossible without a refresh token anyway — is a behaviour change to logout and probably belongs in its own issue rather than here.

Minor

docs/cli/auth.md: "A dedicated encrypted Windows backend for oversized sessions is tracked separately." — a dangling forward reference in a standing doc. Either link #409 or drop the sentence.

Branch is behind dev

GitHub is showing this as conflicting, but that appears to be a stale mergeability computation rather than a real conflict: its cached base.sha is 895b9aac, which is now an ancestor of the dev tip (d40f0eb), and the head (f80f6375) merges cleanly into both locally with zero conflicted paths. Refreshing the branch onto current dev should clear the banner and re-run CI against the real base — merging dev in, as you did in f80f6375, keeps the incremental diff reviewable, so no need to rebase and force-push.

[adriannoes]

Thanks — follow-up addressed:

1. Logout fallback wording: CLI hint, docs/cli/auth.md, and packages/mcp/README.md now trigger the security delete-generic-password -s pipefy step when logout reports Not signed in. Nothing to do. (or fails), matching the real early-return path.
2. Windows dangling sentence: dropped from docs/cli/auth.md (no forward ref to the draft fix(auth): add dpapi keychain backend for oversized Windows sessions #409).
3. Branch refreshed onto current dev (merge, no force-push).

Deferred to its own issue: changing auth_logout to still attempt delete_session when load_session returns None — tracked in #508.

Could you take another look?

[mocha06]

Approving. All three rounds of feedback are resolved, and the fixes are consistent across every surface rather than patched at the one place I pointed at.

Verified on the merged tree: 72 passed, ruff check packages/cli/ clean, branch now MERGEABLE (which confirms the earlier conflict banner was a stale GitHub mergeability computation, not a real conflict).

What I checked on this round:

• Logout fallback wording — _MACOS_HINT, docs/cli/auth.md step 1, and packages/mcp/README.md all now trigger the security delete-generic-password -s pipefy step on the output logout actually produces (Not signed in. Nothing to do.) rather than on logout failing. The docs go a step further than suggested and explain why the no-op happens, which is the right call for a reader who hits it cold. Both fragments are pinned as required in test_auth_keychain_hints.py, so the wording can't drift back silently.
• dev merge resolved correctly — checked specifically for accidental reverts. dev had independently rewritten the README install prose; bc83502e keeps dev's newer wording and retains the errSecInvalidOwnerEdit rename, matching what the commit message claims. Net diff against dev is 10 files with nothing unintended.
• #508 scoping — right call keeping the auth_logout behaviour change out of here. I've added a cleanup checklist to that issue: landing it invalidates the remediation text in _auth_keychain_hints.py, docs/cli/auth.md, and packages/mcp/README.md, and only the test assertions will fail loudly, so the docs/hint update is worth folding into #508's completion contract rather than leaving as a follow-up.

Nothing blocking. Good result on the error-code correction in particular — that one was shipping in a user-facing string across five surfaces.