fix(web): preserve manual session titles - #2102
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd6035a352
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from kimi_cli.session_state import load_session_state, save_session_state | ||
|
|
||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) |
There was a problem hiding this comment.
Switching this endpoint to get_session_or_404 allows renames/archive toggles while the worker is active, but the handler still does a full read-modify-write of state.json (load_session_state then save_session_state) without any lock or CAS. If the running worker writes session state in between (e.g., todo/plan/approval updates via Session.save_state), this PATCH can write an older snapshot back and silently roll back those non-title fields. This race was prevented before by the busy check, so allowing busy edits here introduces state-loss scenarios.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adjusts web session edit behavior so “state-only” mutations (rename/archive/generate-title) are permitted while a session worker is busy, and prevents late AI title generation from overwriting a concurrently finalized manual title.
Changes:
- Backend: split session existence lookup from “busy” validation; allow
PATCH /api/sessions/{id}andPOST /api/sessions/{id}/generate-titlewhile busy; preserve busy guards for delete/fork/upload. - Backend: re-check fresh session state before writing generated/fallback titles to avoid clobbering a manual finalized title.
- Frontend/tests: show failure toasts for session metadata actions; add regression test for the late-title overwrite race.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
web/src/hooks/useSessions.ts |
Adds error toasts for session mutations (delete/rename/archive/unarchive/generate-title). |
tests/web/test_sessions_api.py |
Adds regression coverage ensuring late generate-title does not overwrite a concurrently finalized manual title. |
src/kimi_cli/web/api/sessions.py |
Introduces get_session_or_404, relaxes busy checks for state-only endpoints, and adds fresh-state checks before persisting AI/fallback titles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @router.post("/{session_id}/generate-title", summary="Generate session title using AI") | ||
| async def generate_session_title( | ||
| session_id: UUID, | ||
| request: GenerateTitleRequest | None = None, | ||
| runner: KimiCLIRunner = Depends(get_runner), | ||
| ) -> GenerateTitleResponse: | ||
| """Generate a concise session title using AI based on the first conversation turn. | ||
|
|
||
| If request body is empty or parameters are missing, the backend will | ||
| automatically read the first turn from wire.jsonl. | ||
|
|
||
| Safe to invoke while a worker is running: the final write reloads | ||
| state from disk to merge concurrent worker changes. | ||
| """ | ||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) | ||
| session_dir = session.kimi_cli_session.dir |
There was a problem hiding this comment.
generate_session_title is now allowed while a session is busy, but the new test only covers the late-write race and doesn’t cover the core behavior change (busy session should still accept generate-title). Consider adding a test that sets up a busy SessionProcess and asserts this endpoint returns 200 instead of the previous "Session is busy" error.
| const message = | ||
| err instanceof Error ? err.message : "Failed to delete session"; | ||
| setError(message); | ||
| toast.error(message); |
There was a problem hiding this comment.
deleteSession now calls toast.error(message) after setting the global error state. App.tsx already shows a toast whenever sessionsError is set, so a delete failure will produce duplicate toasts. Consider removing the direct toast here (or stop setting the global error for this path) so failures are surfaced exactly once.
| toast.error(message); |
| from kimi_cli.session_state import load_session_state, save_session_state | ||
|
|
||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) | ||
| session_dir = session.kimi_cli_session.dir | ||
| state = load_session_state(session_dir) |
There was a problem hiding this comment.
This change removes the busy guard for PATCH /api/sessions/{session_id} by switching to get_session_or_404, but there’s no test asserting that updates succeed while a session is busy (and that truly conflicting ops remain blocked). Adding a regression test that simulates a busy SessionProcess and verifies rename/archive still work would help prevent accidental reintroduction of the busy check.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4a7afecf8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fresh = load_session_state(session_dir) | ||
| # Another request or manual rename may have finalized the title while the | ||
| # LLM call was in flight. Preserve that newer title instead of clobbering it. |
There was a problem hiding this comment.
This endpoint is now callable while a worker is busy, but it still does an unlocked read-modify-write (load_session_state then save_session_state). In generate_session_title, if the worker persists session changes (e.g., todos/approval/plan fields via Session.save_state) after fresh is loaded but before this save executes, this handler can write an older snapshot and silently roll back those worker updates. Because the busy check was removed for this route in this commit, this introduces a real state-loss race under concurrent activity.
Useful? React with 👍 / 👎.
# Conflicts: # CHANGELOG.md # docs/en/release-notes/changelog.md # docs/zh/release-notes/changelog.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50cf65fb16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from kimi_cli.session_state import load_session_state, save_session_state | ||
|
|
||
| session = get_editable_session(session_id, runner) | ||
| session = get_session_or_404(session_id) |
There was a problem hiding this comment.
Allowing PATCH /api/sessions/{session_id} to bypass get_editable_session reintroduces lost-update races because worker-side state writes are not uniformly routed through Session.save_state. Fresh evidence: KimiSoul’s first-turn auto-title path performs its own unlocked load_session_state/save_session_state cycle (src/kimi_cli/soul/kimisoul.py, lines 638–643). If rename/archive PATCH runs while that turn is finishing, whichever write lands last can silently revert the other change (for example, an archive flag flipping back), so this endpoint is not safe while busy without locking/CAS.
Useful? React with 👍 / 👎.
wbxl2000
changed the title
# Conflicts: # CHANGELOG.md # docs/en/release-notes/changelog.md # docs/zh/release-notes/changelog.md
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Pressing Enter in the rename input fires handleSaveEdit while the PATCH is in flight; if the user then clicks the resulting toast or otherwise shifts focus, onBlur fires the same handler again, sending a second PATCH and producing a duplicate failure toast. Track an in-flight ref and short-circuit re-entrant calls.
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Related Issue
Refs #2101
Description
This PR now keeps the existing busy-session protection in place and narrows the fix to two concrete web session issues:
generate-titleis in flightThe earlier attempt to allow rename / archive / generate-title while a session is busy has been reverted. Review found that the current state file read-modify-write paths can lose concurrent worker updates without a lock, CAS, or field-level merge helper. Safe busy-session metadata edits are tracked separately in #2101.
What changed
title_generatedwas set while the LLM call or fallback preparation was in progress.get_editable_sessionbusy checks for session rename / archive / generate-title.Tests
uv run pytest tests/web/test_sessions_api.pyuv run pyright src/kimi_cli/web/api/sessions.py tests/web/test_sessions_api.pynpm run typecheckinwebgit diff --checkChecklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.