chore: quality-tightening (oxfmt + oxlint + tsc + vitest + husky + actionlint) - #833
Merged
Conversation
…tionlint) Standard rollout for unity-builder. Most of the work was porting 24 test files from jest 27 to vitest 4. - prettier -> oxfmt - eslint (with @typescript-eslint, github, jest, prettier, unicorn) -> oxlint with eslint-plugin-unicorn - jest 27 + jest-circus + ts-jest + @types/jest + @jest/globals -> vitest 4 + vite 7 + @vitest/coverage-istanbul (jest config files removed) - new: tsgo --noEmit (alongside tsc fallback) - lefthook (and lefthook.yml) -> husky 9 with the standard scripts/ensure-husky.mjs self-heal pattern + lint-staged - new: gitleaks, actionlint, shellcheck as mise-managed binaries - TypeScript bumped target ES2020 -> ES2022 + lib ES2022 + DOM (for Error.cause and modern globals) Test migration (24 files): - Bulk-converted jest.* -> vi.*; jest.Mocked -> Mocked from vitest; jest.MockedFunction -> MockedFunction. - Added vitest imports to all *.test.ts files (and __mocks__/*.ts) that didn't have them. - src/index.ts: extracted runMain() as a named export and gated the module-level invocation behind NODE_ENV !== 'test'. The index-plugin-features test now calls runMain() directly instead of relying on jest's removed vi.isolateModules. - index-plugin-features.test.ts: moved hoisted refs (mockPlugin, mockLoadOrchestratorPlugin) into vi.hoisted() so vi.mock factories can reference them. Replaced arrow constructor mock for ImageTag with regular function() {...} (vitest 4 disallows arrows as ctors). Replaced require('./model') / require('@actions/core') inside test bodies with top-level imports. - model/orchestrator-plugin.test.ts: dropped jest's '{ virtual: true }' flag (vitest doesn't support it); replaced the 'mock factory throws' pattern with 'createPlugin throws' so vitest doesn't wrap the error message at the assertion site. - model/versioning.test.ts: stray jest.spyOn -> vi.spyOn; replaced mockImplementation() with no args (jest pattern) by mockResolvedValue('') / mockImplementation(() => undefined) where the source expects a string return. Workflow shell-quoting cleanup (actionlint): - All bare $GITHUB_STEP_SUMMARY / $GITHUB_OUTPUT / $GITHUB_ENV redirects quoted across 2 workflows (SC2086). - s3://$AWS_STACK_NAME / s3://$BUCKET_NAME -> s3://"$AWS_STACK_NAME" / s3://"$BUCKET_NAME". - 'for i in {1..N}; do ... done' loops where i isn't referenced in the body renamed to 'for _ in' (SC2034). - 'grep ... | wc -l' -> 'grep -c ...' (SC2126). - Multiple consecutive '>> $file' redirects in validate-community-plugins.yml summary block collapsed into a single block redirect (SC2129). - 'cat $file | python3 -c "..."' -> 'python3 -c "..." < $file' (SC2002). - http://${VAR}:port -> http://"${VAR}":port (SC2086). tsgo: kept tsc --noEmit as the default 'typecheck' because unity-builder publishes CommonJS for the GitHub Action consumer, which conflicts with tsgo's bundler/node16 moduleResolution requirement (per playbook trap #9). 'yarn typecheck:tsgo' is wired up for when consumers move to ESM. Caveats: 28 pre-existing oxlint warnings remain (mostly typescript/no-explicit-any across the build-parameter shapes and vitest/no-disabled-tests on 2 explicitly skipped scenarios). Per playbook trap #22 the lint script drops --deny-warnings. Verified locally: format clean, lint 0/28, typecheck clean, test 340/342 (2 pre-existing skipped), actionlint clean across all 12 workflows.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR modernizes the repository's development toolchain by migrating from Jest to Vitest for testing, replacing ESLint/Prettier with Oxlint/Oxfmt for linting and formatting, switching from Lefthook to Husky for git hooks, and enhancing the plugin lifecycle with new hooks for build handling. TypeScript and dependency configurations are updated accordingly. ChangesDevelopment Toolchain Modernization
Plugin System Enhancement
Code Formatting & Refactoring (across multiple files)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR encompasses substantial structural changes across multiple layers: comprehensive test framework migration affecting 40+ test files with mock rewiring, complete development toolchain replacement (build tools, linting, formatting, hooks), new plugin lifecycle hooks with corresponding test coverage, TypeScript configuration updates, and numerous code formatting adjustments. The heterogeneity of changes (test framework migration, configuration removal/addition, interface enhancements, formatting) demands separate reasoning for each layer despite some repetitive patterns in test file updates. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/model/input.test.ts (1)
218-228:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest asserts the wrong property —
androidSymbolTypeis never verified.Line 225 checks
Input.androidExportTypeinside theandroidSymbolTypedescribe block. This is a copy-paste from theandroidExportTypetest above. The spy is mockinggetInputwith symbol-type values ('none','public','debugging'), so the assertion should targetInput.androidSymbolType.🐛 Proposed fix
- expect(Input.androidExportType).toStrictEqual(expected); + expect(Input.androidSymbolType).toStrictEqual(expected);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/model/input.test.ts` around lines 218 - 228, The test in the androidSymbolType block is asserting the wrong property; change the expectation to verify Input.androidSymbolType instead of Input.androidExportType, keeping the mock spy on core.getInput and the test.each table as-is so the symbol-type values ('none','public','debugging') are validated; update the assertion line to expect(Input.androidSymbolType).toStrictEqual(expected) and keep the spy call count assertion.
🧹 Nitpick comments (2)
.github/workflows/build-tests-ubuntu.yml (1)
38-38: 💤 Low valueMinor: whitespace inconsistencies in the interpolated expression.
Inside the
name:string the spacing around${{ ... }}is inconsistent:${{ matrix.unityVersion}}is missing the trailing space and${{startsWith(...)is missing the leading space, while${{ matrix.targetPlatform }}follows the canonical style. Functionally identical, but worth aligning so future formatter passes don't churn the line.♻️ Proposed alignment
- name: "${{ matrix.targetPlatform }} on ${{ matrix.unityVersion}}${{startsWith(matrix.buildProfile, 'Assets') && ' (via Build Profile)' || '' }}" + name: "${{ matrix.targetPlatform }} on ${{ matrix.unityVersion }}${{ startsWith(matrix.buildProfile, 'Assets') && ' (via Build Profile)' || '' }}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-tests-ubuntu.yml at line 38, The interpolated GitHub Actions job name contains inconsistent spacing around the expressions; update the string so all expressions use consistent spacing like `${{ matrix.targetPlatform }}`, `${{ matrix.unityVersion }}`, and `${{ startsWith(matrix.buildProfile, 'Assets') && ' (via Build Profile)' || '' }}` (ensure leading/trailing spaces inside each `${{ ... }}`) to match canonical style and avoid formatter churn.src/index-plugin-features.test.ts (1)
118-123: 💤 Low valueMinor comment inaccuracy — code still uses a dynamic import.
The inline comment says the new approach avoids "round-tripping through dynamic imports," but
runIndexstill doesawait import('./index')on every call. The intent of the comment is probably to contrast against the old pattern ofjest.isolateModules+require, not against dynamic imports generally. Worth a small wording fix to avoid confusion.📝 Suggested wording
- // index.ts exports `runMain` for testability (the file used to rely on - // top-level execution + jest's `vi.isolateModules`, but vitest 4 dropped - // that API). Calling the exported function directly is cleaner than - // round-tripping through dynamic imports. + // index.ts exports `runMain` for testability. The old Jest approach relied on + // jest.isolateModules + require to re-execute the module per test. Calling + // the exported function directly avoids that isolation complexity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index-plugin-features.test.ts` around lines 118 - 123, Update the inaccurate inline comment around the test helper that imports './index' (the block referencing runMain and the helper runIndex) to clarify that we are avoiding the old jest.isolateModules + require pattern, not dynamic imports in general; reword to state we now call the exported runMain directly for clarity while tests still use a dynamic import (await import('./index')) rather than claiming we avoid "round-tripping through dynamic imports." Ensure the comment references runMain and the test helper (runIndex) so future readers understand the distinction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/validate-orchestrator-integration.yml:
- Around line 365-369: The PVC count logic produces two lines when no matches
because `grep -c ... || echo "0"` appends an extra 0; fix the loop that sets
PVC_COUNT (the command substitution used to assign PVC_COUNT) by removing the
`|| echo "0"` or replacing it with a no-op on failure (e.g., `|| true`), or
switch to a reliable count method (e.g., `kubectl get pvc -n default | grep -c
"unity-builder-pvc-"` or `kubectl get pvc -n default --no-headers | grep -c
...`) so PVC_COUNT is always a single integer and the `[ "$PVC_COUNT" -eq 0 ]`
check in the for-loop can succeed and break as intended.
In @.husky/pre-commit:
- Around line 1-7: The pre-commit hook currently can return success even when
earlier steps fail because it lacks "set -e" and does not chain commands; update
the .husky/pre-commit script so failures in "yarn lint-staged" or "yarn
typecheck" cause the hook to exit non-zero — either add "set -e" (or "set -euo
pipefail") at the top of the script, or chain the commands with "&&" (e.g. "yarn
lint-staged && yarn typecheck && { if command -v gitleaks ...; fi }") and keep
the existing gitleaks check inside the final conditional block so the hook’s
exit status reflects earlier failures.
In `@mise.toml`:
- Around line 4-6: The mise.toml entries use "latest" for actionlint,
shellcheck, and gitleaks which causes non-reproducible CI/hooks; update the
three keys actionlint, shellcheck, and gitleaks to explicit version strings
(e.g., semantic version tags or commit SHAs) instead of "latest", commit the
pinned versions, and add a comment or changelog note to document the chosen
versions so future bumps are intentional and auditable.
In `@src/model/input-readers/git-repo.ts`:
- Around line 26-28: The code builds a shell string using Input.projectPath and
passes it to GitRepoReader.runCommand, exposing a command-injection risk and
breaking on paths with spaces; change the call to invoke git directly with
arguments and set the working directory instead of interpolating the path—i.e.,
call GitRepoReader.runCommand with the command/args for "git remote -v" and a
cwd option of Input.projectPath (or use a child_process variant that accepts {
cwd: Input.projectPath }), preserve the .replace() behavior on the output, and
ensure errors are propagated/handled as before.
In `@src/model/versioning.test.ts`:
- Around line 255-259: The test currently checks the Promise object instead of
its rejection; update the assertion to await the Promise rejection: replace
expect(Versioning.parseSemanticVersion()).toMatchObject({}) with await
expect(Versioning.parseSemanticVersion()).rejects.toThrow() (or use
.rejects.toMatchObject({...}) if you want to assert a specific error shape).
Keep the existing spy on Versioning.getVersionDescription() returning
'no-match-can-be-made' and ensure the test name "throws when no match could be
made" still reflects the rejection assertion.
---
Outside diff comments:
In `@src/model/input.test.ts`:
- Around line 218-228: The test in the androidSymbolType block is asserting the
wrong property; change the expectation to verify Input.androidSymbolType instead
of Input.androidExportType, keeping the mock spy on core.getInput and the
test.each table as-is so the symbol-type values ('none','public','debugging')
are validated; update the assertion line to
expect(Input.androidSymbolType).toStrictEqual(expected) and keep the spy call
count assertion.
---
Nitpick comments:
In @.github/workflows/build-tests-ubuntu.yml:
- Line 38: The interpolated GitHub Actions job name contains inconsistent
spacing around the expressions; update the string so all expressions use
consistent spacing like `${{ matrix.targetPlatform }}`, `${{ matrix.unityVersion
}}`, and `${{ startsWith(matrix.buildProfile, 'Assets') && ' (via Build
Profile)' || '' }}` (ensure leading/trailing spaces inside each `${{ ... }}`) to
match canonical style and avoid formatter churn.
In `@src/index-plugin-features.test.ts`:
- Around line 118-123: Update the inaccurate inline comment around the test
helper that imports './index' (the block referencing runMain and the helper
runIndex) to clarify that we are avoiding the old jest.isolateModules + require
pattern, not dynamic imports in general; reword to state we now call the
exported runMain directly for clarity while tests still use a dynamic import
(await import('./index')) rather than claiming we avoid "round-tripping through
dynamic imports." Ensure the comment references runMain and the test helper
(runIndex) so future readers understand the distinction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4dd14922-c3da-42bd-a989-cc3332b989e2
⛔ Files ignored due to path filters (2)
src/model/__snapshots__/versioning.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (63)
.eslintignore.eslintrc.json.github/ISSUE_TEMPLATE/bug_report.md.github/workflows/build-tests-ubuntu.yml.github/workflows/validate-community-plugins.yml.github/workflows/validate-orchestrator-integration.yml.husky/pre-commit.oxfmtrc.json.oxlintrc.json.prettierignore.prettierrc.jsonaction.ymljest.ci.config.jsjest.config.jslefthook.ymlmise.tomlpackage.jsonscripts/ensure-husky.mjssrc/index-plugin-features.test.tssrc/index.tssrc/integrity.test.tssrc/model/__mocks__/input.tssrc/model/__mocks__/versioning.tssrc/model/action.test.tssrc/model/android-versioning.test.tssrc/model/android-versioning.tssrc/model/build-parameters.test.tssrc/model/build-parameters.tssrc/model/cache.test.tssrc/model/cli/cli.tssrc/model/docker.test.tssrc/model/docker.tssrc/model/error/command-execution-error.test.tssrc/model/error/not-implemented-exception.test.tssrc/model/error/validation-error.test.tssrc/model/image-environment-factory.tssrc/model/image-tag.test.tssrc/model/index.test.tssrc/model/index.tssrc/model/input-readers/git-repo.test.tssrc/model/input-readers/git-repo.tssrc/model/input-readers/github-cli.test.tssrc/model/input-readers/test-license-reader.tssrc/model/input.test.tssrc/model/input.tssrc/model/orchestrator-plugin.test.tssrc/model/orchestrator-plugin.tssrc/model/output.test.tssrc/model/platform-setup/setup-android.tssrc/model/platform-setup/setup-mac.tssrc/model/platform-validation/validate-windows.tssrc/model/platform.test.tssrc/model/project.test.tssrc/model/system-integration.test.tssrc/model/system.test.tssrc/model/unity-versioning.test.tssrc/model/unity-versioning.tssrc/model/unity.test.tssrc/model/versioning.test.tssrc/model/versioning.tstsconfig.jsontypes/shell-quote.d.tsvitest.config.mts
💤 Files with no reviewable changes (8)
- .eslintrc.json
- jest.ci.config.js
- lefthook.yml
- jest.config.js
- .prettierignore
- .eslintignore
- .prettierrc.json
- types/shell-quote.d.ts
Comment on lines
+365
to
369
| for _ in {1..30}; do | ||
| PVC_COUNT=$(kubectl get pvc -n default 2>/dev/null | grep -c "unity-builder-pvc-" || echo "0") | ||
| if [ "$PVC_COUNT" -eq 0 ]; then echo "All PVCs deleted"; break; fi | ||
| sleep 1 | ||
| done |
There was a problem hiding this comment.
Minor: grep -c … || echo "0" produces a multi-line PVC_COUNT on the no-match path.
grep -c already prints 0 for no matches and exits with status 1, so the || echo "0" runs and appends a second 0. The command substitution captures both lines, so PVC_COUNT becomes "0\n0" whenever no unity-builder-pvc- PVCs are present. [ "$PVC_COUNT" -eq 0 ] then errors with integer expression expected, the break is never taken, and this cleanup wait runs the full 30 iterations even after PVCs are gone.
🛠 Suggested fix
- for _ in {1..30}; do
- PVC_COUNT=$(kubectl get pvc -n default 2>/dev/null | grep -c "unity-builder-pvc-" || echo "0")
- if [ "$PVC_COUNT" -eq 0 ]; then echo "All PVCs deleted"; break; fi
- sleep 1
- done
+ for _ in {1..30}; do
+ PVC_COUNT=$(kubectl get pvc -n default 2>/dev/null | grep -c "unity-builder-pvc-" || true)
+ if [ "${PVC_COUNT:-0}" -eq 0 ]; then echo "All PVCs deleted"; break; fi
+ sleep 1
+ done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/validate-orchestrator-integration.yml around lines 365 -
369, The PVC count logic produces two lines when no matches because `grep -c ...
|| echo "0"` appends an extra 0; fix the loop that sets PVC_COUNT (the command
substitution used to assign PVC_COUNT) by removing the `|| echo "0"` or
replacing it with a no-op on failure (e.g., `|| true`), or switch to a reliable
count method (e.g., `kubectl get pvc -n default | grep -c "unity-builder-pvc-"`
or `kubectl get pvc -n default --no-headers | grep -c ...`) so PVC_COUNT is
always a single integer and the `[ "$PVC_COUNT" -eq 0 ]` check in the for-loop
can succeed and break as intended.
Comment on lines
+1
to
+7
| #!/usr/bin/env sh | ||
| yarn lint-staged | ||
| yarn typecheck | ||
|
|
||
| if command -v gitleaks >/dev/null 2>&1; then | ||
| gitleaks protect --staged --no-banner --redact | ||
| fi |
There was a problem hiding this comment.
Critical: failures in lint-staged/typecheck are silently swallowed by this hook.
The script has no set -e and the steps are not chained with &&. Husky 9 executes the hook script as-is and uses its final exit status. The last statement is the if … fi, which evaluates to 0 whenever gitleaks is not installed (because the then branch is skipped). On any developer machine without gitleaks, the hook will therefore exit 0 even if yarn lint-staged or yarn typecheck failed — commits with lint/format/type errors will pass through the pre-commit gate, defeating the intent of this PR.
🛡️ Suggested fix
#!/usr/bin/env sh
+set -e
+
yarn lint-staged
yarn typecheck
if command -v gitleaks >/dev/null 2>&1; then
gitleaks protect --staged --no-banner --redact
fiEquivalent alternative if you prefer not to use set -e:
#!/usr/bin/env sh
yarn lint-staged && yarn typecheck && {
if command -v gitleaks >/dev/null 2>&1; then
gitleaks protect --staged --no-banner --redact
fi
}📝 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
| #!/usr/bin/env sh | |
| yarn lint-staged | |
| yarn typecheck | |
| if command -v gitleaks >/dev/null 2>&1; then | |
| gitleaks protect --staged --no-banner --redact | |
| fi | |
| #!/usr/bin/env sh | |
| set -e | |
| yarn lint-staged | |
| yarn typecheck | |
| if command -v gitleaks >/dev/null 2>&1; then | |
| gitleaks protect --staged --no-banner --redact | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.husky/pre-commit around lines 1 - 7, The pre-commit hook currently can
return success even when earlier steps fail because it lacks "set -e" and does
not chain commands; update the .husky/pre-commit script so failures in "yarn
lint-staged" or "yarn typecheck" cause the hook to exit non-zero — either add
"set -e" (or "set -euo pipefail") at the top of the script, or chain the
commands with "&&" (e.g. "yarn lint-staged && yarn typecheck && { if command -v
gitleaks ...; fi }") and keep the existing gitleaks check inside the final
conditional block so the hook’s exit status reflects earlier failures.
Comment on lines
+4
to
+6
| actionlint = "latest" | ||
| shellcheck = "latest" | ||
| gitleaks = "latest" |
There was a problem hiding this comment.
Pin tool versions instead of latest for reproducible CI/hooks.
Using latest on Line 4, Line 5, and Line 6 makes local and CI behavior drift over time and can break pipelines unexpectedly. Pin explicit versions (and bump intentionally).
Suggested change
-actionlint = "latest"
-shellcheck = "latest"
-gitleaks = "latest"
+actionlint = "1.7.7"
+shellcheck = "0.10.0"
+gitleaks = "8.24.2"📝 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
| actionlint = "latest" | |
| shellcheck = "latest" | |
| gitleaks = "latest" | |
| actionlint = "1.7.7" | |
| shellcheck = "0.10.0" | |
| gitleaks = "8.24.2" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mise.toml` around lines 4 - 6, The mise.toml entries use "latest" for
actionlint, shellcheck, and gitleaks which causes non-reproducible CI/hooks;
update the three keys actionlint, shellcheck, and gitleaks to explicit version
strings (e.g., semantic version tags or commit SHAs) instead of "latest", commit
the pinned versions, and add a comment or changelog note to document the chosen
versions so future bumps are intentional and auditable.
Comment on lines
+26
to
+28
| const value = ( | ||
| await GitRepoReader.runCommand(`cd ${Input.projectPath} && git remote -v`) | ||
| ).replace(/ /g, ``); |
There was a problem hiding this comment.
Avoid shell interpolation with Input.projectPath (command-injection risk).
On Line 27, building a shell command with cd ${Input.projectPath} allows shell metacharacter injection and breaks on some valid paths. Use argument-based process APIs with cwd instead of interpolating into a command string.
Suggested refactor
-import { exec } from 'node:child_process';
+import { execFile } from 'node:child_process';
- private static async runCommand(command: string): Promise<string> {
+ private static async runCommand(
+ command: string,
+ args: string[],
+ cwd?: string,
+ ): Promise<string> {
return new Promise<string>((resolve, reject) => {
- exec(command, { maxBuffer: 1024 * 10000 }, (error, stdout) => {
+ execFile(command, args, { cwd, maxBuffer: 1024 * 10000 }, (error, stdout) => {
if (error) {
reject(error);
return;
}
resolve(stdout.toString());
});
});
}
- const value = (
- await GitRepoReader.runCommand(`cd ${Input.projectPath} && git remote -v`)
- ).replace(/ /g, ``);
+ const value = (await GitRepoReader.runCommand('git', ['remote', '-v'], Input.projectPath)).replace(
+ / /g,
+ ``,
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/model/input-readers/git-repo.ts` around lines 26 - 28, The code builds a
shell string using Input.projectPath and passes it to GitRepoReader.runCommand,
exposing a command-injection risk and breaking on paths with spaces; change the
call to invoke git directly with arguments and set the working directory instead
of interpolating the path—i.e., call GitRepoReader.runCommand with the
command/args for "git remote -v" and a cwd option of Input.projectPath (or use a
child_process variant that accepts { cwd: Input.projectPath }), preserve the
.replace() behavior on the output, and ensure errors are propagated/handled as
before.
Comment on lines
255
to
259
| it('throws when no match could be made', async () => { | ||
| jest.spyOn(Versioning, 'getVersionDescription').mockResolvedValue('no-match-can-be-made'); | ||
| vi.spyOn(Versioning, 'getVersionDescription').mockResolvedValue('no-match-can-be-made'); | ||
|
|
||
| await expect(Versioning.parseSemanticVersion()).toMatchObject({}); | ||
| }); |
There was a problem hiding this comment.
Test "throws when no match could be made" never actually asserts a throw — it always passes.
expect(Versioning.parseSemanticVersion()).toMatchObject({}) without .rejects matches the Promise object itself against an empty schema, which is always satisfied. The intended behavior (asserting the function rejects) is never verified.
🐛 Proposed fix
- await expect(Versioning.parseSemanticVersion()).toMatchObject({});
+ await expect(Versioning.parseSemanticVersion()).rejects.toThrow();📝 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
| it('throws when no match could be made', async () => { | |
| jest.spyOn(Versioning, 'getVersionDescription').mockResolvedValue('no-match-can-be-made'); | |
| vi.spyOn(Versioning, 'getVersionDescription').mockResolvedValue('no-match-can-be-made'); | |
| await expect(Versioning.parseSemanticVersion()).toMatchObject({}); | |
| }); | |
| it('throws when no match could be made', async () => { | |
| vi.spyOn(Versioning, 'getVersionDescription').mockResolvedValue('no-match-can-be-made'); | |
| await expect(Versioning.parseSemanticVersion()).rejects.toThrow(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/model/versioning.test.ts` around lines 255 - 259, The test currently
checks the Promise object instead of its rejection; update the assertion to
await the Promise rejection: replace
expect(Versioning.parseSemanticVersion()).toMatchObject({}) with await
expect(Versioning.parseSemanticVersion()).rejects.toThrow() (or use
.rejects.toMatchObject({...}) if you want to assert a specific error shape).
Keep the existing spy on Versioning.getVersionDescription() returning
'no-match-can-be-made' and ensure the test name "throws when no match could be
made" still reflects the rejection assertion.
…tightening
Three issues surfaced in CI after the jest -> vitest port:
1. **Obsolete snapshot blocks Tests job.**
src/model/__snapshots__/versioning.test.ts.snap had two entries
for the same 'throws for invalid strategy' assertion: one in the
vitest format ('Versioning > determineBuildVersion > ...') and one
in the legacy jest format without the '>'. vitest correctly
regenerates the new one and flags the old one as obsolete; CI
runs without --update so 'Test Files 1 failed' even though all
343 tests passed. Removed the obsolete entry.
2. **'Plugin Architecture Health' workflow still calls jest.**
.github/workflows/validate-orchestrator.yml had two 'npx jest'
steps (orchestrator-plugin unit tests + orchestrator-standalone
tests). The unity-builder + orchestrator codebases are both on
vitest now. Replaced both with 'yarn vitest run'.
3. **jest-fail-on-console + src/jest.setup.ts left over.**
The earlier vitest port missed the jest-fail-on-console
integration. yarn install in CI surfaced
YN0002: doesn't provide @jest/globals (requested by
jest-fail-on-console). Removed jest-fail-on-console + jest.setup.ts;
added src/test/setup.ts with the equivalent vitest beforeEach
spies (same as unity-test-runner).
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
===========================================
- Coverage 70.95% 42.70% -28.26%
===========================================
Files 29 37 +8
Lines 878 733 -145
Branches 239 201 -38
===========================================
- Hits 623 313 -310
- Misses 255 381 +126
- Partials 0 39 +39
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/validate-orchestrator.yml (1)
161-165:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftChange
yarn vitest runtoyarn testornpx jest— the orchestrator repo uses Jest, not Vitest.The
orchestrator-standaloneworking directory is a checkout ofgame-ci/orchestrator, which currently has Jest (^27.5.1) in itsdevDependenciesand runs tests vianode --experimental-vm-modules ./node_modules/jest/bin/jest.js. Vitest is not installed in that repo. Runningyarn vitest runwill fail with a module-not-found error and break the workflow.Either run
yarn test(which delegates to the repo's test script) ornpx jestto match the existing test setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate-orchestrator.yml around lines 161 - 165, Update the "Run orchestrator unit tests" workflow step in the orchestrator-standalone job: replace the failing command `yarn vitest run 2>&1 | tail -30` with a command that invokes the repo's Jest tests such as `yarn test 2>&1 | tail -30` or `npx jest 2>&1 | tail -30` so the step uses the existing test runner; keep the same working-directory (orchestrator-standalone) and output piping to tail.
🧹 Nitpick comments (1)
package.json (1)
71-75: ⚡ Quick winRemove stale
dependenciesMeta.lefthookentry.
lefthookwas removed as a dependency in this PR, but thedependenciesMetablock still references it. Yarn silently ignoresdependenciesMetaentries for absent packages, so there's no functional impact — but leaving it is misleading dead configuration in a quality-tightening PR.🧹 Proposed cleanup
- "dependenciesMeta": { - "lefthook": { - "built": true - } - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 71 - 75, Remove the stale dependenciesMeta entry for "lefthook": open package.json, locate the "dependenciesMeta" object and delete the nested "lefthook" block (the "lefthook": { "built": true } entry) so the file no longer references a package that was removed; ensure JSON remains valid (commas adjusted) after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/test/setup.ts`:
- Around line 3-7: The comment in src/test/setup.ts misleads by saying tests can
opt-out with vi.spyOn(console, 'error') even though beforeEach replaces
console.error with fail; update the comment to explain that vi.spyOn alone will
call through to the replaced implementation and still throw, and that tests must
call vi.spyOn(console, 'error').mockImplementation(() => {}) (or similar
mockImplementation) to suppress the throw; reference the beforeEach replacement
of console.error/console.warn and the use of vi.spyOn(console,
'error')/vi.spyOn(console, 'warn') so readers know exactly how to opt out.
---
Outside diff comments:
In @.github/workflows/validate-orchestrator.yml:
- Around line 161-165: Update the "Run orchestrator unit tests" workflow step in
the orchestrator-standalone job: replace the failing command `yarn vitest run
2>&1 | tail -30` with a command that invokes the repo's Jest tests such as `yarn
test 2>&1 | tail -30` or `npx jest 2>&1 | tail -30` so the step uses the
existing test runner; keep the same working-directory (orchestrator-standalone)
and output piping to tail.
---
Nitpick comments:
In `@package.json`:
- Around line 71-75: Remove the stale dependenciesMeta entry for "lefthook":
open package.json, locate the "dependenciesMeta" object and delete the nested
"lefthook" block (the "lefthook": { "built": true } entry) so the file no longer
references a package that was removed; ensure JSON remains valid (commas
adjusted) after removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb4026b4-9e50-4ad1-9319-722663a533b2
⛔ Files ignored due to path filters (2)
src/model/__snapshots__/versioning.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/validate-orchestrator.ymlpackage.jsonsrc/jest.setup.tssrc/test/setup.tsvitest.config.mts
💤 Files with no reviewable changes (1)
- src/jest.setup.ts
Comment on lines
+3
to
+7
| // Fail tests when console.error / console.warn etc are called from | ||
| // production code under test. Mirrors the jest-fail-on-console behaviour | ||
| // the previous jest setup enforced. Tests can opt-out by replacing the | ||
| // method with vi.spyOn(console, 'error') for the duration of that test. | ||
| const original = { |
There was a problem hiding this comment.
The opt-out comment is misleading — vi.spyOn alone still throws.
vi.spyOn wraps an existing method, and the original implementation still works by default. Because beforeEach replaces console.error with fail before the test runs, a bare vi.spyOn(console, 'error') will install a spy that calls through to fail, which still throws. The comment as written will cause confusing failures for any contributor who follows it literally.
.mockImplementation(() => {}) is required to suppress the throw:
📝 Proposed comment fix
-// Tests can opt-out by replacing the method with vi.spyOn(console, 'error') for the duration of that test.
+// Tests can opt-out via vi.spyOn(console, 'error').mockImplementation(() => {}) for the duration of that test.📝 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
| // Fail tests when console.error / console.warn etc are called from | |
| // production code under test. Mirrors the jest-fail-on-console behaviour | |
| // the previous jest setup enforced. Tests can opt-out by replacing the | |
| // method with vi.spyOn(console, 'error') for the duration of that test. | |
| const original = { | |
| // Fail tests when console.error / console.warn etc are called from | |
| // production code under test. Mirrors the jest-fail-on-console behaviour | |
| // the previous jest setup enforced. Tests can opt-out via vi.spyOn(console, 'error').mockImplementation(() => {}) for the duration of that test. | |
| const original = { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/setup.ts` around lines 3 - 7, The comment in src/test/setup.ts
misleads by saying tests can opt-out with vi.spyOn(console, 'error') even though
beforeEach replaces console.error with fail; update the comment to explain that
vi.spyOn alone will call through to the replaced implementation and still throw,
and that tests must call vi.spyOn(console, 'error').mockImplementation(() => {})
(or similar mockImplementation) to suppress the throw; reference the beforeEach
replacement of console.error/console.warn and the use of vi.spyOn(console,
'error')/vi.spyOn(console, 'warn') so readers know exactly how to opt out.
frostebite
approved these changes
May 6, 2026
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
2 participants
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.
Standard rollout for unity-builder. Most of the work was porting 24 test files from jest 27 to vitest 4.
Drops
lefthook.ymlAdds
scripts/ensure-husky.mjsself-heal + lint-stagedtarget ES2020 \u2192 ES2022+lib ES2022 + DOM(forError.causeand modern globals)Test migration (24 files)
jest.*\u2192vi.*;jest.Mocked\u2192Mockedfrom vitest;jest.MockedFunction\u2192MockedFunction.*.test.tsfiles (and__mocks__/*.ts) that didn't have them.src/index.ts: extractedrunMain()as a named export and gated the module-level invocation behindNODE_ENV !== 'test'. Theindex-plugin-featurestest now callsrunMain()directly instead of relying on jest's removedvi.isolateModules.index-plugin-features.test.ts: moved hoisted refs (mockPlugin,mockLoadOrchestratorPlugin) intovi.hoisted()sovi.mockfactories can reference them. Replaced arrow constructor mock forImageTagwith regularfunction() {...}(vitest 4 disallows arrows as ctors). Replacedrequire('./model')/require('@actions/core')inside test bodies with top-level imports.model/orchestrator-plugin.test.ts: dropped jest's{ virtual: true }flag (vitest doesn't support it); replaced the 'mock factory throws' pattern with 'createPlugin throws' so vitest doesn't wrap the error message at the assertion site.model/versioning.test.ts: strayjest.spyOn\u2192vi.spyOn; replacedmockImplementation()with no args (jest pattern) bymockResolvedValue('')/mockImplementation(() => undefined)where the source expects a string return.Workflow shell-quoting cleanup (actionlint)
$GITHUB_STEP_SUMMARY/$GITHUB_OUTPUT/$GITHUB_ENVredirects quoted across 2 workflows (SC2086).s3://$VAR\u2192s3://"$VAR".for i in {1..N}loops whereiisn't referenced renamed tofor _ in(SC2034).grep ... | wc -l\u2192grep -c ...(SC2126).>> $fileredirects in validate-community-plugins.yml summary block collapsed into a single block redirect (SC2129).cat $file | python3 -c '...'\u2192python3 -c '...' < $file(SC2002).http://${VAR}:port\u2192http://"${VAR}":port(SC2086).tsgo
Kept
tsc --noEmitas defaulttypecheckbecause unity-builder publishes CommonJS for the GitHub Action consumer, which conflicts with tsgo's bundler/node16 moduleResolution requirement (per playbook trap #9).yarn typecheck:tsgois wired up for when consumers move to ESM.Caveats
28 pre-existing oxlint warnings remain (mostly
typescript/no-explicit-anyacross the build-parameter shapes andvitest/no-disabled-testson 2 explicitly skipped scenarios). Per playbook trap #22 the lint script drops--deny-warnings.Verified locally
yarn format:check\u2014 clean (110 files)yarn lint\u2014 0 errors, 28 warningsyarn typecheck\u2014 cleanyarn test\u2014 340 / 342 (2 pre-existing skipped)actionlint\u2014 clean across all 12 workflowsSummary by CodeRabbit
Release Notes
New Features
handleBuild,beforeLocalBuild, andafterLocalBuild.Tests
Chores