fix(provider): represent provider install/init lifecycle in the UI - #811
Merged
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughProvider and workspace lifecycle operations now emit structured status events and track asynchronous jobs. IPC forwards job snapshots to the renderer, which displays lifecycle states. Worker logs are streamed separately, and tests cover failure handling, refresh ordering, and lifecycle badges. ChangesLifecycle status and reporting
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
from
July 29, 2026 23:43
c6c1cbb to
79efbc9
Compare
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
6 times, most recently
from
July 30, 2026 14:33
551c499 to
355cd3c
Compare
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
from
July 30, 2026 20:03
355cd3c to
4d39c59
Compare
Adding a provider showed a red "not initialized" badge for several seconds before flipping to "initialized". provider add --use=false persists the provider to config.yaml before init runs, and parseProviderEntries collapsed "not yet known" and "genuinely not initialized" into the same false, so the card rendered the destructive badge for both. Two lifecycle axes were collapsed into one boolean. They're now separate: - Persisted truth stays Initialized bool on disk: "did this binary run Exec.Init". No schema change, no migration. - Transient job state moves to a main-process ProviderJobs registry, fed by the CLI's structured status events and broadcast on the existing providers-changed channel. Main-process ownership means it survives the wizard closing, navigation, and window reload. This also distinguishes states the flag cannot: installing vs initializing vs updating, and failed vs never-initialized. Also included: - provider add/init/set-source emit structured progress (pkg/status Phase/Event/Reporter, as workspace up does), replacing the desktop's "Exit code: 0" string-sniffing. pkg/devcontainer/status -> pkg/status, since it's no longer devcontainer-specific. - Correctness fix: set-source left Initialized stale, so a provider updated to a new binary still reported initialized: true though the new binary's init never ran. Update and version-pin now chain provider init. - runStreaming hang: it registered close but not error, so a spawn failure never invoked onExit, hanging callers and leaking the concurrency slot. Affected every streaming command, not just providers. Squashed from the individual commits on this branch, rebased onto main after #798 merged. Conflicts resolved by keeping both branches' independent fixes (e.g. task PID-reuse guard, envelope validation) alongside this branch's new Pipeline/ProviderJobs work.
Confirmed real by verification, not just the review's say-so: - watcher.ts: refreshProviders() called pollProviders() directly, bypassing the polling/pollQueued coordination schedulePoll uses. A manual refresh (e.g. after an install finishes) could run concurrently with a scheduled poll, and whichever CLI call landed last could overwrite the other's result — reintroducing the same "stale state wins" bug class this PR exists to fix. Both paths now go through a serializing queue scoped to provider polls. Added a unit test that fails against the old code (proved by reverting locally) and passes with the fix. - ipc.ts: runProviderWithStatus discarded cli.runStreaming's promise with `void`, so a rejection from a failure before onExit is wired up would hang the caller forever instead of surfacing. Low probability given runStreaming's own error handling, but a zero-risk safety net. - providers.e2e.ts: wrapped both provider lifecycle tests' assertions in try/finally so a failed assertion still runs the provider_delete cleanup, instead of leaking dirty mock CLI state into later specs. Skipped two other findings from the same review after tracing actual consumers: - cmd/provider/add.go's status.Enter uses the pre-resolution provider name (empty when --name is omitted). No current consumer (CLI plain text output, desktop's job tracking) reads that field for this event, so there's no observable effect to fix. - provider-jobs.ts's generation tracking already prevents a delayed release from clobbering a newer job — verified via an existing test (`does not clear a newer job started while refresh was in flight`). The suggested renderer-side token would be redundant.
skevetter
force-pushed
the
feat/async-provider-lifecycle
branch
from
August 1, 2026 02:24
4d39c59 to
fc964ab
Compare
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
… count The prior version asserted callCount === 2 after two concurrent refreshProviders() calls, which passes whether or not they're serialized — two concurrent queries and two queued ones both finish two queries. Recording start/end order is what actually distinguishes them; verified by reverting the fix locally and confirming this version fails while the count-based one didn't.
skevetter
marked this pull request as ready for review
August 1, 2026 03:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
desktop/src/renderer/src/lib/stores/providers.ts (1)
16-30: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFetch an initial provider-job snapshot.
initProvidersfetches only providers before it registersonProvidersChanged. It updatesproviderJobsonly from future events. If a provider job started before this listener registers and has no later mutation,providerJobsremains empty. The UI then shows the persisted uninitialized state instead of the active job after navigation or window reload.Return an atomic
{ providers, jobs }snapshot from the main process, or request an immediate post-subscription broadcast. Prevent an older initial snapshot from overwriting a newer event. Add coverage for a job that is active beforeinitProviders()runs.🤖 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 `@desktop/src/renderer/src/lib/stores/providers.ts` around lines 16 - 30, Update initProviders to obtain an initial atomic providers-and-providerJobs snapshot from the main process, or trigger an immediate broadcast after registering onProvidersChanged, and initialize both stores from it. Ensure snapshot application cannot overwrite a newer event received during subscription, and add coverage for a job already active before initProviders runs.desktop/src/main/ipc.ts (1)
494-552: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
provider_init_streaminglifecycle against failures.If
cli.runStreamingrejects afterproviderJobs.start(args.name, "initializing"), the provider job stays open and the wizard never receives acommand-progressdoneevent. In addition,onExitis passed asvoid, so rejecting inside it fromproviderJobs.finish(...)can become an unhandled rejection; wrap/fire-and-forget the finish/exit path and send the finalcommand-progressresult.🤖 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 `@desktop/src/main/ipc.ts` around lines 494 - 552, Update the provider_init_streaming handler around cli.runStreaming to handle both rejected streaming calls and failures in the async onExit callback. Ensure every path after providerJobs.start calls providerJobs.finish and sends a command-progress event with done: true, including the cli.runStreaming rejection path, while preventing rejected providerJobs.finish calls from becoming unhandled rejections. Preserve the existing success/error result fields and exit-message behavior for normal completion.
🧹 Nitpick comments (2)
desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts (1)
55-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap job-store cleanup in try/finally.
Each of these new tests sets
providerJobs, renders, asserts, then callsproviderJobs.set({})andunmount()as plain trailing statements. If anexpect(...)in the test body throws, the cleanup calls never run, leavingproviderJobspopulated and the component unmounted only if a globalafterEach(cleanup)handles it.The PR's commit history already fixed this exact class of issue for the E2E suite ("Ensured E2E cleanup runs after assertion failures"). Apply the same pattern here.
♻️ Proposed fix (example for one test)
it("shows installing rather than not initialized during install", () => { providerJobs.set({ ssh: { activity: "installing", phase: "installing_provider" }, }) - const { container, unmount } = render(ProviderCard, { - props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, - }) - - const text = (container.textContent ?? "").toLowerCase() - expect(text).toContain("installing") - expect(text).not.toContain("not initialized") - providerJobs.set({}) - unmount() + const { container, unmount } = render(ProviderCard, { + props: { provider: makeProvider("ssh", { state: { initialized: false } }) }, + }) + try { + const text = (container.textContent ?? "").toLowerCase() + expect(text).toContain("installing") + expect(text).not.toContain("not initialized") + } finally { + providerJobs.set({}) + unmount() + } })Also applies to: 69-82, 84-97, 99-100
🤖 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 `@desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts` around lines 55 - 65, Wrap each affected ProviderCard test’s render, assertions, and cleanup in a try/finally block, ensuring providerJobs.set({}) and unmount() execute even when an expectation fails. Apply this to the tests around the existing providerJobs setup, including the ranges also identified in the review, without changing their assertions.desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte (1)
45-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
ProviderStatusBadgecomponent. Both files implement the identicalproviderStatus-driven ready/busy/failed/fallback badge logic with the samebadgeVariantscalls and labels; the only difference is the busy-state icon, which has already diverged (Loader2vsSpinner) for the same conceptual state.
desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte#L45-L57: replace this branch with a shared<ProviderStatusBadge {status} />component.desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte#L370-L382: replace this branch with the same shared<ProviderStatusBadge {status} />component.Centralizing this logic in one component removes the duplication, keeps the busy-state icon consistent across the card and the sheet, and reduces the risk of the two views drifting apart when a new status kind or label is added later.
🤖 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 `@desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte` around lines 45 - 57, Extract the duplicated provider status rendering into a shared ProviderStatusBadge component that accepts status and preserves the ready, busy, failed, and fallback badge behavior with one consistent busy icon. In desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte lines 45-57 and desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte lines 370-382, replace each status branch with the shared ProviderStatusBadge {status} component.
🤖 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 `@desktop/src/main/ipc.ts`:
- Around line 257-286: The success path in withProviderJob currently treats a
refreshProviders failure from providerJobs.finish(name) as the provider
operation’s failure. Restructure withProviderJob so the operation result and
errors are handled in the try/catch, then call providerJobs.finish(name) after
the try/catch for successful operations, preserving the original error message
only for fn failures and keeping the result return behavior intact.
In `@desktop/src/main/provider-jobs.ts`:
- Around line 88-112: The error branch of finish() must not recreate a provider
job that has already been cleared. In finish(), retrieve the existing job before
writing the failed state, return when no job exists, and only then update it
with phase "failed", preserving the existing activity and error handling.
---
Outside diff comments:
In `@desktop/src/main/ipc.ts`:
- Around line 494-552: Update the provider_init_streaming handler around
cli.runStreaming to handle both rejected streaming calls and failures in the
async onExit callback. Ensure every path after providerJobs.start calls
providerJobs.finish and sends a command-progress event with done: true,
including the cli.runStreaming rejection path, while preventing rejected
providerJobs.finish calls from becoming unhandled rejections. Preserve the
existing success/error result fields and exit-message behavior for normal
completion.
In `@desktop/src/renderer/src/lib/stores/providers.ts`:
- Around line 16-30: Update initProviders to obtain an initial atomic
providers-and-providerJobs snapshot from the main process, or trigger an
immediate broadcast after registering onProvidersChanged, and initialize both
stores from it. Ensure snapshot application cannot overwrite a newer event
received during subscription, and add coverage for a job already active before
initProviders runs.
---
Nitpick comments:
In `@desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte`:
- Around line 45-57: Extract the duplicated provider status rendering into a
shared ProviderStatusBadge component that accepts status and preserves the
ready, busy, failed, and fallback badge behavior with one consistent busy icon.
In desktop/src/renderer/src/lib/components/provider/ProviderCard.svelte lines
45-57 and desktop/src/renderer/src/lib/components/provider/ProviderSheet.svelte
lines 370-382, replace each status branch with the shared ProviderStatusBadge
{status} component.
In `@desktop/src/renderer/src/lib/components/provider/ProviderCard.test.ts`:
- Around line 55-65: Wrap each affected ProviderCard test’s render, assertions,
and cleanup in a try/finally block, ensuring providerJobs.set({}) and unmount()
execute even when an expectation fails. Apply this to the tests around the
existing providerJobs setup, including the ranges also identified in the review,
without changing their assertions.
🪄 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: CHILL
Plan: Pro Plus
Run ID: d6f635fa-a911-4ecb-92c7-9465aea632fa
📒 Files selected for processing (41)
Taskfile.ymlcmd/provider/add.gocmd/provider/configure_shared.gocmd/provider/init.gocmd/provider/set_source.gocmd/provider/status.gocmd/workspace/up/status.godesktop/e2e/app.e2e.tsdesktop/e2e/fixtures/mock-devsy.cjsdesktop/e2e/providers.e2e.tsdesktop/src/main/__tests__/cli.test.tsdesktop/src/main/__tests__/ipc-provider-jobs.test.tsdesktop/src/main/__tests__/provider-jobs.test.tsdesktop/src/main/__tests__/watcher.test.tsdesktop/src/main/cli.tsdesktop/src/main/index.tsdesktop/src/main/ipc.tsdesktop/src/main/provider-jobs.tsdesktop/src/main/watcher.tsdesktop/src/renderer/src/lib/components/provider/ProviderCard.sveltedesktop/src/renderer/src/lib/components/provider/ProviderCard.test.tsdesktop/src/renderer/src/lib/components/provider/ProviderSheet.sveltedesktop/src/renderer/src/lib/components/provider/ProviderSheet.test.tsdesktop/src/renderer/src/lib/components/provider/ProviderWizard.sveltedesktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.sveltedesktop/src/renderer/src/lib/ipc/commands.tsdesktop/src/renderer/src/lib/ipc/events.tsdesktop/src/renderer/src/lib/ipc/mock.tsdesktop/src/renderer/src/lib/stores/providers.tsdesktop/src/renderer/src/lib/types/index.tsdesktop/src/renderer/src/lib/utils/log-parser.test.tsdesktop/src/renderer/src/lib/utils/log-parser.tsdesktop/src/renderer/src/lib/utils/provider-status.tsdesktop/src/renderer/src/pages/WorkspaceDetailPage.sveltepkg/agent/tunnelserver/tunnelserver.gopkg/devcontainer/config/envelope.gopkg/devcontainer/config/envelope_test.gopkg/status/status.gopkg/status/status_test.gopkg/workspace/provider.gopkg/workspace/provider_update_test.go
skevetter
marked this pull request as draft
August 1, 2026 03:45
…ng cleared jobs Both confirmed with reproductions before fixing (see the two new tests, which fail against the prior code and pass against these fixes): - withProviderJob's success-path finish(name) call lived inside the try block, so a rejection from it (e.g. the refresh it awaits failing) was caught by the catch clause and re-reported via finish(name, <refresh error>) as if the operation itself had failed, discarding that fn() actually succeeded. Moved the success-path finish() outside the try/catch. - ProviderJobs.finish()'s error branch wrote a new "failed" job entry unconditionally, even when the job had already been cleared (e.g. by a concurrent provider_delete). A delayed/stale error arriving after that point resurrected UI activity for a provider the user already considers gone — the same invariant report() already documents and enforces. Now returns early when the job is gone.
…ollow Since the async up model landed, the desktop no longer runs `workspace up` directly and streams its output live — it submits `--detach` and polls `workspace task logs --follow`. The worker's real stdout/stderr get redirected to a *.streams file (pkg/command.StartBackground) that nothing ever read back, so the UI only ever saw synthesized phase-transition lines. Enabling debug mode had no effect because the worker's actual log lines, at any level, never reached it. followTask now tails that streams file each poll and forwards new lines to stderr, skipping structured NDJSON envelopes (status/result/ error) since those are already reported from polled task state and would otherwise show up twice. Verified with the CLI directly first (confirmed --debug does emit debug-level lines, and confirmed nothing read the streams file before this change), then with unit tests for the tailer and an end-to-end test that runs followTask against a real task while concurrently writing to its streams file.
…val is in flight Mirrors the ProviderJobs pattern with a WorkspaceJobs registry so the UI reflects an in-progress delete instead of showing stale status until the next poll picks up the CLI's exit.
… job - WorkspaceJobs.finish() now requires the generation start() returned, so a stale exit callback can't misattribute its result to a retry that already superseded it. - A failed delete no longer keeps the row disabled forever: it drops out of "deleting" and shows a dismissible "Delete failed" badge, matching the provider status convention. - mock-devsy.cjs's delayed delete re-reads persisted state instead of writing back a stale in-memory snapshot, matching the existing provider-init fixture's pattern. - workspaces.e2e.ts's intentionally unawaited delete call now swallows its rejection instead of risking an unhandled rejection.
CI's golangci-lint run flagged the variable path passed to os.OpenFile in the append-and-repoll test; it's t.TempDir()-derived, same as the file's other os.WriteFile calls.
Several multi-line comments restated context already clear from the surrounding code; condensed to one line each.
skevetter
marked this pull request as ready for review
August 1, 2026 06:39
…2e test Passed "id" instead of "workspaceId", so the main process fell back to the raw source URL as the log-path segment. Windows rejects the colon in "https://", failing mkdir; Unix tolerates it, so this only surfaced on the Windows CI runner.
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 `@desktop/e2e/fixtures/mock-devsy.cjs`:
- Around line 511-514: Remove the top-level return following
handler(rawArgs.slice(2)) in the dispatch logic of mock-devsy.cjs, since it is
outside a function and prevents Node.js from parsing the fixture. Restructure
the surrounding workspace/non-workspace dispatch with an else branch or move it
into a function so the non-workspace switch is skipped without using a top-level
return.
In `@desktop/src/main/__tests__/ipc-provider-jobs.test.ts`:
- Around line 180-195: Strengthen the test around the provider_init flow so it
positively asserts the successful job state after the post-success refresh
rejects. Update the assertion in “does not blame a successful init for a refresh
failure afterward” to require that the docker job remains present and reflects
successful initialization, rather than only checking that its error differs from
“refresh boom”.
In `@desktop/src/main/ipc.ts`:
- Around line 1134-1154: The delete operation’s `sink.done()` call drops the
available `cliError` on non-zero exit. Update the `cli.runStreaming` exit
callback to pass the structured failure payload used by `workspace_rebuild` and
`workspace_reset`—including error level, `success: false`, and `cliError`—while
preserving the existing success payload.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 3e9d7c0a-6d61-4ce0-a9c5-18d13dc30fa1
📒 Files selected for processing (20)
cmd/workspace/task.gocmd/workspace/task_tail.gocmd/workspace/task_tail_test.gocmd/workspace/up/detach.godesktop/e2e/fixtures/mock-devsy.cjsdesktop/e2e/workspaces.e2e.tsdesktop/src/main/__tests__/ipc-provider-jobs.test.tsdesktop/src/main/__tests__/ipc-workspace-jobs.test.tsdesktop/src/main/__tests__/watcher.test.tsdesktop/src/main/__tests__/workspace-jobs.test.tsdesktop/src/main/index.tsdesktop/src/main/ipc.tsdesktop/src/main/provider-jobs.tsdesktop/src/main/watcher.tsdesktop/src/main/workspace-jobs.tsdesktop/src/renderer/src/lib/ipc/events.tsdesktop/src/renderer/src/lib/stores/workspaces.tsdesktop/src/renderer/src/lib/types/index.tsdesktop/src/renderer/src/pages/WorkspacesPage.sveltepkg/task/task.go
🚧 Files skipped from review as they are similar to previous changes (2)
- desktop/src/main/tests/watcher.test.ts
- desktop/src/main/provider-jobs.ts
Comment on lines
+511
to
+514
| // No unconditional exit(0) here: handleDelete's setTimeout would get killed | ||
| // before firing. `return` still skips the non-workspace switch below. | ||
| handler(rawArgs.slice(2)) | ||
| process.exit(0) | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the top-level return.
Line 514 is outside a function. Node.js cannot parse this fixture. The mock CLI cannot start, so E2E tests cannot run.
Restructure the following dispatch logic with an else block, or place this code in a function, to skip the non-workspace switch without a top-level return.
🧰 Tools
🪛 Biome (2.5.5)
[error] 514-514: Illegal return statement outside of a function
(parse)
🤖 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 `@desktop/e2e/fixtures/mock-devsy.cjs` around lines 511 - 514, Remove the
top-level return following handler(rawArgs.slice(2)) in the dispatch logic of
mock-devsy.cjs, since it is outside a function and prevents Node.js from parsing
the fixture. Restructure the surrounding workspace/non-workspace dispatch with
an else branch or move it into a function so the non-workspace switch is skipped
without using a top-level return.
Source: Linters/SAST tools
Comment on lines
+180
to
+195
|
|
||
| it("does not blame a successful init for a refresh failure afterward", async () => { | ||
| // `provider init` itself succeeds; only the post-success refresh fails. | ||
| const { providerJobs } = setup(() => ({ | ||
| lines: [statusLine("running_init"), statusLine("ready")], | ||
| code: 0, | ||
| })) | ||
| providerJobs.setRefresh(() => Promise.reject(new Error("refresh boom"))) | ||
|
|
||
| await invoke("provider_init", { name: "docker" }) | ||
|
|
||
| // The bug this guards: finish()'s success path rejecting (via a failed | ||
| // refresh) must not get caught and re-reported as if the init command | ||
| // itself had failed with the refresh's error message. | ||
| expect(providerJobs.get("docker")?.error).not.toBe("refresh boom") | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect ProviderJobs.finish() to confirm the expected state when refresh fails after a successful command.
ast-grep run --pattern 'finish($$$) { $$$ }' --lang typescript desktop/src/main/provider-jobs.tsRepository: devsy-org/devsy
Length of output: 464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate files"
git ls-files | rg '(^|/)provider-jobs\.(ts|tsx)$|(^|/)ipc-provider-jobs\.test\.ts$'
echo "## provider-jobs outline"
ast-grep outline desktop/src/main/provider-jobs.ts --view expanded || true
echo "## finish references"
rg -n "finish|setRefresh|getRefresh|error" desktop/src/main/provider-jobs.ts desktop/src/main/__tests__/ipc-provider-jobs.test.ts
echo "## relevant provider-jobs sections"
wc -l desktop/src/main/provider-jobs.ts
sed -n '1,260p' desktop/src/main/provider-jobs.ts
echo "## relevant test sections"
wc -l desktop/src/main/__tests__/ipc-provider-jobs.test.ts
sed -n '130,220p' desktop/src/main/__tests__/ipc-provider-jobs.test.tsRepository: devsy-org/devsy
Length of output: 9974
Assert the expected success-state rather than excluding one message.
When finish() succeeds but the refresh callback rejects, the current behavior retains no job entry. expect(providerJobs.get("docker")?.error).not.toBe("refresh boom") still passes if the job is deleted or if the refresh error is wrapped/reformatted, so assert the positive expected state instead.
Example stronger assertion
- expect(providerJobs.get("docker")?.error).not.toBe("refresh boom")
+ expect(providerJobs.get("docker")).toBeUndefined()📝 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("does not blame a successful init for a refresh failure afterward", async () => { | |
| // `provider init` itself succeeds; only the post-success refresh fails. | |
| const { providerJobs } = setup(() => ({ | |
| lines: [statusLine("running_init"), statusLine("ready")], | |
| code: 0, | |
| })) | |
| providerJobs.setRefresh(() => Promise.reject(new Error("refresh boom"))) | |
| await invoke("provider_init", { name: "docker" }) | |
| // The bug this guards: finish()'s success path rejecting (via a failed | |
| // refresh) must not get caught and re-reported as if the init command | |
| // itself had failed with the refresh's error message. | |
| expect(providerJobs.get("docker")?.error).not.toBe("refresh boom") | |
| }) | |
| it("does not blame a successful init for a refresh failure afterward", async () => { | |
| // `provider init` itself succeeds; only the post-success refresh fails. | |
| const { providerJobs } = setup(() => ({ | |
| lines: [statusLine("running_init"), statusLine("ready")], | |
| code: 0, | |
| })) | |
| providerJobs.setRefresh(() => Promise.reject(new Error("refresh boom"))) | |
| await invoke("provider_init", { name: "docker" }) | |
| // The bug this guards: finish()'s success path rejecting (via a failed | |
| // refresh) must not get caught and re-reported as if the init command | |
| // itself had failed with the refresh's error message. | |
| expect(providerJobs.get("docker")).toBeUndefined() | |
| }) |
🤖 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 `@desktop/src/main/__tests__/ipc-provider-jobs.test.ts` around lines 180 - 195,
Strengthen the test around the provider_init flow so it positively asserts the
successful job state after the post-success refresh rejects. Update the
assertion in “does not blame a successful init for a refresh failure afterward”
to require that the docker job remains present and reflects successful
initialization, rather than only checking that its error differs from “refresh
boom”.
Comment on lines
+1134
to
+1154
| // The card shows "Deleting" until finish() below, whether this | ||
| // succeeds or fails — a failure still leaves the card able to explain | ||
| // why instead of reverting to idle with no context. | ||
| const jobGeneration = workspaceJobs.start(args.workspaceId) | ||
|
|
||
| cli.runStreaming( | ||
| cliArgs, | ||
| (line) => { | ||
| if (!sink.line(formatLogLine(line))) return logStore.onDrain(logPath) | ||
| }, | ||
| (code) => { | ||
| (code, cliError) => { | ||
| void sink.done( | ||
| formatLogLine(`Exit code: ${code}`, code === 0 ? "INFO" : "ERROR"), | ||
| { success: code === 0 }, | ||
| ) | ||
| void workspaceJobs.finish( | ||
| args.workspaceId, | ||
| jobGeneration, | ||
| code === 0 | ||
| ? undefined | ||
| : cliError?.message ?? `delete exited with code ${code}`, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect cli.ts's runStreaming spawn-error handling to confirm onExit is always invoked.
ast-grep outline desktop/src/main/cli.ts --items all
rg -n -B5 -A40 'runStreaming' desktop/src/main/cli.ts
rg -n -B3 -A20 'spawn.*error|ENOENT|reject' desktop/src/main/__tests__/cli.test.tsRepository: devsy-org/devsy
Length of output: 7955
Forward cliError to the delete log sink on failure.
cliError is available when onExit reports a non-zero exit, but sink.done() drops it and only passes { success: code === 0 }. Use the same structured failure payload as workspace_rebuild and workspace_reset, for example { level: "error", success: false, cliError }, so failed deletes preserve the error in the log pane styling and value.
🧰 Tools
🪛 ast-grep (0.45.0)
[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 { execFile } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@desktop/src/main/ipc.ts` around lines 1134 - 1154, The delete operation’s
`sink.done()` call drops the available `cliError` on non-zero exit. Update the
`cli.runStreaming` exit callback to pass the structured failure payload used by
`workspace_rebuild` and `workspace_reset`—including error level, `success:
false`, and `cliError`—while preserving the existing success payload.
skevetter
marked this pull request as draft
August 1, 2026 07:07
waitForExitCallback() slept a fixed 10ms hoping the mock's setTimeout(fn, 0) had already fired. Fake timers make the wait deterministic instead.
skevetter
marked this pull request as ready for review
August 1, 2026 08:00
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
1 participant
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.
Note
Stacked on #798. Base is
feat/async-workspace-up, so this diff shows only the provider work. Merge #798 first; the base then retargets tomainautomatically.Adding a provider showed a red "not initialized" badge for several seconds before flipping to "initialized".
provider add --use=falsepersists the provider toconfig.yamlbefore init runs (cmd/provider/add.goreturns early, skipping the only code that setsInitialized = true), and the watcher faithfully reported that.parseProviderEntriescollapsed "not yet known" and "genuinely not initialized" into the samefalse, so the card rendered the destructive badge for both.The existing mitigation was a renderer-local pending set that only covered init, and was marked after
providerAddhad already returned — missing the entire multi-second install.Approach
Two lifecycle axes were collapsed into one boolean. They're now separate:
Initialized boolon disk — "did this binary runExec.Init". No schema change, no migration.ProviderJobsregistry, fed by the CLI's structured status events and broadcast on the existingproviders-changedchannel. Main-process ownership means it survives the wizard closing, navigation, and window reload — exactly when a long install is still running.This also distinguishes states the flag cannot: installing vs initializing vs updating, and failed vs never-initialized.
Also included
provider add/init/set-sourceemit structured progress (pkg/statusPhase/Event/Reporter, asworkspace updoes), replacing the desktop's"Exit code: 0"string-sniffing.pkg/devcontainer/status→pkg/status, since it's no longer devcontainer-specific.set-sourceleftInitializedstale, so a provider updated to a new binary still reportedinitialized: truethough the new binary's init never ran — a false positive admitting an uninitialized provider throughloadInitializedProvider. Update and version-pin now chainprovider init.runStreaminghang: it registeredclosebut noterror, so a spawn failure never invokedonExit— callers hung forever and the concurrency slot leaked. Affected every streaming command, not just providers.task cli:build:grpcpinned generator versions that didn't match the committed.pb.gofiles and didn't put the plugin dir onPATH, so it failed or produced a 162-line phantom diff.Verification
Driven against the running Electron app, not just tests — that's what caught two bugs the unit tests missed (a job cleared before the provider list refreshed, and a premature
readyphase).golangci-lintcleanA CodeRabbit review of this branch surfaced 5 findings (3 major); all are addressed in
c6c1cbb.Summary by CodeRabbit
New Features
Bug Fixes