fix(backend/copilot): build transcript from SDK messages (atomic full-context) - #12308
fix(backend/copilot): build transcript from SDK messages (atomic full-context)#12308majdyz wants to merge 25 commits into
Conversation
…anscripts and remove double upload Two root causes for copilot "forgetting" conversation history: 1. validate_transcript() required both `type: "user"` AND `type: "assistant"` entries. With --resume, the user's message is passed as a CLI query parameter and does NOT appear in the transcript file. This caused read_transcript_file() to return None in the stop hook, so the transcript was never captured or uploaded. Confirmed via Langfuse: num_turns drops to 1 on subsequent turns across all 3 affected sessions. Fix: Only require `has_assistant` — assistant entries are the meaningful conversation content and are always present. 2. The success path (before the finally block) uploaded the OLD resume file (downloaded transcript from previous turn), then the finally block overwrote it with the stop hook content. This double-upload was wasteful and could overwrite newer data with stale data. Fix: Remove success path upload entirely — the finally block is the single source of truth for transcript uploads.
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes transcript upload into a per-session finally block, introduces a per-session Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Service
participant StopHook
participant TranscriptStore as Storage
rect rgba(200,230,255,0.5)
Client->>Service: start stream_chat_completion_sdk (user messages)
Service->>Client: stream chunks/events (use log_prefix)
Service->>Service: compute turn, track previous_transcript_content
end
rect rgba(220,255,200,0.5)
Service->>StopHook: invoke stop-hook
StopHook-->>Service: writes transcript file to disk
Service->>Service: read stop-hook file -> set previous_transcript_content
end
rect rgba(255,230,200,0.5)
Service->>Storage: _try_upload_transcript(..., log_prefix, previous_content)
Storage-->>Service: ack / error
Service-->>Client: finalize response / errors
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/copilot/sdk/transcript.py (1)
248-262:⚠️ Potential issue | 🟠 MajorRemove the legacy two-line gate.
upload_transcript()validates the stripped JSONL, and stripping can legitimately leave a single assistant entry when the user turn comes from the CLI arg and the rest of the lines are metadata/progress. Thislen(lines) < 2check still rejects that shape, so the hotfix can still drop resume state.Proposed fix
- lines = content.strip().split("\n") - if len(lines) < 2: - return False + lines = [line for line in content.strip().split("\n") if line] + if not lines: + return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/copilot/sdk/transcript.py` around lines 248 - 262, The two-line legacy gate in validate_transcript (the check "if len(lines) < 2: return False") should be removed because stripping JSONL can legitimately leave a single assistant entry when the user's turn is provided via CLI; delete that length check in validate_transcript and instead rely on the existing per-line inspection that looks for at least one assistant message (keep the parsing/assistant-detection logic intact), ensuring transcripts with a single assistant entry pass validation so resume state isn't dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autogpt_platform/backend/backend/copilot/sdk/transcript.py`:
- Around line 266-275: The loop over lines currently breaks out when it finds an
assistant entry, which short-circuits validation and lets malformed JSONL later
in the file slip through; instead, in the function containing the json.loads
loop keep scanning all lines after setting has_assistant (do not break) so any
subsequent JSONDecodeError still triggers the existing except path (return
False); in short, remove the break and let the loop continue, preserving the
existing json.loads exception handling and final return of has_assistant.
---
Outside diff comments:
In `@autogpt_platform/backend/backend/copilot/sdk/transcript.py`:
- Around line 248-262: The two-line legacy gate in validate_transcript (the
check "if len(lines) < 2: return False") should be removed because stripping
JSONL can legitimately leave a single assistant entry when the user's turn is
provided via CLI; delete that length check in validate_transcript and instead
rely on the existing per-line inspection that looks for at least one assistant
message (keep the parsing/assistant-detection logic intact), ensuring
transcripts with a single assistant entry pass validation so resume state isn't
dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac7296b9-c50f-47f2-adc5-2499afe22996
📒 Files selected for processing (2)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: types
- GitHub Check: Seer Code Review
- GitHub Check: Check PR Status
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (4)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/backend/**/*.{py,txt}
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use
poetry runprefix for all Python commands, including testing, linting, formatting, and migrations
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/backend/backend/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use Prisma ORM for database operations in PostgreSQL with pgvector for embeddings
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
🧠 Learnings (3)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
🔇 Additional comments (1)
autogpt_platform/backend/backend/copilot/sdk/service.py (1)
1411-1414: Nice simplification here.Making the
finallyblock the only upload path gives transcript capture a single source of truth and removes the stale resume-file upload path.
…alidation - Fix test_assistant_only_no_user to assert True (was False — the old buggy behavior) - Add test_resume_transcript_without_user_entry: simulates a real --resume stop hook transcript with summary + assistant entries but no user entry - Add test_returns_content_for_resume_transcript: verifies read_transcript_file accepts transcripts without user entries
…e_transcript Remove `break` after finding first assistant entry so all remaining lines are still validated for JSON correctness. Without this, corrupted JSONL after the first assistant entry would slip through and get uploaded as a broken --resume file.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/copilot/sdk/transcript.py (1)
248-274:⚠️ Potential issue | 🔴 CriticalDon’t reject the stripped one-line assistant transcript.
upload_transcript()validates the output ofstrip_progress_entries(), not the raw stop-hook file. For the exact--resumeshape this PR is fixing (queue-operation/file-history-snapshot+ one assistant message), stripping leaves a single assistant JSONL line, butlen(lines) < 2still returnsFalse. That means the upload is skipped and the session can still lose history on the next turn.Proposed fix
def validate_transcript(content: str | None) -> bool: @@ lines = content.strip().split("\n") - if len(lines) < 2: - return False has_assistant = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/copilot/sdk/transcript.py` around lines 248 - 274, The current validate_transcript in function validate_transcript wrongly rejects valid transcripts that are a single JSONL line containing an assistant message due to the check "if len(lines) < 2: return False"; remove or relax that length check so a single non-empty line is accepted and let the existing JSON-parsing loop determine validity (i.e., only return False for empty/whitespace content or for any JSONDecodeError, otherwise return True if any entry.get("type") == "assistant"); this fixes the case where upload_transcript(strip_progress_entries(...)) produces a one-line assistant JSONL and was being skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@autogpt_platform/backend/backend/copilot/sdk/transcript.py`:
- Around line 248-274: The current validate_transcript in function
validate_transcript wrongly rejects valid transcripts that are a single JSONL
line containing an assistant message due to the check "if len(lines) < 2: return
False"; remove or relax that length check so a single non-empty line is accepted
and let the existing JSON-parsing loop determine validity (i.e., only return
False for empty/whitespace content or for any JSONDecodeError, otherwise return
True if any entry.get("type") == "assistant"); this fixes the case where
upload_transcript(strip_progress_entries(...)) produces a one-line assistant
JSONL and was being skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49d614d4-4283-40b2-bbce-9910d219e204
📒 Files selected for processing (2)
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: types
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: Seer Code Review
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (6)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/backend/**/*.{py,txt}
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use
poetry runprefix for all Python commands, including testing, linting, formatting, and migrations
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
autogpt_platform/backend/**/*_test.py: Always review snapshot changes withgit diffbefore committing when updating snapshots withpoetry run pytest --snapshot-update
Use pytest with snapshot testing for API responses in test files
Colocate test files with source files using the*_test.pynaming convention
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/backend/backend/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use Prisma ORM for database operations in PostgreSQL with pgvector for embeddings
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt_platform/backend/**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
poetry run testfor backend testing (runs pytest with docker based postgres + prisma)
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.py
🧠 Learnings (3)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.pyautogpt_platform/backend/backend/copilot/sdk/transcript.py
autogpt-reviewer
left a comment
autogpt-reviewer
left a comment
There was a problem hiding this comment.
📋 Automated Review — PR #12308
PR #12308 — fix(backend/copilot): fix transcript validation and double upload causing session memory loss
Author: majdyz | Files: service.py (-40), transcript.py (+7/-9), transcript_test.py (+30/-1)
🎯 Verdict: APPROVE_WITH_CONDITIONS
What This PR Does
Hotfix for a critical P0 bug where copilot sessions lose all conversation history after the first turn. Two root causes: (1) validate_transcript() required both user and assistant entries, but with --resume the user message is a CLI query parameter and never appears in the transcript file, causing validation to reject valid transcripts; (2) the success path uploaded a stale resume file, then the finally block overwrote it with the stop-hook content (which was None due to the validation bug). Fix relaxes validation to require only assistant entries and consolidates upload to the finally block as the single source of truth.
Specialist Findings
🛡️ Security ✅ — No security concerns. Relaxed validation still rejects empty/malformed JSON. Transcript storage paths remain keyed by (user_id, session_id). Session lock + asyncio.shield() prevent races. Net security improvement by eliminating the data-loss race condition.
🏗️ Architecture ✅ — Single upload path in finally block is the correct design. Covers all exit paths (success, exception, cancellation) with asyncio.shield() protection. Fallback chain preserved (stop-hook content → resume file). Clean separation between transcript.py (validation/IO) and service.py (lifecycle). CodeRabbit's concern about early break in the validation loop is a false flag — the loop iterates all lines.
⚡ Performance ✅ — Net positive. Eliminates one redundant GCS PUT + one disk read per successful stream (~50-200ms savings). No new overhead introduced. Minor suggestion: add break after has_assistant = True to skip unnecessary json.loads() on remaining lines.
🧪 Testing test_returns_content_for_resume_transcript, test_assistant_only_no_user flipped correctly, test_resume_transcript_without_user_entry with realistic data). Gap: No integration test for the double-upload elimination in service.py — the more dangerous half of the fix is untested. Missing edge case test: malformed JSON after a valid assistant entry.
📖 Quality ✅ — Clean -40 line removal with clear explanatory comment. Docstring accurately updated. Test naming and docstrings follow conventions. No dead code remaining.
📦 Product ✅ — Critical P0 hotfix. Every multi-turn copilot user is affected (Langfuse confirms num_turns stuck at 1). Fix is correct, regression risk is low (strictly relaxes a constraint). Ship promptly.
📬 Discussion len(lines) < 2 at transcript.py:261 twice with escalating severity — a --resume transcript stripped by strip_progress_entries() could reduce to a single assistant line and still be rejected. Author has not responded. The break concern from CodeRabbit's first pass was fixed in commit 2. Zero human reviewers. Related PR #12303 already merged, no coordination needed.
🔎 QA ✅ — Live tested: frontend loads, signup works, copilot chat functional with multi-turn session continuity confirmed. All 27 transcript unit tests pass (3 new). 7 screenshots captured.
Conditions for Approval
transcript.py:261— Relaxlen(lines) < 2guard — Afterstrip_progress_entries(), a valid--resumetranscript can have exactly 1 assistant line. The current check rejects it, potentially reproducing the exact bug this PR fixes. Change toif not lines:orif len(lines) < 1:. (Flagged by CodeRabbit twice, unaddressed)
Should Fix (Follow-up OK)
transcript.py:271— Addbreakafterhas_assistant = Trueto avoid unnecessaryjson.loads()on remaining lines (or useany()pattern)service.py— Add a unit test asserting_try_upload_transcriptis called exactly once per successful turn (prevents re-introduction of double upload)transcript_test.py— Add test for malformed JSON line after a valid assistant entry (validates full-scan behavior)transcript_test.py— Add test for single-line transcript boundary (len(lines) < 2behavior)
QA Screenshots
| Step | Screenshot |
|---|---|
| Landing page |  |
| After signup |  |
| Copilot page |  |
| Message sent |  |
| Response |  |
| Follow-up |  |
| Session continuity |  |
Risk Assessment
Merge risk: LOW — Pure bugfix, removes code rather than adding it, well-scoped to 2 files
Rollback: EASY — Revert restores old validation + dual upload path; no schema/migration changes
CI Status
✅ All checks pass (CodeQL, Snyk, tests 3.11/3.12/3.13, types, lint)
@ntindle Critical hotfix for copilot session memory loss — approve with one condition: relax the len(lines) < 2 guard in validate_transcript() to prevent the same bug from recurring on single-line stripped transcripts.
…sistant content When using --resume, the CLI creates a new session and writes synthetic placeholders (model: "<synthetic>", "No response requested.") for all previous assistant turns. This caused the copilot to "forget" its own answers across turns. Changes: - Wire up merge_with_previous_transcript in the upload pipeline: the downloaded transcript from the start of the turn is passed through to upload_transcript, which restores real assistant content before stripping and uploading. - Refactor strip_progress_entries to preserve original JSON line formatting for entries that don't need reparenting, avoiding unnecessary re-serialization. - Add structured log_prefix ([SDK][session][turn]) across all SDK and transcript log lines for better debugging. - Add tests for merge logic and line-preservation behavior.
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🔴 Merge Conflicts DetectedThe following PRs have been tested and will have merge conflicts if merged after this PR. Consider coordinating with the authors.
Summary: 1 conflict(s), 0 medium risk, 0 low risk (out of 1 PRs with file overlap) Auto-generated on push. Ignores: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@autogpt_platform/backend/backend/copilot/sdk/service.py`:
- Around line 780-784: The log prefix computes turn before the incoming user
message is appended, causing off-by-one T0/Tn logs; in
stream_chat_completion_sdk() (and the similar blocks around the later
occurrences), move the computation of turn = sum(1 for m in session.messages if
m.role == "user") and the log_prefix = f"[SDK][{session_id[:12]}][T{turn}]" to
after you append the new user message (or increment turn after append) so the
prefix reflects the current turn for session and session_id; update all
occurrences (including the block at the later 799-812 region) accordingly.
- Around line 1451-1455: The current logger.info call ("%s Stream completed
successfully with %d messages") runs even when ended_with_stream_error is true;
update the post-stream logging around logger.info to check the
ended_with_stream_error flag (used in the same function) and, if true, emit an
error/warn log that the stream ended with an error including relevant context
(e.g., log_prefix and len(session.messages)) instead of the success message;
modify the branch that references logger.info and ended_with_stream_error so
successful completion is only logged when ended_with_stream_error is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fe23b2e-b1ba-4cef-92c7-d3a29deb9260
📒 Files selected for processing (3)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: types
- GitHub Check: Seer Code Review
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: Check PR Status
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/backend/**/*.{py,txt}
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use
poetry runprefix for all Python commands, including testing, linting, formatting, and migrations
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/backend/backend/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use Prisma ORM for database operations in PostgreSQL with pgvector for embeddings
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
autogpt_platform/backend/**/*_test.py: Always review snapshot changes withgit diffbefore committing when updating snapshots withpoetry run pytest --snapshot-update
Use pytest with snapshot testing for API responses in test files
Colocate test files with source files using the*_test.pynaming convention
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.py
autogpt_platform/backend/**/*test*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run
poetry run testfor backend testing (runs pytest with docker based postgres + prisma)
Files:
autogpt_platform/backend/backend/copilot/sdk/transcript_test.py
🧠 Learnings (3)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/transcript.pyautogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/transcript_test.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
autogpt_platform/backend/backend/copilot/sdk/service.py (2)
780-783:⚠️ Potential issue | 🟡 Minor
log_prefixis still one turn behind.This is built before the new user message is appended at Lines 799-812, so normal requests still log the first turn as
T0and later turns lag by one. Recompute it after the append path, or increment when a new user message is added, so the transcript/download/upload logs stay aligned for the current turn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/copilot/sdk/service.py` around lines 780 - 783, The log_prefix is computed from turn = sum(1 for m in session.messages if m.role == "user") before the new user message is appended, so it lags by one; update the computation so logs reflect the current turn: either move/recompute the turn and log_prefix after the code path that appends the new user message to session.messages (the block that adds the incoming user message), or increment turn by 1 when you append a user message. Adjust references to turn and log_prefix accordingly so transcript/download/upload logs use the corrected current-turn value.
1451-1455:⚠️ Potential issue | 🟡 MinorDon't log SDK stream failures as successful completions.
ended_with_stream_errorcan already beTruehere, so this still emits a success line after a failed stream and makes the new per-turn telemetry misleading.Suggested fix
- logger.info( - "%s Stream completed successfully with %d messages", - log_prefix, - len(session.messages), - ) + if ended_with_stream_error: + logger.warning( + "%s Stream ended with SDK error after %d messages", + log_prefix, + len(session.messages), + ) + else: + logger.info( + "%s Stream completed successfully with %d messages", + log_prefix, + len(session.messages), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/backend/backend/copilot/sdk/service.py` around lines 1451 - 1455, The logger.info call reporting "%s Stream completed successfully..." runs even when ended_with_stream_error is True; update the post-stream logging logic in the function that uses logger.info, log_prefix, session.messages and ended_with_stream_error to only emit the successful completion message when ended_with_stream_error is False, and emit an appropriate error/warning log (or skip success telemetry) when ended_with_stream_error is True so failed streams are not recorded as successes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@autogpt_platform/backend/backend/copilot/sdk/service.py`:
- Around line 780-783: The log_prefix is computed from turn = sum(1 for m in
session.messages if m.role == "user") before the new user message is appended,
so it lags by one; update the computation so logs reflect the current turn:
either move/recompute the turn and log_prefix after the code path that appends
the new user message to session.messages (the block that adds the incoming user
message), or increment turn by 1 when you append a user message. Adjust
references to turn and log_prefix accordingly so transcript/download/upload logs
use the corrected current-turn value.
- Around line 1451-1455: The logger.info call reporting "%s Stream completed
successfully..." runs even when ended_with_stream_error is True; update the
post-stream logging logic in the function that uses logger.info, log_prefix,
session.messages and ended_with_stream_error to only emit the successful
completion message when ended_with_stream_error is False, and emit an
appropriate error/warning log (or skip success telemetry) when
ended_with_stream_error is True so failed streams are not recorded as successes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94575d55-00de-41b0-afbe-a7ef3ac3d4f8
📒 Files selected for processing (1)
autogpt_platform/backend/backend/copilot/sdk/service.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: types
- GitHub Check: Seer Code Review
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (4)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
Files:
autogpt_platform/backend/backend/copilot/sdk/service.py
autogpt_platform/backend/**/*.{py,txt}
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use
poetry runprefix for all Python commands, including testing, linting, formatting, and migrations
Files:
autogpt_platform/backend/backend/copilot/sdk/service.py
autogpt_platform/backend/backend/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/CLAUDE.md)
Use Prisma ORM for database operations in PostgreSQL with pgvector for embeddings
Files:
autogpt_platform/backend/backend/copilot/sdk/service.py
autogpt_platform/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/sdk/service.py
🧠 Learnings (3)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/sdk/service.py
majdyz
changed the title
- Fix turn number off-by-one: compute log_prefix after appending user message - Fix stream error logging: check ended_with_stream_error before logging success - Initialize ended_with_stream_error at function start for pyright
|
✅ Fixed all CodeRabbit review comments in commit fc48944:
All tests passing ✅, pyright clean ✅ |
autogpt-reviewer
left a comment
autogpt-reviewer
left a comment
There was a problem hiding this comment.
📋 Automated Review — PR #12308
PR #12308 — fix(backend/copilot): fix transcript validation, double upload, and synthetic entries causing session memory loss
Author: majdyz | Files: service.py (+141/-115), transcript.py (+152/-50), transcript_test.py (+179/-1)
🎯 Verdict: APPROVE_WITH_CONDITIONS
What This PR Does
Critical hotfix for copilot sessions losing all conversation history after the first turn ("forgetting"). Three root causes: (1) validate_transcript() required both user AND assistant entries, but with --resume the user message is a CLI query parameter — never in the transcript file — so validation rejected valid transcripts and they were never uploaded; (2) the success path uploaded a stale resume file, then the finally block overwrote it with stop-hook content (which was None due to #1), losing the current turn; (3) --resume creates synthetic placeholder entries (model: "<synthetic>") that replace real assistant responses. Fix relaxes validation to require only assistant entries, consolidates upload to a single finally-block path, and adds merge_with_previous_transcript() to restore real assistant content via UUID matching.
Specialist Findings
🛡️ Security ✅ — No security concerns. Relaxed validation still rejects empty/malformed JSON. Transcript storage paths remain sanitized via _sanitize_id() (hex + hyphens only, 36 char max). Merge function operates only on server-controlled data with UUID matching. asyncio.shield() correctly protects critical persistence. Net security improvement by eliminating the data-loss race condition.
🏗️ Architecture ✅ — Single upload path in finally is the correct design — covers all exit paths (success, error, cancellation) with asyncio.shield(). Clean separation: service.py handles lifecycle, transcript.py handles transformation (merge → strip → validate → upload). Pipeline ordering is correct: merge before stripping, since synthetic entries have type: "assistant". log_prefix threading via parameters (no global state) is clean. Minor: _on_stop bypasses read_transcript_file() intentionally — well-documented with comment.
⚡ Performance ✅ — Net positive. Eliminates one full GCS round-trip per successful turn (~100-500ms savings). strip_progress_entries() now preserves original JSON lines for non-reparented entries (avoids O(n) json.dumps). Minor: merge_with_previous_transcript() double-parses JSONL (merge pass + strip pass) — could combine in future. Unbounded transcript growth over many turns is a latent risk worth monitoring.
🧪 Testing transcript.py: validation relaxation (including single-line transcripts), merge function (happy path, guards, UUID matching, preserving real entries), and strip formatting preservation. Gap: Zero tests for service.py behavioral changes — the double-upload elimination and stop-hook rewrite are the actual "fix" but are untested. Missing edge cases: malformed JSON in previous content, multi-turn chained merge, whitespace-only previous content.
📖 Quality ✅ — Excellent "why" comments (stop hook bypass, strip formatting preservation). Good function naming and docstrings. Clean dead code removal. Minor: log_prefix not threaded to read_transcript_file, mixed %-format vs f-string logging, validate_transcript docstring slightly stale.
📦 Product ✅ — Critical P0 hotfix affecting every multi-turn copilot user. Langfuse evidence confirms systemic impact (num_turns stuck at 1). Fix is correct and backward compatible (previous_content=None skips merging). Low rollback risk. Complementary with already-merged PR #12303.
📬 Discussion len(lines) < 2 guard removed ✅, (2) break in validation loop removed ✅, (3) log_prefix off-by-one fixed ✅, (4) stream error logging fixed ✅. Author was responsive (~2.5h turnaround). Zero human reviewers. Merge conflict with PR #12282 (2 conflicts in service.py). CI pending on latest HEAD (fc48944).
🔎 QA ✅ — Live tested: frontend loads, signup works, copilot multi-turn session memory confirmed working across 3 turns (introduced self → asked for recall → asked for summary — all context retained). 35/35 unit tests pass. 5 screenshots captured.
QA Screenshots
| Step | Screenshot |
|---|---|
| Landing page | |
| Copilot dashboard |  |
| Turn 1 response |  |
| Turn 2 recall |  |
| Turn 3 summary |  |
Conditions for Approval
-
service.py:~1553— Guardlen(captured_transcript.raw_content)againstNone— Ifraw_contentis everNone, this crashes thefinallyblock before the upload executes. Uselen(captured_transcript.raw_content or ""). (Flagged by Architect) -
Coordinate merge with PR #12282 — Overlap detection shows 2 conflicts (~49 lines) in
copilot/sdk/service.py. Must be resolved before or at merge time.
Should Fix (Follow-up OK)
service.py— Add unit test for_try_upload_transcript()verifyingprevious_contentis forwarded and upload happens exactly once (prevents re-introduction of double upload)transcript.py— Add test for malformed JSON inprevious_content(merge function handles it via try/except, but untested)transcript.py— Threadlog_prefixtoread_transcript_file()for full log traceabilityservice.py— Initializesource = "none"(not"stop_hook") to avoid misleading logs when no transcript is capturedtranscript.py— Standardize on %-format logging (avoid f-strings in logger calls)service.py:~1000— Add cross-reference comment in_on_stoppointing toread_transcript_file()for maintainabilitytranscript.py— Consider combining merge + strip into a single JSONL parse pass (currently double-parses)- Future — Add transcript size monitoring/warning for long-lived sessions (unbounded growth risk)
Risk Assessment
Merge risk: LOW — Pure bugfix, removes code rather than adding it, well-scoped to 2 files + tests
Rollback: EASY — Revert restores old validation + dual upload path; no schema/migration changes
CI Status
⏳ Tests (3.11/3.12/3.13) pending on latest HEAD (fc48944) — all other checks pass
@ntindle Critical P0 hotfix for copilot session memory loss — approve with 2 conditions: guard len() crash in finally block, coordinate merge with #12282. All CodeRabbit concerns resolved. QA confirms multi-turn memory works.
…tten merge logic - Use simple model=='<synthetic>' check (not content - avoids false positives) - Rename _is_synthetic_assistant_entry() -> _is_synthetic() (concise) - Flatten merge_with_previous_transcript() with early returns/continues - Use walrus operator where appropriate - Reduce nesting and improve readability - Keep debug logging to diagnose UUID mismatch issues Note: SDK doesn't expose SYNTHETIC_MODEL constant - it's a CLI detail.
…ID matching MAJOR SIMPLIFICATION: Instead of detecting '<synthetic>' marker (fragile), just replace ANY assistant entry if its UUID exists in previous transcript. This works because: - Previous transcript has real content with UUIDs: a1, a2, ... - New transcript (--resume) has placeholders with SAME UUIDs + new content with NEW UUID - Matching UUIDs = old turns that need real content restored ✅ - Non-matching UUID = current turn's new real content, keep as-is ✅ Benefits: - No fragile '<synthetic>' constant to maintain - No SDK/CLI version compatibility concerns - Simpler logic: just UUID matching - Works even if CLI changes synthetic format Credit: @majdyz for the insight!
autogpt-reviewer
left a comment
autogpt-reviewer
left a comment
There was a problem hiding this comment.
📋 Automated Re-review #2 — PR #12308
PR #12308 — fix(backend/copilot): fix transcript validation, double upload, and synthetic entries causing session memory loss
Author: majdyz | HEAD: 9135969 | Files: 25 (+1,169/-138)
🎯 Verdict: APPROVE
Previous conditions: ✅ BOTH RESOLVED
- (1) Guard
len(captured_transcript.raw_content)against None →raw_content: str = ""by design,len("")= 0, never crashes - (2) Coordinate merge with PR #12282 → Both PRs report MERGEABLE; standard merge-order coordination at merge time
New commits since last review (fc48944 → 9135969):
- Added 3 unit tests for
_try_upload_transcript(success, timeout, exception) — addresses previous should-fix - Added
conversation_turnto Langfuse trace metadata for observability - Fixed metadata type to string for Langfuse compatibility
What This PR Does
Critical P0 hotfix for copilot sessions losing all conversation history after the first turn ("forgetting"). Three root causes: (1) validate_transcript() required both user AND assistant entries, but with --resume the user message is a CLI parameter — never in the transcript — so validation rejected valid transcripts; (2) success path uploaded stale resume file, then finally block overwrote with None stop-hook content; (3) --resume creates synthetic placeholder entries replacing real assistant responses. Fix relaxes validation, consolidates to single finally-block upload, and adds UUID-based merge to restore real assistant content.
Specialist Findings
🛡️ Security ✅ — No concerns. Auth properly scoped on new PATCH endpoint (user_id + session_id). _sanitize_id() prevents path traversal. Merge function uses UUID dict lookup (no eval/injection). asyncio.shield() correctly protects persistence. Relaxed validation still rejects empty/malformed JSON. conversation_turn metadata exposes only an integer count — no PII.
🏗️ Architecture ✅ — Single upload path in finally is the correct design — covers all exit paths with asyncio.shield(). Clean merge→strip→validate→upload pipeline. CapturedTranscript.raw_content: str = "" is safe — no None path exists. _on_stop bypassing read_transcript_file() is correct (defers validation to upload time). log_prefix threading via parameters (no global state) is clean.
⚡ Performance ✅ — Net positive. Eliminates one GCS PUT per turn (~100-500ms savings). strip_progress_entries() preserves original JSON for non-reparented entries. Minor: merge_with_previous_transcript() double-parses JSONL (merge pass + strip pass) — <10ms typical, acceptable. Should monitor unbounded transcript growth over many turns (no size cap currently).
🧪 Testing ✅ — Meaningfully improved. New TestTryUploadTranscript (3 tests) covers error-handling contract. Existing 12 test methods in transcript_test.py cover validation relaxation, merge function (synthetic replacement, UUID matching, real entry preservation), strip formatting. Minor gaps: no test for previous_content kwarg passthrough, malformed JSON in previous content. Double-upload fix structurally enforced (single call site) — adequate without integration test.
📖 Quality ✅ — Good code quality. Excellent inline comments explaining why (stop hook bypass, single-upload rationale). _is_synthetic helper is clean DRY extraction. Minor: mixed logging format styles (%-format vs f-string) throughout both files — recommend standardizing. Stale docstring on _update_title_async doesn't mention only_if_empty behavior.
📦 Product ✅ — Critical P0 hotfix affecting every multi-turn copilot user. Langfuse evidence confirms num_turns stuck at 1. Fix is correct and backward compatible. Note: PR scope is broader than "hotfix" — includes merged UX features (rename, new chat button, auto-title) and model additions (Claude Sonnet 4.6) from dev. This doesn't affect correctness but complicates potential rollback.
📬 Discussion ✅ — All 5 CodeRabbit review passes resolved. Author was highly responsive (~2.5h turnaround). Previous conditions both addressed. Zero human reviewers — needs human approval from @ntindle or @kcze. PR #12282 still open with overlapping changes but both report MERGEABLE.
🔎 QA ✅ — Live tested: frontend loads, signup works, copilot multi-turn session continuity confirmed (2 messages with context retained). Session rename works end-to-end. Auto-generated title not overwritten after user rename (only_if_empty guard working). "New Chat" button visible at top of sidebar. 11 screenshots captured.
QA Screenshots
| Step | Screenshot |
|---|---|
| Landing page | |
| Dashboard |  |
| Copilot initial |  |
| Message 1 response |  |
| Sidebar rename |  |
| Rename submitted |  |
| Message 2 (continuity) |  |
| Storybook message |  |
Should Fix (Follow-up OK)
- Logging style — Standardize on %-format logging throughout
service.pyandtranscript.py(currently mixed %-format and f-strings) - Test gap — Add test verifying
previous_contentkwarg is forwarded through_try_upload_transcripttoupload_transcript - Test gap — Add test for malformed JSON in
previous_content(merge handles via try/except but untested) - Transcript size — Add warning log when transcript exceeds a size threshold (e.g., 2MB/500 lines) for operational visibility
- Docstring — Update
_update_title_asyncdocstring to mentiononly_if_emptybehavior - Double-parse — Consider combining merge + strip into single JSONL parse pass if transcripts grow large
Risk Assessment
Merge risk: LOW — Core fix removes code rather than adding it, well-scoped transcript changes
Rollback: EASY — Revert restores old validation + dual upload; no schema/migration changes
CI Status
✅ lint, integration_test, CodeQL, Snyk all pass
⏳ test (3.11/3.12/3.13), types, e2e pending on latest HEAD
@ntindle P0 hotfix for copilot session memory loss — both previous conditions resolved. All 8 specialists approve. QA confirms multi-turn sessions retain context. Ready for human approval.
…hing only
Remove fragile synthetic entry detection ("<synthetic>" string check) in favor
of simple UUID-based matching: previous transcript always wins for matching UUIDs,
new UUIDs are added.
This approach is more robust and doesn't depend on CLI implementation details
that could change. The test `test_preserves_real_entries` was updated to reflect
the new behavior since the scenario it tested (same UUID with different real content)
is not a known real-world case.
github-actions
Bot
removed
documentation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
… CLI fails to append
autogpt-reviewer
left a comment
autogpt-reviewer
left a comment
There was a problem hiding this comment.
📋 Automated Re-review #3 — PR #12308
PR #12308 — fix(backend/copilot): fix transcript validation, double upload, and synthetic entries causing session memory loss
Author: majdyz | HEAD: 440a06a | Previous verdict: APPROVE (at 8725844)
New commits: 4 (8725844 → 440a06a) — simplified UUID matching, debug logging, fix for CLI failing to append
🎯 Verdict: APPROVE
What Changed Since Last APPROVE
4 commits refactoring the merge logic:
f19a423ca— Simplified merge to "previous always wins" for matching UUIDs (removes synthetic detection)6c83a91ae+8ec706c12— Added diagnostic logging for merge debugging440a06ad9— Added safety net: if CLI transcript is smaller than previous, manually prepend previous content
What This PR Does (unchanged)
Critical P0 hotfix for copilot sessions losing all conversation history after the first turn. Three root causes: (1) validate_transcript() rejected valid --resume transcripts, (2) success path uploaded stale resume file then finally block overwrote with None, (3) --resume creates synthetic placeholders replacing real assistant responses. Fix relaxes validation, consolidates to single finally-block upload, and adds UUID-based merge to restore real assistant content.
Specialist Findings
🛡️ Security ✅ — No security vulnerabilities. UUID matching is safe (internally generated, not user-supplied). Size-comparison concatenation is safe (JSONL lines parsed independently). content[:500] and stripped[:500] logged at WARNING level expose user conversation content (PII risk). Downgrade to DEBUG.
🏗️ Architecture ✅ — Single finally-block upload path, merge→strip→validate→upload pipeline all compose correctly. Manual append + merge interaction is architecturally sound (redundant but harmless). previous_transcript_content doesn't end with \n, first line of raw_transcript joins with last line of previous, creating invalid JSON line. Add if not previous_transcript_content.endswith("\n"): previous_transcript_content += "\n". Also: document the "previous always wins" invariant (depends on CLI never modifying old assistant content).
⚡ Performance ✅ — Net positive (eliminates one GCS PUT per turn). Merge is O(P+N) linear. Manual append is O(n) single string concat on fallback path only.
🧪 Testing ✅ — Test rename test_preserves_real_entries → test_previous_wins_for_matching_uuids is correct — assertion flip matches new behavior. New test_malformed_json_after_valid_assistant_returns_false is a good addition. Merge function has 5 meaningful tests. Integration gap for manual append logic is acknowledged and reasonably deferred. No blocking test issues.
📖 Quality ✅ — Good comments explaining merge strategy with concrete UUID examples. _is_synthetic helper is clean DRY. new_assistant_uuids list (transcript.py:~115) is populated but never read — dead code. Mixed logging styles (%-format vs f-string) persist but improved. Content preview at WARNING level is a debug remnant.
📦 Product ✅ — Core changes directly fix real user-impacting bug. "Previous always wins" is correct — CLI only creates synthetic placeholders for old UUIDs during --resume, never legitimate updates. Manual append safety net prevents catastrophic history loss. Low rollback risk.
📬 Discussion @coderabbitai resume). Sentry flagged blank-line crash in validate_transcript() (json.loads("") → JSONDecodeError on empty lines) — unaddressed. 2 debug commits should ideally be squashed before merge. PR #12282 still has merge conflicts (~49 lines in service.py).
🔎 QA ✅ — Live tested: frontend loads, signup works, 3-turn copilot session memory confirmed working (introduced self → asked for recall → asked for summary — all context retained). No stream errors, no lost context, no regressions. 5 screenshots captured.
QA Screenshots
| Step | Screenshot |
|---|---|
| Landing page | |
| Copilot initial | |
| Turn 1 response |  |
| Turn 2 memory recall |  |
| Turn 3 session summary |  |
Should Fix (Follow-up OK)
service.py:~1588— Add newline boundary check before manual concatenation:if not previous_transcript_content.endswith("\n"): previous_transcript_content += "\n"— prevents creating malformed JSONL line at the boundarytranscript.py:482-483— Downgrade content preview logging from WARNING to DEBUG — logs up to 500 chars of user conversation content (PII risk). Flagged by 5/8 specialiststranscript.py:131,138— Downgrade per-UUID merge logging from INFO to DEBUG — O(n) log lines per turn; keep aggregate summary (line 148) at INFOtranscript.py:~115— Remove deadnew_assistant_uuidslist — populated but never readtranscript.py:~65— Document "previous always wins" invariant — add comment noting this depends on CLI never modifying old assistant content during--resumetranscript.py:382— Filter blank lines invalidate_transcript()beforejson.loads()— empty lines causeJSONDecodeError→ false rejection (flagged by Sentry)- Squash debug commits (
6c83a91ae,8ec706c12) before merge for clean history - Coordinate merge with PR #12282 — 2 conflicts (~49 lines) in
copilot/sdk/service.py
Risk Assessment
Merge risk: LOW — Core fix removes code, new commits add a defensive safety net
Rollback: EASY — No schema/migration changes
CI Status
✅ All checks pass (tests 3.11/3.12/3.13, types, CodeQL, Snyk, lint)
@ntindle P0 hotfix for copilot session memory loss — APPROVE maintained. New commits add a reasonable safety net for CLI append failures. 8/8 specialists approve. QA confirms 3-turn session memory works. Needs human approval — zero human reviewers across all iterations.
… full-context) Replace CLI file reading (race condition) with direct SDK message capture. Transcript now represents COMPLETE active context, not incremental changes. Changes: - NEW: transcript_builder.py - TranscriptBuilder class - REMOVED: Stop hook + file reading logic (~200 lines) - REMOVED: merge_with_previous_transcript, read_transcript_file - SIMPLIFIED: upload_transcript (no merge, atomic replace) - CLEANED: Removed gap-based compression fallback Benefits: - Eliminates race conditions (Stop hook unreliable) - Atomic transcript (full context always) - -372 total lines removed - Cleaner, more maintainable code Transcript flow (atomic): Turn N: Download full context → Add new messages → Upload complete (REPLACE)
- Remove TestReadTranscriptFile class (read_transcript_file deleted) - Remove TestMergeWithPreviousTranscript class (merge_with_previous_transcript deleted) - Remove TestTryUploadTranscript class (_try_upload_transcript deleted) - All remaining tests pass (23 tests in transcript_test.py)
Skip empty lines instead of treating them as parse errors, preventing silent data loss from transcripts with blank lines. Addresses PR comment #2894841670
Covers the fix in ad70449 that skips empty lines instead of treating them as parse errors.
Move TextBlock, ThinkingBlock, ToolResultBlock imports from inside _format_sdk_content_blocks to top-level, following code style guidelines (prefer top-level imports over function-local imports).
…dd upload timeout 1. Downgrade content preview logging from WARNING to DEBUG (transcript.py:325-326) - Prevents logging up to 500 chars of user conversation content (PII risk) - Keep validation failure at WARNING, only preview at DEBUG 2. Add 30s timeout to upload_transcript in finally block (service.py:1557) - Prevents session lock from hanging indefinitely if upload stalls - Uses asyncio.timeout wrapper around asyncio.shield Addresses PR review #3903048969 item #2 and discussion r2895449830
majdyz
changed the title
|
@autogpt-reviewer review |
HIGH severity fix: When upload_transcript times out after 30s, the shielded coroutine continues running but becomes orphaned (no reference). Python's GC can reclaim the task before completion, causing silent data loss. Fix: If TimeoutError occurs, explicitly create task and track in _background_tasks to maintain strong reference. Upload completes in background without blocking session lock release. Addresses PR discussion r2895491552
Previous fix created NEW task after timeout, causing double upload: - Original shielded task still running - New task also uploading same transcript Correct fix: Create task FIRST, then shield it. If timeout occurs, track the SAME task (no double upload). Fixes double-upload bug in b8c65e3
github-project-automation
Bot
moved this from 🆕 Needs initial review
to ✅ Done
in AutoGPT development kanban
2 participants
🎯 Summary
Fixes CoPilot sessions losing conversation history by building transcripts directly from SDK message streams instead of reading CLI files, implementing atomic full-context storage, and eliminating race conditions.
🔧 Changes
Core Architecture: TranscriptBuilder Pattern
New approach: Build transcript from SDK messages during streaming
Flow:
Code Changes
transcript_builder.pyservice.pytranscript.pysecurity_hooks.pyNet result: -382 lines of code removed, massive simplification
Specific Fixes
Blank lines handling (ad70449)
validate_transcript()instead of treating as parse errorsPII risk mitigation (0eddb6f)
Session lock safety (0eddb6f)
upload_transcriptin finally blockCode style (042ed42)
🧪 Testing
test_blank_lines_are_skipped)📝 Transcript == Active Context
The transcript is now truly active context:
SDK compaction is captured: When SDK compacts internally, the transcript reflects the post-compaction state.
🔗 Related
None - fully backward compatible with existing transcripts.
📊 Code Cleanup Summary