Repository navigation
refactor(chat): chat_sessions is the one durable home of a conversation turn; transcripts are read-only legacy, found where they really are (#18293) - #18309
Conversation
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Deletion can leave conversation data available again when a reply finishes concurrently, and legacy transcript handling still has conditional file-safety and malformed-data failures. Resolve or explicitly accept these risks before merging. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check |  |
Most changes support #18293 or #18316. However, the PR also registers unrelated deployment-mode and egress guards and re-pins unrelated reach floors such as kb-admin-read-bypass, `audio-extension-al… |
Remove the unrelated guard registrations and reach-floor changes from this PR, or move them to a separate maintenance PR. Keep only guard and floor updates required to validate #18293 and #18316. |
| Docstring Coverage | |
Docstring coverage is 42.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 187 functions across 32 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarises the main change: chat_sessionsbecomes the durable store, while transcripts become read-only legacy data. It is specific and related to the pull request.Linked Issues check The PR meets the coding requirements for #18293and#18316. For#18293, conversation turns are persisted inchat_sessions, context is projected from that store, transcript writes and request-path tr…
Full details: Out of Scope Changes check
-
Explanation
Most changes support
#18293or#18316. However, the PR also registers unrelated deployment-mode and egress guards and re-pins unrelated reach floors such askb-admin-read-bypass,audio-extension-allowlist, andredaction-concept-census. These changes do not implement chat storage or chat deletion requirements. The PR description also states that no open issue covered at least part of this work.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Merger review at a9c7bf3: BLOCKED. One HIGH. Fixing the resolver turns a dormant ordering bug into a live cross-user delete.
#18293 ACs: AC5 ("retiring B never deletes data directly") is not met, and AC6 is partial, both because of the HIGH. Verified and fine: id path-safety (no |
…r does not durably own; admin route 403s; location from SSOT config (#18293)
|
Merger delta review, a9c7bf3 → 0795940: still BLOCKED. Fixed:
The ownership premise is still bypassable:
#18293: AC5 and AC6 are not met at this head (H1 and H2 delete without a proposal). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
2e826d5b-a6b1-4627-a78f-7658b6660508
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (27)
autobot-backend/api/chat_sessions.pyautobot-backend/api/chat_sessions_delete_cleanup.pyautobot-backend/api/data_storage.pyautobot-backend/api/data_storage_categories.pyautobot-backend/api/legacy_transcript_disposal.pyautobot-backend/api/legacy_transcript_location_18293_test.pyautobot-backend/api/session_delete_legacy_transcript_18293_test.pyautobot-backend/chat_history/legacy_transcripts.pyautobot-backend/chat_history/persist_conversation_dedup_test.pyautobot-backend/chat_history/session.pyautobot-backend/chat_history/session_reply_backfill.pyautobot-backend/chat_history/session_reply_backfill_test.pyautobot-backend/chat_history/turn_projection.pyautobot-backend/chat_history/turn_projection_test.pyautobot-backend/chat_workflow/conversation.pyautobot-backend/chat_workflow/conversation_turn_store_18293_test.pyautobot-backend/chat_workflow/manager.pyautobot-backend/services/legacy_transcript_disposal_action.pyautobot-infrastructure/shared/scripts/utilities/init_memory_graph_redis.pyautobot_shared/ssot_config.pyautobot_shared/store_authority.pydocs/architecture/AUTOBOT_MEMORY_GRAPH_ARCHITECTURE.mddocs/database/REDIS_MEMORY_GRAPH_SPECIFICATION.mddocs/guides/chat-ollama-configuration.mddocs/security/OWNERSHIP_BACKFILL_ANALYSIS.mdrepo_tests/conversation_turn_single_store_18293_test.pyrepo_tests/reach_scope_claim_17844_test.py
💤 Files with no reviewable changes (2)
- autobot-backend/chat_history/persist_conversation_dedup_test.py
- autobot-backend/chat_workflow/manager.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| real = os.path.realpath(str(validate_relative_path(f"{session_id}.json", root))) | ||
| if not real.startswith(os.path.realpath(str(root)) + os.sep): | ||
| raise ValueError("Invalid session ID") | ||
| if await asyncio.to_thread(os.path.exists, real): | ||
| await asyncio.to_thread(os.remove, real) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Reject symlinked transcript entries for both operations. The read and removal paths follow symlinks rather than operating only on the requested transcript entry. A symlink within the directory can make removal delete another session’s file; a symlink outside it can make the reader use content outside the legacy store.
autobot-backend/chat_history/legacy_transcripts.py#L80-L84: reject a symlinked session entry and do not pass its resolved target toos.remove.autobot-backend/chat_history/legacy_transcripts.py#L109-L113: enforce containment and open the session entry without following a final-component symlink.
Based on learnings, symlink-sensitive file operations must reject symlink entries and use no-follow opening where applicable.
🧰 Tools
🪛 Ruff (0.16.8)
[warning] 80-80: Async functions should not perform blocking os.path operations
(ASYNC240)
[warning] 81-81: Async functions should not perform blocking os.path operations
(ASYNC240)
[warning] 82-82: Abstract raise to an inner function
(TRY301)
[warning] 82-82: Avoid specifying long messages outside the exception class
(TRY003)
📍 Affects 1 file
autobot-backend/chat_history/legacy_transcripts.py#L80-L84(this comment)autobot-backend/chat_history/legacy_transcripts.py#L109-L113
Source: Learnings
| try: | ||
| messages = await ChatHistoryManager().load_session(session_id) | ||
| except (PermissionError, ValueError, OSError) as exc: | ||
| logger.error("Cannot read chat session %s for LLM context: %s", session_id, exc) | ||
| return None | ||
| return project_turn_pairs(messages, LLM_CONTEXT_TURN_LIMIT) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A missing session file disables the legacy fallback.
load_session documents FileNotFoundError as a propagated exception (session.py Line 300). FileNotFoundError is a subclass of OSError. The except on Line 76 therefore returns None for it. _load_conversation_history then returns [] and never reads the legacy transcript.
The trigger is a session whose file is removed between _resolve_session_file_path and aiofiles.open. A B-only legacy session, which has no chat_sessions file at all, normally takes the chat_data is None → [] path. That path still works. The race narrows the impact, but this behaviour contradicts the "None = unreadable" contract in the docstring. A missing file is the case where the legacy fallback must apply.
Proposed fix
--- "a/autobot-backend/chat_workflow/conversation.py"
+++ "b/autobot-backend/chat_workflow/conversation.py"
@@ -71,9 +71,11 @@
from chat_history import ChatHistoryManager
from chat_history.turn_projection import LLM_CONTEXT_TURN_LIMIT, project_turn_pairs
try:
messages = await ChatHistoryManager().load_session(session_id)
+ except FileNotFoundError:
+ messages = []
except (PermissionError, ValueError, OSError) as exc:
logger.error("Cannot read chat session %s for LLM context: %s", session_id, exc)
return None
return project_turn_pairs(messages, LLM_CONTEXT_TURN_LIMIT)📝 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.
| try: | |
| messages = await ChatHistoryManager().load_session(session_id) | |
| except (PermissionError, ValueError, OSError) as exc: | |
| logger.error("Cannot read chat session %s for LLM context: %s", session_id, exc) | |
| return None | |
| return project_turn_pairs(messages, LLM_CONTEXT_TURN_LIMIT) | |
| try: | |
| messages = await ChatHistoryManager().load_session(session_id) | |
| except FileNotFoundError: | |
| messages = [] | |
| except (PermissionError, ValueError, OSError) as exc: | |
| logger.error("Cannot read chat session %s for LLM context: %s", session_id, exc) | |
| return None | |
| return project_turn_pairs(messages, LLM_CONTEXT_TURN_LIMIT) |
🧰 Tools
🪛 Ruff (0.16.8)
[warning] 77-77: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
| result = await remove_legacy_transcript(session_id) | ||
| outcome = "removed" if result["transcript_deleted"] else "did NOT remove" | ||
| body = f"Legacy transcript cleanup {outcome} the transcript of session {session_id}" | ||
| if result["error"]: | ||
| body += " (refused or failed; the backend log has the reason)" | ||
| session.add(ApprovalComment(approval_id=approval.id, author="system", author_type="system", body=body)) | ||
| await session.commit() | ||
| await audit_log( |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect the audit contract and the approval dispatcher's failure path.
rg -n -A35 -B5 'async def audit_log\b|def audit_log\b' autobot-backend/services
rg -n -C8 'register_post_approval_action|post_approval_action|execute_legacy_transcript_delete' autobot-backend/servicesRepository: mrveiss/AutoBot-AI
Length of output: 39155
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for name, start, end in [
("autobot-backend/services/legacy_transcript_disposal_action.py", 41, 64),
("autobot-backend/services/approval_execution.py", 99, 170),
]:
path = Path(name)
lines = path.read_text().splitlines()
print(f"--- {name}:{start}-{end} ---")
for number in range(start, min(end, len(lines)) + 1):
print(f"{number}: {lines[number - 1]}")
PYRepository: mrveiss/AutoBot-AI
Length of output: 4684
Guarantee the legacy_transcript.delete audit entry after removal.
execute_legacy_transcript_delete commits an approval comment before it calls audit_log. If that call raises, the dispatcher commits a durable failure comment, but its fallback also swallows failures from its own audit_log. The deletion therefore retains an approval trail, but the promised legacy_transcript.delete audit entry is not guaranteed. Make the audit record durable when either audit call fails.
…cy-only id cannot be taken, and deploy templates pin the location (#18293)
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
594befd5-7f37-448e-8f0f-e033a4bcb5fa
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (23)
autobot-backend/api/chat_sessions.pyautobot-backend/api/chat_sessions_delete_cleanup.pyautobot-backend/api/data_storage.pyautobot-backend/api/legacy_transcript_disposal.pyautobot-backend/api/session_delete_legacy_transcript_18293_test.pyautobot-backend/chat_history/legacy_transcripts.pyautobot-backend/chat_history/persist_conversation_dedup_test.pyautobot-backend/chat_history/session.pyautobot-backend/chat_history/session_reply_backfill.pyautobot-backend/chat_history/session_reply_backfill_test.pyautobot-backend/chat_workflow/conversation.pyautobot-backend/chat_workflow/conversation_turn_store_18293_test.pyautobot-backend/chat_workflow/manager.pyautobot-backend/services/legacy_transcript_disposal_action.pyautobot-infrastructure/shared/scripts/utilities/init_memory_graph_redis.pyautobot-slm-backend/ansible/roles/backend/templates/backend.env.j2autobot_shared/store_authority.pydocker-compose.ymlrepo_tests/collected_modules_inert_on_import_test.pyrepo_tests/conversation_turn_single_store_18293_test.pyrepo_tests/kb_admin_read_bypass_guard_test.pyrepo_tests/model_revision_pinning_enforced_17804_test.pyrepo_tests/reach_scope_claim_17844_test.py
💤 Files with no reviewable changes (2)
- autobot-backend/chat_history/persist_conversation_dedup_test.py
- autobot-backend/chat_workflow/manager.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| async def _pending_proposal_id(db, session_id: str) -> str | None: | ||
| from models.approval import Approval, ApprovalStatus | ||
|
|
||
| query = select(Approval.id).where( | ||
| Approval.status == ApprovalStatus.PENDING.value, | ||
| Approval.context["action"].astext == ACTION, | ||
| Approval.context["session_id"].astext == session_id, | ||
| ) | ||
| found = (await db.execute(query)).scalars().first() | ||
| return str(found) if found else None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoff
Pending-proposal lookup and creation are not atomic.
_pending_proposal_id and create_approval run in one session without a lock. Two concurrent deletes for the same ID can both see no pending proposal. Both then create one. The result is a duplicate approval, and an approver may handle the same transcript twice. The second approval will report "did NOT remove". The impact is low, and the second decision is harmless.
Use a unique partial index, or an advisory lock keyed on session_id, if duplicates matter.
| async def _refer(session_id: str, requested_by: str | None, why: str) -> str | None: | ||
| """Propose disposal; a failure to propose leaves the file and is reported, never raised.""" | ||
| try: | ||
| return await propose_transcript_disposal(session_id, requested_by, why) | ||
| except Exception as exc: | ||
| logger.error("Could not record a #17038 proposal for session %s's transcript: %s", session_id, exc) | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Proposal failure is reported, but the delete_session_or_refer 404 hides it.
In delete_session_or_refer, a failed _refer returns None. The 404 message then carries no hint that the proposal was not recorded. The legacy-only transcript stays on disk with no review proposal and no signal to the caller. dispose_session_transcript reports this case in its error field. The 404 path does not.
Add a short suffix to the message when _refer returns None for an existing legacy transcript.
The blind except Exception here is intentional and logged. Ruff's BLE001 can be silenced with a noqa and a reason. Use logger.exception instead of logger.error so the traceback is kept.
🧰 Tools
🪛 Ruff (0.16.8)
[warning] 76-76: Do not catch blind exception: Exception
(BLE001)
[warning] 77-77: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
Source: Linters/SAST tools
| chat_mgr, | ||
| redis_client=None, | ||
| transcript_dir: str = _TRANSCRIPT_DIR, | ||
| transcript_dir: str | None = None, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial
🔎 Supported by static analysis
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- target file ---'
nl -ba autobot-backend/chat_history/session_reply_backfill.py | sed -n '1,260p'
printf '%s\n' '--- repair_session references ---'
rg -n -F --glob '*.py' -- 'repair_session(' autobot-backend || test "$?" -eq 1
printf '%s\n' '--- ownership/session identifiers ---'
rg -n -F --glob '*.py' -- 'legacy_conversation_transcripts' autobot-backend || test "$?" -eq 1
rg -n -F --glob '*.py' -- 'session_owner' autobot-backend || test "$?" -eq 1
rg -n -F --glob '*.py' -- 'owner_id' autobot-backend/chat_history autobot-backend/api autobot-backend || test "$?" -eq 1Repository: mrveiss/AutoBot-AI
Length of output: 40947
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- legacy transcript reader ---'
nl -ba autobot-backend/chat_history/legacy_transcripts.py | sed -n '1,180p'
printf '%s\n' '--- session owner implementation ---'
nl -ba autobot-backend/chat_history/session.py | sed -n '860,940p'
printf '%s\n' '--- focused backfill tests ---'
nl -ba autobot-backend/chat_history/session_reply_backfill_test.py | sed -n '220,355p'Repository: mrveiss/AutoBot-AI
Length of output: 16945
Require explicit owner confirmation before backfilling.
repair_session matches legacy pairs by user text and writes the assistant text to chat_sessions. The legacy reader does not provide owner metadata, so this path cannot compare transcript and session owners. Document that the operator must verify ownership for every named session, or require an explicit confirmation flag in _main.
| # #18293: the retired chat-transcript store (read-only legacy). It lives where its old | ||
| # writer put it -- data/conversation_transcripts under the service's WorkingDirectory | ||
| # (autobot-backend.service.j2), NOT under AUTOBOT_DATA_DIR. Pinned so it is never inferred. | ||
| AUTOBOT_LEGACY_TRANSCRIPTS_DIR={{ backend_code_dir | default(backend_install_dir) }}/data/conversation_transcripts |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Set the legacy transcript directory from the same variable as AUTOBOT_BASE_DIR.
Line 12 sets AUTOBOT_BASE_DIR={{ backend_code_dir }}. Line 25 uses backend_code_dir | default(backend_install_dir). If backend_code_dir is undefined, Line 12 already fails to render. The default on Line 25 is therefore dead code. It also suggests a fallback that does not exist.
Use {{ backend_code_dir }} on Line 25. Alternatively, derive the value from AUTOBOT_BASE_DIR. The path instructions say an override variable keeps its override and only the fallback is derived from the SSOT.
Source: Path instructions
|
Merger note: this PR rides merge train #3 (branch train-2026-10-11-3). Its two |
…18293-conversation-turn-store # Conflicts: # repo_tests/collected_modules_inert_on_import_test.py # repo_tests/reach_scope_claim_17844_test.py
…sue-18293-conversation-turn-store # Conflicts: # repo_tests/python_file_size_ratchet_baseline.py # scripts/python_file_size_known_large.py
…er the tree grew (#18293)
…ob cannot delete alice's chat, and one guard for un-awaited ChatHistoryManager coroutines in the API (#18316)
…ion-turn-store # Conflicts: # repo_tests/credential_field_name_only_17336_test.py # repo_tests/kb_admin_read_bypass_guard_test.py # repo_tests/model_revision_pinning_enforced_17804_test.py # repo_tests/python_file_size_ratchet_baseline.py # repo_tests/reach_scope_claim_17844_test.py # scripts/python_file_size_known_large.py
…ensus mid-window after the merged tree grew (#18293)
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Correct the test docstring for the B-only session. · session_reply_backfill_test.py:317-318
autobot-backend/chat_history/session_reply_backfill_test.py:317-318
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the test docstring for the B-only session.
The docstring says that a B-only session "is served read-only for LLM context". After
#18293, no request path reads legacy transcripts, so a B-only session gets empty context. The docstring therefore contradicts the module contract. Update the docstring.Proposed fix
- """A B-only session is served read-only for LLM context, never moved by this repair.""" + """A B-only session is never created or moved by this repair; it awaits an approved #17038 backfill."""
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
6cf38a05-deb8-4ef0-9a10-24cb634799f6
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (23)
autobot-backend/api/chat.pyautobot-backend/api/chat_delete_by_id_real_store_18316_test.pyautobot-backend/api/chat_delete_by_id_test.pyautobot-backend/api/chat_sessions_delete_cleanup.pyautobot-backend/api/data_storage.pyautobot-backend/api/legacy_transcript_location_18293_test.pyautobot-backend/api/session_delete_legacy_transcript_18293_test.pyautobot-backend/chat_history/legacy_transcripts.pyautobot-backend/chat_history/persist_conversation_dedup_test.pyautobot-backend/chat_history/session.pyautobot-backend/chat_history/session_reply_backfill.pyautobot-backend/chat_history/session_reply_backfill_test.pyautobot-backend/chat_workflow/conversation.pyautobot-backend/chat_workflow/manager.pyautobot-infrastructure/shared/scripts/utilities/init_memory_graph_redis.pyautobot_shared/store_authority.pyrepo_tests/_reach_policy.pyrepo_tests/audio_extension_allowlist_test.pyrepo_tests/chat_history_async_calls_awaited_18316_test.pyrepo_tests/chat_single_delete_implementation_18316_test.pyrepo_tests/kb_admin_read_bypass_guard_test.pyrepo_tests/reach_scope_claim_17844_test.pyrepo_tests/redaction_concept_census_test.py
💤 Files with no reviewable changes (2)
- autobot-backend/chat_history/persist_conversation_dedup_test.py
- autobot-backend/chat_workflow/manager.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| @pytest.mark.asyncio | ||
| async def test_a_non_owner_is_refused_and_nothing_is_deleted(): | ||
| """The ownership Depends is declared on the route and rejects before the handler runs.""" | ||
| from fastapi.routing import APIRoute | ||
|
|
||
| route = next( | ||
| r | ||
| for r in chat_api.router.routes | ||
| if isinstance(r, APIRoute) and r.path == "/chats/{chat_id}" and "DELETE" in r.methods | ||
| ) | ||
| dependants = {d.call for d in route.dependant.dependencies} | ||
| assert chat_api.validate_chat_ownership in dependants | ||
|
|
||
| def _forbidden(*_a, **_k): | ||
| raise HTTPException(status_code=403, detail="not your session") | ||
|
|
||
| with patch.object(chat_api, "validate_chat_ownership", side_effect=_forbidden): | ||
| with pytest.raises(HTTPException) as excinfo: | ||
| await chat_api.validate_chat_ownership("c1", _request()) | ||
| assert excinfo.value.status_code == 403 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🔎 Supported by static analysis
🏁 Script executed:
sed -n '90,120p' autobot-backend/api/chat_delete_by_id_test.py
sed -n '60,92p' autobot-backend/api/chat_delete_by_id_real_store_18316_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 2594
🏁 Script executed:
rg -n -F --glob '*.py' -- 'validate_chat_ownership' autobot-backend/api/chat.py autobot-backend/api/chat_delete_by_id_test.py autobot-backend/api/chat_delete_by_id_real_store_18316_test.py
sed -n '1,180p' autobot-backend/api/chat_delete_by_id_real_store_18316_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 6284
🏁 Script executed:
sed -n '80,112p' autobot-backend/api/chat.py
sed -n '1588,1620p' autobot-backend/api/chat.py
rg -n -F -- 'def ownership' autobot-backend/api/session_delete_legacy_transcript_18293_test.py
sed -n '1,180p' autobot-backend/api/session_delete_legacy_transcript_18293_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 10540
Remove the self-call from the dependency-registration test.
The patched call invokes chat_api.validate_chat_ownership directly, so it only checks the mock’s 403 side effect. The real-store test bypasses FastAPI dependency resolution, but it uses the real ownership validator and handler and asserts 403 with unchanged data. The route metadata assertion already checks dependency registration.
This is redundant cleanup, not a missing high-value ownership regression.
Suggested cleanup
-async def test_a_non_owner_is_refused_and_nothing_is_deleted():
- """The ownership Depends is declared on the route and rejects before the handler runs."""
+async def test_delete_route_declares_ownership_dependency():
+ """The DELETE route declares the ownership dependency."""
from fastapi.routing import APIRoute
@@
dependants = {d.call for d in route.dependant.dependencies}
assert chat_api.validate_chat_ownership in dependants
-
- def _forbidden(*_a, **_k):
- raise HTTPException(status_code=403, detail="not your session")
-
- with patch.object(chat_api, "validate_chat_ownership", side_effect=_forbidden):
- with pytest.raises(HTTPException) as excinfo:
- await chat_api.validate_chat_ownership("c1", _request())
- assert excinfo.value.status_code == 403📝 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.
| @pytest.mark.asyncio | |
| async def test_a_non_owner_is_refused_and_nothing_is_deleted(): | |
| """The ownership Depends is declared on the route and rejects before the handler runs.""" | |
| from fastapi.routing import APIRoute | |
| route = next( | |
| r | |
| for r in chat_api.router.routes | |
| if isinstance(r, APIRoute) and r.path == "/chats/{chat_id}" and "DELETE" in r.methods | |
| ) | |
| dependants = {d.call for d in route.dependant.dependencies} | |
| assert chat_api.validate_chat_ownership in dependants | |
| def _forbidden(*_a, **_k): | |
| raise HTTPException(status_code=403, detail="not your session") | |
| with patch.object(chat_api, "validate_chat_ownership", side_effect=_forbidden): | |
| with pytest.raises(HTTPException) as excinfo: | |
| await chat_api.validate_chat_ownership("c1", _request()) | |
| assert excinfo.value.status_code == 403 | |
| @pytest.mark.asyncio | |
| async def test_delete_route_declares_ownership_dependency(): | |
| """The DELETE route declares the ownership dependency.""" | |
| from fastapi.routing import APIRoute | |
| route = next( | |
| r | |
| for r in chat_api.router.routes | |
| if isinstance(r, APIRoute) and r.path == "/chats/{chat_id}" and "DELETE" in r.methods | |
| ) | |
| dependants = {d.call for d in route.dependant.dependencies} | |
| assert chat_api.validate_chat_ownership in dependants |
🧰 Tools
🪛 Ruff (0.16.8)
[warning] 110-110: Missing return type annotation for private function _forbidden
Add return type annotation: NoReturn
(ANN202)
[warning] 110-110: Missing type annotation for *_a
(ANN002)
[warning] 110-110: Missing type annotation for **_k
(ANN003)
[warning] 113-114: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
1 participant
Thinking Path
#18293: every conversation turn was persisted twice, by separate code.
chat_sessions—chat:session:{id}plus the session JSON. It is the declared system of record, and the UI reads it.data/conversation_transcripts/{id}.json, written bychat_workflow/conversation.py. It was undeclared instore_authority, yet the LLM context fell back to it.The two had already diverged.
session_reply_backfill.pyrebuilt A from B and called B "the source of truth", and B's path literal was copied. Fix: A is the one durable home, LLM context is projected from A, and B becomes a read-only legacy store. No stored data is moved or deleted by code; the data move is a #17038 proposal.A read-only check on a live install found a second defect: the data-storage view and session-delete cleanup resolved B's directory against the project root, while the writer had used the backend's working directory, so they never saw the real files.
What Changed
3b67603fcc:_persist_conversationwrites no durable copy, and the transcript writers are deleted._load_conversation_historyreads the Redis cache, then projects from A (chat_history/turn_projection.project_turn_pairs).It reads B only when A has no completed turns.Since round 3 it never reads B: when A has no completed turns, or can't be read, it returns[].store_authority:chat_workflow/conversation.pyjoinschat_sessions.write_sites(it writes the projection cache). B is declared aslegacy_conversation_transcripts, withwrite_sites=().chat_history/legacy_transcripts.pyholds B's only content reader,read_legacy_transcript_pairs. Since round 3 it serves only the human-run chore(chat): backfill assistant turns into sessions damaged before #13214 #13293 repair; no request path reads B, and the guard pins this.session_reply_backfill.py(chore(chat): backfill assistant turns into sessions damaged before #13214 #13293) is kept as the B-only repair. It reads B through the legacy reader, and its docstring names A as the record.chat:conversation:{id}now caches A's projection.a9c7bf36d2:store_authority.legacy_conversation_transcripts_dir(), resolved the way the retired writer resolved it. Every reader uses it: the legacy reader, the backfill, the delete cleanup, data-storage and the infra migrate script. Session ids containing/,\or..are rejected.chat:conversation:{id}together withchat:session:{id}.Guard (
repo_tests/conversation_turn_single_store_18293_test.py, 20 tests):store_authority, and is joined onto a path only there;chat:conversation:*writer is a declared write site, and the legacy entry has no write sites.Against the old
mainit flags each of the old sites.Behaviour changes
chat:conversation:{id}survived until its TTL#17038 data-move proposal (not implemented)
Verification
chat_workflow+chat_history+ the two API tests: 788 passed.store_authority16, the new guard 20, symbol-fork 17, size ratchet 31, ceiling parity 11, guard-reach meta 23,reach_scope_claim_1784410.manager.py's ceiling is lowered to 3975 in both baselines.Not done, stated rather than implied
The resolver depends on the working directory.Fixed in round 1 (SSOT setting) and pinned in the deploy templates in round 2.api/data_storage.pyhas the same project-root mismatch for its other categories; on a live install,chatspoints at a nonexistent directory. Filed as fix(data-storage): every data_storage category is rooted at the project root's data/, but the writers use the backend working dir — the chats view and delete miss the real files #18308.Closes #18293
Closes #18316
Refs #18315
Review round 1 fixes (
0795940a8a)reason == "owner_match"and the owner recorded on the session file is the caller (api/legacy_transcript_disposal.py:41). A Redis-only claim is not enough.legacy_migration,org_admin,log_only_modeand every other grant, the file stays and a Umbrella: SLM Data hygiene page — detect and safely clean up orphaned data across services #17038destructive_actionproposal is raised (:70). An approved proposal is executed byservices/legacy_transcript_disposal_action.py, with an approval comment and an audit entry.remove_legacy_transcripthas exactly two callers: the owner-verified delete and the approved proposal.DELETE /data-storage/conversations/{id}returns 403 when a legacy transcript exists, the same as bulk cleanup.legacy_transcripts_dir(AUTOBOT_LEGACY_TRANSCRIPTS_DIR, defaultdata/conversation_transcriptsunderbase_dir). There is no working-directory dependence. On a standard install the Ansible template setsAUTOBOT_BASE_DIRto the backend code directory, so the default resolves to where the retired writer wrote. That is established from the template; the live env file is readable only by the service account and was not read.legacy_conversation_transcripts(no frontend reference).chat:conversation:{id}after a delete (LOW) is fix(chat): a reply in flight can re-create chat:conversation:{id} after its session was deleted #18313.Review round 2 fixes (
ca39d44854)The "session file records the caller" premise could be manufactured, by H2 (create over a legacy-only id) and by H1 (a
legacy_migrationclaim, thenPOST /chats/{id}/savestampingmetadata.owner). Ownership of a legacy file cannot be proven from current state, so:destructive_actionproposal is raised. Its context records the requesting user ({"action": "legacy_transcript_delete", "session_id", "requested_by"}), so the approver sees who asked.is_verified_owner/read_durable_ownerare removed.remove_legacy_transcripthas exactly one caller, the approved-proposal executor, and the guard pins this.transcript_cleanup.errorinstead of returning a silentproposal_id: None._reject_session_id_collisionrefuses creating a session whose id has a legacy transcript and no chat session, so a legacy-only id can never be taken over (this also closes reading another user's legacy context through the LLM fallback).api/session_delete_legacy_transcript_18293_test.py, 5). These replace the round-1 tests that stubbedreason/owner. The realSessionOwnershipValidatorruns in enforced mode with an in-memory Redis, a realChatHistoryManagerwrites undertmp_path, and the realcreate_session/save_chat_by_id/delete_sessionbodies run. Only identity, the side cleanups and the Postgres proposal write are stubbed.legacy_migration, decided by the validator), then save (the file is stampedowner=alice), thenowner_match, then delete. The file stays and the proposal is recorded withrequested_by=alice.0795940a8a: 3 failed (H2 did not raise; H1 and the owner case deleted the file), 2 passed.backend.env.j2setsAUTOBOT_LEGACY_TRANSCRIPTS_DIR={{ backend_code_dir | default(backend_install_dir) }}/data/conversation_transcripts, which is the unit's ownWorkingDirectory, where the old writer wrote.docker-compose.yml(autobot-backend) sets/app/data/conversation_transcripts, under the imageWORKDIR(/app; the entrypoint keeps that working directory).WorkingDirectoryand the Dockerfile'sWORKDIR, so the pin cannot drift from them.chat_save_owner_test11.store_authority16, the new guard 23, symbol-fork 17, size ratchet 31, ceiling parity 11, guard-reach meta 23,reach_scope_claim_1784410,reach_declarations411,configuration_guide_env_vars3.chat_sessions.py's ceiling is lowered to 1938 in both baselines.Review round 3 fixes (
43de3788a7;origin/mainmerged as4191498f88; reach fixesa6f1e9f9f6)No request path reads a legacy transcript. The LLM-context fallback is removed.
_load_conversation_historyreads the cache and then A's projection, and returns[]when A has no turns.load_legacy_context_pairsis deleted.read_legacy_transcript_pairs, is now called only bychat_history/session_reply_backfill.py, the human-run chore(chat): backfill assistant turns into sessions damaged before #13214 #13293 CLI.legacy_transcript_file, but only as an existence check for the create 409, the delete proposal and the admin 403. That check never opens the file.test_no_request_path_reads_a_legacy_transcriptpins all of this. Atca39d44854the legacy module itself called the reader, so the check fails there.Tests:
ConversationHandlerMixin._load_conversation_history): a B-only id claimed aslegacy_migrationgets empty context and the file is untouched. It fails onca39d44854(B's turns were served).[].Results: chat_workflow + chat_history 773; e2e, location, delete, save-owner, proposer and session-ownership 87. Guards each run alone:
store_authority16, the new guard 25, symbol-fork 17, size ratchet 31, ceiling parity 11, guard-reach meta 23,reach_scope_claim_1784410,reach_declarations411.After merging
origin/main(75 commits; one import-line conflict inreach_scope_claim_17844_test.py, resolved by keeping both lines),a6f1e9f9f6:reach_scope_claim_17844failed on cleanorigin/main(6c683455c5). Its scope record namesdeployment-mode-detector-scanand the twoegress-default-*declarations, but never imports their modules, so the declarations were never registered. The modules are now imported. No open PR or issue covered this.REACH.window():kb-admin-read-bypass2567→2650,model-revision-pinning-sweep2900→2983,collectable-modules-inert-on-import2730→2815.reach_scope_claim_1784410 andreach_declarations477.#18293 AC6, stated plainly
AC6 reads: "Turns from sessions that exist only in B (older than the #13214 fix) stay readable, through #13293's backfill or a read-only fallback, until a human has approved their disposition."
repair_sessionreturns 0 when the session has no turns in A; it repairs damaged A sessions only.It is thereforeSuperseded: the owner ruled on 2026-10-11 that AC6 is amended (files are kept untouched, and content returns only via an approved backfill, feat(chat): an approval-gated backfill of transcript-only legacy sessions into chat_sessions (#18293 AC6) #18315), so this PR closes canonical(state): every chat turn is persisted twice — chat_sessions and an undeclared transcript store, already diverged once #18293.Refs #18293, pending the owner's ruling.AC6 ruling (owner, 2026-10-11): AC6 is amended on #18293. The transcript files are kept untouched, and their content returns only through an approved backfill. The backfill tool is #18315. This PR therefore closes #18293.
Round 4: #18318 folded in, train fallout (
1f334dd4cc,486f0c84a3; baseorigin/main@5efe902e72merged asfa082b2853)4ac328ea60,65e3bb2c70) are merged fromissue-18316-chat-delete. The two PRs depended on each other, so they ship as one.DELETE /chats/{id}delegates to the canonical session delete (api/chat_sessions.delete_session); its tests (api/chat_delete_by_id_test.py) and guard are on this branch.chat_single_delete_implementation_18316_testfixed:chat_history_manager.delete_session(isapi/legacy_transcript_disposal.delete_session_or_refer. The guard now names that module and function as canonical.[delete_session_or_refer]. The old check looked for the call inchat_sessions.py, which no longer contains it._MIN_FILES_SCANNED = 100is migrated todeclare(): thechat-single-delete-implementationfloor is 396 (mid-window of 416 measured, growth 40), with rootsautobot-backend/api. A_reach_policyABSOLUTE/SUBSET_DRIFTS verdict, a_DECLARED_SCOPESentry and thereach_scope_claimimport are added.security.session_posture.authenticated_user, which the harness did not patch, so every request authenticated as the dev-mode default user. The harness now sets the caller's identity in every namespace that asks.ownershipdependency isvalidate_session_ownership/validate_chat_ownership, and neither selects STRICT). SoSessionOwnershipValidator, the DEFAULT body, is the right validator to drive.chat_history/legacy_transcripts.legacy_transcript_file, in the sink's own scope:fullmatchon[A-Za-z0-9_-]{1,255}. That covers every idutils.chat_utils.validate_chat_session_idaccepts (UUIDs, UUID-with-suffix,[a-zA-Z0-9_-]+);os.path.normpathon the join with astartswith(root + os.sep)guard;_transcript_pathcarries the same checks. Tests cover 9 hostile ids and 4 accepted shapes.REACH.window():credential-field-name-only-callers6262→6384. (collectable-modules-inert-on-importcame in from the train at 2822.)chat_single_delete_implementation_183163,reach_scope_claim_1784410,reach_declarations507,reach_floor_migration9, guard-reach meta 23,store_authority16, symbol-fork 18, size ratchet 31, ceiling parity 11.Round 5: #18316 AC2 and AC3 delivered (
26a5ae8b93)DELETE /chats/{id}is tested against a REALChatHistoryManager(api/chat_delete_by_id_real_store_18316_test.py).tmp_path, with afakerediscache in front (already in the venv; nothing installed). The real two-posture validator decides, anddelete_chat_by_iditself runs.chat:session:{id}cache entry is gone,get_sessionreturnsNone, andload_sessionreturns[].repo_tests/chat_history_async_calls_awaited_18316_test.py).async defthatChatHistoryManagerexposes through its MRO (53 today).create_task/gather/ensure_future/wait_for/shield.delete_session, and a sync method.declare("chat-history-async-calls-awaited")(floor 396, rootsautobot-backend/api), with a_reach_policyverdict, a_DECLARED_SCOPESentry and the import.autobot-backend/api, all consumed. The detector does flag the one bug(chat): DELETE /chats/{id} never awaits delete_session — reports success and deletes nothing #18316 fixed: run on the pre-fixapi/chat.py(4ac328ea60^) it reports1941:chat_history_manager.delete_session.browser_mcp.py:1283,1321,playwright.py:742,834,scrape_templates.py:204). Each is the research browser manager's synchronousget_session, not a chat-history call, which is why receivers are matched by type evidence and not by method name.DELETE /chats/{id}now deletes the session's conversation files (file_action="delete", the canonical route's default). The old route was a no-op: it deleted nothing, files included. It now performs the same cleanup asDELETE /chat/sessions/{id}, apart from the legacy transcript, which is always only proposed.delete_chat_by_id:reach_scope_claim_1784410,reach_declarations513, guard-reach meta 23,reach_floor_migration9.origin/main(40 commits behind after the push, merged asb48a0144d8). The conflicts were floor values and import lines; main's re-measured floors were taken, and the size ceilings kept at this branch's measured counts.redaction-concept-census3247→3371 (mid-window), andaudio-extension-allowlist5937→6000.audio-extension-allowlistcould not go to its mid-window value (6100): its sweep completes only 6068 files, because it skips more than its declaredskips=300. 6000 clears the bottom band and stays under what completes. The declared-skips mismatch is that guard's own defect and is left to its owner.reach_scope_claim_1784410;reach_declarations537; guard-reach meta 23; floor migration 9; size ratchet 31; ceiling parity 11;store_authority16; symbol-fork 18.Model Used
Claude Opus 5.5 (master session: brief, host check, PR). Implementation by a Claude Opus subagent.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes