Skip to content

fix: allow keychain reads without account#559

Open
rohithgoud30 wants to merge 1 commit into
robinebers:mainfrom
rohithgoud30:fix/keychain-optional-account
Open

fix: allow keychain reads without account#559
rohithgoud30 wants to merge 1 commit into
robinebers:mainfrom
rohithgoud30:fix/keychain-optional-account

Conversation

@rohithgoud30
Copy link
Copy Markdown
Contributor

@rohithgoud30 rohithgoud30 commented Jun 4, 2026

Description

Fixes the host keychain binding so host.keychain.readGenericPassword(service) works when plugins omit the optional account argument.

The plugin API documents account as optional, and several providers call the method with only service. The Rust binding now collects the account with Rest<Option<String>>, so one-argument calls reach the keychain implementation while existing two-argument calls keep the same account trimming behavior.

A regression test covers the one-argument JavaScript call so this does not come back.

Related Issue

Fixes #549.

Type of Change

  • Bug fix
  • New feature
  • New provider plugin
  • Documentation
  • Performance improvement
  • Other (describe below)

Testing

  • I ran cargo test keychain_read_generic_password_accepts_optional_account_arg_from_js --lib
  • I ran cargo test --lib
  • I ran git diff --check

Screenshots

No UI/layout changes, behavior fix only.

Checklist

  • I read CONTRIBUTING.md
  • My PR targets the main branch
  • I did not introduce new dependencies without justification

Summary by cubic

Allow keychain reads without an account by fixing the host keychain binding to accept single-argument calls to readGenericPassword(service). This restores plugin compatibility while keeping the existing account trimming behavior.

  • Bug Fixes
    • Updated rquickjs binding to accept Rest<Option<String>>, so one-arg calls reach the keychain implementation; empty or whitespace-only accounts still resolve to None.
    • Added a regression test for the single-argument JavaScript call.

Written for commit c337ea7. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Fixed keychain.readGenericPassword to properly handle single-argument function calls and accept an optional second parameter for enhanced flexibility.
  • Tests

    • Added test coverage for keychain password retrieval with single arguments.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f903a62-779a-4ba0-a259-04d344c31680

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5b337 and c337ea7.

📒 Files selected for processing (1)
  • src-tauri/src/plugin_engine/host_api.rs

📝 Walkthrough

Walkthrough

This PR fixes a keychain binding that was rejecting single-argument JavaScript calls. The keychain.readGenericPassword method now accepts an optional second argument for the account value, resolving the "2 where expected" error that prevented plugins from reading stored credentials.

Changes

Optional Account Argument for Keychain Read

Layer / File(s) Summary
Optional account argument implementation
src-tauri/src/plugin_engine/host_api.rs
The Rest import is added to support variadic/optional argument patterns. The keychain.readGenericPassword closure is updated to accept account_args: Rest<Option<String>>, extracting and trimming the optional account value from the Rest container before passing it to the macOS security find-generic-password invocation.
Single-argument call validation
src-tauri/src/plugin_engine/host_api.rs
A unit test verifies that the binding can be called from JavaScript with only the required service argument, confirming single-argument invocations no longer trigger argument-count errors.

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: making the keychain account parameter optional, which directly addresses the core issue.
Linked Issues check ✅ Passed The PR fully addresses the linked issue #549 by fixing the argument-count mismatch that prevented plugins from calling readGenericPassword with only the service argument.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the keychain binding to accept optional account arguments, with no extraneous modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the rust Pull requests that update rust code label Jun 4, 2026
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

Re-trigger cubic

@validatedev
Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to read GitHub CLI credentials from Keychain

2 participants