fix: dispose failed planning startup agents

[plarson]

Summary

• dispose the planning agent when non-streaming planning startup exhausts JSON parsing before the first question
• keep failed startup cleanup best-effort so malformed/empty AI responses return an API error without leaking model/transport handles in the long-running dashboard
• add a regression covering unparsable first responses, no process.exit, and agent disposal

Verification

• COREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm --filter @fusion/dashboard exec vitest run src/tests/routes-planning.test.ts --reporter=dot
• COREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm --filter @fusion/dashboard typecheck
• COREPACK_ENABLE_DOWNLOAD_PROMPT=0 corepack pnpm lint

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling and resource cleanup when AI responses are invalid; sessions and associated agent resources are now disposed and cleared to prevent leaks and avoid leaving stale handles.

• Tests

  • Added a regression test ensuring the system returns an error for invalid AI responses, prevents shutdown, and confirms agent disposal.
  • Updated a release workflow test to accept alternate Linux AppImage filename variants.

[coderabbitai]

📝 Walkthrough

Walkthrough

When the first AI response cannot be parsed, the session is deleted/unpersisted, the agent session's optional dispose is awaited and errors are warned, and the agent reference is cleared; tests add a regression ensuring the planning route returns HTTP 500 without calling process.exit. A separate test widens a Linux AppImage filename expectation.

Changes

AI Agent Cleanup on Unparsable Response

Layer / File(s)	Summary
Agent disposal on parse failure packages/dashboard/src/planning.ts	When getFirstQuestionFromAgent fails to parse the initial AI response, it now awaits and calls session.agent.session.dispose?.(), logs disposal errors as diagnostics warnings, clears session.agent, and still deletes/unpersists the session.
Regression test for unparsable response packages/dashboard/src/__tests__/routes-planning.test.ts	New test mocks an agent whose first assistant payload is empty/unparsable, asserts /api/planning/start returns HTTP 500 with "Failed to get first question from AI", verifies process.exit is not invoked, and confirms the mocked agent's dispose is called once.

Release workflow artifact test

Layer / File(s)	Summary
Linux AppImage filename variants in test packages/desktop/src/__tests__/release-workflow.test.ts	Update the Vitest expectation to accept both x64 and x86_64 variants in the Linux AppImage artifact filename check.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PlanningRoute
  participant getFirstQuestionFromAgent
  participant SessionStore
  participant AgentSession
  Client->>PlanningRoute: POST /planning/start
  PlanningRoute->>getFirstQuestionFromAgent: request first question
  getFirstQuestionFromAgent->>AgentSession: receive unparsable response
  getFirstQuestionFromAgent->>SessionStore: delete session & unpersist
  getFirstQuestionFromAgent->>AgentSession: await dispose?.()
  getFirstQuestionFromAgent->>PlanningRoute: return error (HTTP 500)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 When the AI's first whisper comes back unclear,
I tidy the handles so nothing sticks near.
A bumped-off session, a gentle dispose—
Safe routes return errors and nobody dozes. 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main purpose of the PR—disposing planning agents when startup fails—matching the core changes in planning.ts and the regression test.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR fixes a resource leak in the non-streaming planning startup path by disposing the AI agent when JSON parsing exhausts all retries before a first question is obtained. Without the fix, a malformed or empty AI response left the underlying model/transport handles alive in the long-running dashboard process.

• planning.ts: Inside getFirstQuestionFromAgent, the failure branch now calls await session.agent.session.dispose?.() in a best-effort try/catch before throwing, and nulls out session.agent to release the reference. Matches the disposal pattern already used in cleanupInMemorySession.
• routes-planning.test.ts: Adds a regression test that drives an empty AI response through the startup path, asserts a 500 error is returned, confirms process.exit is not called, and verifies dispose is invoked exactly once.
• release-workflow.test.ts: Loosens an AppImage filename assertion to accept x64 or x86_64 suffixes, accommodating renamed artifact output.

Confidence Score: 5/5

Safe to merge — the change is a narrow, well-tested bug fix that prevents agent handle leaks on a specific failure path without altering the happy path.

The disposal logic mirrors the already-proven cleanupInMemorySession pattern, is wrapped in a try/catch so a dispose failure cannot mask the original error, and is covered by a direct regression test that checks all three observable effects (error response, no process.exit, dispose called once). The unrelated test change in the desktop package is a minor regex loosening with no production impact.

No files require special attention.

Important Files Changed

Filename	Overview
packages/dashboard/src/planning.ts	Adds best-effort agent disposal in the !parsed failure branch of getFirstQuestionFromAgent; pattern mirrors the existing cleanupInMemorySession helper and is guarded by a try/catch with diagnostic logging.
packages/dashboard/src/tests/routes-planning.test.ts	New regression test covering unparsable first AI responses; verifies status 500, no process.exit, and exactly one dispose call.
packages/desktop/src/tests/release-workflow.test.ts	Relaxes AppImage filename assertion to accept both x64 and x86_64 artifact name variants.

Sequence Diagram

sequenceDiagram
    participant Route as POST /api/planning/start
    participant createSession as createSession()
    participant getFirstQ as getFirstQuestionFromAgent()
    participant Agent as AI Agent

    Route->>createSession: call with initialPlan
    createSession->>Agent: createFnAgent(...)
    Agent-->>createSession: agentResult (session.agent set)
    createSession->>getFirstQ: await getFirstQuestionFromAgent(session, initialPlan)
    getFirstQ->>Agent: session.prompt(message)
    Agent-->>getFirstQ: empty / unparsable response

    loop up to MAX_PARSE_RETRIES
        getFirstQ->>Agent: prompt("respond with valid JSON")
        Agent-->>getFirstQ: still unparsable
    end

    note over getFirstQ: parsed === undefined
    getFirstQ->>getFirstQ: sessions.delete(session.id)
    getFirstQ->>getFirstQ: unpersistSession(session.id)
    getFirstQ->>Agent: session.dispose?.() [best-effort]
    getFirstQ->>getFirstQ: "session.agent = undefined"
    getFirstQ-->>createSession: throw Error("Failed to get first question from AI")
    createSession-->>Route: 500 API error (no process.exit, no handle leak)

Loading

_{Reviews (3): Last reviewed commit: "fix: dispose failed planning startup age..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

packages/desktop/src/__tests__/release-workflow.test.ts (1)

38-38: ⚡ Quick win

Consider asserting the specific suffix that electron-builder produces.

The workflow accepts both x64 and x86_64 suffixes, but according to the actual workflow file, electron-builder consistently produces x86_64 for AppImage artifacts. Additionally, this creates an inconsistency with Line 37, which uses toContain for the arm64 variant.

Suggested refinement for precision and consistency

      expect(workflow).toContain("Fusion-*-linux-arm64.AppImage");
-      expect(workflow).toMatch(/Fusion-\*-linux-(x64|x86_64)\.AppImage/);
+      expect(workflow).toContain("Fusion-*-linux-x86_64.AppImage");
      expect(workflow).toContain("name: fusion-desktop-linux");

If there's a specific reason to accept both variants (e.g., the two workflow files use different suffixes), consider adding a comment explaining why the alternation is necessary.

As per coding guidelines, prefer narrow seams and targeted assertions in tests. As per relevant code snippet .github/workflows/release.yml:299-321, the workflow comment states "electron-builder names the x64 AppImage with the x86_64 arch suffix" and uses the glob pattern Fusion-*-linux-x86_64.AppImage.

🤖 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 `@packages/desktop/src/__tests__/release-workflow.test.ts` at line 38, The test
assertion allowlisting both `x64` and `x86_64` is too permissive; update the
regex in the assertion that references
`expect(workflow).toMatch(/Fusion-\*-linux-(x64|x86_64)\.AppImage/);` to match
the exact suffix electron-builder produces (use `x86_64` only) so it becomes
consistent with the workflow and the arm64 assertion on the previous line; if
there is a deliberate reason to accept both suffixes, add a brief inline comment
explaining that rationale instead of widening the regex.

🤖 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.

Nitpick comments:
In `@packages/desktop/src/__tests__/release-workflow.test.ts`:
- Line 38: The test assertion allowlisting both `x64` and `x86_64` is too
permissive; update the regex in the assertion that references
`expect(workflow).toMatch(/Fusion-\*-linux-(x64|x86_64)\.AppImage/);` to match
the exact suffix electron-builder produces (use `x86_64` only) so it becomes
consistent with the workflow and the arm64 assertion on the previous line; if
there is a deliberate reason to accept both suffixes, add a brief inline comment
explaining that rationale instead of widening the regex.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 33f74a24-294c-48c3-aece-54ce83ebe344

📥 Commits

Reviewing files that changed from the base of the PR and between ad45f7b and 5f3e344.

📒 Files selected for processing (3)

• packages/dashboard/src/__tests__/routes-planning.test.ts
• packages/dashboard/src/planning.ts
• packages/desktop/src/__tests__/release-workflow.test.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• packages/dashboard/src/tests/routes-planning.test.ts
• packages/dashboard/src/planning.ts