Add GitHub CLI login flow#23
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough
Changesログイン認証フロー
Sequence Diagram(s)sequenceDiagram
participant CLI as Slidict::CLI
participant AuthClient as Slidict::AuthClient
participant API as slidict.io
participant Credentials as Slidict::Credentials
CLI->>AuthClient: request_device_code
AuthClient->>API: POST /api/cli/device/code
API-->>AuthClient: device_code, user_code, verification_uri, interval, expires_in
CLI->>CLI: print URL and user code
loop until expires_in
CLI->>AuthClient: poll_token(device_code:)
AuthClient->>API: POST /api/cli/device/token
API-->>AuthClient: Pending or access_token
AuthClient-->>CLI: access_token or Pending
CLI->>CLI: sleep(interval)
end
CLI->>Credentials: write_cli_token!(access_token:)
Credentials-->>CLI: credentials.json path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/slidict/auth_client.rb`:
- Around line 39-40: `AuthClient` currently maps both `authorization_pending`
and `slow_down` to `Pending`, which hides the server’s backoff signal. Update
the error handling in `AuthClient` so `slow_down` raises a distinct exception
(for example `SlowDown`) while keeping `authorization_pending` as `Pending`, and
adjust `CLI#login` to rescue that new `AuthClient::SlowDown` path and increase
the retry interval before sleeping.
In `@lib/slidict/cli.rb`:
- Around line 147-162: The CLI login flow in the `slidict login` path only
rescues `AuthClient::Error` and `KeyError`, so failures from
`credentials.write_cli_token!` can escape and crash the command. Update the
rescue handling around the token save logic in `CLI#login` to also catch the
credential write failure path, print a clear “saving credentials failed” message
distinct from GitHub auth errors, and return 1 while keeping the existing
`AuthClient::Pending` retry behavior intact.
In `@lib/slidict/credentials.rb`:
- Around line 15-22: The credential directory setup in `Slidict::Credentials`
only applies `0o700` on creation via `FileUtils.mkdir_p`, so existing
directories can keep weaker permissions. After the `mkdir_p` call, explicitly
enforce the directory mode with `File.chmod` on `File.dirname(`@path`)` before
writing the JSON file, while keeping the file itself at `0o600`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ec7490f-f55d-4dc0-aa09-604bf5e41430
📒 Files selected for processing (6)
lib/slidict.rblib/slidict/auth_client.rblib/slidict/cli.rblib/slidict/credentials.rbspec/slidict/cli_spec.rbspec/slidict/credentials_spec.rb
| error = response["error"].to_s | ||
| raise Pending if error == "authorization_pending" || error == "slow_down" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
slow_down を authorization_pending と同一視しないでください。
ここで両方を Pending に潰すと、CLI#login 側は常に固定間隔で再試行するしかなく、サーバーが要求した追加バックオフを守れません。slow_down が返る環境ではログインがタイムアウトしやすくなります。
修正案
class AuthClient
Error = Class.new(StandardError)
Pending = Class.new(StandardError)
+ SlowDown = Class.new(StandardError)
@@
- raise Pending if error == "authorization_pending" || error == "slow_down"
+ raise Pending if error == "authorization_pending"
+ raise SlowDown if error == "slow_down"# lib/slidict/cli.rb
rescue AuthClient::SlowDown
interval += 5
`@sleeper.sleep`(interval)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error = response["error"].to_s | |
| raise Pending if error == "authorization_pending" || error == "slow_down" | |
| error = response["error"].to_s | |
| raise Pending if error == "authorization_pending" | |
| raise SlowDown if error == "slow_down" |
🧰 Tools
🪛 RuboCop (1.88.0)
[convention] 40-40: Avoid comparing a variable with multiple items in a conditional, use Array#include? instead.
(Style/MultipleComparison)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/slidict/auth_client.rb` around lines 39 - 40, `AuthClient` currently maps
both `authorization_pending` and `slow_down` to `Pending`, which hides the
server’s backoff signal. Update the error handling in `AuthClient` so
`slow_down` raises a distinct exception (for example `SlowDown`) while keeping
`authorization_pending` as `Pending`, and adjust `CLI#login` to rescue that new
`AuthClient::SlowDown` path and increase the retry interval before sleeping.
| path = credentials.write_cli_token!( | ||
| access_token: token.fetch("access_token"), | ||
| token_type: token.fetch("token_type", "Bearer"), | ||
| provider: token.fetch("provider", "github") | ||
| ) | ||
| @output.puts "4. Saved CLI access token to #{path}" | ||
| return 0 | ||
| rescue AuthClient::Pending | ||
| return login_expired if Time.now >= deadline | ||
|
|
||
| @sleeper.sleep(device[:interval]) | ||
| end | ||
| end | ||
| rescue AuthClient::Error, KeyError => error | ||
| @output.puts "Error: GitHub login failed (#{error.message})" | ||
| 1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
認証情報の保存失敗が未捕捉です。
write_cli_token! が失敗しても、ここでは AuthClient::Error / KeyError しか拾わないので slidict login がそのまま例外終了します。ホーム配下が書けないケースでも、認証エラーとは分けてメッセージを出して 1 を返したいです。
修正案
begin
token = client.poll_token(device_code: device[:device_code])
- path = credentials.write_cli_token!(
- access_token: token.fetch("access_token"),
- token_type: token.fetch("token_type", "Bearer"),
- provider: token.fetch("provider", "github")
- )
+ begin
+ path = credentials.write_cli_token!(
+ access_token: token.fetch("access_token"),
+ token_type: token.fetch("token_type", "Bearer"),
+ provider: token.fetch("provider", "github")
+ )
+ rescue SystemCallError => error
+ `@output.puts` "Error: failed to save CLI access token (#{error.message})"
+ return 1
+ end
`@output.puts` "4. Saved CLI access token to #{path}"
return 0
rescue AuthClient::Pending📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path = credentials.write_cli_token!( | |
| access_token: token.fetch("access_token"), | |
| token_type: token.fetch("token_type", "Bearer"), | |
| provider: token.fetch("provider", "github") | |
| ) | |
| @output.puts "4. Saved CLI access token to #{path}" | |
| return 0 | |
| rescue AuthClient::Pending | |
| return login_expired if Time.now >= deadline | |
| @sleeper.sleep(device[:interval]) | |
| end | |
| end | |
| rescue AuthClient::Error, KeyError => error | |
| @output.puts "Error: GitHub login failed (#{error.message})" | |
| 1 | |
| begin | |
| path = credentials.write_cli_token!( | |
| access_token: token.fetch("access_token"), | |
| token_type: token.fetch("token_type", "Bearer"), | |
| provider: token.fetch("provider", "github") | |
| ) | |
| rescue SystemCallError => error | |
| `@output.puts` "Error: failed to save CLI access token (#{error.message})" | |
| return 1 | |
| end | |
| `@output.puts` "4. Saved CLI access token to #{path}" | |
| return 0 | |
| rescue AuthClient::Pending | |
| return login_expired if Time.now >= deadline | |
| `@sleeper.sleep`(device[:interval]) | |
| end | |
| end | |
| rescue AuthClient::Error, KeyError => error | |
| `@output.puts` "Error: GitHub login failed (#{error.message})" | |
| 1 |
🧰 Tools
🪛 GitHub Actions: Test / 1_Ruby tests (4.0).txt
[error] 157-157: RSpec failure: NoMethodError - private method `sleep' called on #<InstanceDouble(Kernel) (anonymous)> while running Slidict::CLI#login (triggered by Slidict::CLI#run).
🪛 GitHub Actions: Test / 3_Ruby tests (3.4).txt
[error] 157-157: RSpec failure in Slidict::CLI#run during GitHub CLI login device flow. NoMethodError: private method `sleep' called on #<InstanceDouble(Kernel)> (RSpec/Casting issue). Location: ./lib/slidict/cli.rb:157 in 'block in Slidict::CLI#login'.
🪛 GitHub Actions: Test / Ruby tests (3.4)
[error] 157-157: RSpec failure: NoMethodError - private method `sleep' called on #<InstanceDouble(Kernel)>. Failure occurs in Slidict::CLI#login at ./lib/slidict/cli.rb:157.
🪛 GitHub Actions: Test / Ruby tests (4.0)
[error] 157-157: RSpec failure: NoMethodError - private method sleep called on an InstanceDouble(Kernel) during Slidict::CLI#login. @sleeper.sleep(device[:interval])
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/slidict/cli.rb` around lines 147 - 162, The CLI login flow in the
`slidict login` path only rescues `AuthClient::Error` and `KeyError`, so
failures from `credentials.write_cli_token!` can escape and crash the command.
Update the rescue handling around the token save logic in `CLI#login` to also
catch the credential write failure path, print a clear “saving credentials
failed” message distinct from GitHub auth errors, and return 1 while keeping the
existing `AuthClient::Pending` retry behavior intact.
| FileUtils.mkdir_p(File.dirname(@path), mode: 0o700) | ||
| File.write(@path, JSON.pretty_generate({ | ||
| "access_token" => access_token, | ||
| "token_type" => token_type, | ||
| "provider" => provider, | ||
| "kind" => "cli_access_token" | ||
| }) + "\n", mode: "w", perm: 0o600) | ||
| File.chmod(0o600, @path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
ruby -e "
require 'fileutils'
require 'tmpdir'
Dir.mktmpdir do |tmpdir|
subdir = File.join(tmpdir, 'sub')
# 1. 権限 0644 で作成
Dir.mkdir(subdir, 0o644)
puts \"Before mkdir_p: #{subdir.lstat.mode.oct}\"
# 2. mkdir_p ... mode: を実行
FileUtils.mkdir_p(subdir, mode: 0o700)
puts \"After mkdir_p (mode: 0o700): #{subdir.lstat.mode.oct}\"
# 3. mkdir_p ... mode: で新規作成
subdir2 = File.join(tmpdir, 'sub2')
FileUtils.mkdir_p(subdir2, mode: 0o700)
puts \"New directory via mkdir_p: #{subdir2.lstat.mode.oct}\"
end
"Repository: slidict/slidict-cli
Length of output: 528
🏁 Script executed:
ruby -e "
require 'fileutils'
require 'tmpdir'
Dir.mktmpdir do |tmpdir|
# 既存ディレクトリ作成
subdir = File.join(tmpdir, 'sub')
Dir.mkdir(subdir, 0o644)
puts \"Before mkdir_p: #{FileUtils.lstat(subdir).mode & 0o7777}\"
# mkdir_p実行
FileUtils.mkdir_p(subdir, mode: 0o700)
puts \"After mkdir_p (mode: 0o700): #{FileUtils.lstat(subdir).mode & 0o7777}\"
# 明示的 chmod 実行
File.chmod(0o700, subdir)
puts \"After explicit chmod: #{FileUtils.lstat(subdir).mode & 0o7777}\"
end
"Repository: slidict/slidict-cli
Length of output: 512
🏁 Script executed:
ruby -e "
require 'fileutils'
require 'tmpdir'
Dir.mktmpdir do |tmpdir|
subdir = File.join(tmpdir, 'sub')
Dir.mkdir(subdir, 0o644)
puts \"Before: #{File.lstat(subdir).mode.oct}\"
FileUtils.mkdir_p(subdir, mode: 0o700)
puts \"After mkdir_p: #{File.lstat(subdir).mode.oct}\"
File.chmod(0o700, subdir)
puts \"After chmod: #{File.lstat(subdir).mode.oct}\"
end
"Repository: slidict/slidict-cli
Length of output: 493
🏁 Script executed:
ruby -e "
require 'fileutils'
require 'tmpdir'
Dir.mktmpdir do |tmpdir|
subdir = File.join(tmpdir, 'sub')
# 1. 0644 で作成
Dir.mkdir(subdir, 0o644)
puts \"Before: %o\" % [File.lstat(subdir).mode]
# 2. mkdir_p with mode: 0o700
FileUtils.mkdir_p(subdir, mode: 0o700)
puts \"After mkdir_p: %o\" % [File.lstat(subdir).mode]
# 3. 明示的 chmod
File.chmod(0o700, subdir)
puts \"After chmod: %o\" % [File.lstat(subdir).mode]
# 新規ケース確認
subdir2 = File.join(tmpdir, 'sub2')
FileUtils.mkdir_p(subdir2, mode: 0o700)
puts \"New dir via mkdir_p: %o\" % [File.lstat(subdir2).mode]
end
"Repository: slidict/slidict-cli
Length of output: 313
既存の認証ディレクトリに 0700 権限を強制できていません。
FileUtils.mkdir_p(path, mode: 0o700) の mode: オプションは、ディレクトリが新規作成される場合のみ適用され、すでに存在するディレクトリには影響しません。コード実行の結果、既存ディレクトリ（例：~/.config/slidict）が別の権限（例：0644 等）で保持されるままであることが確認されました。このままだと、セキュリティ要件である「ディレクトリは 0700」の保証が崩れます。
既存ディレクトリに対しても権限を強制するには、mkdir_p の後に明示的に File.chmod を呼ぶ必要があります。
修正案
def write_cli_token!(...)
- FileUtils.mkdir_p(File.dirname(`@path`), mode: 0o700)
+ dir = File.dirname(`@path`)
+ FileUtils.mkdir_p(dir, mode: 0o700)
+ File.chmod(0o700, dir)
File.write(`@path`, ...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FileUtils.mkdir_p(File.dirname(@path), mode: 0o700) | |
| File.write(@path, JSON.pretty_generate({ | |
| "access_token" => access_token, | |
| "token_type" => token_type, | |
| "provider" => provider, | |
| "kind" => "cli_access_token" | |
| }) + "\n", mode: "w", perm: 0o600) | |
| File.chmod(0o600, @path) | |
| dir = File.dirname(`@path`) | |
| FileUtils.mkdir_p(dir, mode: 0o700) | |
| File.chmod(0o700, dir) | |
| File.write(`@path`, JSON.pretty_generate({ | |
| "access_token" => access_token, | |
| "token_type" => token_type, | |
| "provider" => provider, | |
| "kind" => "cli_access_token" | |
| }) + "\n", mode: "w", perm: 0o600) | |
| File.chmod(0o600, `@path`) |
🧰 Tools
🪛 RuboCop (1.88.0)
[convention] 16-21: Prefer string interpolation to string concatenation.
(Style/StringConcatenation)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/slidict/credentials.rb` around lines 15 - 22, The credential directory
setup in `Slidict::Credentials` only applies `0o700` on creation via
`FileUtils.mkdir_p`, so existing directories can keep weaker permissions. After
the `mkdir_p` call, explicitly enforce the directory mode with `File.chmod` on
`File.dirname(`@path`)` before writing the JSON file, while keeping the file
itself at `0o600`.
The login flow spec mocked the sleeper with instance_double(Kernel), which models Kernel#sleep as a private instance method. Production code calls Kernel.sleep (public module method), so the double didn't match real behavior and rspec failed with a NoMethodError. Switch to a plain double. Also clean up redundant begin/rescue and string concatenation introduced in the new auth/credentials code.
Review notesReviewed the device-flow login implementation and pushed a follow-up commit (858bad5) with fixes. Found a broken test (the important one): Minor rubocop cleanup (scoped to the new code only, didn't touch pre-existing files):
Logic review: the device-code flow, polling/backoff, token persistence, and file permission handling (0700 dir / 0600 file, enforced via explicit |
1 participant
Motivation
slidict loginto obtain a CLI-specific access token separated from web sessions and store it at~/.config/slidict/credentials.json.Description
Slidict::AuthClient(lib/slidict/auth_client.rb) to request a device code and poll for a token from the Slidict CLI auth endpoints using a GitHub provider by default.Slidict::Credentials(lib/slidict/credentials.rb) to persist a CLI access token as JSON withkind: "cli_access_token"and ensure the directory/file use private permissions (0700for dir,0600for file).lib/slidict/cli.rb) to recognize thelogincommand, print activation instructions, poll the auth server with backoff/timeout handling, and save the returned token viaSlidict::Credentials.lib/slidict.rb) and add specs for the login flow and credential persistence (spec/slidict/cli_spec.rb,spec/slidict/credentials_spec.rb).Testing
ruby -cfor the modified files and they passed.ruby -Ilib -e 'require "slidict"; puts Slidict::CLI.new.run(["-h"])'and it produced the updated help text.AuthClientand fakeCredentialsand observed successful prompt output and token save behavior.Slidict::Credentials#write_cli_token!writes expected JSON and the saved file has0600permissions, butbundle exec rspeccould not be run becausebundle installfailed in this environment due to arubygems.orgHTTP 403 error.Codex Task
Summary by CodeRabbit
loginコマンドを追加し、デバイスコードフローで認証を進めてCLIトークンを取得・保存できるようになりました。loginを反映しました。