Skip to content

feat(auth): implement terminal authentication methods and checks#1056

Open
YoungY620 wants to merge 5 commits intomainfrom
acp-auth
Open

feat(auth): implement terminal authentication methods and checks#1056
YoungY620 wants to merge 5 commits intomainfrom
acp-auth

Conversation

@YoungY620
Copy link
Collaborator

@YoungY620 YoungY620 commented Feb 9, 2026

Related Issue

N/A

Description

Implement terminal authentication flow for ACP server, adding proper auth checks and AUTH_REQUIRED error handling.

Changes

  • Add _auth_methods instance variable to cache auth methods during initialization
  • Refactor initialize() to build terminal auth data with proper type, args, and env fields
  • Add _check_auth() method to verify OAuth token status and raise AUTH_REQUIRED error when needed
  • Add authentication checks in new_session() and load_session() before creating/loading sessions
  • Implement authenticate() method (previously raised NotImplementedError)

Checklist

  • I have read the CONTRIBUTING document.
  • I have linked the related issue, if any.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run make gen-changelog to update the changelog.
  • I have run make gen-docs to update the user documentation.

Copilot AI review requested due to automatic review settings February 9, 2026 04:05
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Copy link
Contributor

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 adds an ACP “terminal authentication” flow to ACPServer by advertising a login auth method, checking for a local OAuth token before allowing session creation/loading, and implementing the previously-stubbed authenticate() RPC.

Changes:

  • Cache and advertise ACP auth_methods during initialize() (terminal-based login).
  • Add _check_auth() and call it from new_session() / load_session() to raise AUTH_REQUIRED when not authenticated.
  • Implement authenticate() to validate the login method.

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

token = load_tokens(ref)

if token is None or not token.access_token:
# Build AUTH_REQUIRED error data per AUTHENTICATION.md spec
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

Comment mentions an AUTHENTICATION.md spec, but there is no such file in the repository. Please update the reference to an existing doc/location or remove the spec mention to avoid leaving misleading guidance.

Suggested change
# Build AUTH_REQUIRED error data per AUTHENTICATION.md spec
# Build AUTH_REQUIRED error data for clients

Copilot uses AI. Check for mistakes.
Comment on lines +313 to +316
ref = OAuthRef(storage="file", key=KIMI_CODE_OAUTH_KEY)
token = load_tokens(ref)

if token and token.access_token:
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

Same issue as _check_auth(): OAuthRef(storage="file", ...) prevents load_tokens() from checking/migrating keyring-stored credentials. Use storage="keyring" (or try both) so existing keyring users can authenticate without re-login.

Suggested change
ref = OAuthRef(storage="file", key=KIMI_CODE_OAUTH_KEY)
token = load_tokens(ref)
if token and token.access_token:
# Prefer keyring-backed tokens, but fall back to file-backed tokens for
# backward compatibility with existing users.
token = None
for storage in ("keyring", "file"):
ref = OAuthRef(storage=storage, key=KIMI_CODE_OAUTH_KEY)
token = load_tokens(ref)
if token and getattr(token, "access_token", None):
break
if token and getattr(token, "access_token", None):

Copilot uses AI. Check for mistakes.
else:
logger.warning("Authentication not complete for method: {id}", id=method_id)
raise acp.RequestError.auth_required(
{"message": "Please complete login in terminal first"}
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

_check_auth() raises auth_required with an authMethods payload, but authenticate() raises auth_required with only a message. This inconsistency can make client handling harder (some clients will expect authMethods to render UI/actions). Consider returning/raising a consistent auth_required payload that includes the cached auth method metadata.

Suggested change
{"message": "Please complete login in terminal first"}
{
"message": "Please complete login in terminal first",
"authMethods": self._auth_methods,
}

Copilot uses AI. Check for mistakes.

def _check_auth(self) -> None:
"""Check if Kimi Code authentication is complete. Raise AUTH_REQUIRED if not."""
ref = OAuthRef(storage="file", key=KIMI_CODE_OAUTH_KEY)
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

load_tokens() only falls back to keyring (and migrates keyring -> file) when OAuthRef.storage is set to "keyring". Using OAuthRef(storage="file", ...) here will treat users who still have tokens stored only in keyring as unauthenticated and force a new login. Use OAuthRef(storage="keyring", key=...) (it still prefers the file token first) or explicitly try both backends.

Suggested change
ref = OAuthRef(storage="file", key=KIMI_CODE_OAUTH_KEY)
ref = OAuthRef(storage="keyring", key=KIMI_CODE_OAUTH_KEY)

Copilot uses AI. Check for mistakes.
Comment on lines 104 to 110
def _check_auth(self) -> None:
"""Check if Kimi Code authentication is complete. Raise AUTH_REQUIRED if not."""
ref = OAuthRef(storage="file", key=KIMI_CODE_OAUTH_KEY)
token = load_tokens(ref)

if token is None or not token.access_token:
# Build AUTH_REQUIRED error data per AUTHENTICATION.md spec
Copy link

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

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

This auth gate is hard-coded to the Kimi Code OAuth token, which will block ACP usage even when the configured/default provider is API-key based (or a non-Kimi provider like OpenAI) and no Kimi Code OAuth token exists. Consider deriving the required credential from the loaded config/provider (e.g., only require OAuth when the selected provider has provider.oauth set, otherwise allow API-key auth), so ACP works with non-OAuth configurations.

Copilot uses AI. Check for mistakes.
patrick added 2 commits February 9, 2026 12:10
- Add ACP auth entry to CHANGELOG.md
- Sync changelog to docs/en and docs/zh
- Update kimi-web.md with 1.9 features (session fork, archive, batch ops)
- Add tests for ACPServer._check_auth method
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

View 9 additional findings in Devin Review.

Open in Devin Review

Comment on lines +322 to +326
raise acp.RequestError.auth_required(
{
"message": "Please complete login in terminal first",
"authMethods": self._auth_methods,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 authenticate() passes raw AuthMethod objects instead of serialized dicts to auth_required error

In the authenticate method, self._auth_methods (a list of acp.schema.AuthMethod schema objects) is passed directly into the auth_required error data, while the _check_auth method correctly serializes each AuthMethod into a plain dict before passing it.

Root Cause

At src/kimi_cli/acp/server.py:325, the code passes self._auth_methods directly:

raise acp.RequestError.auth_required(
    {
        "message": "Please complete login in terminal first",
        "authMethods": self._auth_methods,  # raw AuthMethod objects!
    }
)

But in _check_auth (src/kimi_cli/acp/server.py:111-124), the same self._auth_methods list is carefully converted into plain dicts with specific fields (id, name, description, type, command, args, env) before being passed to auth_required. These are the fields the ACP client expects.

When the JSON-RPC layer attempts to serialize the error response to JSON, the raw acp.schema.AuthMethod objects inside authMethods may not serialize correctly (depending on the JSON encoder used), or they will serialize with a different structure than what the client expects (e.g., including field_meta instead of top-level type/command/args/env fields).

Impact: When authenticate() is called and the user hasn't completed login, the error response will either crash during serialization or send a malformed AUTH_REQUIRED error that the client cannot parse correctly to show the terminal auth prompt.

Prompt for agents
In src/kimi_cli/acp/server.py, in the authenticate() method (around line 322-326), replace the direct use of self._auth_methods with the same serialization logic used in _check_auth(). Extract the auth method serialization into a helper method (e.g., _build_auth_methods_data) that both _check_auth and authenticate can call, which converts each AuthMethod into a plain dict with keys: id, name, description, type, command, args, env. Then use that helper in both places.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Signed-off-by: Young E <49367723+YoungY620@users.noreply.github.com>
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.

1 participant

Comments