ファイル構造のリファクタリング + unitテストの追加 #507
Conversation
- Removed outdated files related to GitHub Actions runner and Hetzner server management. - Introduced new implementations for GitHub and Hetzner functionalities in the `src/probot` directory. - Updated `package.json` to include `msw` for mocking API requests in tests. - Reorganized test files and added mocks for GitHub and Hetzner APIs. - Enhanced server creation and management logic with improved error handling and logging. - Updated TypeScript configuration to reflect new file structure.
mafreud
changed the title
WalkthroughProbot の GitHub/Hetzner ユーティリティを Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub (webhook)
participant App as Probot App
participant HZ as Hetzner API
participant GHA as GitHub Actions API
participant VM as Runner VM (SSH)
GH->>App: workflow_job / issue イベント送信
App->>App: isJobRequired(context)
alt ジョブが必要
App->>HZ: createServer(apiKey, serverSpec)
HZ-->>App: { serverId, ipv4 }
App->>GHA: POST /actions/runners/generate-jitconfig
GHA-->>App: { encoded_jit_config }
App->>VM: ssh "apt-get install tmux" (exit-code チェック)
App->>VM: ssh initRunner(scriptVersion, os, arch) (exit-code チェック)
App->>VM: ssh startRunner(encoded_jit_config) (tmux で起動)
else ジョブ不要
App-->>GH: noop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #507 +/- ##
============================================
+ Coverage 12.44% 34.56% +22.11%
============================================
Files 4 4
Lines 241 324 +83
Branches 3 10 +7
============================================
+ Hits 30 112 +82
- Misses 210 211 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openci-runner/firebase/functions/src/probot/hetzner.ts (3)
45-55: POST ヘッダー不足（Content-Type）とエラーハンドリング
Content-Type: application/jsonが無く、一律でresponse.json()しています。response.okを確認し、失敗時は詳細を投げてください。export async function createServer( apiKey: string, serverSpec: HetznerServerSpec, ): Promise<HetznerResponse> { - const response = await fetch(baseUrl, { + const response = await fetch(baseUrl, { body: JSON.stringify(serverSpec), headers: { Authorization: `Bearer ${apiKey}`, + "Content-Type": "application/json", + Accept: "application/json", }, method: "POST", }); + if (!response.ok) { + const text = await response.text().catch(() => ""); + throw new Error(`Hetzner createServer failed: ${response.status} ${response.statusText} ${text}`); + }
20-27: DELETE の結果未確認
deleteServerもresponse.okの確認を追加してください。export async function deleteServer(id: string, apiKey: string) { - await fetch(`${baseUrl}/${id}`, { + const res = await fetch(`${baseUrl}/${id}`, { headers: { Authorization: `Bearer ${apiKey}`, }, method: "DELETE", }); + if (!res.ok) { + const text = await res.text().catch(() => ""); + throw new Error(`Hetzner deleteServer failed: ${res.status} ${res.statusText} ${text}`); + } }
70-81: GET の結果未確認
getServerStatusByIdもresponse.okをチェックしてください。export async function getServerStatusById( serverId: number, apiKey: string, ): Promise<string> { - const _response = await fetch(`${baseUrl}/${serverId}`, { + const _response = await fetch(`${baseUrl}/${serverId}`, { headers: { Authorization: `Bearer ${apiKey}`, }, }); + if (!_response.ok) { + const text = await _response.text().catch(() => ""); + throw new Error(`Hetzner getServerStatusById failed: ${_response.status} ${_response.statusText} ${text}`); + } const jsonRes = await _response.json(); return jsonRes.server.status; }
🧹 Nitpick comments (10)
openci-runner/firebase/functions/tsconfig.json (1)
16-17: 冗長なincludeエントリを削除してください
"src"は既にすべてのサブディレクトリ（src/probotを含む）を再帰的に含むため、"src/probot"の明示的な追加は冗長です。次の差分を適用して冗長なエントリを削除してください:
"include": [ - "src", - "src/probot" + "src" ]openci-runner/firebase/functions/test/src/mocks/github_api_handlers.ts (1)
3-14: リクエストボディのバリデーションを追加してください以前のnockベースのテストでは、リクエストボディが期待される内容と一致するか検証していました（例:
expect(body).toEqual(issueCreatedBody)）。新しいMSWハンドラーはリクエストを検証せず、空のオブジェクトを返すだけです。これによりテストカバレッジが低下しています。次のようにリクエストボディの検証を追加することを検討してください:
export const githubApiHandlers = [ http.post("https://api.github.com/app/installations/2/access_tokens", () => { return HttpResponse.json({}); }), http.post( "https://api.github.com/repos/hiimbex/testing-things/issues/1/comments", - () => { + async ({ request }) => { + const body = await request.json(); + // リクエストボディの検証をここに追加 return HttpResponse.json({}); }, ), ];openci-runner/firebase/functions/test/src/index.test.ts (2)
6-6: lib への import 依存はビルド前提です
../../lib/probot/index.jsは事前ビルドが必要です。Vitest 実行前にビルドが走らない環境だと失敗します。srcから直接 import するか（tsconfig/vitest 設定でトランスパイル）、pretest でビルドするよう見直してください。
41-42: 空の afterEach は削除を不要なフックは消して簡潔にしましょう。
openci-runner/firebase/functions/test/src/probot/hetzner.test.ts (1)
1-7: 同様に lib import はビルド前提
../../../lib/probot/hetzner.jsへの依存は prebuild が必要です。srcからの import へ切り替えるか、テスト前にビルドされることを確認してください。openci-runner/firebase/functions/src/probot/hetzner.ts (1)
3-5: 型の所在
OctokitTokenは Hetzner ではなく GitHub に関する型です。github.tsか共通types.tsに移動を検討してください。openci-runner/firebase/functions/src/probot/index.ts (2)
63-75: 無制限ポーリング状態待ちの
while (true)は上限・タイムアウトを設定してください（例: 最大 N 回 or タイムアウト ms）。
106-127: 後続コマンドは github.ts の不具合の影響を受けます
initRunner/startRunnerのコマンド生成に不具合（後述の github.ts）があるため、ここはその修正が入るまで必ず失敗します。先に github.ts を修正してください。openci-runner/firebase/functions/src/probot/github.ts (2)
44-46:mkdirは冪等に再実行で失敗しないよう
-pを付与してください。-const mkdir = "mkdir actions-runner"; +const mkdir = "mkdir -p actions-runner";
5-8: 型:context: anyを避ける
probotのContext型を使ってください（必要なら最小限の型を定義）。-// biome-ignore lint/suspicious/noExplicitAny: <Fill the type later> -export function isJobRequired(context: any): boolean { +import type { Context } from "probot"; +export function isJobRequired(context: Context): boolean { return context.payload.workflow_job.labels.includes(runnerName); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
openci-runner/firebase/functions/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
openci-runner/firebase/functions/package.json(1 hunks)openci-runner/firebase/functions/probot/github.ts(0 hunks)openci-runner/firebase/functions/src/index.ts(1 hunks)openci-runner/firebase/functions/src/probot/github.ts(1 hunks)openci-runner/firebase/functions/src/probot/hetzner.ts(2 hunks)openci-runner/firebase/functions/src/probot/index.ts(3 hunks)openci-runner/firebase/functions/test/index.test.ts(0 hunks)openci-runner/firebase/functions/test/src/index.test.ts(1 hunks)openci-runner/firebase/functions/test/src/mocks/github_api_handlers.ts(1 hunks)openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts(1 hunks)openci-runner/firebase/functions/test/src/mocks/node.ts(1 hunks)openci-runner/firebase/functions/test/src/probot/hetzner.test.ts(1 hunks)openci-runner/firebase/functions/test/vitest.setup.ts(1 hunks)openci-runner/firebase/functions/tsconfig.json(1 hunks)openci-runner/firebase/functions/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (2)
- openci-runner/firebase/functions/probot/github.ts
- openci-runner/firebase/functions/test/index.test.ts
🧰 Additional context used
🧬 Code graph analysis (12)
openci-runner/firebase/functions/test/src/mocks/node.ts (3)
openci-runner/firebase/functions/test/src/mocks/github_api_handlers.ts (1)
githubApiHandlers(3-14)openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts (1)
hetznerApiHandlers(5-30)openci-runner/firebase/functions/probot/hetzner.ts (1)
createServer(21-49)
openci-runner/firebase/functions/test/src/mocks/github_api_handlers.ts (2)
openci-runner/firebase/functions/test/index.test.ts (5)
issueCreatedBody(36-59)probot(22-65)nock(27-34)nock(61-64)body(45-48)openci-runner/firebase/functions/probot/index.ts (3)
context(21-25)app(17-134)context(27-105)
openci-runner/firebase/functions/test/vitest.setup.ts (2)
openci-runner/firebase/functions/test/src/mocks/node.ts (1)
server(5-5)openci-runner/firebase/functions/test/index.test.ts (4)
probot(22-65)nock(61-64)issueCreatedBody(36-59)nock(27-34)
openci-runner/firebase/functions/vitest.config.ts (1)
openci-runner/firebase/functions/test/index.test.ts (5)
probot(22-65)issueCreatedBody(36-59)nock(61-64)nock(27-34)body(45-48)
openci-runner/firebase/functions/src/probot/hetzner.ts (1)
openci-runner/firebase/functions/probot/index.ts (2)
app(17-134)context(107-133)
openci-runner/firebase/functions/src/index.ts (2)
openci-runner/firebase/functions/probot/index.ts (1)
app(17-134)openci-runner/firebase/functions/test/index.test.ts (2)
nock(27-34)probot(22-65)
openci-runner/firebase/functions/tsconfig.json (3)
openci-runner/firebase/functions/test/index.test.ts (3)
probot(22-65)issueCreatedBody(36-59)nock(27-34)openci-runner/firebase/functions/probot/index.ts (4)
app(17-134)context(107-133)context(21-25)context(27-105)openci-runner/firebase/functions/probot/hetzner.ts (2)
initRunner(64-73)OctokitToken(3-5)
openci-runner/firebase/functions/test/src/index.test.ts (2)
openci-runner/firebase/functions/src/probot/index.ts (1)
appFn(24-171)openci-runner/firebase/functions/test/index.test.ts (5)
probot(22-65)issueCreatedBody(36-59)nock(27-34)body(45-48)nock(61-64)
openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts (1)
openci-runner/firebase/functions/probot/hetzner.ts (3)
createServer(21-49)deleteServer(7-14)getServerStatusById(51-62)
openci-runner/firebase/functions/src/probot/index.ts (2)
openci-runner/firebase/functions/src/probot/hetzner.ts (2)
createServerSpec(29-43)createServer(45-68)openci-runner/firebase/functions/src/probot/github.ts (2)
initRunner(59-70)startRunner(72-75)
openci-runner/firebase/functions/test/src/probot/hetzner.test.ts (1)
openci-runner/firebase/functions/src/probot/hetzner.ts (4)
deleteServer(20-27)createServerSpec(29-43)createServer(45-68)getServerStatusById(70-81)
openci-runner/firebase/functions/src/probot/github.ts (2)
openci-runner/firebase/functions/probot/hetzner.ts (2)
initRunner(64-73)OctokitToken(3-5)openci-runner/firebase/functions/probot/index.ts (4)
app(17-134)context(27-105)context(107-133)context(21-25)
🔇 Additional comments (5)
openci-runner/firebase/functions/package.json (1)
12-12: LGTM! MSWへの移行テストのHTTPモック機能をnockからMSWへ移行しています。MSWは現代的でより強力なモックライブラリです。
openci-runner/firebase/functions/src/index.ts (1)
5-5: LGTM! ディレクトリ構造の更新インポートパスの変更は、probotディレクトリがsrc/配下に移動したことを正しく反映しています。
openci-runner/firebase/functions/test/vitest.setup.ts (1)
1-6: LGTM! MSWライフサイクルのセットアップVitestのライフサイクルフックに対するMSWサーバーの標準的なセットアップです。beforeAll/afterEach/afterAllの構成は適切です。
openci-runner/firebase/functions/test/src/mocks/node.ts (1)
1-5: LGTM! MSWサーバーの集約GitHubとHetznerのAPIハンドラーを単一のMSWサーバーインスタンスに統合する、明確で標準的なアプローチです。
openci-runner/firebase/functions/vitest.config.ts (1)
1-7: LGTM! Vitest設定setupFilesがセットアップファイルを正しく参照している標準的なVitest設定です。
| export function createServerSpec( | ||
| image: string = "ubuntu-24.04", | ||
| location: string = "fsn1", | ||
| name: string = crypto.randomUUID(), | ||
| serverType: string = "cpx41", | ||
| sshKeyName: string = "openci-runner-probot", | ||
| ): HetznerServerSpec { |
There was a problem hiding this comment.
crypto.randomUUID() の未インポート（Node の型解決）
TS でグローバル crypto が解決されない場合があり得ます。node:crypto から randomUUID を import して使用してください。
+import { randomUUID } from "node:crypto";
...
-export function createServerSpec(
+export function createServerSpec(
image: string = "ubuntu-24.04",
location: string = "fsn1",
- name: string = crypto.randomUUID(),
+ name: string = randomUUID(),
serverType: string = "cpx41",
sshKeyName: string = "openci-runner-probot",
): HetznerServerSpec {📝 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.
| export function createServerSpec( | |
| image: string = "ubuntu-24.04", | |
| location: string = "fsn1", | |
| name: string = crypto.randomUUID(), | |
| serverType: string = "cpx41", | |
| sshKeyName: string = "openci-runner-probot", | |
| ): HetznerServerSpec { | |
| import { randomUUID } from "node:crypto"; | |
| export function createServerSpec( | |
| image: string = "ubuntu-24.04", | |
| location: string = "fsn1", | |
| name: string = randomUUID(), | |
| serverType: string = "cpx41", | |
| sshKeyName: string = "openci-runner-probot", | |
| ): HetznerServerSpec { |
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/src/probot/hetzner.ts around lines 29 to 35,
the default value uses crypto.randomUUID() which can fail TS resolution for the
global crypto; import randomUUID from 'node:crypto' at the top of the file and
replace crypto.randomUUID() with randomUUID() (or reference the imported symbol)
so the identifier is resolved correctly by Node typings and bundlers.
There was a problem hiding this comment.
どういう場合に、cryptoが解決されませんか？
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Node.js version requirement and TypeScript configuration
cat openci-runner/firebase/functions/package.json | jq -r '.engines'
echo "---"
cat openci-runner/firebase/functions/tsconfig.json
Length of output: 404
🏁 Script executed:
#!/bin/bash
# Check if @types/node is present and what version
cat openci-runner/firebase/functions/package.json | jq '.dependencies["@types/node"], .devDependencies["@types/node"]'
Length of output: 9
🏁 Script executed:
#!/bin/bash
# Check if there are any TypeScript compilation errors related to crypto
cd openci-runner/firebase/functions
npx tsc --noEmit 2>&1 | grep -i "crypto" || echo "No crypto-related TypeScript errors found"
Length of output: 41
@mafreud さん、申し訳ございません。私の以前の指摘は不正確でした。
実際にコードをチェックした結果、現在の設定では crypto.randomUUID() は問題なく動作します。理由は以下の通りです：
-
Node.js 22を使用している:
crypto.randomUUID()は Node.js 16.7.0 から利用可能で、Node.js 22 では完全にサポートされています -
TypeScript設定が適切:
tsconfig.jsonでmodule: "NodeNext"とmoduleResolution: "nodenext"を使用しているため、TypeScript は自動的に Node.js のビルトイン型を認識します -
コンパイルエラーなし: TypeScript のコンパイル時に crypto 関連のエラーは発生していません
したがって、node:crypto からの import は不要です。現在のコードのままで問題ありません。
私の以前のレビューコメントは解決（resolve）させていただきます。ご迷惑をおかけして申し訳ございませんでした。
🧬 Code Graph Analysis Results
openci-runner/firebase/functions/src/probot/hetzner.ts: createServerSpec (lines 29-43)
Summary:
- Function: createServerSpec(image, location, name, serverType, sshKeyName)
- Notes: Uses crypto.randomUUID() to generate a default server name when none is provided. This is relevant to understanding potential runtime issues if crypto is not available in the environment.
| test("creates a comment when an issue is opened", async () => { | ||
| await probot.receive({ | ||
| id: "", | ||
| name: "issues", | ||
| payload, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
アサーション不足で効果が検証できていません
API呼び出し（issues.createComment）が行われたことを検証してください。MSW で該当エンドポイントへの POST をアサートするか、ハンドラ内で expect を行う形にしましょう。
例（MSW での一例）:
test("creates a comment when an issue is opened", async () => {
- await probot.receive({
+ expect.assertions(1)
+ server.use(
+ rest.post("https://api.github.com/repos/hiimbex/testing-things/issues/1/comments", async (req, res, ctx) => {
+ const body = await req.json()
+ expect(body).toEqual({ body: "Hello, World!" })
+ return res(ctx.status(200))
+ })
+ )
+ await probot.receive({
id: "",
name: "issues",
payload,
});
});📝 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.
| test("creates a comment when an issue is opened", async () => { | |
| await probot.receive({ | |
| id: "", | |
| name: "issues", | |
| payload, | |
| }); | |
| }); | |
| test("creates a comment when an issue is opened", async () => { | |
| expect.assertions(1) | |
| server.use( | |
| rest.post( | |
| "https://api.github.com/repos/hiimbex/testing-things/issues/1/comments", | |
| async (req, res, ctx) => { | |
| const body = await req.json() | |
| expect(body).toEqual({ body: "Hello, World!" }) | |
| return res(ctx.status(200)) | |
| } | |
| ) | |
| ) | |
| await probot.receive({ | |
| id: "", | |
| name: "issues", | |
| payload, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/test/src/index.test.ts around lines 33 to
39, the test invokes probot.receive but contains no assertion verifying that the
GitHub API call (issues.createComment) occurred; update the test to assert the
POST to the issues.createComment endpoint—either by configuring MSW to expect
and assert a POST to the appropriate REST path (and awaiting/inspecting the
request), or by adding an expect inside the MSW handler to confirm it was called
with the correct payload; ensure the test awaits any async handlers and includes
assertions on request method, URL/path, and body (or that the handler was
invoked) so the effect is actually verified.
openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts
Outdated
Show resolved
Hide resolved
| test("delete a server", async () => { | ||
| await deleteServer("id", "apiKey"); | ||
| }); |
There was a problem hiding this comment.
アサーションなしのテストです
「delete a server」は何も検証していません。MSW ハンドラで DELETE が呼ばれたことをアサートしてください。
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/test/src/probot/hetzner.test.ts around lines
10-12, the "delete a server" test contains no assertions; update it to register
an MSW DELETE handler (or wrap the existing handler with a jest.fn) that records
when the DELETE endpoint is invoked, call deleteServer("id","apiKey"), then
assert the handler was called (e.g., expect(handlerMock).toHaveBeenCalled()) —
optionally also assert the request URL or response status to ensure the correct
endpoint was hit.
There was a problem hiding this comment.
@mafreud さん、以下のコードで DELETE エンドポイントが呼ばれたことを検証できます:
方法1: テスト内でハンドラーをオーバーライドする
import { describe, expect, test, vi } from "vitest";
import { http, HttpResponse } from "msw";
import { server } from "../mocks/node.js";
import {
createServer,
createServerSpec,
deleteServer,
getServerStatusById,
} from "../../../lib/probot/hetzner.js";
describe("Hetzner", () => {
test("delete a server", async () => {
// スパイ関数を作成
const deleteSpy = vi.fn();
// このテスト用にハンドラーをオーバーライド
server.use(
http.delete("https://api.hetzner.cloud/v1/servers/:id", ({ params }) => {
deleteSpy(params.id);
return HttpResponse.json({});
})
);
await deleteServer("id", "apiKey");
// DELETE が呼ばれたことを検証
expect(deleteSpy).toHaveBeenCalledWith("id");
expect(deleteSpy).toHaveBeenCalledTimes(1);
});
});方法2: グローバルハンドラーにスパイを追加する
test/src/mocks/hetzner_api_handlers.ts を修正:
import { http, HttpResponse } from "msw";
const baseUrl = "https://api.hetzner.cloud/v1/servers";
// エクスポート可能なスパイを作成
export const deleteServerSpy = vi.fn();
export const hetznerApiHandlers = [
http.delete(`${baseUrl}/:id`, ({ params }) => {
deleteServerSpy(params.id);
return HttpResponse.json({});
}),
// ... 他のハンドラー
];そしてテストで:
import { describe, expect, test, beforeEach } from "vitest";
import { deleteServerSpy } from "../mocks/hetzner_api_handlers.js";
import { deleteServer } from "../../../lib/probot/hetzner.js";
describe("Hetzner", () => {
beforeEach(() => {
// 各テスト前にスパイをリセット
deleteServerSpy.mockClear();
});
test("delete a server", async () => {
await deleteServer("id", "apiKey");
expect(deleteServerSpy).toHaveBeenCalledWith("id");
expect(deleteServerSpy).toHaveBeenCalledTimes(1);
});
});方法1 の方がテストの独立性が高いため推奨します。各テストで必要なハンドラーのみをオーバーライドできます。
…loadRunnerScriptAndUnZip function
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openci-runner/firebase/functions/src/probot/github.ts (2)
55-57:runGHAScriptが値を返していません（未修正）戻り値が
undefinedのため、startRunnerが壊れます。returnを追加してください。この問題は以前のレビューで指摘されましたが、まだ修正されていません。以下の diff を適用してください：
function runGHAScript(runnerConfig: string) { - `tmux new -d -s runner "RUNNER_ALLOW_RUNASROOT=true ./run.sh --jitconfig ${runnerConfig}"`; + return `tmux new -d -s runner "RUNNER_ALLOW_RUNASROOT=true ./run.sh --jitconfig ${runnerConfig}"`; }
72-75:startRunnerのコマンド生成が壊れています（未修正）
runGHAScriptがundefinedを返すため、結合されたコマンドが"cd actions-runner&&undefined"になり、無効なコマンドになります。この問題は以前のレビューで指摘されましたが、まだ修正されていません。さらに、戻り値型を明示し、
&&の結合に空白を追加してください。以下の diff を適用してください（
runGHAScriptの修正後）：-export function startRunner(runnerConfig: string) { +export function startRunner(runnerConfig: string): string { const command = [cdActionRunner, runGHAScript(runnerConfig)]; - return command.join("&&"); + return command.join(" && "); }
🧹 Nitpick comments (2)
openci-runner/firebase/functions/src/probot/github.ts (2)
10-32: 冗長なテンプレートリテラルを削除できます
ownerとrepoは既に文字列なので、テンプレートリテラルでラップする必要はありません。以下の diff を適用してください：
labels: ["openci-runner-beta"], name: `OpenCI ランナー ${serverId}`, - owner: `${owner}`, - repo: `${repo}`, + owner: owner, + repo: repo, runner_group_id: 1, work_folder: "_work",
44-53: コマンド結合に空白を追加することを推奨します
&&の前後に空白を追加すると、シェルコマンドの可読性が向上します。以下の diff を適用してください：
const fileName = `actions-runner-${os}-${architecture}-${scriptVersion}.tar.gz`; - return `curl -o ${fileName} -L https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`; + return `curl -o ${fileName} -L https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
openci-runner/firebase/functions/src/probot/github.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/src/probot/github.ts (2)
openci-runner/firebase/functions/probot/index.ts (4)
app(17-134)context(27-105)context(107-133)context(21-25)openci-runner/firebase/functions/probot/hetzner.ts (2)
initRunner(64-73)OctokitToken(3-5)
🔇 Additional comments (2)
openci-runner/firebase/functions/src/probot/github.ts (2)
1-4: LGTM!インポートと定数定義は適切です。
34-42: 修正完了を確認しましたEnum を文字列列挙型に変更した点は良好です。これにより、
toString()の問題が解決されました。
| // biome-ignore lint/suspicious/noExplicitAny: <Fill the type later> | ||
| export function isJobRequired(context: any): boolean { | ||
| return context.payload.workflow_job.labels.includes(runnerName); | ||
| } |
There was a problem hiding this comment.
labels の null チェックを追加してください
workflow_job.labels が undefined または null の場合、ランタイムエラーが発生します。includes を呼ぶ前に存在チェックを追加してください。
以下の diff を適用して修正してください：
export function isJobRequired(context: any): boolean {
- return context.payload.workflow_job.labels.includes(runnerName);
+ return context.payload.workflow_job.labels?.includes(runnerName) ?? false;
}📝 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.
| // biome-ignore lint/suspicious/noExplicitAny: <Fill the type later> | |
| export function isJobRequired(context: any): boolean { | |
| return context.payload.workflow_job.labels.includes(runnerName); | |
| } | |
| // biome-ignore lint/suspicious/noExplicitAny: <Fill the type later> | |
| export function isJobRequired(context: any): boolean { | |
| return context.payload.workflow_job.labels?.includes(runnerName) ?? false; | |
| } |
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/src/probot/github.ts around lines 5 to 8,
isJobRequired calls context.payload.workflow_job.labels.includes(...) without
guarding for null/undefined and will throw if labels is missing; update the
function to first check that context.payload?.workflow_job?.labels is an array
(e.g. Array.isArray) and return false if not, then call includes on the
validated array (or use optional chaining with a boolean fallback) so includes
is only invoked on a defined array.
… for ActionsRunner enums
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
openci-runner/firebase/functions/src/probot/github.ts (2)
72-75: 過去のレビューで指摘された改善がまだ一部未対応です戻り値の型を明示し、コマンド連結に空白を追加することを推奨します。
以下の diff を適用してください：
-export function startRunner(runnerConfig: string) { +export function startRunner(runnerConfig: string): string { const command = [cdActionRunner, runGHAScript(runnerConfig)]; - return command.join("&&"); + return command.join(" && "); }
5-8: 過去のレビューで指摘されたlabelsの null チェックがまだ未対応です
context.payload.workflow_job.labelsがundefinedまたはnullの場合、ランタイムエラーが発生します。過去のレビューで指摘されたこの問題を修正してください。以下の diff を適用してください：
export function isJobRequired(context: any): boolean { - return context.payload.workflow_job.labels.includes(runnerName); + return context.payload.workflow_job.labels?.includes(runnerName) ?? false; }
🧹 Nitpick comments (1)
openci-runner/firebase/functions/src/probot/github.ts (1)
59-70: シェルコマンドの連結に空白を追加することを推奨します
command.join("&&")は動作しますが、可読性のために&&の前後に空白を追加することをお勧めします。以下の diff を適用してください：
export function initRunner( scriptVersion: string, os: ActionsRunnerOS, architecture: ActionsRunnerArchitecture, ): string { const command = [ mkdir, cdActionRunner, downloadRunnerScriptAndUnZip(scriptVersion, os, architecture), ]; - return command.join("&&"); + return command.join(" && "); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
openci-runner/firebase/functions/src/probot/github.ts(1 hunks)openci-runner/firebase/functions/test/src/probot/github.test.ts(1 hunks)
🔇 Additional comments (2)
openci-runner/firebase/functions/src/probot/github.ts (2)
10-32: LGTM! JIT config の取得実装は適切ですGitHub API を使用して JIT config を生成する実装が正しく行われています。エラーハンドリングは暗黙的ですが、この実装で十分です。
34-42: LGTM! 過去の enum の問題が修正されています過去のレビューで指摘された数値列挙の問題が、文字列列挙への変更によって適切に解決されています。これにより
actions-runner-linux-x64-...のような正しいファイル名が生成されます。
| import { describe, expect, test } from "vitest"; | ||
| import { | ||
| ActionsRunnerArchitecture, | ||
| ActionsRunnerOS, | ||
| } from "../../../src/probot/github"; | ||
|
|
||
| describe("github", () => { | ||
| test("ActionsRunnerOS", () => { | ||
| const osx = ActionsRunnerOS.osx; | ||
| expect(osx).toBe("osx"); | ||
|
|
||
| const linux = ActionsRunnerOS.linux; | ||
| expect(linux).toBe("linux"); | ||
| }); | ||
|
|
||
| test("ActionsRunnerArchitecture", () => { | ||
| const arm64 = ActionsRunnerArchitecture.arm64; | ||
| expect(arm64).toBe("arm64"); | ||
|
|
||
| const x64 = ActionsRunnerArchitecture.x64; | ||
| expect(x64).toBe("x64"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
テストカバレッジが不十分です
このテストファイルは enum の値のみを検証していますが、以下の重要な関数がテストされていません：
isJobRequired(特に null チェックの動作)getJitConfig(API 統合)initRunner(コマンド生成)startRunner(コマンド生成)
これらの関数に対するテストを追加することを強く推奨します。特に isJobRequired の null/undefined ケースのテストは重要です。
テストコードの生成をお手伝いしましょうか？新しい issue を作成しますか？
🤖 Prompt for AI Agents
In openci-runner/firebase/functions/test/src/probot/github.test.ts lines 1-23,
the current tests only assert enum values and miss coverage for key functions;
add unit tests covering isJobRequired (including explicit null/undefined and
various job payloads to assert true/false behavior), getJitConfig (mock the API
responses and assert returned config and error handling), initRunner (verify
generated command/args given different inputs and edge cases), and startRunner
(verify final composed command, environment variables, and handling of missing
params); use vitest mocks for external calls, supply representative fixtures for
job payloads, and include negative/null cases for isJobRequired as requested.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
openci-runner/firebase/functions/test/src/probot/github.test.ts (2)
8-23: enum テストは冗長ですこれらのテストは enum 値を読み取って同じ値を期待するだけで、実質的な価値がありません。TypeScript により型安全性は保証されているため、このようなテストは不要です。
より意味のあるテストに置き換えるか、削除することを検討してください。例えば、これらの enum が実際に使用される関数（
initRunnerやstartRunnerなど）の動作をテストする方が有益です。
25-42: エッジケースのテストを追加してください現在のテストは正常系のみをカバーしていますが、以下のエッジケースのテストを追加することを推奨します：
- 空文字列の
ownerやrepo- 特殊文字を含む文字列（例：
ownerに/や@を含む）- 負の値や極端に大きい値の
serverIdserverIdが0の場合のラベル名の検証これらのテストにより、関数の堅牢性を確認できます。
適用可能な差分の例：
+ test("jitConfigRequestBody with edge cases", () => { + // Empty strings + const emptyRes = jitConfigRequestBody("", "", 0); + expect(emptyRes.owner).toBe(""); + expect(emptyRes.repo).toBe(""); + + // Special characters + const specialRes = jitConfigRequestBody("owner/test", "repo@123", 999); + expect(specialRes.owner).toBe("owner/test"); + expect(specialRes.repo).toBe("repo@123"); + expect(specialRes.name).toBe("OpenCI ランナー 999"); + + // Negative serverId + const negativeRes = jitConfigRequestBody(owner, repo, -1); + expect(negativeRes.name).toBe("OpenCI ランナー -1"); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
openci-runner/firebase/functions/src/probot/github.ts(1 hunks)openci-runner/firebase/functions/test/src/index.test.ts(1 hunks)openci-runner/firebase/functions/test/src/probot/github.test.ts(1 hunks)openci-runner/firebase/functions/test/src/probot/hetzner.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- openci-runner/firebase/functions/test/src/index.test.ts
- openci-runner/firebase/functions/test/src/probot/hetzner.test.ts
- openci-runner/firebase/functions/src/probot/github.ts
🧰 Additional context used
🧬 Code graph analysis (1)
openci-runner/firebase/functions/test/src/probot/github.test.ts (1)
openci-runner/firebase/functions/src/probot/github.ts (1)
jitConfigRequestBody(23-39)
…up unused imports in github.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openci-runner/firebase/functions/test/src/probot/github.test.ts (1)
1-23: 既存のテストは正確ですが、カバレッジは依然として不完全です
jitConfigRequestBody関数のテストは適切に実装されており、すべてのプロパティが正しく検証されています。ただし、以前のレビューで指摘されたように、以下の重要な関数のテストが依然として不足しています:
isJobRequired(特にnull/undefinedケース)getJitConfig(API統合)initRunner(コマンド生成)startRunner(コマンド生成)これらのテストは将来的に追加することをお勧めします。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts(1 hunks)openci-runner/firebase/functions/test/src/probot/github.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts (1)
openci-runner/firebase/functions/probot/hetzner.ts (3)
createServer(21-49)deleteServer(7-14)getServerStatusById(51-62)
openci-runner/firebase/functions/test/src/probot/github.test.ts (2)
openci-runner/firebase/functions/src/probot/github.ts (1)
jitConfigRequestBody(23-39)openci-runner/firebase/functions/probot/github.ts (1)
getJitConfig(10-32)
🔇 Additional comments (2)
openci-runner/firebase/functions/test/src/mocks/hetzner_api_handlers.ts (2)
6-8: 過去のレビューコメントで指摘された問題が修正されていますパスパラメータ
:idの使用により、実際のAPIコール（例:/12345）に正しくマッチするようになりました。
23-29: LGTM!GETハンドラーの実装は正確で、パスパラメータも適切に使用されています。
…ber instead of string
…e as string, and update command concatenation for clarity. Update ssh command in appFn to include package update before installation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openci-runner/firebase/functions/src/probot/github.ts (1)
6-8:labelsの null/undefined チェックが未対応です。過去のレビューで指摘されましたが、
context.payload.workflow_job.labelsがundefinedまたはnullの場合、ランタイムエラーが発生します。以下のdiffを適用して修正してください:
export function isJobRequired(context: any): boolean { - return context.payload.workflow_job.labels.includes(runnerName); + return context.payload.workflow_job.labels?.includes(runnerName) ?? false; }
🧹 Nitpick comments (2)
openci-runner/firebase/functions/src/probot/hetzner.ts (1)
29-43: オブジェクト短縮記法の使用を検討してください。プロパティ名と変数名が同じ場合、TypeScriptのオブジェクト短縮記法を使用するとコードがより簡潔になります。
以下のdiffを適用してリファクタリングできます:
export function createServerSpec( image: string = "ubuntu-24.04", location: string = "fsn1", name: string = crypto.randomUUID(), serverType: string = "cpx41", sshKeyName: string = "openci-runner-probot", ): HetznerServerSpec { return { - image: image, - location: location, - name: name, + image, + location, + name, server_type: serverType, ssh_keys: [sshKeyName], }; }openci-runner/firebase/functions/src/probot/index.ts (1)
108-119: ランナーのバージョンを設定可能にすることを検討してください。行110でランナースクリプトのバージョン
"2.328.0"がハードコードされています。新しいバージョンがリリースされた際の保守性を向上させるため、環境変数や設定ファイルから取得できるようにすることを推奨します。例:
const scriptVersion = process.env.RUNNER_VERSION || "2.328.0"; const resInitRunner = await sshResult.execCommand( initRunner( scriptVersion, ActionsRunnerOS.linux, ActionsRunnerArchitecture.x64, ), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
openci-runner/firebase/functions/src/probot/github.ts(1 hunks)openci-runner/firebase/functions/src/probot/hetzner.ts(2 hunks)openci-runner/firebase/functions/src/probot/index.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
openci-runner/firebase/functions/src/probot/github.ts (3)
openci-runner/firebase/functions/probot/index.ts (4)
app(17-134)context(27-105)context(107-133)context(21-25)openci-runner/firebase/functions/probot/hetzner.ts (2)
initRunner(64-73)OctokitToken(3-5)openci-runner/firebase/functions/probot/github.ts (2)
getJitConfig(10-32)isJobRequired(6-8)
openci-runner/firebase/functions/src/probot/index.ts (2)
openci-runner/firebase/functions/src/probot/hetzner.ts (2)
createServerSpec(29-43)createServer(45-68)openci-runner/firebase/functions/src/probot/github.ts (2)
initRunner(83-94)startRunner(96-99)
openci-runner/firebase/functions/src/probot/hetzner.ts (2)
openci-runner/firebase/functions/probot/index.ts (2)
app(17-134)context(107-133)openci-runner/firebase/functions/probot/github.ts (1)
getJitConfig(10-32)
🔇 Additional comments (6)
openci-runner/firebase/functions/src/probot/hetzner.ts (1)
12-18: LGTM:ssh_keysが配列型に修正されました。過去のレビューで指摘された
ssh_keys: [string](1要素タプル) の問題がssh_keys: string[]に修正されており、複数のSSHキーを許容できるようになっています。openci-runner/firebase/functions/src/probot/index.ts (2)
57-61: LGTM:serverSpecを使用したサーバー作成フローが適切に実装されています。
createServerSpec()を使用してサーバー仕様を作成し、createServer()に渡す実装は明確で保守性が高いです。
99-106: LGTM: tmuxのインストールが堅牢になりました。過去のレビューで指摘された非対話的インストールの問題が修正されています:
apt-get update -y && apt-get install -y tmuxを使用- 終了コードをチェックして失敗時にエラーをスロー
実装は適切です。
openci-runner/firebase/functions/src/probot/github.ts (3)
23-39: LGTM: JIT設定リクエストボディの構築が適切です。関数の分離により、テスタビリティと保守性が向上しています。
56-64: LGTM: 列挙型が文字列enumに修正されました。過去のレビューで指摘された数値enumの問題が解決されており、
toString()が正しい文字列値を返すようになっています。これによりファイル名が正しく構築されます。
96-99: LGTM: コマンドの結合が適切に修正されました。過去のレビューで指摘されたコマンド結合の問題が解決されており、
&&で正しく結合されています。
| function downloadRunnerScriptAndUnZip( | ||
| scriptVersion: string, | ||
| os: ActionsRunnerOS, | ||
| architecture: ActionsRunnerArchitecture, | ||
| ) { | ||
| const fileName = `actions-runner-${os}-${architecture}-${scriptVersion}.tar.gz`; | ||
| return `curl -o ${fileName} -L https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`; | ||
| } |
There was a problem hiding this comment.
curlコマンドにエラーハンドリングとチェックサム検証を追加してください。
現在の実装には以下の問題があります:
- curlの
-fフラグが未指定: HTTPエラー(404, 500など)が発生してもダウンロードが成功したと判断されます - チェックサム検証なし: ダウンロードしたバイナリの整合性が検証されていないため、破損したファイルや改ざんされたファイルを実行する可能性があります
以下の修正を推奨します:
function downloadRunnerScriptAndUnZip(
scriptVersion: string,
os: ActionsRunnerOS,
architecture: ActionsRunnerArchitecture,
) {
const fileName = `actions-runner-${os}-${architecture}-${scriptVersion}.tar.gz`;
- return `curl -o ${fileName} -L https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`;
+ return `curl -fsSL -o ${fileName} https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`;
}さらに、チェックサム検証を追加することを強く推奨します。GitHubはランナーリリースと共にSHA256チェックサムを提供しています。
📝 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.
| function downloadRunnerScriptAndUnZip( | |
| scriptVersion: string, | |
| os: ActionsRunnerOS, | |
| architecture: ActionsRunnerArchitecture, | |
| ) { | |
| const fileName = `actions-runner-${os}-${architecture}-${scriptVersion}.tar.gz`; | |
| return `curl -o ${fileName} -L https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`; | |
| } | |
| function downloadRunnerScriptAndUnZip( | |
| scriptVersion: string, | |
| os: ActionsRunnerOS, | |
| architecture: ActionsRunnerArchitecture, | |
| ) { | |
| const fileName = `actions-runner-${os}-${architecture}-${scriptVersion}.tar.gz`; | |
| return `curl -fsSL -o ${fileName} https://github.com/actions/runner/releases/download/v${scriptVersion}/${fileName} && tar xzf ./${fileName}`; | |
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue for curl error handling improvement
gh issue create \
--title "curlコマンドにエラーハンドリングとチェックサム検証を追加" \
--body "## 概要
\`downloadRunnerScriptAndUnZip\` 関数内の curl コマンドにエラーハンドリングとチェックサム検証を追加する必要があります。
## 問題点
1. **curlの \`-f\` フラグが未指定**: HTTPエラー(404, 500など)が発生してもダウンロードが成功したと判断されます
2. **チェックサム検証なし**: ダウンロードしたバイナリの整合性が検証されていないため、破損したファイルや改ざんされたファイルを実行する可能性があります
## 推奨される修正
\`curl -fsSL\` フラグを使用し、GitHubが提供するSHA256チェックサムを使用した検証を追加することを推奨します。
## 関連リンク
- PR: https://github.com/open-ci-io/openci/pull/507
- コメント: https://github.com/open-ci-io/openci/pull/507#discussion_r2427855769
- リクエスト者: @mafreud
## 対象ファイル
\`openci-runner/firebase/functions/src/probot/github.ts\` の \`downloadRunnerScriptAndUnZip\` 関数" \
--assignee mafreud
Length of output: 47
curlコマンドのエラーハンドリングとチェックサム検証の追加に関する詳細を記載し、あなたにアサインしました。
src/probotdirectory.package.jsonto includemswfor mocking API requests in tests.Summary by CodeRabbit