ci: split package unit jobs#125
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughSplit the single Linux Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant RunnerU as Runner (ubuntu)
participant RunnerW as Runner (windows)
participant Art as Artifact Store
PR->>GH: push/open triggers workflow
GH->>RunnerU: run `changes` job (docs-only detection)
alt docs-only == false
GH->>RunnerU: run `typecheck`
GH->>RunnerU: run `unit-app`
GH->>RunnerU: run `unit-opencode`
GH->>RunnerU: run `unit-desktop`
RunnerU->>Art: upload JUnit reports & artifacts per Linux job
GH->>RunnerW: run `unit-windows` (continue-on-error)
RunnerW->>Art: upload Windows JUnit & artifacts
end
GH->>GH: `check` aggregates Linux unit results (ignores windows) and posts combined status
GH->>PR: report combined CI status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request focuses on improving test stability and CI workflow validation. It increases timeouts and adds detailed error reporting in seed-e2e.test.ts, refactors ci-workflow.test.ts to comprehensively validate GitHub Actions configurations, and enhances edit.test.ts with type-safe event subscriptions and asynchronous test helpers. Feedback from the reviewer suggests refining the withTimeout utility for better resource management and increasing a test timeout to 5,000ms to reduce flakiness in CI environments.
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)
packages/opencode/test/config/seed-e2e.test.ts (1)
16-65:⚠️ Potential issue | 🟡 MinorReplace manual temp directory creation with
tmpdir()andawait usingfor cleaner cleanup.The test can improve by using the standard
tmpdir()helper withawait usingsyntax instead of manualmkdtempandfs.rmcleanup. Seeacp/event-subscription.test.tsfor examples of this pattern in non-Effect tests:Example refactor
await using tmp = await tmpdir() const [home, data, cache, config, state] = await Promise.all([ tmpdir({ init: async () => "home" }).then(t => t.path), // ... or distribute across separate tmpdir calls ]) // cleanup is automatic when scope exitsThe test doesn't require migration to
testEffect(...)/it.live(...)since it doesn't exercise Effect services—it's a subprocess orchestration test that works fine with standard async/await.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/config/seed-e2e.test.ts` around lines 16 - 65, Replace the manual mkdir+Promise.all and explicit fs.rm cleanup in the seed-e2e.test's test block with the tmpdir() helper and JavaScript's "await using" scope so temporary directories are auto-cleaned; specifically, inside the withConfigDepsLock callback create tmpdir instances (via tmpdir() or tmpdir({init: ...}) as needed) instead of calling mkdir("opencode-seed-*"), extract their .path values for OPENCODE_TEST_HOME/XDG_* env entries, and remove the Promise.allSettled fs.rm block and clearTimeout cleanup since the await using scope will dispose the tmpdirs automatically while keeping the existing AbortController/timer logic and Process.run usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/config/seed-e2e.test.ts`:
- Around line 51-57: Remove the redundant assertion after the explicit throw:
the code already throws an Error when out.code !== 0 (the block that constructs
`new Error(...)` using `out.code`, `out.stdout`, and `out.stderr`), so delete
the final `expect(out.code).toBe(0)` assertion that follows it; keep the
existing throw-based check and any surrounding test scaffolding that relies on
the `out` variable.
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 56-78: Replace the custom ManagedRuntime and Promise helpers by
converting each test to use testEffect(...) with it.live(...): remove
subscribeBus, deferred, withTimeout and the ManagedRuntime setup and instead
write tests as Effect.gen(...) that yields required services (EditTool,
Bus.Service, FileWatcher) directly and uses runtime-provided effect combinators
for async/waiting; ensure bus subscriptions use Bus.Service.subscribeCallback
via yielded bus, and replace manual timeouts with effect-based test timeouts or
testEffect's timeout helpers so each test runs live under testEffect/it.live.
---
Outside diff comments:
In `@packages/opencode/test/config/seed-e2e.test.ts`:
- Around line 16-65: Replace the manual mkdir+Promise.all and explicit fs.rm
cleanup in the seed-e2e.test's test block with the tmpdir() helper and
JavaScript's "await using" scope so temporary directories are auto-cleaned;
specifically, inside the withConfigDepsLock callback create tmpdir instances
(via tmpdir() or tmpdir({init: ...}) as needed) instead of calling
mkdir("opencode-seed-*"), extract their .path values for
OPENCODE_TEST_HOME/XDG_* env entries, and remove the Promise.allSettled fs.rm
block and clearTimeout cleanup since the await using scope will dispose the
tmpdirs automatically while keeping the existing AbortController/timer logic and
Process.run usage intact.
🪄 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 Plus
Run ID: 05cd13f1-cf52-4df1-bdfb-d55e6bb134ee
📒 Files selected for processing (4)
.github/workflows/ci.ymlpackages/opencode/test/config/seed-e2e.test.tspackages/opencode/test/github/ci-workflow.test.tspackages/opencode/test/tool/edit.test.ts
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/ci.yml (1)
132-343: 🧹 Nitpick | 🔵 TrivialConsider: The three Linux unit jobs share significant boilerplate.
While the current structure provides clear failure isolation per package, the repeated steps (checkout, node/bun setup, caching, install) could be consolidated using a matrix strategy or reusable composite action. However, given the PR objectives explicitly call for package-scoped jobs that make failures "easier to localize," and the tests validate this exact structure, the current approach is a reasonable trade-off for clarity.
💡 Alternative: Matrix-based approach (optional)
If duplication maintenance becomes burdensome, a matrix could consolidate the jobs while preserving package-scoped reporting:
unit: needs: changes if: needs.changes.outputs.docs_only != 'true' runs-on: ubuntu-latest timeout-minutes: 30 strategy: fail-fast: false matrix: include: - package: app filter: "@opencode-ai/app" path: packages/app - package: opencode filter: "opencode" path: packages/opencode - package: desktop filter: "@opencode-ai/desktop-electron" path: packages/desktop-electron # ... steps using ${{ matrix.filter }}, ${{ matrix.path }}, etc.This would require adjusting the
checkjob to reference matrix job results differently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 132 - 343, The three jobs unit-app, unit-opencode, and unit-desktop duplicate identical setup/install steps; replace them with a single job (e.g., unit) that uses strategy.matrix.include entries for each package (include keys: package, filter, path) and convert the repeated steps (actions/checkout, actions/setup-node, oven-sh/setup-bun, actions/cache, bun install, bun turbo test:ci) to use ${{ matrix.filter }} and ${{ matrix.path }}; update the Publish unit reports and Upload unit artifacts steps to reference the matrix values (e.g., report_paths: ${{ matrix.path }}/.artifacts/unit/junit.xml and artifact name using ${{ matrix.package }}), set strategy.fail-fast: false to preserve per-package failure isolation, and ensure any other jobs that read these results (like the check job) are adjusted to consume matrix job outputs if needed.
♻️ Duplicate comments (1)
packages/opencode/test/tool/edit.test.ts (1)
61-78: 🛠️ Refactor suggestion | 🟠 MajorPlease avoid adding more ad hoc Promise test helpers in this Effect test file.
deferred/withTimeoutextend manual async orchestration instead of moving this suite to the standard Effect test harness pattern already requested.#!/bin/bash set -euo pipefail # Verify this file still uses custom runtime/promise wrappers and does not use the standard harness. rg -n --type=ts 'ManagedRuntime\.make|runPromise\(|function deferred<|function withTimeout<|testEffect\(|\bit\.(effect|live)\(' packages/opencode/test/tool/edit.test.tsAs per coding guidelines: "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows." and "Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/edit.test.ts` around lines 61 - 78, The test file introduces ad-hoc Promise helpers deferred and withTimeout which bypass the standard Effect test harness; remove the functions deferred and withTimeout from packages/opencode/test/tool/edit.test.ts and refactor any tests that use them to use the existing test harness API testEffect (from test/lib/effect.ts) and its provided runtime utilities (e.g., runPromise/testEffect's effect assertions or timeouts) so the tests run under the managed Effect runtime instead of manual promise orchestration; search for usages of deferred, withTimeout, runPromise, ManagedRuntime.make, attach, or it.effect/it.live in this file and replace those patterns with testEffect and the harness equivalents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/plugin/workspace-adaptor.test.ts`:
- Around line 129-130: The waitFor predicate and subsequent assertion
incorrectly treat a missing status entry as success; update the polling to first
find the entry via Workspace.status().find(item => item.workspaceID === info.id)
and ensure the entry exists and its status !== "connecting" (e.g., entry &&
entry.status !== "connecting"), and update the expect to assert the found entry
exists and its status is not "connecting" so the test only passes after the
workspace status actually transitions.
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 351-353: The 1,000ms hardcoded timeout on
withTimeout(event.promise, 1_000, ...) is too short for CI and causes flakes;
update the call to use a larger timeout (e.g. 5_000) or replace the literal with
a shared test timeout constant (e.g. TEST_TIMEOUT = 5000) and reference that in
the withTimeout invocation to give filesystem/bus propagation more headroom.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 132-343: The three jobs unit-app, unit-opencode, and unit-desktop
duplicate identical setup/install steps; replace them with a single job (e.g.,
unit) that uses strategy.matrix.include entries for each package (include keys:
package, filter, path) and convert the repeated steps (actions/checkout,
actions/setup-node, oven-sh/setup-bun, actions/cache, bun install, bun turbo
test:ci) to use ${{ matrix.filter }} and ${{ matrix.path }}; update the Publish
unit reports and Upload unit artifacts steps to reference the matrix values
(e.g., report_paths: ${{ matrix.path }}/.artifacts/unit/junit.xml and artifact
name using ${{ matrix.package }}), set strategy.fail-fast: false to preserve
per-package failure isolation, and ensure any other jobs that read these results
(like the check job) are adjusted to consume matrix job outputs if needed.
---
Duplicate comments:
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 61-78: The test file introduces ad-hoc Promise helpers deferred
and withTimeout which bypass the standard Effect test harness; remove the
functions deferred and withTimeout from packages/opencode/test/tool/edit.test.ts
and refactor any tests that use them to use the existing test harness API
testEffect (from test/lib/effect.ts) and its provided runtime utilities (e.g.,
runPromise/testEffect's effect assertions or timeouts) so the tests run under
the managed Effect runtime instead of manual promise orchestration; search for
usages of deferred, withTimeout, runPromise, ManagedRuntime.make, attach, or
it.effect/it.live in this file and replace those patterns with testEffect and
the harness equivalents.
🪄 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 Plus
Run ID: d3f43f08-2a4d-4382-a95f-effc78726331
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackages/opencode/test/plugin/workspace-adaptor.test.tspackages/opencode/test/tool/edit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/opencode/test/tool/edit.test.ts (1)
56-75: 🛠️ Refactor suggestion | 🟠 MajorThis extends the custom runtime/Promise harness instead of moving the file onto
testEffect(...).The new
subscribeBus,deferred, andwithTimeouthelpers add more ad hoc scaffolding around Effect services in a file that should use the repository’s Effect test harness.Run this to confirm the file is still growing the custom-runtime pattern instead of the repo-standard
testEffect(...)/it.live(...)pattern:#!/bin/bash set -euo pipefail rg -n --type=ts 'ManagedRuntime\.make|subscribeBus|deferred\(|withTimeout\(' packages/opencode/test/tool/edit.test.ts printf '\n--- reference patterns elsewhere ---\n' rg -n --type=ts 'const it = testEffect|it\.live\(' packages/opencode/testAs per coding guidelines, "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows." and "Avoid customManagedRuntime,attach(...), or ad hocrun(...)wrappers in Effect tests whentestEffect(...)already provides the runtime."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/edit.test.ts` around lines 56 - 75, The test file is using ad-hoc runtime helpers (subscribeBus, deferred, withTimeout and runtime.runPromise/Bus.Service.use) instead of the repo-standard Effect test harness; refactor the test to use testEffect/it.live from test/lib/effect.ts: remove subscribeBus, deferred, and withTimeout helpers, move the test body into testEffect(...) or it.live(...) and perform Bus subscription inside the testEffect environment (use Bus.Service within the testEffect effect instead of runtime.runPromise/Bus.Service.use), replace manual promise/timeouts with the harness's async/timeout utilities or Effect-friendly equivalents, and ensure assertions run inside the testEffect effect so the standard runtime handles lifecycle and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/github/ci-workflow.test.ts`:
- Around line 22-28: The helper function runStep should be renamed to stepByName
for clarity; update the function declaration (rename runStep to stepByName) and
change every call site that currently calls runStep(...) to call
stepByName(...), keeping the same parameter order and return behavior; ensure
any nearby helpers like checkoutStep remain unchanged and run the tests to
verify no references to runStep remain.
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 330-353: The test subscribes to FileWatcher.Event.Updated and
resolves the deferred on the first event regardless of file; update the
subscription callback (from subscribeBus(FileWatcher.Event.Updated, ...)) to
inspect the incoming payload.properties.file and only call event.resolve(...)
when payload.properties.file === filepath (and then optionally call
unsubUpdated() to stop listening), so the deferred (event.promise) only resolves
for the specific file edit triggered by edit.execute; ensure the current
references to event.resolve, event.promise, subscribeBus,
FileWatcher.Event.Updated, edit.execute, and filepath are used to implement this
filter.
---
Duplicate comments:
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 56-75: The test file is using ad-hoc runtime helpers
(subscribeBus, deferred, withTimeout and runtime.runPromise/Bus.Service.use)
instead of the repo-standard Effect test harness; refactor the test to use
testEffect/it.live from test/lib/effect.ts: remove subscribeBus, deferred, and
withTimeout helpers, move the test body into testEffect(...) or it.live(...) and
perform Bus subscription inside the testEffect environment (use Bus.Service
within the testEffect effect instead of runtime.runPromise/Bus.Service.use),
replace manual promise/timeouts with the harness's async/timeout utilities or
Effect-friendly equivalents, and ensure assertions run inside the testEffect
effect so the standard runtime handles lifecycle and cleanup.
🪄 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 Plus
Run ID: 719677fe-ba89-4c3e-9f31-533fa6d9446b
📒 Files selected for processing (9)
.github/workflows/ci.ymlpackages/app/src/shell-frame-contract.test.tspackages/desktop-electron/scripts/ci-smoke.test.tspackages/desktop-electron/scripts/prepare-embedded-server.test.tspackages/opencode/test/config/config.test.tspackages/opencode/test/config/seed-e2e.test.tspackages/opencode/test/github/ci-workflow.test.tspackages/opencode/test/plugin/workspace-adaptor.test.tspackages/opencode/test/tool/edit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/opencode/test/tool/edit.test.ts (1)
76-92:⚠️ Potential issue | 🟠 MajorUse the actual watcher payload and scope the subscription to the target file.
FileWatcher.Event.Updatedis published as{ file, event }, sopayload.propertiesis the wrong shape here. Separately, resolving on the first update from any file can still pick up unrelated watcher traffic and reintroduce the flake this helper is supposed to remove. Pass the expected path intonextFileUpdate, ignore non-matching events, and resolve the deferred withpayloaditself.🛠️ Suggested fix
const nextFileUpdate = <A, E, R>( + file: string, trigger: Effect.Effect<A, E, R>, ) => Effect.acquireUseRelease( Effect.gen(function* () { const deferred = yield* Deferred.make<FileUpdate>() const bus = yield* Bus.Service const unsubscribe = yield* bus.subscribeCallback(FileWatcher.Event.Updated, (payload) => { - Deferred.doneUnsafe(deferred, Effect.succeed(payload.properties)) + if (payload.file !== file) return + Deferred.doneUnsafe(deferred, Effect.succeed(payload)) }) return { deferred, unsubscribe } }), @@ - const event = yield* nextFileUpdate( + const event = yield* nextFileUpdate( + filepath, run({ filePath: filepath, oldString: "", newString: "content", }), @@ - const event = yield* nextFileUpdate( + const event = yield* nextFileUpdate( + filepath, run({ filePath: filepath, oldString: "original", newString: "modified", }),Also applies to: 135-141, 267-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/edit.test.ts` around lines 76 - 92, The helper nextFileUpdate should accept the expected file path, subscribe to FileWatcher.Event.Updated, filter events to only resolve when payload.file matches that path, and resolve the Deferred with the full payload (not payload.properties); update the signature of nextFileUpdate to take a path string, change the subscribeCallback handler in the acquisition to check payload.file === expectedPath and call Deferred.doneUnsafe(deferred, Effect.succeed(payload)), and keep the same unsubscribe/timeout behavior; apply the same change to the other occurrences referenced (around the other ranges mentioned) so the helper only resolves on matching-file updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 575-591: The current test spawns two concurrent run(...) edits and
only asserts that at least one succeeded; update the assertion to verify exactly
one success and one failure (use Exit.isSuccess on each element of results to
count successes) and then read the final file content for filepath to assert it
equals either "1" or "2" (this ensures edits were serialized per-file). Locate
the test's run invocation and results variable in edit.test.ts and replace the
loose expect(...) with: count successes == 1 and count failures == 1, followed
by a file-read check that the file content is strictly "1" or "2".
---
Duplicate comments:
In `@packages/opencode/test/tool/edit.test.ts`:
- Around line 76-92: The helper nextFileUpdate should accept the expected file
path, subscribe to FileWatcher.Event.Updated, filter events to only resolve when
payload.file matches that path, and resolve the Deferred with the full payload
(not payload.properties); update the signature of nextFileUpdate to take a path
string, change the subscribeCallback handler in the acquisition to check
payload.file === expectedPath and call Deferred.doneUnsafe(deferred,
Effect.succeed(payload)), and keep the same unsubscribe/timeout behavior; apply
the same change to the other occurrences referenced (around the other ranges
mentioned) so the helper only resolves on matching-file updates.
🪄 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 Plus
Run ID: 1e399ea5-8357-4c34-a965-233f7d0cb8f2
📒 Files selected for processing (1)
packages/opencode/test/tool/edit.test.ts
35b243f to
7ea96e9
Compare
7ea96e9 to
9aab003
Compare
Summary
Why
CI has been failing too often from a mix of broad unit jobs and known flaky tests. This keeps the required
ci / checkgate intact while making Linux unit failures easier to localize and adding Windows signal without blocking merges yet.Related Issue
Closes #121
How To Verify
Screenshots or Recordings
Not applicable, CI-only change.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Tests
Chores