Conversation
|
Hi @riturajFi, thanks for the PR! The overall approach looks reasonable. I have a few comments before we can move forward: Issues to address:
Looking forward to the updated PR! |
0726a75 to
f79adc2
Compare
|
Hi @GenerQAQ, thanks for the detailed review. Addressed all items:
Please take another look when you get a chance. |
f79adc2 to
417358f
Compare
|
@GenerQAQ kindly review the changes |
There was a problem hiding this comment.
Thanks for the PR! The overall approach — non-blocking, best-effort session title generation with a one-time gate — is solid. There are several items to address before this can be merged.
Must Fix
1. Rebase on latest dev
The branch is 1 commit behind dev — missing c953a28 feat(session): add copy session functionality with SDK support (#280). Since that commit also touches session-related code and SDKs, please rebase to ensure compatibility.
2. Too many plan files
This PR includes 22 plan markdown files (one per milestone/sub-task). These are intermediate development artifacts and shouldn't be committed to the repository — they add significant noise to plans/. Please remove all milestone-level plans and keep only one summary plan (e.g., ai-session-display-title-first-message.md).
3. Replace runtime schema patch with proper migration
schema_migrations.py applies raw SQL (ALTER TABLE ... ADD COLUMN IF NOT EXISTS) at application startup. This bypasses the project's migration management — it can't be tracked, versioned, or rolled back. Please use the standard database migration approach instead. See inline comment.
4. Add CORE unit tests
The pure helper functions (normalize_title_input_text, check_title_input_quality, extract_first_user_message_text, sanitize_generated_title) have zero test coverage. These are ideal candidates for unit testing — they are pure functions with well-defined inputs and outputs. Please add tests for edge cases (empty input, Unicode, CJK text, very long input, boundary lengths, etc.).
5. Incomplete variable renaming (session → db_session)
The PR description says all DB context variables were renamed from session to db_session to avoid shadowing, but the learning_space query block still uses session. See inline comment at line 258.
Suggestions
See inline comments for:
TITLE_GENERATION_MAX_TOKENSmay be too small for non-English languagesTITLE_INPUT_MIN_CHARSmay be too strict- Redundant
normalize_title_input_textcall incheck_title_input_quality
| ) | ||
|
|
||
|
|
||
| async def apply_runtime_schema_patches(db_session: AsyncSession) -> list[str]: |
There was a problem hiding this comment.
This runtime schema-patch approach (raw SQL at startup) bypasses the project's migration management. It can't be tracked, versioned, or rolled back, and may conflict with future migrations that touch the same table.
Please use a proper database migration (e.g., Alembic) to add the display_title column. This file and the _apply_schema_migrations call in db.py should be removed in favor of a migration script.
|
|
||
| TITLE_INPUT_MAX_CHARS = 512 | ||
| TITLE_INPUT_MIN_CHARS = 12 | ||
| TITLE_GENERATION_MAX_TOKENS = 24 |
There was a problem hiding this comment.
24 max tokens works for English titles (3-8 words), but CJK and other non-English languages often use 2-3 tokens per character. A Chinese title of 8 characters could easily require 16-24 tokens, leaving no margin.
Consider increasing to 48-64 to support multilingual title generation.
| from ..data import session as SD | ||
|
|
||
| TITLE_INPUT_MAX_CHARS = 512 | ||
| TITLE_INPUT_MIN_CHARS = 12 |
There was a problem hiding this comment.
12 characters may be too strict as a minimum. Reasonable first messages like "Fix this bug" (12 chars) are borderline, and shorter but meaningful inputs like "Debug my app" (12 chars) barely pass.
Consider lowering to 8-10 characters.
| def check_title_input_quality(text: str | None) -> tuple[bool, str]: | ||
| if text is None: | ||
| return False, "empty" | ||
| normalized = normalize_title_input_text(text) |
There was a problem hiding this comment.
Redundant normalization: extract_first_user_message_text already calls normalize_title_input_text on the extracted text before returning it (line 75). Calling normalize_title_input_text again here is a no-op on already-normalized input.
Consider removing this call and operating directly on text, or document why the double normalization is intentional.
| ) | ||
|
|
||
| ls_session = None | ||
| async with DB_CLIENT.get_session_context() as session: |
There was a problem hiding this comment.
This session variable was not renamed to db_session, inconsistent with the rest of the function. The PR description says all DB context variables were renamed to avoid shadowing.
| async with DB_CLIENT.get_session_context() as session: | |
| async with DB_CLIENT.get_session_context() as db_session: |
Why we need this PR?
This PR introduces automatic session title generation from the first meaningful user message so session lists are easier to scan and manage.
Today, sessions can remain untitled/opaque in UI/API consumers, especially when many sessions exist. This change adds a nullable
display_titlefield and populates it once, non-blockingly, during pending-message processing.Describe your solution
What changed
display_titletoSessionmodels in both services (API + CORE ORM sync):src/server/api/go/internal/modules/model/session.gosrc/server/core/acontext_core/schema/orm/session.pyupdate_session_display_title(...): persists generated titleshould_generate_session_display_title(...): one-time gate (skip if already set)session_recordfor claritysrc/server/core/acontext_core/service/data/session.pymin length, skip low-signal inputs likehi,test, etc.)llm_complete(...)with dedicated prompt id:session.display_title.first_usertask_agent_curd(description aligned with code)db_sessionto avoid shadowingsrc/server/core/acontext_core/service/controller/message.pysrc/server/core/acontext_core/infra/schema_migrations.pydb.pyto call patch application aftercreate_all:src/server/core/acontext_core/infra/db.pyALTER TABLE sessions ADD COLUMN IF NOT EXISTS display_title TEXT;Sessiontype includes nullabledisplay_title:src/client/acontext-py/src/acontext/types/session.pySessionSchemaincludes nullabledisplay_title:src/client/acontext-ts/src/types/session.tssrc/client/acontext-py/tests/test_client.pysrc/client/acontext-py/tests/test_async_client.pysrc/client/acontext-ts/tests/mocks.tssrc/client/acontext-ts/tests/client.test.tsBehavioral guarantees
Non-functional notes
.vscode/settings.json(removed)src/server/.env.local-apiis not part of this PR change set.Implementation Tasks
Please ensure your pull request meets the following requirements:
display_titleto API session model (src/server/api/go/internal/modules/model/session.go)display_titleto CORE ORM session model (src/server/core/acontext_core/schema/orm/session.py)src/server/core/acontext_core/service/data/session.py)src/server/core/acontext_core/service/controller/message.py)src/server/core/acontext_core/infra/schema_migrations.py)display_title(Python + TypeScript).vscode/settings.jsonfrom PR scopeImpact Areas
Which part of Acontext would this feature affect?
Checklist
devbranch.