feat: resolve all follow-up tasks for unity-cli migration (#8)#12
Conversation
Implement all 7 sub-issues for the post-migration tracking issue: - #1: LSP local test environment (CI workflow, pre-push hook, Dockerfile, CONTRIBUTING.md) - #2: Unity E2E test infrastructure (CI job, e2e-test.sh, docs) - #3: Performance benchmarks (benchmark.sh, baseline docs) - #4: Specs audit and migration notes (specs/ directory with architecture docs) - #5: UNITY_MCP_* deprecation policy (config.rs warnings, configuration docs) - #6: Release pipeline hardening (publish.sh, release workflow, RELEASE.md update) - #7: MIT attribution templates (ATTRIBUTION.md, UPM docs links) Also add unity-mcp-server successor context to CLAUDE.md and README.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthroughunity-cli移行後のフォローアップを実装:リリース自動化、.NET テスト CI 統合、E2E テスト基盤、ベンチマークツール、環境変数廃止ポリシー、包括的なドキュメント更新を追加。 Changes
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
- Apply cargo fmt formatting to config.rs - Regenerate pnpm-lock.yaml to match package.json - Update specs/specs.md via update-specs-readme.sh Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/development.md (1)
123-130:⚠️ Potential issue | 🟡 Minorリリースフローのワークフローファイル名を確認してください。
Line 127 と Line 311 で
.github/workflows/unity-cli-release.ymlを参照していますが、本 PR で追加されたワークフローは.github/workflows/release.ymlの可能性があります。specs/migration-notes.mdにも同様の記述があり、一括で修正が必要です。Also applies to: 307-314
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development.md` around lines 123 - 130, Update every occurrence of the workflow filename string ".github/workflows/unity-cli-release.yml" to the actual workflow name ".github/workflows/release.yml" across the docs introduced in this PR (search whole repo for that exact string), e.g. the references appearing in development.md and migration-notes.md, so all documentation points to the correct workflow file.scripts/publish.sh (3)
169-187:⚠️ Potential issue | 🟡 Minorプッシュ失敗時のエラーハンドリングが不十分です。
Line 173 で
git push --follow-tagsが失敗した場合、echoで警告を出すだけで処理を継続します。Line 174 と 178 でも|| trueによりタグプッシュの失敗が無視されます。set -eが有効ですが|| true/|| echoでマスクされているため、プッシュが部分的に失敗した状態(コミットは送信済み・タグは未送信、またはその逆)が発生し得ます。Line 189-207 のリトライロジックが後続で実行されるため致命的ではありませんが、
allモードでのコミットプッシュ失敗は検出されずに見過ごされる可能性があります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish.sh` around lines 169 - 187, In the PUSH_MODE case handling (symbols: PUSH_MODE, the 'all' case, git push --follow-tags, and git push "$REMOTE" "$TAG"), stop masking failures with "|| echo" and "|| true": let git push commands return non-zero on failure so set -e/your retry logic can detect them; replace the silent fallthroughs by emitting a clear error to stderr (e.g., with >&2) and exiting non‑zero or invoking the existing retry routine when git push --follow-tags or the explicit tag push fails, and do the same for the 'tags' case so tag push failures are not ignored. Ensure the change keeps the current logging messages but propagates errors instead of swallowing them.
6-16:⚠️ Potential issue | 🔴 Critical
publish.shの引数仕様とRELEASE.mdの記述が矛盾しています。スクリプトは
<major|minor|patch>を受け付けますが(Line 6, 16)、RELEASE.mdでは./scripts/publish.sh 0.2.0のように具体的なバージョン番号を渡す手順が記載されています。また、RELEASE.mdには「Cargo.tomlのバージョンが一致するか検証」「mainブランチであることを確認」等のチェックが記載されていますが、スクリプトにはこれらのロジックが存在しません。ドキュメントとスクリプトのどちらかを修正して整合させてください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish.sh` around lines 6 - 16, The doc/scrips mismatch: make publish.sh accept a specific semver instead of the current <major|minor|patch> mode and add the missing checks referenced in RELEASE.md. Update the usage() and LEVEL parsing to accept a semver string (e.g., 0.2.0) and implement: a semver validation step for LEVEL, a check_git_branch function to ensure current branch is main, and a compare_version function that reads Cargo.toml (and other manifests) to verify their versions match LEVEL before tagging; on mismatch print a clear error and exit. Keep existing flags (--tags-only|--no-push|--remote) behavior unchanged and update the usage text and comments to match RELEASE.md.
97-123:⚠️ Potential issue | 🟡 MinormacOS(BSD sed)での
\n置換の動作を修正してください。
sed -i.bakは\nをダブルクォート内で使用している場合、GNU sed(Linux)では改行として解釈されますが、BSD sed(macOS)では改行ではなく文字列\nそのものが挿入されます(105, 110, 115, 120行)。この場合、XML の PropertyGroup 要素が正しくフォーマットされません。macOS での動作を保証する場合は、以下のいずれかの方法を採用してください:
- ANSI-C クォーティング(
$'...')を使用する:sed -i.bak -E $'s|<PropertyGroup>|<PropertyGroup>\\\n <Version>${ver}</Version>|'- リテラル改行を含める:バックスラッシュの後に実際の改行を入力する
printfを使用して XML を生成する(より堅牢)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish.sh` around lines 97 - 123, The sed substitutions in function sync_props insert the literal sequence "\n" on macOS (BSD sed) instead of a newline, breaking PropertyGroup formatting; update the four sed invocations that append a new tag (the ones that replace "<PropertyGroup>") in sync_props to produce real newlines on macOS — for example switch the sed argument to ANSI-C quoting ($'...') so "\\n" becomes an actual newline, or alternatively build the replacement string with printf and pipe into sed (or use a literal newline in the sed script); ensure each replacement for Version, AssemblyVersion, FileVersion, and AssemblyInformationalVersion uses the same cross-platform approach and keep the existing .bak behavior and cleanup with rm -f "$file.bak".
🤖 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 28-45: The Determine tag step (id: extract) currently expands the
GitHub expression ${{ inputs.release_tag }} directly into the shell, which
allows command injection; instead, read the input into a safe environment
variable (e.g., RELEASE_TAG) via the step's env mapping and reference that env
var inside the script (use "$RELEASE_TAG" with proper quoting), then perform the
same regex validation against the env var and derive TAG and VERSION from it
before writing to GITHUB_OUTPUT; update references to TAG in the step to use the
env-sourced value to eliminate direct expression expansion.
- Around line 51-58: The current extraction of CARGO_VERSION using grep
'^version' is fragile because it can match version fields outside the [package]
section; replace that extraction in the workflow (the CARGO_VERSION assignment)
with a robust method such as invoking cargo metadata to read the package version
(e.g., use `cargo metadata --no-deps --format-version 1` and jq to select
.packages[0].version) or implement a parser that reads the version specifically
from the [package] section; ensure the variable CARGO_VERSION is set from this
robust source and keep the subsequent comparison against ${{
steps.extract.outputs.version }} unchanged.
In @.github/workflows/test.yml:
- Around line 30-31: Update the CI test step named "Run cargo test" so its run
command matches docs/development.md by using the same flags; replace the current
plain `cargo test` invocation with `cargo test --all-targets` (or explicitly
document why you are intentionally diverging) to ensure CI builds/tests
docs/benchmarks and other targets as described.
- Around line 33-45: Add NuGet package caching to the lsp-test job to speed CI
runs: in the lsp-test job (job name "lsp-test") add a step using
actions/cache@v4 before restoring/building/tests to cache the NuGet package
folder (e.g., ~/.nuget/packages and optionally ~/.local/share/NuGet or
%USERPROFILE%/.nuget/packages on Windows) with a cache key that includes
runner.os and the solution/lockfile checksum (e.g., nuget-${{ runner.os }}-${{
hashFiles('**/*.csproj') }} or packages.lock.json) so dotnet commands in the
subsequent "Run dotnet test" step can reuse packages.
In `@ATTRIBUTION.md`:
- Line 16: Update each bare copyright string "Copyright (c) akiojin" to include
the copyright year (e.g., "Copyright (c) 2024 akiojin"); replace the occurrences
at the shown spot and the other occurrences mentioned (the lines containing the
same "Copyright (c) akiojin" strings around the other reported locations) so all
instances are consistent and include the year.
In `@CONTRIBUTING.md`:
- Line 54: The sentence ".NET SDK 9 が必要です。" is in the English section of
CONTRIBUTING.md; move it into the Japanese section or replace it with an English
equivalent (e.g., "The .NET SDK 9 is required. Verify with `dotnet --version`
that you have 9.x installed.") so the language sections remain consistent;
update the surrounding paragraph accordingly and ensure the `dotnet --version`
instruction stays intact.
In `@RELEASE.md`:
- Around line 3-19: RELEASE.md の「Quick Start」が scripts/publish.sh
の実際の振る舞いと不一致しています; 修正方法は二択です:A) ドキュメント側を scripts/publish.sh の実装に合わせて更新し、引数が
major|minor|patch であること、スクリプトが main ブランチチェックや Cargo.toml
バージョン検証、対話的確認を行わないこと、そして実際の公開は行わず `cargo publish --dry-run` のみ実行する旨を明記する、または B)
scripts/publish.sh を拡張して RELEASE.md
に書かれている動作を満たす(引数受け取りの仕様をバージョン番号に合わせるか追加し、main ブランチチェックと Cargo.toml
バージョン検証を実装し、対話的確認プロンプトを追加し、実際の `cargo publish`
を行うオプションを追加する)—どちらを採るか決めたら該当箇所(RELEASE.md の手順記述と scripts/publish.sh
の引数処理・publish ロジック)をそれぞれ整合させてください。
In `@scripts/benchmark.sh`:
- Around line 104-112: The fallback timestamp command using date +%s%N is
invalid on macOS and can return a literal "N", breaking arithmetic; update the
start_ns and end_ns command substitutions (the lines setting start_ns="$(python3
... || date +%s%N)" and end_ns=...) to use a portable nanosecond fallback such
as invoking perl/Time::HiRes to print an integer nanosecond timestamp (or
alternatively use date +%s and multiply by 1_000_000_000 with proper
zero-padding), ensuring the variables start_ns and end_ns always contain numeric
values so elapsed_ms and total_ms arithmetic in the for loop (RUNS,
measurements, total_ms, elapsed_ms) succeeds.
- Around line 64-67: run_benchmark の第1引数 name が割り当てられているだけで使われていないため SC2034
を発生させています。run_benchmark 内で name をベンチマーク開始/終了ログやエラーメッセージに含めて参照する(例: "Starting
benchmark: ${name}" など)か、意図的に未使用であれば引数名を _name にリネームして未使用を明示してください。関数名
run_benchmark と引数 name を探して、どちらの方針を採るか決めて一貫して適用してください。
- Around line 184-215: The current here-doc directly interpolates shell
variables into JSON (when JSON_OUTPUT is true), which can produce invalid JSON
if values like CLI_VERSION or BINARY_PATH contain quotes or backslashes; replace
the literal heredoc construction with a safe serializer: collect the variables
(TIMESTAMP, OS_NAME, OS_VERSION, ARCH, RUST_VERSION, CLI_VERSION, BINARY_PATH,
HAS_HYPERFINE, WARMUP, RUNS, HELP_MEAN, HELP_STDDEV, TOOL_LIST_MEAN,
TOOL_LIST_STDDEV, PING_MEAN, PING_STDDEV, PING_SKIPPED) and emit JSON via a safe
tool (e.g., call python3 -c using json.dumps or use jq -n with --arg/--argjson)
so strings are properly escaped and numeric/boolean values are typed correctly;
update the JSON_OUTPUT branch to build and print JSON using that serializer
instead of the unescaped heredoc.
In `@scripts/e2e-test.sh`:
- Around line 7-8: The script sets PORT="8080" which conflicts with the
project's documented default and UNITY_CLI_PORT (6400); update the PORT default
to "6400" to match docs/configuration.md and UNITY_CLI_PORT so defaults are
consistent across the codebase (change the PORT assignment in the script to
PORT="6400").
- Around line 14-20: The case branches for --host and --port access $2 directly
which triggers an "unbound variable" error under set -u when the option has no
following argument; update the --host and --port handlers to first verify there
is a next argument (e.g. check if $# -ge 2) and that $2 is not another option
(doesn't start with '-') before assigning HOST="$2" or PORT="$2", otherwise
print a clear error message explaining the missing value for --host/--port and
exit with non-zero status; ensure you still shift by 2 only when the value check
passes.
In `@scripts/publish.sh`:
- Around line 52-67: The script runs the test block after running npm version
which creates a commit and tag; to fix either move the test block to run before
the npm version call (so tests are validated prior to bumping) and remove the
duplicate post-bump test block, or wrap the npm version step with rollback
logic: run npm version, then run the existing test block, and if tests fail run
git reset --hard HEAD~1 and git tag -d "$(node -p
\"require('./package.json').version\")" (or parse the new version from npm
output) to remove the bump commit and tag before exiting nonzero; update the
script around the npm version invocation and the test block accordingly.
In `@specs/architecture.md`:
- Around line 105-116: The environment variable table in specs/architecture.md
is missing the default for UNITY_CLI_TIMEOUT_MS; update the table entry to show
the default value 30000 to match src/config.rs (where UNITY_CLI_TIMEOUT_MS is
set to 30000) and README.md (which already lists 30000), ensuring the table
formatting remains intact and consistent with the other rows.
In `@src/config.rs`:
- Around line 34-38: CI is failing due to rustfmt differences in src/config.rs;
run rustfmt (cargo fmt --all) and commit the formatted changes so the call to
read_env_with_deprecation (and its long argument tuple of ("UNITY_MCP_MCP_HOST",
"UNITY_CLI_HOST") / ("UNITY_MCP_UNITY_HOST", "UNITY_CLI_HOST")) is line-wrapped
according to rustfmt rules; ensure DEFAULT_HOST fallback remains unchanged and
that symbols read_env_with_deprecation, DEFAULT_HOST, UNITY_CLI_HOST,
UNITY_MCP_MCP_HOST, UNITY_MCP_UNITY_HOST are preserved exactly.
- Around line 92-110: The parsing helpers read_env_u16_with_deprecation and
read_env_u64_with_deprecation silently drop invalid numeric env values; change
them to first call read_env_with_deprecation to get the raw string, and if
present attempt parse and on parse failure emit a warning log that includes the
env key(s) and the invalid value (e.g., using the crate's logging macro like
warn! or tracing::warn!), then return None so the default is used; ensure you
reference the same primary_keys/deprecated_keys when logging so the message
helps the user find the bad setting.
- Around line 112-134: The tests use env::set_var/remove_var under a
module-local Mutex (ENV_LOCK) via with_env_vars, which doesn't prevent
cross-module races when cargo runs tests multithreaded; fix by serializing
access across the whole test suite—either run tests single-threaded (cargo test
-- --test-threads=1) or add a global synchronization strategy: introduce a
shared test helper (e.g., a tests::global_lock or a new crate/module) and
replace per-module ENV_LOCK uses (and have with_env_vars acquire that global
lock), or add the serial_test crate and annotate env-mutating tests to run
serially so that ENV_LOCK and with_env_vars serialize environment changes across
modules.
---
Outside diff comments:
In `@docs/development.md`:
- Around line 123-130: Update every occurrence of the workflow filename string
".github/workflows/unity-cli-release.yml" to the actual workflow name
".github/workflows/release.yml" across the docs introduced in this PR (search
whole repo for that exact string), e.g. the references appearing in
development.md and migration-notes.md, so all documentation points to the
correct workflow file.
In `@scripts/publish.sh`:
- Around line 169-187: In the PUSH_MODE case handling (symbols: PUSH_MODE, the
'all' case, git push --follow-tags, and git push "$REMOTE" "$TAG"), stop masking
failures with "|| echo" and "|| true": let git push commands return non-zero on
failure so set -e/your retry logic can detect them; replace the silent
fallthroughs by emitting a clear error to stderr (e.g., with >&2) and exiting
non‑zero or invoking the existing retry routine when git push --follow-tags or
the explicit tag push fails, and do the same for the 'tags' case so tag push
failures are not ignored. Ensure the change keeps the current logging messages
but propagates errors instead of swallowing them.
- Around line 6-16: The doc/scrips mismatch: make publish.sh accept a specific
semver instead of the current <major|minor|patch> mode and add the missing
checks referenced in RELEASE.md. Update the usage() and LEVEL parsing to accept
a semver string (e.g., 0.2.0) and implement: a semver validation step for LEVEL,
a check_git_branch function to ensure current branch is main, and a
compare_version function that reads Cargo.toml (and other manifests) to verify
their versions match LEVEL before tagging; on mismatch print a clear error and
exit. Keep existing flags (--tags-only|--no-push|--remote) behavior unchanged
and update the usage text and comments to match RELEASE.md.
- Around line 97-123: The sed substitutions in function sync_props insert the
literal sequence "\n" on macOS (BSD sed) instead of a newline, breaking
PropertyGroup formatting; update the four sed invocations that append a new tag
(the ones that replace "<PropertyGroup>") in sync_props to produce real newlines
on macOS — for example switch the sed argument to ANSI-C quoting ($'...') so
"\\n" becomes an actual newline, or alternatively build the replacement string
with printf and pipe into sed (or use a literal newline in the sed script);
ensure each replacement for Version, AssemblyVersion, FileVersion, and
AssemblyInformationalVersion uses the same cross-platform approach and keep the
existing .bak behavior and cleanup with rm -f "$file.bak".
| - name: Determine tag | ||
| id: extract | ||
| run: | | ||
| if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then | ||
| TAG="${{ inputs.release_tag }}" | ||
| else | ||
| TAG="${GITHUB_REF#refs/tags/}" | ||
| fi | ||
|
|
||
| if ! [[ "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| echo "ERROR: Tag '$TAG' does not match vX.Y.Z pattern" | ||
| exit 1 | ||
| fi | ||
|
|
||
| VERSION="${TAG#v}" | ||
| echo "tag=$TAG" >> "$GITHUB_OUTPUT" | ||
| echo "version=$VERSION" >> "$GITHUB_OUTPUT" | ||
| echo "Tag: $TAG, Version: $VERSION" |
There was a problem hiding this comment.
workflow_dispatch 入力値のスクリプトインジェクション脆弱性があります。
Line 32 で ${{ inputs.release_tag }} をシェルスクリプトに直接展開しています。workflow_dispatch を実行できるユーザーが悪意ある入力(例: v1.0.0"; echo pwned; #)を渡した場合、任意のコマンドが実行される可能性があります。
入力値を環境変数経由で渡すことで安全に処理できます。
修正案
- name: Determine tag
id: extract
+ env:
+ INPUT_TAG: ${{ inputs.release_tag }}
run: |
if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
- TAG="${{ inputs.release_tag }}"
+ TAG="${INPUT_TAG}"
else
TAG="${GITHUB_REF#refs/tags/}"
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 28 - 45, The Determine tag step
(id: extract) currently expands the GitHub expression ${{ inputs.release_tag }}
directly into the shell, which allows command injection; instead, read the input
into a safe environment variable (e.g., RELEASE_TAG) via the step's env mapping
and reference that env var inside the script (use "$RELEASE_TAG" with proper
quoting), then perform the same regex validation against the env var and derive
TAG and VERSION from it before writing to GITHUB_OUTPUT; update references to
TAG in the step to use the env-sourced value to eliminate direct expression
expansion.
| - name: Verify Cargo.toml version matches tag | ||
| run: | | ||
| CARGO_VERSION="$(grep '^version' Cargo.toml | head -1 | sed 's/.*"\(.*\)".*/\1/')" | ||
| if [[ "$CARGO_VERSION" != "${{ steps.extract.outputs.version }}" ]]; then | ||
| echo "ERROR: Cargo.toml version ($CARGO_VERSION) does not match tag (${{ steps.extract.outputs.version }})" | ||
| exit 1 | ||
| fi | ||
| echo "Version check passed: $CARGO_VERSION" |
There was a problem hiding this comment.
Cargo.toml のバージョン抽出が脆弱です。
grep '^version' Cargo.toml | head -1 は [package] セクション以外の version フィールド(例: [dependencies] 内のバージョン指定)にもマッチする可能性があります。
より堅牢な抽出方法
- CARGO_VERSION="$(grep '^version' Cargo.toml | head -1 | sed 's/.*"\(.*\)".*/\1/')"
+ CARGO_VERSION="$(sed -n '/^\[package\]/,/^\[/{ s/^version *= *"\(.*\)"/\1/p; }' Cargo.toml | head -1)"または cargo metadata を使用:
CARGO_VERSION="$(cargo metadata --no-deps --format-version 1 | jq -r '.packages[0].version')"🤖 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 - 58, The current extraction
of CARGO_VERSION using grep '^version' is fragile because it can match version
fields outside the [package] section; replace that extraction in the workflow
(the CARGO_VERSION assignment) with a robust method such as invoking cargo
metadata to read the package version (e.g., use `cargo metadata --no-deps
--format-version 1` and jq to select .packages[0].version) or implement a parser
that reads the version specifically from the [package] section; ensure the
variable CARGO_VERSION is set from this robust source and keep the subsequent
comparison against ${{ steps.extract.outputs.version }} unchanged.
| - name: Run cargo test | ||
| run: cargo test |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
cargo test のフラグが docs/development.md と一致していません。
docs/development.md のローカルコマンドセクションでは cargo test --all-targets が推奨されていますが、CI では cargo test のみです。意図的な差異であれば問題ありませんが、CI でもドキュメント・ベンチマーク等のビルド確認を含めたい場合は --all-targets を追加してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 30 - 31, Update the CI test step
named "Run cargo test" so its run command matches docs/development.md by using
the same flags; replace the current plain `cargo test` invocation with `cargo
test --all-targets` (or explicitly document why you are intentionally diverging)
to ensure CI builds/tests docs/benchmarks and other targets as described.
| lsp-test: | ||
| name: LSP Tests (required) | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 15 | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| - name: Setup .NET | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup .NET SDK 9 | ||
| uses: actions/setup-dotnet@v4 | ||
| with: | ||
| dotnet-version: 9.0.x | ||
| - name: Run LSP tests | ||
| dotnet-version: "9.0.x" | ||
|
|
||
| - name: Run dotnet test | ||
| run: dotnet test lsp/Server.Tests.csproj |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
lsp-test ジョブに .NET のキャッシュがありません。
rust-test ジョブでは Cargo のキャッシュが設定されていますが、lsp-test ジョブには NuGet パッケージのキャッシュがありません。テスト頻度が高い場合、CI 実行時間の短縮に寄与します。
📝 修正案
lsp-test:
name: LSP Tests (required)
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup .NET SDK 9
uses: actions/setup-dotnet@v4
with:
dotnet-version: "9.0.x"
+ - name: Cache NuGet packages
+ uses: actions/cache@v4
+ with:
+ path: ~/.nuget/packages
+ key: ${{ runner.os }}-nuget-${{ hashFiles('**/Server.Tests.csproj', '**/Server.csproj') }}
+ restore-keys: ${{ runner.os }}-nuget-
+
- name: Run dotnet test
run: dotnet test lsp/Server.Tests.csproj📝 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.
| lsp-test: | |
| name: LSP Tests (required) | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 15 | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Setup .NET | |
| - uses: actions/checkout@v4 | |
| - name: Setup .NET SDK 9 | |
| uses: actions/setup-dotnet@v4 | |
| with: | |
| dotnet-version: 9.0.x | |
| - name: Run LSP tests | |
| dotnet-version: "9.0.x" | |
| - name: Run dotnet test | |
| run: dotnet test lsp/Server.Tests.csproj | |
| lsp-test: | |
| name: LSP Tests (required) | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: Setup .NET SDK 9 | |
| uses: actions/setup-dotnet@v4 | |
| with: | |
| dotnet-version: "9.0.x" | |
| - name: Cache NuGet packages | |
| uses: actions/cache@v4 | |
| with: | |
| path: ~/.nuget/packages | |
| key: ${{ runner.os }}-nuget-${{ hashFiles('**/Server.Tests.csproj', '**/Server.csproj') }} | |
| restore-keys: ${{ runner.os }}-nuget- | |
| - name: Run dotnet test | |
| run: dotnet test lsp/Server.Tests.csproj |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 33 - 45, Add NuGet package caching
to the lsp-test job to speed CI runs: in the lsp-test job (job name "lsp-test")
add a step using actions/cache@v4 before restoring/building/tests to cache the
NuGet package folder (e.g., ~/.nuget/packages and optionally
~/.local/share/NuGet or %USERPROFILE%/.nuget/packages on Windows) with a cache
key that includes runner.os and the solution/lockfile checksum (e.g., nuget-${{
runner.os }}-${{ hashFiles('**/*.csproj') }} or packages.lock.json) so dotnet
commands in the subsequent "Run dotnet test" step can reuse packages.
| ``` | ||
| MIT License | ||
|
|
||
| Copyright (c) akiojin |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
著作権表示に年の記載がありません。
Copyright (c) akiojin に著作権年(例: Copyright (c) 2024 akiojin)を追加することを検討してください。MIT ライセンスの一般的な慣行として年の記載が推奨されます。同じ修正は Line 54, 86, 124 にも適用してください。
提案例
-Copyright (c) akiojin
+Copyright (c) 2024 akiojin📝 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.
| Copyright (c) akiojin | |
| Copyright (c) 2024 akiojin |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ATTRIBUTION.md` at line 16, Update each bare copyright string "Copyright (c)
akiojin" to include the copyright year (e.g., "Copyright (c) 2024 akiojin");
replace the occurrences at the shown spot and the other occurrences mentioned
(the lines containing the same "Copyright (c) akiojin" strings around the other
reported locations) so all instances are consistent and include the year.
| # ────────────────────────────────────────────── | ||
| # Pre-publish validation | ||
| # ────────────────────────────────────────────── | ||
|
|
||
| echo "[step] pre-publish validation" | ||
|
|
||
| # Clean working tree check | ||
| if ! git diff --quiet || ! git diff --cached --quiet; then | ||
| echo "[error] Git working tree is not clean. Commit or stash changes before releasing." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ -n "$(git ls-files --others --exclude-standard)" ]; then | ||
| echo "[error] Untracked files detected. Commit or remove them before releasing." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
バリデーションとバージョンバンプの実行順序に問題があります。
プレバリデーション(クリーンな作業ツリーの確認)はバージョンバンプの前に実行されますが、テスト(Line 131-138)はバンプの後に実行されます。npm version(Line 75)はコミットとタグを自動生成するため、テストが失敗した場合、リポジトリにはロールバックされないバージョンバンプのコミットとタグが残ります。
テストをバージョンバンプの前に移動するか、テスト失敗時にロールバック処理を追加してください。
修正案: テストをバンプ前に移動
echo "[step] pre-publish validation"
# Clean working tree check
if ! git diff --quiet || ! git diff --cached --quiet; then
echo "[error] Git working tree is not clean. Commit or stash changes before releasing." >&2
exit 1
fi
if [ -n "$(git ls-files --others --exclude-standard)" ]; then
echo "[error] Untracked files detected. Commit or remove them before releasing." >&2
exit 1
fi
+# ──────────────────────────────────────────────
+# Run tests before version bump
+# ──────────────────────────────────────────────
+
+echo "[step] running cargo test..."
+cargo test || { echo "[error] cargo test failed." >&2; exit 1; }
+
+echo "[step] running dotnet test lsp/Server.Tests.csproj..."
+dotnet test lsp/Server.Tests.csproj || { echo "[error] dotnet test failed." >&2; exit 1; }
+
+echo "[step] running cargo publish --dry-run..."
+cargo publish --dry-run || { echo "[error] cargo publish --dry-run failed." >&2; exit 1; }
+
# 事前情報
CUR_VER=$(node -p "require('./package.json').version")そして Line 127-138 のテストブロックを削除します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/publish.sh` around lines 52 - 67, The script runs the test block
after running npm version which creates a commit and tag; to fix either move the
test block to run before the npm version call (so tests are validated prior to
bumping) and remove the duplicate post-bump test block, or wrap the npm version
step with rollback logic: run npm version, then run the existing test block, and
if tests fail run git reset --hard HEAD~1 and git tag -d "$(node -p
\"require('./package.json').version\")" (or parse the new version from npm
output) to remove the bump commit and tag before exiting nonzero; update the
script around the npm version invocation and the test block accordingly.
| ## 環境変数 | ||
|
|
||
| | 変数名 | 用途 | デフォルト | | ||
| |--------|------|-----------| | ||
| | `UNITY_CLI_HOST` | Unity Editor の接続先ホスト | `localhost` | | ||
| | `UNITY_CLI_PORT` | Unity Editor の接続先ポート | `6400` | | ||
| | `UNITY_CLI_TIMEOUT_MS` | コマンドタイムアウト (ミリ秒) | ― | | ||
| | `UNITY_CLI_LSP_MODE` | LSP 起動モード | `off` | | ||
| | `UNITY_CLI_LSP_COMMAND` | LSP 実行コマンド | ― | | ||
| | `UNITY_CLI_LSP_BIN` | LSP 実行ファイルパス | ― | | ||
| | `UNITY_PROJECT_ROOT` | Unity プロジェクトルート | ― | | ||
|
|
There was a problem hiding this comment.
環境変数テーブルにデフォルト値の記載漏れがあります。
src/config.rs では UNITY_CLI_TIMEOUT_MS のデフォルト値が 30000(30秒)に設定されていますが、このテーブルでは「―」となっています。README.md(Line 111)では正しく 30000 と記載されているため、整合性を取るべきです。
📝 修正案
-| `UNITY_CLI_TIMEOUT_MS` | コマンドタイムアウト (ミリ秒) | ― |
+| `UNITY_CLI_TIMEOUT_MS` | コマンドタイムアウト (ミリ秒) | `30000` |📝 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.
| ## 環境変数 | |
| | 変数名 | 用途 | デフォルト | | |
| |--------|------|-----------| | |
| | `UNITY_CLI_HOST` | Unity Editor の接続先ホスト | `localhost` | | |
| | `UNITY_CLI_PORT` | Unity Editor の接続先ポート | `6400` | | |
| | `UNITY_CLI_TIMEOUT_MS` | コマンドタイムアウト (ミリ秒) | ― | | |
| | `UNITY_CLI_LSP_MODE` | LSP 起動モード | `off` | | |
| | `UNITY_CLI_LSP_COMMAND` | LSP 実行コマンド | ― | | |
| | `UNITY_CLI_LSP_BIN` | LSP 実行ファイルパス | ― | | |
| | `UNITY_PROJECT_ROOT` | Unity プロジェクトルート | ― | | |
| ## 環境変数 | |
| | 変数名 | 用途 | デフォルト | | |
| |--------|------|-----------| | |
| | `UNITY_CLI_HOST` | Unity Editor の接続先ホスト | `localhost` | | |
| | `UNITY_CLI_PORT` | Unity Editor の接続先ポート | `6400` | | |
| | `UNITY_CLI_TIMEOUT_MS` | コマンドタイムアウト (ミリ秒) | `30000` | | |
| | `UNITY_CLI_LSP_MODE` | LSP 起動モード | `off` | | |
| | `UNITY_CLI_LSP_COMMAND` | LSP 実行コマンド | ― | | |
| | `UNITY_CLI_LSP_BIN` | LSP 実行ファイルパス | ― | | |
| | `UNITY_PROJECT_ROOT` | Unity プロジェクトルート | ― | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/architecture.md` around lines 105 - 116, The environment variable table
in specs/architecture.md is missing the default for UNITY_CLI_TIMEOUT_MS; update
the table entry to show the default value 30000 to match src/config.rs (where
UNITY_CLI_TIMEOUT_MS is set to 30000) and README.md (which already lists 30000),
ensuring the table formatting remains intact and consistent with the other rows.
| read_env_with_deprecation( | ||
| &["UNITY_CLI_HOST"], | ||
| &[("UNITY_MCP_MCP_HOST", "UNITY_CLI_HOST"), ("UNITY_MCP_UNITY_HOST", "UNITY_CLI_HOST")], | ||
| ) | ||
| .unwrap_or_else(|| DEFAULT_HOST.to_string()) |
There was a problem hiding this comment.
CI パイプラインが cargo fmt チェックで失敗しています。
パイプラインログによると、src/config.rs にフォーマットの差分があります。cargo fmt を実行してフォーマットを修正してください。特にこの行は長いため、自動フォーマットで折り返される可能性があります。
cargo fmt --all🧰 Tools
🪛 GitHub Actions: Lint
[error] cargo fmt --all -- --check failed. Formatting changes are required in src/config.rs: run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 34 - 38, CI is failing due to rustfmt differences
in src/config.rs; run rustfmt (cargo fmt --all) and commit the formatted changes
so the call to read_env_with_deprecation (and its long argument tuple of
("UNITY_MCP_MCP_HOST", "UNITY_CLI_HOST") / ("UNITY_MCP_UNITY_HOST",
"UNITY_CLI_HOST")) is line-wrapped according to rustfmt rules; ensure
DEFAULT_HOST fallback remains unchanged and that symbols
read_env_with_deprecation, DEFAULT_HOST, UNITY_CLI_HOST, UNITY_MCP_MCP_HOST,
UNITY_MCP_UNITY_HOST are preserved exactly.
| /// Read a `u16` environment variable with deprecation warning support. | ||
| fn read_env_u16_with_deprecation( | ||
| primary_keys: &[&str], | ||
| deprecated_keys: &[(&str, &str)], | ||
| ) -> Option<u16> { | ||
| read_env_with_deprecation(primary_keys, deprecated_keys) | ||
| .and_then(|value| value.parse::<u16>().ok()) | ||
| .filter(|port| *port > 0) | ||
| } | ||
|
|
||
| fn read_env_u64(keys: &[&str]) -> Option<u64> { | ||
| keys.iter() | ||
| .find_map(|key| env::var(key).ok()) | ||
| .and_then(|value| value.trim().parse::<u64>().ok()) | ||
| /// Read a `u64` environment variable with deprecation warning support. | ||
| fn read_env_u64_with_deprecation( | ||
| primary_keys: &[&str], | ||
| deprecated_keys: &[(&str, &str)], | ||
| ) -> Option<u64> { | ||
| read_env_with_deprecation(primary_keys, deprecated_keys) | ||
| .and_then(|value| value.parse::<u64>().ok()) | ||
| .filter(|timeout| *timeout > 0) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
数値パースの失敗が無警告でデフォルト値にフォールバックします。
UNITY_CLI_PORT=abc のように不正な値が設定された場合、.parse::<u16>().ok() により None が返り、最終的にデフォルト値(6400)が使われますが、ユーザーには何のフィードバックもありません。設定ミスの発見が遅れる原因になり得ます。
📝 修正案(警告ログの追加)
fn read_env_u16_with_deprecation(
primary_keys: &[&str],
deprecated_keys: &[(&str, &str)],
) -> Option<u16> {
- read_env_with_deprecation(primary_keys, deprecated_keys)
- .and_then(|value| value.parse::<u16>().ok())
- .filter(|port| *port > 0)
+ read_env_with_deprecation(primary_keys, deprecated_keys).and_then(|value| {
+ match value.parse::<u16>() {
+ Ok(v) if v > 0 => Some(v),
+ Ok(_) => {
+ tracing::warn!("Port value '0' is not valid, using default");
+ None
+ }
+ Err(e) => {
+ tracing::warn!("Failed to parse port value '{}': {}, using default", value, e);
+ None
+ }
+ }
+ })
}🧰 Tools
🪛 GitHub Actions: Lint
[error] cargo fmt --all -- --check failed. Formatting changes are required in src/config.rs: run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 92 - 110, The parsing helpers
read_env_u16_with_deprecation and read_env_u64_with_deprecation silently drop
invalid numeric env values; change them to first call read_env_with_deprecation
to get the raw string, and if present attempt parse and on parse failure emit a
warning log that includes the env key(s) and the invalid value (e.g., using the
crate's logging macro like warn! or tracing::warn!), then return None so the
default is used; ensure you reference the same primary_keys/deprecated_keys when
logging so the message helps the user find the bad setting.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::sync::Mutex; | ||
|
|
||
| // Serialize env-var tests so they don't interfere with each other. | ||
| static ENV_LOCK: Mutex<()> = Mutex::new(()); | ||
|
|
||
| /// Helper to temporarily set env vars and clean them up after the closure runs. | ||
| fn with_env_vars<F, R>(vars: &[(&str, &str)], f: F) -> R | ||
| where | ||
| F: FnOnce() -> R, | ||
| { | ||
| let _lock = ENV_LOCK.lock().unwrap(); | ||
| for (key, value) in vars { | ||
| env::set_var(key, value); | ||
| } | ||
| let result = f(); | ||
| for (key, _) in vars { | ||
| env::remove_var(key); | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if other test modules also modify environment variables
echo "=== env::set_var usage outside config.rs tests ==="
rg -n 'env::set_var|env::remove_var|set_var|remove_var' --type rust -g '!src/config.rs'
echo ""
echo "=== Rust edition and version info ==="
cat Cargo.toml | grep -E 'edition|rust-version' || echo "No rust-version specified"Repository: akiojin/unity-cli
Length of output: 1793
🏁 Script executed:
#!/bin/bash
# Check if env var modifications are in test contexts
echo "=== Check instances.rs lines 243-289 context ==="
sed -n '235,295p' src/instances.rs | head -60
echo ""
echo "=== Check if tests are configured with serialization ==="
grep -n '#\[cfg(test)\]' src/instances.rs src/local_tools.rsRepository: akiojin/unity-cli
Length of output: 2327
🏁 Script executed:
#!/bin/bash
# Find where env_lock is defined
echo "=== Search for env_lock definition ==="
rg -n 'fn env_lock|static.*env_lock|ENV_LOCK' --type rust
echo ""
echo "=== Check if local_tools.rs uses env_lock ==="
sed -n '1162,1220p' src/local_tools.rs | head -60Repository: akiojin/unity-cli
Length of output: 2736
テストでの env::set_var / env::remove_var の使用に関する注意。
Rust 1.66 以降、env::set_var と env::remove_var はマルチスレッド環境での安全性が保証されません(Rust 1.67 で soft-deprecated)。現在、複数のテストモジュール(instances.rs, local_tools.rs など)で環境変数を変更していますが、各モジュールは独立した Mutex を使用しているため、モジュール間の同期が行われていません。
cargo test はデフォルトで複数スレッドを使用するため、異なるモジュールのテストが同時に実行される際に、環境変数の競合が発生する可能性があります。例えば、config.rs のテストと instances.rs のテストが同時に実行される場合、互いの環境変数操作が干渉する可能性があります。
テストスイート全体の安定性を確保するため、cargo test -- --test-threads=1 による直列実行、または serial_test クレートの導入を強く推奨します。
🧰 Tools
🪛 GitHub Actions: Lint
[error] cargo fmt --all -- --check failed. Formatting changes are required in src/config.rs: run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config.rs` around lines 112 - 134, The tests use env::set_var/remove_var
under a module-local Mutex (ENV_LOCK) via with_env_vars, which doesn't prevent
cross-module races when cargo runs tests multithreaded; fix by serializing
access across the whole test suite—either run tests single-threaded (cargo test
-- --test-threads=1) or add a global synchronization strategy: introduce a
shared test helper (e.g., a tests::global_lock or a new crate/module) and
replace per-module ENV_LOCK uses (and have with_env_vars acquire that global
lock), or add the serial_test crate and annotate env-mutating tests to run
serially so that ENV_LOCK and with_env_vars serialize environment changes across
modules.
feat: resolve all follow-up tasks for unity-cli migration (#8)
New first-class tool (akiojin#12 Project Settings, Pass-1 breadth proof) reading and writing UnityEngine.VFX project settings in ProjectSettings/VFXManager.asset. - VfxGraphHandler.Settings: op `get` reports a `properties` block (public static VFXManager props — fixedTimeStep/maxDeltaTime, round-trip on re-read) plus a `serialized` block (asset fields m_MaxCapacity/m_MaxScrubTime/ m_BatchEmptyLifetime). op `set` writes through the static property setter when one exists (via:"property"), else the matching serialized field m_PascalCase (via:"serialized"). - 5-layer vertical: catalog name+schema+description (parity count 133→134, +unit test), host case, skill section + example, docs/tools.md row. - Tests: 3 contract (unsupported op, set without setting/value) + 2 behavioral (get reports fixedTimeStep/maxDeltaTime; set fixedTimeStep round-trips via re-read, restored in finally). All 45 EditMode tests green, 0 skipped. Verified live via raw: get returns current values; set fixedTimeStep=0.02 re-reads as 0.02; maxCapacity exercises the serialized fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kiojin#12) GetVfxSettings now surfaces the VFXManager compute/empty shader refs + runtime-resources ScriptableObject (m_IndirectShader/m_CopyBufferShader/ m_PrefixSumShader/m_SortShader/m_StripUpdateShader/m_EmptyShader/ m_RuntimeResources/m_RenderPipeSettingsPath) as {type,name,assetPath}, and AssignSerialized gained an ObjectReference branch so set writes them by asset path (empty clears). These are usually Unity-managed defaults — surfaced for inspection, overridable when needed. EditMode test does a safe cross-assign round-trip with restore (VFXManager.asset stays clean). Skill v0.23.0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
New first-class tool (akiojin#12 Project Settings, Pass-1 breadth proof) reading and writing UnityEngine.VFX project settings in ProjectSettings/VFXManager.asset. - VfxGraphHandler.Settings: op `get` reports a `properties` block (public static VFXManager props — fixedTimeStep/maxDeltaTime, round-trip on re-read) plus a `serialized` block (asset fields m_MaxCapacity/m_MaxScrubTime/ m_BatchEmptyLifetime). op `set` writes through the static property setter when one exists (via:"property"), else the matching serialized field m_PascalCase (via:"serialized"). - 5-layer vertical: catalog name+schema+description (parity count 133→134, +unit test), host case, skill section + example, docs/tools.md row. - Tests: 3 contract (unsupported op, set without setting/value) + 2 behavioral (get reports fixedTimeStep/maxDeltaTime; set fixedTimeStep round-trips via re-read, restored in finally). All 45 EditMode tests green, 0 skipped. Verified live via raw: get returns current values; set fixedTimeStep=0.02 re-reads as 0.02; maxCapacity exercises the serialized fallback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…kiojin#12) GetVfxSettings now surfaces the VFXManager compute/empty shader refs + runtime-resources ScriptableObject (m_IndirectShader/m_CopyBufferShader/ m_PrefixSumShader/m_SortShader/m_StripUpdateShader/m_EmptyShader/ m_RuntimeResources/m_RenderPipeSettingsPath) as {type,name,assetPath}, and AssignSerialized gained an ObjectReference branch so set writes them by asset path (empty clears). These are usually Unity-managed defaults — surfaced for inspection, overridable when needed. EditMode test does a safe cross-assign round-trip with restore (VFXManager.asset stays clean). Skill v0.23.0. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Resolves #8 by implementing all 7 sub-issues for the post-migration follow-up:
test.yml), pre-push hook, Dockerfile,CONTRIBUTING.mdscripts/e2e-test.sh, docsscripts/benchmark.sh,docs/benchmark-results.mdspecs/directory witharchitecture.md,migration-notes.mdUNITY_MCP_*deprecation policy —src/config.rswarnings,docs/configuration.mdscripts/publish.sh,.github/workflows/release.yml,RELEASE.mdATTRIBUTION.md, UPM docs linksAdditionally:
unity-mcp-serverTest plan
cargo test— all 34 tests pass (including 12 new config deprecation tests)dotnet test lsp/Server.Tests.csproj— LSP testsscripts/publish.shdry-runscripts/e2e-test.shwith running Unity EditorCloses #1, closes #2, closes #3, closes #4, closes #5, closes #6, closes #7, closes #8
🤖 Generated with Claude Code
Summary by CodeRabbit
リリースノート
ドキュメント
互換性