Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates DevOpsBot from ConoHa VPS API to Sakura VPS API. The implementation includes updating the module name, replacing the authentication mechanism from token-based to Bearer token authentication, and adapting all API endpoints and response structures to match the Sakura VPS API specification.
Changes:
- Renamed module from
github.com/traPtitech/DevOpsBottogithub.com/traPtitech/sakura-DevOpsBot - Replaced ConoHa API configuration and authentication with Sakura VPS Bearer token authentication
- Updated server operations (restart, hosts) to use Sakura VPS API endpoints and response structures
- Removed unused dependencies (golang.org/x/sync) and simplified the restart command to remove the soft/hard restart type distinction
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated module name and removed golang.org/x/sync dependency |
| go.sum | Updated checksums for dependency changes |
| main.go | Updated import path to use sakura-DevOpsBot module name |
| Dockerfile | Updated build command to reference new module name |
| cmd/root.go | Updated import paths |
| cmd/server.go | Updated import paths |
| pkg/config/config.go | Replaced ConoHa configuration with Sakura VPS configuration including origin, API path, and bearer token |
| pkg/server/token.go | Commented out entire ConoHa token authentication implementation |
| pkg/server/server.go | Added Japanese comment for command registration |
| pkg/server/restart.go | Refactored to use Sakura VPS force-reboot endpoint, simplified to single server ID argument, removed soft/hard restart types |
| pkg/server/hosts.go | Completely rewritten to match Sakura VPS API response structure, removed parallel goroutine fetching, updated logging format |
| pkg/bot/bot.go | Updated import paths |
| pkg/bot/command.go | Updated import paths |
| pkg/bot/help.go | Updated import paths |
| pkg/bot/traq/bot.go | Updated import paths, reordered imports |
| pkg/bot/traq/context.go | Updated import paths, reordered imports |
| pkg/bot/slack/bot.go | Updated import paths, reordered imports |
| pkg/bot/slack/context.go | Updated import paths, reordered imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/server/restart.go
Outdated
Comment on lines
48
to
56
| logStr := fmt.Sprintf(`Request | ||
| - URL: %s | ||
| - RestartType: %s | ||
|
|
||
| Response | ||
| - Header: %+v | ||
| - Body: %s | ||
| - Status: %s (Expected: 202) | ||
| `, req.URL.String(), restartType, resp.Header, string(respBody), resp.Status) | ||
| `, req.URL.String(), resp.Header, string(respBody), resp.Status) | ||
| log.Println(logStr) |
There was a problem hiding this comment.
Logging the full response body and headers may expose sensitive information. Consider reviewing what information is included in the response and whether it's safe to log it. If the response contains any authentication tokens, server credentials, or other sensitive data, this could be a security concern.
Collaborator
Author
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
一つ前の修正に漏れがあった。
… improve environment variable handling
Collaborator
Author
|
テスト確認できました。 |
cp-20
reviewed
Jan 20, 2026
cp-20
left a comment
cp-20
left a comment
There was a problem hiding this comment.
動作として動いてるなら良さそうで、仕様的なところ (+nits) でいくつかコメントしました 別々のコードでデプロイするにしろ、一緒のコードでデプロイするにしろ、適当にコメントを反映させてもらえると良いと思います
Comment on lines
+22
to
25
| serverID, err := strconv.Atoi(args[0]) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid arguments, server id must be an integer: %q", args[0]) | ||
| } |
There was a problem hiding this comment.
仕様として Name も受け付けて内部で Name から ID への変換をやるとかすると使い勝手が良いかもだけど、仕様がやや複雑になるのでどっちもどっち
Collaborator
Author
There was a problem hiding this comment.
やりたいです。
やります。
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughプロジェクトを sakura-DevOpsBot にリブランドし、ConoHa 統合を廃止して Sakura API へ移行。モジュール/インポート、設定スキーマ、サーバー API（ページング対応）および CI/リリース設定とビルド設定が更新された。 Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI"
participant Config as "Config\n(pkg/config)"
participant SakuraAPI as "Sakura API\n(HTTP)"
participant Console as "Console\n(Table Printer)"
rect rgba(60,120,255,0.5)
CLI->>Config: 設定読み込み\n(servers.sakura.origin,\nserversAPIURLPath,bearerToken)
end
rect rgba(20,200,100,0.5)
CLI->>SakuraAPI: GET /servers?per_page=...&page=1\nAuthorization: Bearer <token>
SakuraAPI-->>CLI: 200 {results,count,next,...}
CLI->>SakuraAPI: (繰返し) GET /servers?page=N
SakuraAPI-->>CLI: 200 {results,next/previous}
end
rect rgba(200,120,20,0.5)
CLI->>Console: 集約データを渡して表示\n(printAlignedTable)
Console-->>CLI: 完了ログ (総件数)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/server/server.go (1)
1-1:⚠️ Potential issue | 🟡 Minorパッケージコメントが旧プロバイダー名のままです。
ConoHaへの言及が残っており、Sakura への移行後も旧名称が参照されています。🔧 修正案
-// Package server provides ConoHa server related operations +// Package server provides Sakura server related operations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/server.go` at line 1, パッケージコメントが旧プロバイダー名「ConoHa」を参照したままなので、pkg/serverのパッケージコメント（ファイル冒頭の package server コメント）を「Sakura」へ表記を差し替えし、説明文中のプロバイダー名や関連記述もSakuraに合わせて更新してください（パッケージコメントの文言をSakura向けに整えることで一貫性を保ちます）。.github/workflows/release.yml (1)
38-40:⚠️ Potential issue | 🟠 Major
imageジョブにpackages: writeを明示してください。Lines 55-60 で GHCR への認証に
${{ secrets.GITHUB_TOKEN }}を使用し、Lines 62-71 でイメージをプッシュしていますが、ジョブレベルのpermissionsが指定されていません。リポジトリまたは組織のデフォルト権限設定が不十分な場合、プッシュに失敗する可能性があります。packages: write（およびcontents: read）をジョブに明示することで、環境依存のエラーを回避できます。修正案
image: name: Build Docker Image runs-on: ubuntu-latest + permissions: + contents: read + packages: write steps:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 38 - 40, The "image" job (Build Docker Image) lacks explicit job-level permissions needed for GHCR pushes; add a permissions block on the image job that includes at minimum packages: write and contents: read so the job can authenticate with the GITHUB_TOKEN used for login and push (see the GHCR auth and docker push steps referenced in the diff). Locate the image job definition (name: Build Docker Image / job id image) and insert a permissions section with packages: write and contents: read to avoid failures when repository/org default permissions are restricted.pkg/server/restart.go (1)
44-45:⚠️ Potential issue | 🟠 Major外部 API 呼び出しにタイムアウトが設定されていません。
Line 44 の
http.DefaultClientはタイムアウト未設定のため、ネットワーク障害時にサーバー再起動コマンドが無限にハングする可能性があります。明示的なTimeoutを持つカスタムhttp.Clientを使用してください。修正案
import ( "fmt" "log" "net/http" "strconv" "strings" + "time" @@ - resp, err := http.DefaultClient.Do(req) + client := &http.Client{Timeout: 15 * time.Second} + resp, err := client.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/restart.go` around lines 44 - 45, http.DefaultClient.Do(req) has no timeout and can hang; replace it by creating and using a custom *http.Client with a sensible Timeout (e.g., time.Duration) and call client.Do(req) instead of http.DefaultClient.Do(req); update the code around where resp, err := http.DefaultClient.Do(req) is used (in restart.go) to construct the client (and optionally use a request context with timeout) and use client.Do(req) so the external restart API cannot hang indefinitely.
🧹 Nitpick comments (4)
.gitignore (1)
10-10:docker-compose.yamlの ignore は運用意図を再確認してください。ルートの
docker-compose.yamlを無条件で除外すると、開発/検証用の共通定義を共有できなくなる可能性があります。ローカル差分や機密値回避が目的なら、docker-compose.override.yamlや.envの除外に寄せる方が一般的です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 10, ルートの "docker-compose.yaml" を無条件で .gitignore に追加しているため共有すべき開発/検証用定義が除外されてしまう問題があります。該当の ignore 行を削除してリポジトリで基本定義を保持し、機密やローカル差分は "docker-compose.override.yaml" や "docker-compose.local.yaml"、および ".env" を個別に .gitignore に追加して運用するようにしてください（または ".env.example" をリポジトリに残して使い方を README に明記してください）。pkg/server/server.go (1)
18-18: 型名hostsCommandとコマンドキー"info"が一致していません。
hostsCommandという型名は旧来の"hosts"コマンドに由来しており、新しいキー"info"と意味的に対応していません。将来の保守性のために型名をinfoCommandに改名することを検討してください。♻️ 修正案（hostsCommand → infoCommand）
server.go:- cmd.Commands["info"] = &hostsCommand{} + cmd.Commands["info"] = &infoCommand{}
hostsCommandを定義しているファイル（例:hosts.go）内でも型名を変更:-type hostsCommand struct { ... } -func (c *hostsCommand) Execute(...) { ... } +type infoCommand struct { ... } +func (c *infoCommand) Execute(...) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/server.go` at line 18, The symbol mapping cmd.Commands["info"] currently instantiates a type named hostsCommand which is semantically mismatched; rename the type hostsCommand to infoCommand (and any constructor/receiver method names) in its defining file and update all references (methods, receivers, interface implementations) to use infoCommand, then update the registration in server.go to construct &infoCommand{} instead of &hostsCommand{} so cmd.Commands["info"], the type name, and all usages are consistent.cmd/root.go (1)
42-42: コメント内の旧プロジェクト名が残っています。コメントアウトされたフラグのデフォルトパスが旧プロジェクト名のままです。将来有効化された際に混乱を招く可能性があります。
🔧 修正案
- // rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.DevOpsBot.yaml)") + // rootCmd.PersistentFlags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.sakura-DevOpsBot.yaml)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` at line 42, コメントアウトされたフラグのデフォルトパスに旧プロジェクト名 ".DevOpsBot.yaml" が残っているため、将来有効化されたときに誤解を招きます — 更新対象は rootCmd.PersistentFlags().StringVar 呼び出しと cfgFile 変数の説明文字列で、コメント内のデフォルトパスを現在のプロジェクト名（または汎用的な "$HOME/.config.yaml" など）に置き換えるか、デフォルト表記自体を削除して将来の混乱を防いでください。pkg/server/restart.go (1)
31-33: 設定値は書き換えず、ローカル変数で URL パスを正規化してください。Line 31-33 でグローバル設定を更新しているため、副作用が残ります。Line 37 で使う値だけローカルで組み立てる方が安全です。
修正案
- if !strings.HasPrefix(config.C.Servers.Sakura.ServersAPIURLPath, "/") { - config.C.Servers.Sakura.ServersAPIURLPath = "/" + config.C.Servers.Sakura.ServersAPIURLPath - } + apiPath := config.C.Servers.Sakura.ServersAPIURLPath + if !strings.HasPrefix(apiPath, "/") { + apiPath = "/" + apiPath + } req, err := sling.New(). Base(config.C.Servers.Sakura.Origin). - Post(fmt.Sprintf("%s/%d/force-reboot", config.C.Servers.Sakura.ServersAPIURLPath, serverID)). + Post(fmt.Sprintf("%s/%d/force-reboot", apiPath, serverID)).Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/restart.go` around lines 31 - 33, Don't mutate the global config.C.Servers.Sakura.ServersAPIURLPath; instead create a local variable (e.g., normalizedPath) that ensures it begins with "/" and use that local variable when building the URL later (the usage at the code that currently reads the value on line 37). Update the code around the adjustment to compute normalizedPath from config.C.Servers.Sakura.ServersAPIURLPath without assigning back to config, and replace subsequent uses (the URL assembly that references ServersAPIURLPath) to use normalizedPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 17-18: goreleaser ジョブで現在設定されている permissions の "contents: read" は
GitHub リリースの作成/更新を行う GoReleaser 実行時に不足するため、workflow の goreleaser ジョブの
permissions ブロックで "contents: write" に変更して書き込み権限を付与してください（参照箇所: goreleaser ジョブの
permissions 定義）。
In `@pkg/server/hosts.go`:
- Around line 101-103: The code trims config.C.Servers.Sakura.BearerToken only
for the empty check but uses the original value when sending the Authorization
header, which can cause auth failures if the configured token has surrounding
whitespace; normalize the token once (e.g., token :=
strings.TrimSpace(config.C.Servers.Sakura.BearerToken) or assign the trimmed
value back to config.C.Servers.Sakura.BearerToken) and use that trimmed value
both for the empty check and when setting the request header (also apply the
same change at the other occurrence mentioned around lines 118-119).
- Around line 125-126: The call uses http.DefaultClient.Do(req) without a
timeout which can hang; replace it with an http.Client that has a reasonable
Timeout (or perform the request with a context having a deadline) and use that
client to execute the request instead of http.DefaultClient. Specifically,
change the code that builds and sends the request (the variable req and the call
http.DefaultClient.Do(req) in hosts.go) to either create a client like
http.Client{Timeout: time.Second * <N>} and call client.Do(req) or wrap req with
a context via context.WithTimeout and use http.NewRequestWithContext so the
request is cancelled after the timeout. Ensure you import time/context if needed
and choose an appropriate timeout value.
In `@pkg/server/restart.go`:
- Around line 22-25: strconv.Atoi の後に負数や 0 を通してしまう問題があるため、serverID
の下限チェックを追加してください: strconv.Atoi でパースした直後（serverID 変数がある箇所）に serverID <= 0
を判定して早期に fmt.Errorf で「server id must be a positive
integer」等のわかりやすいエラーメッセージを返すように修正してください（参照: serverID, strconv.Atoi）。
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 38-40: The "image" job (Build Docker Image) lacks explicit
job-level permissions needed for GHCR pushes; add a permissions block on the
image job that includes at minimum packages: write and contents: read so the job
can authenticate with the GITHUB_TOKEN used for login and push (see the GHCR
auth and docker push steps referenced in the diff). Locate the image job
definition (name: Build Docker Image / job id image) and insert a permissions
section with packages: write and contents: read to avoid failures when
repository/org default permissions are restricted.
In `@pkg/server/restart.go`:
- Around line 44-45: http.DefaultClient.Do(req) has no timeout and can hang;
replace it by creating and using a custom *http.Client with a sensible Timeout
(e.g., time.Duration) and call client.Do(req) instead of
http.DefaultClient.Do(req); update the code around where resp, err :=
http.DefaultClient.Do(req) is used (in restart.go) to construct the client (and
optionally use a request context with timeout) and use client.Do(req) so the
external restart API cannot hang indefinitely.
In `@pkg/server/server.go`:
- Line 1: パッケージコメントが旧プロバイダー名「ConoHa」を参照したままなので、pkg/serverのパッケージコメント（ファイル冒頭の
package server
コメント）を「Sakura」へ表記を差し替えし、説明文中のプロバイダー名や関連記述もSakuraに合わせて更新してください（パッケージコメントの文言をSakura向けに整えることで一貫性を保ちます）。
---
Nitpick comments:
In @.gitignore:
- Line 10: ルートの "docker-compose.yaml" を無条件で .gitignore
に追加しているため共有すべき開発/検証用定義が除外されてしまう問題があります。該当の ignore
行を削除してリポジトリで基本定義を保持し、機密やローカル差分は "docker-compose.override.yaml" や
"docker-compose.local.yaml"、および ".env" を個別に .gitignore に追加して運用するようにしてください（または
".env.example" をリポジトリに残して使い方を README に明記してください）。
In `@cmd/root.go`:
- Line 42: コメントアウトされたフラグのデフォルトパスに旧プロジェクト名 ".DevOpsBot.yaml"
が残っているため、将来有効化されたときに誤解を招きます — 更新対象は rootCmd.PersistentFlags().StringVar 呼び出しと
cfgFile 変数の説明文字列で、コメント内のデフォルトパスを現在のプロジェクト名（または汎用的な "$HOME/.config.yaml"
など）に置き換えるか、デフォルト表記自体を削除して将来の混乱を防いでください。
In `@pkg/server/restart.go`:
- Around line 31-33: Don't mutate the global
config.C.Servers.Sakura.ServersAPIURLPath; instead create a local variable
(e.g., normalizedPath) that ensures it begins with "/" and use that local
variable when building the URL later (the usage at the code that currently reads
the value on line 37). Update the code around the adjustment to compute
normalizedPath from config.C.Servers.Sakura.ServersAPIURLPath without assigning
back to config, and replace subsequent uses (the URL assembly that references
ServersAPIURLPath) to use normalizedPath.
In `@pkg/server/server.go`:
- Line 18: The symbol mapping cmd.Commands["info"] currently instantiates a type
named hostsCommand which is semantically mismatched; rename the type
hostsCommand to infoCommand (and any constructor/receiver method names) in its
defining file and update all references (methods, receivers, interface
implementations) to use infoCommand, then update the registration in server.go
to construct &infoCommand{} instead of &hostsCommand{} so cmd.Commands["info"],
the type name, and all usages are consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
.github/workflows/build.yml.github/workflows/release.yml.gitignore.goreleaser.ymlDockerfilecmd/root.gocmd/server.gogo.modmain.gopkg/bot/bot.gopkg/bot/command.gopkg/bot/help.gopkg/bot/slack/bot.gopkg/bot/slack/context.gopkg/bot/traq/bot.gopkg/bot/traq/context.gopkg/config/config.gopkg/server/hosts.gopkg/server/restart.gopkg/server/server.gopkg/server/token.go
💤 Files with no reviewable changes (1)
- pkg/server/token.go
権限不足だった Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
サーバーID加減チェック Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
38-60:⚠️ Potential issue | 🔴 Critical
imageジョブにpackages: write権限が不足しています。GHCR へ Docker イメージを push するには
GITHUB_TOKENにpackages: writeが必要です。このジョブにはpermissionsブロックがなく、ワークフローにも top-level のpermissions定義がないため、リポジトリのデフォルト権限に依存します。GitHub の新規リポジトリデフォルトは read-only であるため、push 時に 403 エラーで失敗します。修正案
image: name: Build Docker Image runs-on: ubuntu-latest + permissions: + packages: write steps:
github.repository_ownerの大文字小文字問題
github.repository_ownerはtraPtitechのようにオーナー名の原表記を返します。OCI イメージ参照は小文字でなければならないため、タグ生成時（70-71 行目）で大文字を含む場合、"invalid reference format" エラーで push が失敗します。小文字変換の例
- name: Extract version from tag id: version run: echo "tag=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT + - name: Lowercase owner + id: owner + run: echo "name=$(echo '${{ github.repository_owner }}' | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT + - name: Set up QEMUその後、タグ参照を以下のように置き換えます:
tags: | - ${{ env.REGISTRY }}/${{ github.repository_owner }}/${{ env.IMAGE_NAME }}:latest - ${{ env.REGISTRY }}/${{ github.repository_owner }}/${{ env.IMAGE_NAME }}:${{ steps.version.outputs.tag }} + ${{ env.REGISTRY }}/${{ steps.owner.outputs.name }}/${{ env.IMAGE_NAME }}:latest + ${{ env.REGISTRY }}/${{ steps.owner.outputs.name }}/${{ env.IMAGE_NAME }}:${{ steps.version.outputs.tag }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 38 - 60, The image job is missing write permission for packages and uses the raw github.repository_owner which can contain uppercase letters; add a permissions block granting packages: write for the image job (so the GITHUB_TOKEN used in the docker/login-action can push to GHCR) and update any tag/registry string generation that references github.repository_owner to use the toLower(...) expression (e.g. toLower(github.repository_owner)) so OCI image names are all lowercase; locate these changes around the job named "image", the step id "version" that writes the tag, and the docker/login-action step that uses ${{ github.repository_owner }} / ${{ env.REGISTRY }}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 38-60: The image job is missing write permission for packages and
uses the raw github.repository_owner which can contain uppercase letters; add a
permissions block granting packages: write for the image job (so the
GITHUB_TOKEN used in the docker/login-action can push to GHCR) and update any
tag/registry string generation that references github.repository_owner to use
the toLower(...) expression (e.g. toLower(github.repository_owner)) so OCI image
names are all lowercase; locate these changes around the job named "image", the
step id "version" that writes the tag, and the docker/login-action step that
uses ${{ github.repository_owner }} / ${{ env.REGISTRY }}.
…ient with timeout" This reverts commit add14e2.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/server/restart.go (1)
52-52:⚠️ Potential issue | 🟠 Major外部API呼び出し2箇所ともタイムアウト未設定です
Line 52 と Line 92 の
http.DefaultClient.Do(req)は、API停止時に処理がハングするリスクがあります。共通のタイムアウト付きクライアントを使ってください。修正例
import ( "encoding/json" "fmt" "io" "log" "net/http" "sort" "strconv" "strings" + "time" @@ - serverID, serverName, err := resolveServerID(token, serverKey) + client := &http.Client{Timeout: 15 * time.Second} + serverID, serverName, err := resolveServerID(client, token, serverKey) @@ - resp, err := http.DefaultClient.Do(req) + resp, err := client.Do(req) @@ -func resolveServerID(token string, serverKey string) (int, string, error) { +func resolveServerID(client *http.Client, token string, serverKey string) (int, string, error) { @@ - resp, err := http.DefaultClient.Do(req) + resp, err := client.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/restart.go` at line 52, The two calls using http.DefaultClient.Do(req) (the occurrences of "resp, err := http.DefaultClient.Do(req)") lack timeouts and can hang; create and use a shared http.Client with a Timeout (e.g., var httpClient = &http.Client{Timeout: 10*time.Second}) or use request contexts with timeouts (req = req.WithContext(ctxWithTimeout)) and replace both DefaultClient.Do usages with httpClient.Do(req) (and add the necessary time import), ensuring both call sites in restart.go use the same timeout-enabled client.
♻️ Duplicate comments (2)
pkg/server/hosts.go (1)
126-126:⚠️ Potential issue | 🟠 MajorHTTPリクエストが無期限待機になり得ます
Line 126 の
http.DefaultClient.Do(req)はタイムアウト未設定のため、外部API遅延時にコマンドが戻らなくなる可能性があります。タイムアウト付きhttp.Clientかcontext.WithTimeoutを使ってください。修正例
import ( "encoding/json" "fmt" "io" "log" "net/http" "os" "sort" "strings" + "time" @@ - for { + client := &http.Client{Timeout: 15 * time.Second} + for { @@ - resp, err := http.DefaultClient.Do(req) + resp, err := client.Do(req)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/server/hosts.go` at line 126, The http.DefaultClient.Do(req) call in hosts.go can hang because no timeout is set; replace it by creating a timeout-aware context or client: use context.WithTimeout(...) to derive a ctx and attach it to the request (via http.NewRequestWithContext or req = req.WithContext(ctx)) or instantiate an http.Client with a non-zero Timeout and call that client's Do; ensure you cancel the context (or use defer cancel()), use the same request variable (req) and keep the resp/err handling (and resp.Body close) intact so the request won't block indefinitely..github/workflows/release.yml (1)
17-18:contents: writeが正しく設定されています。以前のレビューで指摘された問題（
contents: readでは GoReleaser がリリースを作成できない）は、contents: writeに修正されています。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 17 - 18, The permissions setting in the workflow currently uses "permissions: contents: write" which fixes the earlier issue where GoReleaser needed write access; ensure the "permissions" section keeps "contents: write" (not "contents: read") so GoReleaser can create releases and do not revert this change in the "permissions" block of the release workflow.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
64-66: ログインのユーザー名と image タグのオーナー名の大文字小文字が不統一です。Line 65 の
usernameはgithub.repository_owner（元のケース）を使用していますが、Line 76-77 の image タグではsteps.owner.outputs.name（小文字化済み）を使っています。GHCR のログイン自体はケースインセンシティブのため動作上の問題はありませんが、統一性のために小文字化済みの値を使用することを検討してください。♻️ 修正案: username を小文字化済みの値に統一
- name: Login to GitHub Container Registry uses: docker/login-action@v3 with: registry: ${{ env.REGISTRY }} - username: ${{ github.repository_owner }} + username: ${{ steps.owner.outputs.name }} password: ${{ secrets.GITHUB_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 64 - 66, The username value is inconsistent with the image tag owner casing — change the login username to use the already lowercased owner value to match the image tags by replacing the current username reference (github.repository_owner) with the lowercased output (steps.owner.outputs.name) or apply toLower(github.repository_owner); update the username field so it uses steps.owner.outputs.name (or the toLower variant) to keep casing consistent with the image tag usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 51-53: The step "Lowercase owner" (id: owner) exposes a shell
injection risk by expanding ${{ github.repository_owner }} directly in the run
command; instead set github.repository_owner into an environment variable for
that step (e.g., OWNER) and reference the environment variable inside the run so
the workflow expression is not directly expanded into the shell, then write the
lowercased value to GITHUB_OUTPUT (preserve use of tr '[:upper:]' '[:lower:]' on
the env var and continue appending to $GITHUB_OUTPUT).
In `@pkg/server/restart.go`:
- Around line 68-74: resolveServerID currently returns int causing downcast from
int64 elsewhere and potential overflow on 32-bit platforms; change
resolveServerID's return type from int to int64, replace strconv.Atoi with
strconv.ParseInt (base 10, 64-bit) to parse into an int64, validate positivity
against 0, and return the parsed int64 along with the same string/error
behavior; update all callers that expect int (e.g., places converting from int64
at call sites) to accept the new int64 return type so no narrowing cast occurs.
---
Outside diff comments:
In `@pkg/server/restart.go`:
- Line 52: The two calls using http.DefaultClient.Do(req) (the occurrences of
"resp, err := http.DefaultClient.Do(req)") lack timeouts and can hang; create
and use a shared http.Client with a Timeout (e.g., var httpClient =
&http.Client{Timeout: 10*time.Second}) or use request contexts with timeouts
(req = req.WithContext(ctxWithTimeout)) and replace both DefaultClient.Do usages
with httpClient.Do(req) (and add the necessary time import), ensuring both call
sites in restart.go use the same timeout-enabled client.
---
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 17-18: The permissions setting in the workflow currently uses
"permissions: contents: write" which fixes the earlier issue where GoReleaser
needed write access; ensure the "permissions" section keeps "contents: write"
(not "contents: read") so GoReleaser can create releases and do not revert this
change in the "permissions" block of the release workflow.
In `@pkg/server/hosts.go`:
- Line 126: The http.DefaultClient.Do(req) call in hosts.go can hang because no
timeout is set; replace it by creating a timeout-aware context or client: use
context.WithTimeout(...) to derive a ctx and attach it to the request (via
http.NewRequestWithContext or req = req.WithContext(ctx)) or instantiate an
http.Client with a non-zero Timeout and call that client's Do; ensure you cancel
the context (or use defer cancel()), use the same request variable (req) and
keep the resp/err handling (and resp.Body close) intact so the request won't
block indefinitely.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 64-66: The username value is inconsistent with the image tag owner
casing — change the login username to use the already lowercased owner value to
match the image tags by replacing the current username reference
(github.repository_owner) with the lowercased output (steps.owner.outputs.name)
or apply toLower(github.repository_owner); update the username field so it uses
steps.owner.outputs.name (or the toLower variant) to keep casing consistent with
the image tag usage.
Comment on lines
+51
to
+53
| - name: Lowercase owner | ||
| id: owner | ||
| run: echo "name=$(echo '${{ github.repository_owner }}' | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
スクリプトインジェクションリスク: ${{ github.repository_owner }} を直接シェルコマンドに展開しています。
GitHub Actions の式展開はシェル実行前に行われるため、シングルクォートでも保護されません。github.repository_owner は英数字とハイフンのみに制限されているため実害は低いですが、環境変数経由に統一することが安全なパターンです。
🛡️ 修正案: 環境変数経由でインジェクションリスクを回避
- name: Lowercase owner
id: owner
- run: echo "name=$(echo '${{ github.repository_owner }}' | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT
+ run: echo "name=$(echo "$OWNER" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_OUTPUT
+ env:
+ OWNER: ${{ github.repository_owner }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 51 - 53, The step "Lowercase
owner" (id: owner) exposes a shell injection risk by expanding ${{
github.repository_owner }} directly in the run command; instead set
github.repository_owner into an environment variable for that step (e.g., OWNER)
and reference the environment variable inside the run so the workflow expression
is not directly expanded into the shell, then write the lowercased value to
GITHUB_OUTPUT (preserve use of tr '[:upper:]' '[:lower:]' on the env var and
continue appending to $GITHUB_OUTPUT).
pkg/server/restart.go
Outdated
Comment on lines
68
to
74
| func resolveServerID(token string, serverKey string) (int, string, error) { | ||
| if serverID, err := strconv.Atoi(serverKey); err == nil { | ||
| if serverID <= 0 { | ||
| return 0, "", fmt.Errorf("invalid arguments, server id must be a positive integer: %q", serverKey) | ||
| } | ||
| return serverID, "(id specified)", nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find and examine the restart.go file
find . -name "restart.go" -path "*/server/*" | head -5Repository: traPtitech/sakura-DevOpsBot
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read the file to see the full context around the mentioned lines
wc -l pkg/server/restart.goRepository: traPtitech/sakura-DevOpsBot
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Check the resolveServerID function and surrounding code
sed -n '60,160p' pkg/server/restart.goRepository: traPtitech/sakura-DevOpsBot
Length of output: 2936
🏁 Script executed:
#!/bin/bash
# Search for type definitions that might contain ID fields
rg "type.*struct" pkg/server/ -A 10 | grep -A 10 "ID"Repository: traPtitech/sakura-DevOpsBot
Length of output: 2233
🏁 Script executed:
#!/bin/bash
# Search for usages of resolveServerID
rg "resolveServerID" pkg/server/Repository: traPtitech/sakura-DevOpsBot
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Check the requestServerRestart function to see how resolveServerID is used
sed -n '40,67p' pkg/server/restart.goRepository: traPtitech/sakura-DevOpsBot
Length of output: 793
intへの縮小変換でIDオーバーフロー余地があります
APIのサーバーID は int64 ですが、resolveServerID 関数は int を返しており、ライン 141 と 154 で int64 から int への縮小変換が発生しています。32ビット環境では int64 値が 2^31 を超える場合にオーバーフローする可能性があるため、resolveServerID の戻り値型を int64 に統一し、strconv.Atoi を strconv.ParseInt に変更することを推奨します。
修正例
-func resolveServerID(token string, serverKey string) (int, string, error) {
- if serverID, err := strconv.Atoi(serverKey); err == nil {
+func resolveServerID(token string, serverKey string) (int64, string, error) {
+ if serverID, err := strconv.ParseInt(serverKey, 10, 64); err == nil {
if serverID <= 0 {
return 0, "", fmt.Errorf("invalid arguments, server id must be a positive integer: %q", serverKey)
}
return serverID, "(id specified)", nil
}
@@
- return int(exactMatches[0].ID), exactMatches[0].Name, nil
+ return exactMatches[0].ID, exactMatches[0].Name, nil
@@
- return int(matched[0].ID), matched[0].Name, nil
+ return matched[0].ID, matched[0].Name, nil📝 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.
Suggested change
| func resolveServerID(token string, serverKey string) (int, string, error) { | |
| if serverID, err := strconv.Atoi(serverKey); err == nil { | |
| if serverID <= 0 { | |
| return 0, "", fmt.Errorf("invalid arguments, server id must be a positive integer: %q", serverKey) | |
| } | |
| return serverID, "(id specified)", nil | |
| } | |
| func resolveServerID(token string, serverKey string) (int64, string, error) { | |
| if serverID, err := strconv.ParseInt(serverKey, 10, 64); err == nil { | |
| if serverID <= 0 { | |
| return 0, "", fmt.Errorf("invalid arguments, server id must be a positive integer: %q", serverKey) | |
| } | |
| return serverID, "(id specified)", nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/server/restart.go` around lines 68 - 74, resolveServerID currently
returns int causing downcast from int64 elsewhere and potential overflow on
32-bit platforms; change resolveServerID's return type from int to int64,
replace strconv.Atoi with strconv.ParseInt (base 10, 64-bit) to parse into an
int64, validate positivity against 0, and return the parsed int64 along with the
same string/error behavior; update all callers that expect int (e.g., places
converting from int64 at call sites) to accept the new int64 return type so no
narrowing cast occurs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DevOpsBotをさくらVPSのAPIに対応させました。
CIがまだ整備できていません。
かつまだテストしてないのでDraftです。
このあとローカルテストします。
Summary by CodeRabbit
New Features
Refactor
Chores