[codex] Path-gate Workbench PR package smoke - #455
Conversation
📝 WalkthroughWalkthroughThe PR adds changed-file planning for PR checks, uses it to gate Windows unit tests and package smoke, narrows unit testing to macOS, and rewrites the thin-app smoke job to verify an unpacked ARM64 app. ChangesPR Check Planning and Gated Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
996e373 to
13f6880
Compare
13f6880 to
b3fa65b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/pr-checks.yml:
- Around line 294-313: The coverage failure branch in the GitHub Actions summary
block is writing the ::warning:: message into $GITHUB_STEP_SUMMARY instead of
emitting a real workflow annotation. Update the logic around the coverage
outcome check so the warning is sent directly to the Actions log/annotation
stream, while keeping the summary output separate in the same step. Use the
existing coverage.outcome check and the surrounding summary block to locate the
fix.
- Line 67: The workflow still references external GitHub Actions by tag instead
of full commit SHAs. Update each external action reference in the pr-checks
workflow—actions/checkout, actions/setup-node, oven-sh/setup-bun,
actions/setup-python, actions/cache, actions/upload-artifact, and
codecov/codecov-action—to use pinned SHAs, while leaving the local
./.github/actions/checkout-pr reference unchanged.
- Around line 103-112: The “Publish check plan summary” step in pr-checks.yml is
interpolating planner outputs directly inside the bash script, which can allow
shell metacharacters from `windows_reasons_json` and
`package_smoke_reasons_json` to expand before execution. Move the
`steps.plan.outputs.*` values into `env` for this step and have the script write
them with `printf` (or equivalent safe printing) instead of inline `${{ ... }}`
expansion, using the existing summary block as the place to update.
- Around line 75-90: The changed-files listing step can undercount large PRs
because `gh api --paginate` may stop at GitHub’s file ceiling, which can cause
`run_package_smoke` to be skipped incorrectly. Update the `List changed files`
logic to compare the number of filenames returned from `repos/${{
github.repository }}/pulls/${PR_NUMBER}/files` against the PR’s `changed_files`
value from `pulls/{n}`, and if they do not match, fail closed by forcing package
smoke to run. Use the existing `PR_NUMBER`, `GH_TOKEN`, and `changed-files.txt`
flow in the workflow to locate the fix.
- Around line 450-468: The Build unpacked app only job is exposing sensitive
tokens to untrusted PR code via the env block used by node
scripts/build-with-builder.js. Move any private artifact fetching that needs
GH_TOKEN or EVAOS_DESKTOP_BRIDGE_SOURCE_TOKEN into a separate trusted step or
action before this build, and pass only resulting file paths into the build
step. If a token must still be used, scope it to that fetch step only with a
disposable read-only credential and keep the build step itself token-free.
In `@scripts/evaosPrCheckPlan.js`:
- Around line 55-65: The exported planner helpers are part of the public API but
lack JSDoc on their function declarations. Add JSDoc comments directly above
planPrChecks, requiresWindowsChecks, requiresPackageSmoke, and
isSafePackageSmokeSkip to describe each function’s purpose, parameters, and
return value/contract, instead of documenting only the export block; use the
existing function names to locate the declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51821054-7e28-49a4-b944-d45b7a7f8b69
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Thin App Smoke (macos-arm64)
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
tests/unit/process/evaosPrCheckPlan.test.tsscripts/evaosPrCheckPlan.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
🪛 zizmor (1.26.1)
.github/workflows/pr-checks.yml
[warning] 66-67: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 90-90: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 108-108: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 109-109: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 110-110: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 111-111: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 67-67: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[warning] 166-167: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 167-167: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 176-176: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 181-181: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 295-295: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 386-387: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 387-387: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 440-440: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (1)
tests/unit/process/evaosPrCheckPlan.test.ts (1)
1-90: LGTM!
b3fa65b to
06b2700
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-checks.yml:
- Around line 509-520: The smoke check around BRIDGE_DIR currently treats a
missing Bridge resource as a non-failing “not bundled” status, which allows
packaging regressions to pass. Update the Bridge validation block in the
pr-checks workflow so the absence of the Bridge directory is treated as an
error, not a pass, and fail the job when BRIDGE_DIR is missing or incomplete.
Keep the existing executable/manifest validation for evaos-desktop-bridge and
manifest.json, but ensure the overall smoke step only succeeds when the Bridge
resource is present and valid.
In `@scripts/evaosPrCheckPlan.js`:
- Around line 141-146: The module.exports object in evaosPrCheckPlan.js is a
multi-line export object and should follow the trailing-comma guideline; update
the export list containing isSafePackageSmokeSkip, planPrChecks,
requiresPackageSmoke, and requiresWindowsChecks so the last property keeps a
trailing comma.
In `@tests/unit/process/evaosPrCheckPlan.test.ts`:
- Around line 19-21: The multi-line array passed to prCheckPlan.planPrChecks
violates the trailing-comma style rule. Update the array literals in this test
and the other affected multi-line arrays in evaosPrCheckPlan.test.ts to include
trailing commas, keeping the formatting consistent with the repository’s
require-trailing-comma lint rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 81be901b-8445-4231-a8a6-6d8c44413a64
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⚠️ CI failures not shown inline (6)
GitHub Actions: PR Checks / Thin App Smoke (macos-arm64): [codex] Path-gate Workbench PR package smoke
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "=========================================="�[0m
�[36;1mecho "VERIFY UNPACKED APP RESOURCE SHAPE"�[0m
�[36;1mecho "=========================================="�[0m
�[36;1mAPP_PATH="$(find out -type d -name '*.app' -print -quit)"�[0m
�[36;1mif [ -z "$APP_PATH" ]; then�[0m
�[36;1m echo "::error::No .app produced under out/"�[0m
GitHub Actions: PR Checks / 5_Coverage Test.txt: [codex] Path-gate Workbench PR package smoke
Conclusion: failure
| 0 | 22-329
...zeToolCall.ts | 31.93 | 21.64 | 58.82 | 36.55 | ...26,234,239-242
sideQuestion.ts | 0 | 0 | 0 | 0 | 17
.../chat/approval | 0 | 0 | 0 | 0 |
ApprovalStore.ts | 0 | 0 | 0 | 0 | 38-80
index.ts | 0 | 0 | 0 | 0 |
.../chat/document | 0 | 0 | 0 | 0 |
...tConverter.ts | 0 | 0 | 0 | 0 | 20-234
...hat/navigation | 0 | 0 | 0 | 0 |
...nterceptor.ts | 0 | 0 | 0 | 0 | 15-259
index.ts | 0 | 0 | 0 | 0 |
...mon/chat/slash | 0 | 0 | 0 | 0 |
availability.ts | 0 | 0 | 0 | 0 | 33-36
types.ts | 0 | 0 | 0 | 0 |
.../common/config | 59.76 | 53.33 | 58.82 | 60.74 |
appEnv.ts | 0 | 0 | 0 | 0 | 20-22
configKeys.ts | 0 | 0 | 0 | 0 |
...gMigration.ts | 69.9 | 53.57 | 66.66 | 72.34 | 112-170,353,363
configService.ts | 50 | 60 | 47.05 | 49.2 | ...8,44,63,84-133
constants.ts | 86.66 | 50 | 100 | 100 | 56-57
i18n.ts | 0 | 0 | 0 | 0 | 13-93
...tionMcpEnv.ts | 83.78 | 78.94 | 90 | 83.33 | ...98,124,144-148
storage.ts | 100 | 100 | 100 | 100 |
storageKeys.ts | 0 | 100 | 100 | 0 | 16
...ommon/platform | 14.13 | 8.33 | 6.45 | 14.44 |
...rmServices.ts | 0 | 0 | 0 | 0 | 7-71
...rmServices.ts | 0 | 0 | 0 | 0 |
...rmServices.ts | 28.57 | 0 | 9.09 | 28.57 | ...47,52,65-67,75
index.ts | 14.7 | 16.66 | 10.52 | 15.15 | 12-13,33-78,88
...r-electron.ts | 0 | 100 | 100 | ...
GitHub Actions: PR Checks / Unit Tests (macos-14): [codex] Path-gate Workbench PR package smoke
Conclusion: failure
32m 89�[2mms�[22m�[39m
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
�[90mstderr�[2m | tests/unit/renderer/useAcpMessage.dom.test.ts�[2m > �[22m�[2museAcpMessage�[2m > �[22m�[2mcompletes thinking as soon as the first non-thinking message arrives
�[22m�[39mAn update to TestComponent inside a test was not wrapped in act(...).
When testing, code that cau...
GitHub Actions: PR Checks / 3_Unit Tests (macos-14).txt: [codex] Path-gate Workbench PR package smoke
Conclusion: failure
e events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
An update to TestComponent inside a test was not wrapped in act(...).
When testing, code that causes React state updates should be wrapped into act(...):
act(() => {
/* fire events that update state */
});
/* assert on the output */
This ensures that you're testing the behavior the user would see in the browser. Learn more at https://react.dev/link/wrap-tests-with-act
�[90mstderr�[2m | tests/unit/assistants/AssistantListPanel.dom.test.tsx�[2m > �[22m�[2mAssistantListPanel�[2m > �[22m�[2mrenders without crashing (smoke)
�[22m�[39mAccessing element.ref was removed in React 19. ref is now a regular prop. It will be removed from the JSX Ele...
GitHub Actions: PR Checks / Coverage Test: [codex] Path-gate Workbench PR package smoke
Conclusion: failure
20 {
[cause]: Error: connect ECONNREFUSED 127.0.0.1:13400
�[90m at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1638:16)�[39m {
errno: �[33m-111�[39m,
code: �[32m'ECONNREFUSED'�[39m,
syscall: �[32m'connect'�[39m,
address: �[32m'127.0.0.1'�[39m,
port: �[33m13400�[39m
}
}
�[90mstdout�[2m | tests/unit/assets/presetAssistantResources.test.ts�[2m > �[22m�[2mpresetAssistantResources�[2m > �[22m�[2mhandles multiple calls independently
�[22m�[39m[httpBridge] GET /api/assistants (no body)
�[90mstdout�[2m | tests/unit/assets/presetAssistantResources.test.ts�[2m > �[22m�[2mpresetAssistantResources�[2m > �[22m�[2mhandles multiple calls independently
�[22m�[39m[httpBridge] GET /api/assistants (no body)
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/assets/presetAssistantResources.test.ts �[2m(�[22m�[2m6 tests�[22m�[2m)�[22m�[32m 69�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/renderer/warmupConversation.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 9�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/common/platformConstants.test.ts �[2m(�[22m�[2m6 tests�[22m�[2m)�[22m�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/common-adapter/workspaceMapper.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 5�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/extension/extensionMapperIntegration.test.ts �[2m(�[22m�[2m3 tests�[22m�[2m)�[22m�[32m 9�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/common/normalizeToolCall.test.ts �[2m(�[22m�[2m1 test�[22m�[2m)�[22m�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/cron/cronUtils.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 7�[2mms�[22m�[39m
�[90mstdout�[2m | tests/unit/chat/toolCallLogging.test.ts
�[22m�[39mbuild.buildStorage global
build.buildStorage system
�[32m✓�[39m �[30m�[43m node �[49m�[39m tests/unit/chat/toolCallLogging.test.ts �[2m(�[22m...
GitHub Actions: PR Checks / 0_Thin App Smoke (macos-arm64).txt: [codex] Path-gate Workbench PR package smoke
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "=========================================="�[0m
�[36;1mecho "VERIFY UNPACKED APP RESOURCE SHAPE"�[0m
�[36;1mecho "=========================================="�[0m
�[36;1mAPP_PATH="$(find out -type d -name '*.app' -print -quit)"�[0m
�[36;1mif [ -z "$APP_PATH" ]; then�[0m
�[36;1m echo "::error::No .app produced under out/"�[0m
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
tests/unit/process/evaosPrCheckPlan.test.tsscripts/evaosPrCheckPlan.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
🪛 zizmor (1.26.1)
.github/workflows/pr-checks.yml
[warning] 66-67: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 90-90: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 108-108: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 109-109: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 110-110: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[info] 111-111: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 67-67: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[warning] 202-203: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 203-203: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 212-212: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 217-217: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 295-295: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 386-387: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 387-387: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 440-440: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (2)
scripts/evaosPrCheckPlan.js (1)
55-67: Existing JSDoc finding still applies.The exported planner helpers still need JSDoc on their declarations. As per coding guidelines,
**/*.{ts,tsx,js,jsx}:provide JSDoc for public functions.Also applies to: 141-146
Source: Coding guidelines
.github/workflows/pr-checks.yml (1)
485-494: 🎯 Functional CorrectnessVerify hub contents, not only directory existence.
An empty
hubdirectory currently passes. Add explicit checks for the hub manifest/entrypoint defined by the packaging contract, matching the “resource shape” goal for this smoke.
8d2aee1 to
0c2378a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-checks.yml (1)
365-381: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
workflow_dispatchi18n checks the wrong revision.This job is enabled for manual PR runs, but unlike the other PR-aware jobs it never resolves
inputs.pr_numbervia./.github/actions/checkout-pr. On manual dispatch it will validate the default checkout, not the requested PR head.Match the other jobs
- name: Checkout code uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 with: persist-credentials: false + + - name: Resolve PR context + uses: ./.github/actions/checkout-pr + with: + pr_number: ${{ inputs.pr_number }} + github_token: ${{ github.token }}🤖 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/pr-checks.yml around lines 365 - 381, The i18n workflow job is checking out the default branch instead of the requested PR revision for manual dispatch runs. Update the job setup to match the other PR-aware jobs by resolving inputs.pr_number through ./.github/actions/checkout-pr before actions/checkout, so the i18n checks run against the PR head rather than the default checkout.
♻️ Duplicate comments (1)
.github/workflows/pr-checks.yml (1)
524-556: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSmoke still passes when
Contents/Resources/Bridgeis missing.The PR objective says this lane verifies Bridge, bundled AionCore, and hub packaging, but this step only enforces the latter two. A regression that drops the Bridge bundle still reports success.
Fail closed on missing Bridge
+ BRIDGE_DIR="$APP_PATH/Contents/Resources/Bridge" AIONCORE_DIR="$APP_PATH/Contents/Resources/bundled-aioncore" HUB_DIR="$APP_PATH/Contents/Resources/hub" - for required_dir in "$AIONCORE_DIR" "$HUB_DIR"; do + for required_dir in "$BRIDGE_DIR" "$AIONCORE_DIR" "$HUB_DIR"; do if [ ! -d "$required_dir" ]; then echo "::error::Missing required packaged resource directory: $required_dir" find "$APP_PATH/Contents/Resources" -maxdepth 2 -print || true exit 1 fi done + + BRIDGE_BIN="$BRIDGE_DIR/evaos-desktop-bridge" + BRIDGE_MANIFEST="$BRIDGE_DIR/manifest.json" + if [ ! -x "$BRIDGE_BIN" ] || [ ! -f "$BRIDGE_MANIFEST" ]; then + echo "::error::Bridge resources are incomplete; expected executable and manifest.json" + find "$BRIDGE_DIR" -maxdepth 2 -print || true + exit 1 + 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 @.github/workflows/pr-checks.yml around lines 524 - 556, The packaging smoke check currently validates bundled AionCore and hub resources but never verifies the Bridge bundle, so the workflow can pass when Bridge is missing. Update the resource validation block in the workflow step that checks APP_PATH/Contents/Resources to also assert the Bridge directory exists, using a unique symbol/path check alongside the existing AIONCORE_DIR and HUB_DIR validation, and fail the job with an error message plus a directory listing when Bridge is absent.
🤖 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 `@scripts/evaosPrCheckPlan.js`:
- Around line 17-18: The smoke-trigger list is too broad because the
`evaosPrCheckPlan` matcher for `src/process/` catches every process change and
forces the packaged-app lane. Narrow or remove that blanket regex in
`scripts/evaosPrCheckPlan.js` so only the intended process-related entrypoints
opt in, and keep the more specific `evaosBrokerSession` and
`evaosNativeCompanionStatus` patterns as the targeted fallback.
- Around line 35-39: The path normalization in normalizeFilePath is rewriting
the repository-relative path before classification, which can cause a
PR-controlled filename to be treated as a safe-skip path. Update
normalizeFilePath and the downstream matching in evaosPrCheckPlan.js to classify
the exact changed path as provided, without trim()-based alteration or other
rewriting that changes what was actually changed, so unknown paths remain
fail-closed; make sure the same handling is applied wherever normalizeFilePath
is used, including the related logic around the later matching block.
---
Outside diff comments:
In @.github/workflows/pr-checks.yml:
- Around line 365-381: The i18n workflow job is checking out the default branch
instead of the requested PR revision for manual dispatch runs. Update the job
setup to match the other PR-aware jobs by resolving inputs.pr_number through
./.github/actions/checkout-pr before actions/checkout, so the i18n checks run
against the PR head rather than the default checkout.
---
Duplicate comments:
In @.github/workflows/pr-checks.yml:
- Around line 524-556: The packaging smoke check currently validates bundled
AionCore and hub resources but never verifies the Bridge bundle, so the workflow
can pass when Bridge is missing. Update the resource validation block in the
workflow step that checks APP_PATH/Contents/Resources to also assert the Bridge
directory exists, using a unique symbol/path check alongside the existing
AIONCORE_DIR and HUB_DIR validation, and fail the job with an error message plus
a directory listing when Bridge is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2b8e136-9c21-45f9-af0b-f1e20f97f6ce
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Thin App Smoke (macos-arm64)
- GitHub Check: Coverage Test
- GitHub Check: Unit Tests (macos-14)
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
tests/unit/process/evaosPrCheckPlan.test.tsscripts/evaosPrCheckPlan.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
0c2378a to
8119106
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pr-checks.yml:
- Around line 305-307: The success summary is currently keyed only on the
presence of coverage/lcov.info, so it can report an upload even when the Codecov
step is skipped by its repository condition. Update the summary logic in the
workflow to depend on the actual “Upload coverage to Codecov (Linux coverage
only)” step outcome, or mirror the same repository and file checks used by that
step. Apply the same fix to the other coverage upload/summary branches
referenced in the workflow so the success message only appears when the
corresponding Codecov upload really ran.
In `@tests/unit/process/evaosPrCheckPlan.test.ts`:
- Around line 17-109: Add one CLI-level contract test for the PR check planner
output, since this suite currently only covers planPrChecks() internals. Extend
the test coverage around the main() CLI path to verify it prints the expected
github-output keys such as run_package_smoke and package_smoke_reasons_json,
using a representative input that matches the existing planPrChecks() cases.
This should catch stdout contract regressions that the current evaOS PR check
plan tests would miss.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b6d9949-c733-4fab-aa59-eec1c57db67f
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Thin App Smoke (macos-arm64)
- GitHub Check: Unit Tests (macos-14)
- GitHub Check: Coverage Test
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
scripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
🔇 Additional comments (1)
scripts/evaosPrCheckPlan.js (1)
143-146: 🔒 Security & PrivacyLine-oriented input doesn’t create a bypass here. The workflow already compares the emitted file count to
changed_filesand forcesforce_package_smoke=trueon any mismatch, so a newline in a filename cannot suppressrun_package_smokein this path.> Likely an incorrect or invalid review comment.
8119106 to
a5e791f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pr-checks.yml:
- Around line 104-119: The gate planner is currently executed from the PR
workspace, so a PR can alter scripts/evaosPrCheckPlan.js and influence whether
downstream checks run. Update the Plan macOS-first PR checks step to run the
planner from a trusted checkout of the base/default ref instead of the PR code,
while still passing in changed-files.txt and preserving the existing outputs.
Use the existing plan step and scripts/evaosPrCheckPlan.js as the key locations
to adjust.
- Around line 95-118: The truncated PR file listing is only forcing package
smoke, while run_windows_checks still derives from the incomplete
changed-files.txt and can miss Windows-sensitive changes. Update the PR checks
planning flow in the changed-files step and the Plan macOS-first PR checks step
so that a count mismatch also forces Windows checks to run, using the existing
changed-files.outputs.force_package_smoke signal and the plan logic that
computes RUN_WINDOWS_CHECKS/FORCE_PACKAGE_SMOKE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61f5a8be-3450-4681-9caa-e7cc5dee6644
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Thin App Smoke (macos-arm64)
- GitHub Check: Coverage Test
- GitHub Check: Unit Tests (macos-14)
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
tests/unit/process/evaosPrCheckPlan.test.tsscripts/evaosPrCheckPlan.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
🪛 ast-grep (0.44.0)
tests/unit/process/evaosPrCheckPlan.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (13)
scripts/evaosPrCheckPlan.js (1)
1-175: LGTM!tests/unit/process/evaosPrCheckPlan.test.ts (1)
1-125: LGTM!.github/workflows/pr-checks.yml (11)
538-570: Duplicate: Bridge is still not required by the smoke.The resource check validates AionCore and hub only, so a missing Bridge can still pass this job. This matches the previously flagged unresolved Bridge-validation gap.
15-24: LGTM!
121-135: LGTM!
145-162: LGTM!
179-220: LGTM!
221-263: LGTM!
273-360: LGTM!
373-426: LGTM!
427-521: LGTM!
571-588: LGTM!
598-600: LGTM!
a5e791f to
e1455e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-checks.yml (1)
438-456: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport i18n failures, not just warnings.
scripts/check-i18n.jsexits non-zero on errors, but this summary can still say “No i18n warnings detected.” Includesteps.i18n.outcomeso failed validations are summarized correctly.Proposed fix
- name: Publish i18n summary if: always() shell: bash + env: + I18N_OUTCOME: ${{ steps.i18n.outcome }} run: | { echo "## i18n validation" - if grep -q "⚠️" i18n-check.log; then + if [ "$I18N_OUTCOME" = "failure" ]; then + echo "i18n validation failed. Please review errors below." + elif grep -q "⚠️" i18n-check.log; then echo "Missing/incomplete translations found. Please review warnings below." else echo "No i18n warnings detected."🤖 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/pr-checks.yml around lines 438 - 456, The i18n summary step only checks i18n-check.log for warning symbols, so it can incorrectly report success when scripts/check-i18n.js fails. Update the Publish i18n summary block in the workflow to also inspect steps.i18n.outcome and summarize failures when that step did not succeed, while keeping the existing log dump and warning handling. Use the existing i18n check step and its outcome in the summary logic so errors and warnings are both reported accurately.
🤖 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/pr-checks.yml:
- Around line 337-338: The Codecov upload step still uses a stale repository
guard, so it will not run for the current repo. Update the condition in the
codecov-upload step to use the current repository name in github.repository, or
centralize the repo value in a single shared config so the guard stays in sync
with future renames.
- Around line 71-75: The reusable action reference in the Resolve PR context
step is being loaded from the checked-out PR code, which can let a PR override
checkout-pr and run arbitrary code with github.token. Update the Resolve PR
context usages in pr-checks.yml to load checkout-pr from a trusted ref (or move
the GH API calls inline) so the action source cannot be changed by the PR
itself.
---
Outside diff comments:
In @.github/workflows/pr-checks.yml:
- Around line 438-456: The i18n summary step only checks i18n-check.log for
warning symbols, so it can incorrectly report success when scripts/check-i18n.js
fails. Update the Publish i18n summary block in the workflow to also inspect
steps.i18n.outcome and summarize failures when that step did not succeed, while
keeping the existing log dump and warning handling. Use the existing i18n check
step and its outcome in the summary logic so errors and warnings are both
reported accurately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8334a1c8-9526-4df0-8982-641d4c4c9bcc
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymlscripts/evaosPrCheckPlan.jstests/unit/process/evaosPrCheckPlan.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Unit Tests (windows-2022)
- GitHub Check: Thin App Smoke (macos-arm64)
- GitHub Check: Coverage Test
🧰 Additional context used
📓 Path-based instructions (4)
**/*[!.module].ts
📄 CodeRabbit inference engine (AGENTS.md)
Utility files must use camelCase naming (e.g., formatDate.ts)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Unused function parameters must be prefixed with underscore (_)
Use single-element arrays inline if they fit on one line; require trailing commas in multi-line arrays/objects; use single quotes for strings
Use English for code comments; provide JSDoc for public functions
Files:
tests/unit/process/evaosPrCheckPlan.test.tsscripts/evaosPrCheckPlan.js
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use@icon-park/reactfor icons
TypeScript strict mode must be enabled; no 'any' types, no implicit returns
Use path aliases:@/,@process/,@renderer/* for imports
Prefer 'type' over 'interface' for type definitions (per Oxlint config)
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest 4 for testing with a coverage target of at least 80%
Files:
tests/unit/process/evaosPrCheckPlan.test.ts
🪛 ast-grep (0.44.0)
tests/unit/process/evaosPrCheckPlan.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (4)
scripts/evaosPrCheckPlan.js (1)
3-175: LGTM!tests/unit/process/evaosPrCheckPlan.test.ts (1)
1-125: LGTM!.github/workflows/pr-checks.yml (2)
553-600: Bridge validation is still missing from the thin smoke.The current resource check covers AionCore and hub only; the Bridge verification gap was already flagged in a previous review and still applies here.
15-24: LGTM!Also applies to: 55-70, 77-150, 152-167, 176-193, 210-263, 270-294, 304-320, 339-391, 404-407, 414-420, 458-472, 479-552, 629-631
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-checks.yml (1)
503-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin Bun to a fixed version.
bun-version: latestappears in sixsetup-bunsteps here, so CI behavior can shift whenever Bun releases. Pin it to a specific version, or centralize the version in one workflow/env value and reuse it everywhere.🤖 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/pr-checks.yml around lines 503 - 507, The setup-bun steps are using a moving Bun target, which makes CI behavior unstable across releases. Update the repeated Setup bun configuration in the workflow to use a fixed Bun version instead of latest, or define the version once in a shared workflow/env value and reference it from each setup-bun step so all Bun installs stay consistent.
🤖 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.
Outside diff comments:
In @.github/workflows/pr-checks.yml:
- Around line 503-507: The setup-bun steps are using a moving Bun target, which
makes CI behavior unstable across releases. Update the repeated Setup bun
configuration in the workflow to use a fixed Bun version instead of latest, or
define the version once in a shared workflow/env value and reference it from
each setup-bun step so all Bun installs stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3de60bfb-e1b1-4b2c-beb1-54882d3f7043
📒 Files selected for processing (1)
.github/workflows/pr-checks.yml
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Thin App Smoke (macos-arm64)
- GitHub Check: Unit Tests (windows-2022)
- GitHub Check: Coverage Test
- GitHub Check: Unit Tests (macos-14)
🔇 Additional comments (2)
.github/workflows/pr-checks.yml (2)
675-707: Bridge resource validation is still missing.This block verifies
bundled-aioncoreandhub, but notContents/Resources/Bridge; the PR objective says the thin smoke should verify Bridge as well. This matches the prior unresolved review comment.
552-552: LGTM!Also applies to: 719-719
1 participant
Summary
Closes #374.
This PR adds a risk-aware PR check planner for evaOS Workbench and removes full release-style packaged builds from ordinary PR checks.
What changed
scripts/evaosPrCheckPlan.jswithrun_package_smoke/package_smoke_reasons_jsonoutputs plus existing Windows override support..appsmoke that verifies Bridge, bundled AionCore, and hub resource shape.Validation
./node_modules/.bin/vitest run tests/unit/process/evaosPrCheckPlan.test.ts --maxWorkers=1actionlint .github/workflows/pr-checks.ymlNote: the issue text lists
--runInBand, but this repo's Vitest 4 CLI rejects that Jest-style flag. I used the supported single-worker equivalent above.Summary by CodeRabbit