fix(copilot): bundle of chat stream stability fixes (PK dedup, race, compaction, errors, chips) - #12948
Conversation
…fe chips Three user-reported regressions on dev (chat-mode-option flag), all in one PR because they share the same surface area: 1. Disappearing queued messages — chips were stored as bare strings keyed by array index; the mid-turn poll captured a stale snapshot and used a slice-based ``setQueuedMessages(remaining)`` that overwrote any chip the user appended during the in-flight peek. Fixed by giving each chip a frontend-only UUID, promoting one bubble per chip, and using a functional ``setChips(prev => prev.filter(c => !drainedIds.has(c.id)))`` so newly appended chips survive the race. 2. Prompt-too-long recurring on the same session for days (SENTRY-1207, 191 occurrences) — the T2+ context-error retry branch dropped session_id to dodge "Session ID already in use", so the recovery CLI wrote to a random path and the post-turn upload silently grabbed the stale pre-failure file. Next turn re-resumed from the same bloated GCS copy and re-tripped, ad infinitum. Fixed by clearing the local session file first via the new ``delete_stale_cli_session_file`` helper, then keeping ``session_id`` so the CLI's recovery write lands on the predictable path that ``upload_transcript`` reads. 3. Double error UI — backend appends a persisted error marker to ``session.messages`` AND yields a ``StreamError`` SSE event on the same final-failure path. Frontend rendered the marker as an in-line ErrorCard bubble and ``error`` from useChat as a trailing red banner — same string, twice. Fixed by adding a top-level ``lastAssistantHasErrorMarker`` memo in ChatMessagesContainer and gating the banner on ``!lastIsErrorMarker``. Tests: 216 backend SDK tests pass, 29 focused frontend tests pass (useCopilotPendingChips, ChatMessagesContainer error-banner-dedup, makePromotedBubble, plus regression coverage for the new delete_stale_cli_session_file helper and the retry session_id reuse).
|
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:
WalkthroughAdds a guarded helper to delete deterministic local CLI session JSONL and integrates it into SDK restore/retry logic; frontend changes convert pending-chip state to chips with stable ids, promote one bubble per chip, and suppress duplicate trailing error banners when inline error markers exist. Changes
Sequence Diagram(s)(omitted — changes are localized helpers, retry behavior, and UI logic; no new multi-component sequential flow requiring a diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🔍 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.
🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 4 conflict(s), 0 medium risk, 8 low risk (out of 12 PRs with file overlap) Auto-generated on push. Ignores: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1307-1340: The delete_stale_cli_session_file function has a TOCTOU
due to calling Path.exists() before unlink and logs full path on unlink errors;
remove the exists() check and perform the unlink directly after validating the
real_path prefix (using cli_session_path and projects_base as currently done),
catch FileNotFoundError and return False, and catch other OSError exceptions but
log only os.path.basename(real_path) (not the full path) in the error message
before returning False.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93e33973-968d-4ed6-9ac1-fa7806ae19c7
📒 Files selected for processing (8)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/backend/backend/copilot/sdk/service_helpers_test.pyautogpt_platform/frontend/src/app/(platform)/copilot/__tests__/useCopilotPendingChips.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/ChatMessagesContainer.tsxautogpt_platform/frontend/src/app/(platform)/copilot/components/ChatMessagesContainer/__tests__/ChatMessagesContainer.test.tsxautogpt_platform/frontend/src/app/(platform)/copilot/helpers/__tests__/makePromotedBubble.test.tsautogpt_platform/frontend/src/app/(platform)/copilot/helpers/makePromotedBubble.tsautogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #12948 +/- ##
==========================================
+ Coverage 69.62% 69.64% +0.02%
==========================================
Files 2135 2135
Lines 157921 158133 +212
Branches 16312 16322 +10
==========================================
+ Hits 109946 110131 +185
- Misses 44727 44745 +18
- Partials 3248 3257 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts (1)
128-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against stale peek responses after session switches.
A response started for an old
sessionIdcan still resolve and callsetChips, causing old-session chips to reappear in the new session. Add a stale-response guard before mutating state.Suggested fix
useEffect(() => { @@ - void getV2GetPendingMessages(sessionId).then((res) => { + const requestSessionId = sessionId; + let cancelled = false; + + void getV2GetPendingMessages(sessionId).then((res) => { + if (cancelled) return; + if (prevSessionIdRef.current !== requestSessionId) return; if (res.status !== 200) return; @@ setChips(() => res.data.count > 0 @@ ); }); + return () => { + cancelled = true; + }; }, [sessionId, status, setChips]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts around lines 128 - 146, The promise handler for getV2GetPendingMessages(sessionId) can mutate state after the user has switched sessions; capture the sessionId at request time (e.g., const callSessionId = sessionId) and before any setChips or other state changes verify that the current sessionId still equals callSessionId (or return early if it doesn't). Apply this stale-response guard inside the then callback before the turnStarting/sessionChanged logic so setChips is only called for the matching session; reference getV2GetPendingMessages, sessionId, setChips, turnStarting, and sessionChanged when locating where to add the check.
🤖 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/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts:
- Around line 233-237: The bubble ID generation is inconsistent: auto-continue
uses `${assistantId}-${chip.id}` while mid-turn uses `chip.id`, causing
duplicate filtering to miss matches; update the calls to makePromotedUserBubble
(e.g., the call that currently passes `chip.id` around mid-turn and the call at
auto-continue) to use a single deterministic ID format such as
`${assistantId}-${chip.id}` so both promotion paths produce the same bubble ID
for the same chip and duplicate filtering works correctly.
---
Outside diff comments:
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts:
- Around line 128-146: The promise handler for
getV2GetPendingMessages(sessionId) can mutate state after the user has switched
sessions; capture the sessionId at request time (e.g., const callSessionId =
sessionId) and before any setChips or other state changes verify that the
current sessionId still equals callSessionId (or return early if it doesn't).
Apply this stale-response guard inside the then callback before the
turnStarting/sessionChanged logic so setChips is only called for the matching
session; reference getV2GetPendingMessages, sessionId, setChips, turnStarting,
and sessionChanged when locating where to add the check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e87b16b1-c686-4e4d-91c2-015430e4e74a
📒 Files selected for processing (2)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
🚧 Files skipped from review as they are similar to previous changes (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). (18)
- GitHub Check: lint
- GitHub Check: integration_test
- GitHub Check: check API types
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.13)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: lint
- GitHub Check: test (3.11)
- GitHub Check: Seer Code Review
- GitHub Check: check-overlaps
- GitHub Check: types
- GitHub Check: lint
- GitHub Check: end-to-end tests
- GitHub Check: Check PR Status
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (typescript)
🧰 Additional context used
📓 Path-based instructions (8)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Use Node.js 21+ with pnpm package manager for frontend development
Always run 'pnpm format' for formatting and linting code in frontend developmentFormat frontend code using
pnpm format
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Fully capitalize acronyms in symbols, e.g.graphID,useBackendAPI
No linter suppressors (//@ts-ignore``,// eslint-disable) — fix the actual issue
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{tsx,ts}: Use function declarations for components and handlers (not arrow functions) in React components
Only use arrow functions for small inline lambdas (map, filter, etc.) in React components
Use PascalCase for component names and camelCase with 'use' prefix for hook names in React
Use Tailwind CSS utilities only for styling in frontend components
Use design system components from 'src/components/' (atoms, molecules, organisms) in frontend development
Never use 'src/components/legacy/' in frontend code
Only use Phosphor Icons (@phosphor-icons/react) for icons in frontend components
Use generated API hooks from '@/app/api/generated/endpoints/' instead of deprecated 'BackendAPI' or 'src/lib/autogpt-server-api/'
Use React Query for server state (via generated hooks) in frontend development
Default to client components ('use client') in Next.js; only use server components for SEO or extreme TTFB needs
Use '' component for rendering errors in frontend UI; use toast notifications for mutation errors; use 'Sentry.captureException()' for manual exceptions
Separate render logic from data/behavior in React components; keep comments minimal (code should be self-documenting)
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx}: No barrel files or 'index.ts' re-exports in frontend code
Regenerate API hooks with 'pnpm generate:api' after backend OpenAPI spec changes in frontend development
autogpt_platform/frontend/**/*.{ts,tsx}: Use function declarations (not arrow functions) for components/handlers
Noanytypes unless the value genuinely can be anything
Keep render functions and hooks under ~50 lines; extract named helpers or sub-components when they grow longer
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
autogpt_platform/frontend/src/**/*.{ts,tsx}: Use generated API hooks from@/app/api/__generated__/endpoints/following the patternuse{Method}{Version}{OperationName}, and regenerate withpnpm generate:api
Separate render logic from business logic using component.tsx + useComponent.ts + helpers.ts pattern, colocate state when possible and avoid creating large components, use sub-components in local/componentsfolder
Use function declarations for components and handlers, use arrow functions only for callbacks
Do not useuseCallbackoruseMemounless asked to optimise a given function
autogpt_platform/frontend/src/**/*.{ts,tsx}: Keep files under ~200 lines; extract sub-components or hooks into their own files when a file grows beyond this
Use generated API hooks from@/app/api/__generated__/endpoints/with patternuse{Method}{Version}{OperationName}
Always import the-Icon-suffixed alias from@phosphor-icons/react(e.g.TrashIcon,PlusIcon,SquareIcon) — bare exports are deprecated
Do not useuseCallbackoruseMemounless asked to optimize a given function
Never usesrc/components/__legacy__/*— use design system components fromsrc/components/
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
No barrel files or
index.tsre-exports in the frontend
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not type hook returns, let Typescript infer as much as possible
autogpt_platform/frontend/src/**/*.ts: Extract component logic into custom hooks grouped by concern, not by component, with each hook in its own.tsfile
Do not type hook returns; let TypeScript infer as much as possible
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never type with
any, if no types available useunknown
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (autogpt_platform/frontend/AGENTS.md)
Avoid index and barrel files
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/copilot/pending_messages.py:52-64
Timestamp: 2026-04-14T14:36:25.545Z
Learning: In `autogpt_platform/backend/backend/copilot` (PR `#12773`, commit d7bced0c6): when draining pending messages into `session.messages`, each message's text is sanitized via `strip_user_context_tags` before persistence to prevent user-controlled `<user_context>` injection from bypassing the trusted server-side context prefix. Additionally, if `upsert_chat_session` fails after draining, the drained `PendingMessage` objects are requeued back to Redis to avoid silent message loss. Do NOT flag the drain-then-requeue pattern as redundant — it is the intentional failure-resilience strategy for the pending buffer.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12445
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1071-1072
Timestamp: 2026-03-17T06:48:26.471Z
Learning: In Significant-Gravitas/AutoGPT (autogpt_platform), the AI SDK enforces `z.strictObject({type, errorText})` on SSE `StreamError` responses, so additional fields like `retryable: bool` cannot be added to `StreamError` or serialized via `to_sse()`. Instead, retry signaling for transient Anthropic API errors is done via the `COPILOT_RETRYABLE_ERROR_PREFIX` constant prepended to persisted session messages (in `ChatMessage.content`). The frontend detects retryable errors by checking `markerType === "retryable_error"` from `parseSpecialMarkers()` — no SSE schema changes and no string matching on error text. This pattern was established in PR `#12445`, commit 64d82797b.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-25T02:53:53.964Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (PR `#12918`, commit 6576bf561):
- `_flush_unresolved_tool_calls` was renamed to `flush_unresolved_tool_calls` (public); all call sites updated, `# noqa: SLF001` suppressor removed.
- `_flush_orphan_tool_uses_to_session` and `_InterruptedAttempt.finalize` both return `list[StreamBaseResponse]`; the post-loop caller yields those events directly to avoid double-flush and skipped UI cleanup events.
- The three former post-loop blocks (partial restore + redundant re-flush + two separate `yield StreamError` sites) are collapsed into a single block driven by `_classify_final_failure` returning a `_FinalFailure(display_msg, code, retryable)` dataclass, so history marker and SSE yield share one source of truth.
Do NOT flag double-flush risk or mismatched history/SSE marker as issues in the post-loop section of `stream_chat_completion_sdk`.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12797
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1991-2021
Timestamp: 2026-04-15T13:44:34.273Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (`_run_stream_attempt`), the pre-create block (PR `#12797`) intentionally does NOT call `state.transcript_builder.append_assistant(...)` when inserting the empty assistant placeholder into `ctx.session.messages`. The transcript is left ending at the `tool_result` entry (N entries) while `message_count` metadata is N+1. This mismatch is benign and deliberate: on the next `--resume`, the SDK sees the transcript ending at `tool_result` and correctly regenerates the assistant response. Pre-appending the assistant turn to the transcript would suppress regeneration while leaving `session.messages[-1].content = ""` permanently (worse outcome). On the gap-fallback path, `transcript_msg_count (N+1) >= msg_count-1 (N)` means no gap is injected for the empty placeholder, which is correct because injecting an empty assistant message as context would mislead the SDK. Do NOT flag this transcript/message_count discrepancy as a bug.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12796
File: autogpt_platform/backend/backend/api/features/chat/routes.py:504-527
Timestamp: 2026-04-16T12:33:44.990Z
Learning: In `autogpt_platform/backend/backend/api/features/chat/routes.py`, `get_session` (PR `#12796`, commit 3771bfad9c1) closes the TOCTOU race between the initial `stream_registry.get_active_session()` pre-check and `get_chat_messages_paginated()` with a post-check re-verification: after the DB fetch, if `is_initial_load and active_session is not None`, it calls `get_active_session` a second time; if `post_active is None` (stream completed during the window), it resets `from_start=True`, `forward_paginated=True`, and re-fetches messages from sequence 0. Do NOT flag the double `get_active_session` call pattern as redundant — it is the intentional TOCTOU mitigation for pagination direction selection.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-28T03:31:29.696Z
Learning: In Significant-Gravitas/AutoGPT PR `#12933` (`fix/stripe-checkout-link-auth-loop`), the initial approach of pinning `payment_method_types=["card"]` in `top_up_intent` and `create_subscription_checkout` (in `autogpt_platform/backend/backend/data/credit.py`) was reverted in commit `584b43a71` as it patched a symptom. The true root cause was in `update_subscription_tier()` in `v1.py`: a `current_tier_price_id is not None` guard was gating admin-granted DB-tier flips and short-circuiting them when the BUSINESS tier was pruned from the price-id LaunchDarkly flag. Do NOT flag `payment_method_types` absence in these checkout helpers as a Stripe Link bypass issue; the fix lives in the subscription tier update guard logic.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12356
File: autogpt_platform/backend/backend/copilot/constants.py:9-12
Timestamp: 2026-03-10T08:39:22.025Z
Learning: In Significant-Gravitas/AutoGPT PR `#12356`, the `COPILOT_SYNTHETIC_ID_PREFIX = "copilot-"` check in `create_auto_approval_record` (human_review.py) is intentional and safe. The `graph_exec_id` passed to this function comes from server-side `PendingHumanReview` DB records (not from user input); the API only accepts `node_exec_id` from users. Synthetic `copilot-*` IDs are only ever created server-side in `run_block.py`. The prefix skip avoids a DB lookup for a `AgentGraphExecution` record that legitimately does not exist for CoPilot sessions, while `user_id` scoping is enforced at the auth layer and on the resulting auto-approval record.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12873
File: autogpt_platform/backend/backend/copilot/baseline/reasoning.py:0-0
Timestamp: 2026-04-21T17:31:26.829Z
Learning: In `autogpt_platform/backend/backend/copilot/baseline/reasoning.py` (`BaselineReasoningEmitter`), when `render_in_ui=False`, BOTH the `StreamReasoning*` wire events AND the `ChatMessage(role="reasoning")` persistence append must be suppressed together. `convertChatSessionToUiMessages.ts` unconditionally re-renders all persisted `role="reasoning"` rows as `{type:"reasoning"}` UI parts on reload, so persisting rows while silencing live wire events would resurrect the reasoning collapse on page refresh. The audit trail is preserved through the provider transcript and `_format_sdk_content_blocks` (SDK path) instead. The baseline and SDK paths mirror each other: flag off → no live wire event, no persisted row, no hydrated collapse. This was established in PR `#12873`, commit 7ef10b26c.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/blocks/autopilot.py:631-638
Timestamp: 2026-04-14T07:35:11.464Z
Learning: In `autogpt_platform/backend/backend/copilot/executor/utils.py`, `CoPilotExecutionEntry` includes a `permissions: CopilotPermissions | None` field (added in PR `#12773` / commit a0184c87b9). `enqueue_copilot_turn` accepts and serializes this field into the queue entry, `_enqueue_for_recovery` in `autopilot.py` accepts and forwards `permissions` to `enqueue_copilot_turn`, and `_execute_async` in `processor.py` restores `entry.permissions` and passes it into `stream_chat_completion_sdk`/`stream_chat_completion_baseline` via `set_execution_context`. This ensures recovered sub-agent turns respect the same tool/block permission ceiling as the original in-process execution (mirroring `_merge_inherited_permissions`). Do NOT flag recovered turns as losing their permission ceiling — it is now fully propagated through the queue.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12814
File: autogpt_platform/backend/backend/copilot/model.py:0-0
Timestamp: 2026-04-16T13:28:28.641Z
Learning: In `autogpt_platform/backend/backend/copilot/model.py` (PR `#12814`, commit 259d37083): `append_and_save_message` uses `async with _get_session_lock(session_id)` — the same shared context manager used across the module — which internally acquires `redis-py`'s built-in `Lock` (key `copilot:session_lock:{session_id}`, timeout=10s, blocking_timeout=2s) via an atomic Lua-script. Lock release is also owner-verified via Lua so a slow pod can never delete a lock it no longer holds. On Redis failure the lock is skipped with a warning; the in-function idempotency check (`session.messages[-1].role` and `.content` comparison) still runs as a fallback. Do NOT expect a raw `redis.set(nx=True)` / `redis.delete()` pattern here — that intermediate approach was replaced in commit 259d37083.
📚 Learning: 2026-04-14T14:36:25.545Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/copilot/pending_messages.py:52-64
Timestamp: 2026-04-14T14:36:25.545Z
Learning: In `autogpt_platform/backend/backend/copilot` (PR `#12773`, commit d7bced0c6): when draining pending messages into `session.messages`, each message's text is sanitized via `strip_user_context_tags` before persistence to prevent user-controlled `<user_context>` injection from bypassing the trusted server-side context prefix. Additionally, if `upsert_chat_session` fails after draining, the drained `PendingMessage` objects are requeued back to Redis to avoid silent message loss. Do NOT flag the drain-then-requeue pattern as redundant — it is the intentional failure-resilience strategy for the pending buffer.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-11T08:40:59.673Z
Learnt from: kcze
Repo: Significant-Gravitas/AutoGPT PR: 12328
File: autogpt_platform/frontend/src/app/(platform)/copilot/useLoadMoreMessages.ts:49-61
Timestamp: 2026-03-11T08:40:59.673Z
Learning: In `autogpt_platform/frontend/src/app/(platform)/copilot/useLoadMoreMessages.ts`, clearing `olderMessages` (and resetting `oldestSequence`/`hasMore`) when `initialOldestSequence` shifts on the same session is intentional. Pages already fetched were based on a now-stale cursor; retaining them risks sequence gaps or duplicates. `ScrollPreserver` keeps the currently visible viewport intact, so only unvisited older pages are dropped. This is a deliberate safe-refetch design tradeoff.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-25T02:53:53.964Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-25T02:53:53.964Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (PR `#12918`, commit 6576bf561):
- `_flush_unresolved_tool_calls` was renamed to `flush_unresolved_tool_calls` (public); all call sites updated, `# noqa: SLF001` suppressor removed.
- `_flush_orphan_tool_uses_to_session` and `_InterruptedAttempt.finalize` both return `list[StreamBaseResponse]`; the post-loop caller yields those events directly to avoid double-flush and skipped UI cleanup events.
- The three former post-loop blocks (partial restore + redundant re-flush + two separate `yield StreamError` sites) are collapsed into a single block driven by `_classify_final_failure` returning a `_FinalFailure(display_msg, code, retryable)` dataclass, so history marker and SSE yield share one source of truth.
Do NOT flag double-flush risk or mismatched history/SSE marker as issues in the post-loop section of `stream_chat_completion_sdk`.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-30T03:25:37.606Z
Learnt from: CR
Repo: Significant-Gravitas/AutoGPT PR: 0
File: autogpt_platform/frontend/AGENTS.md:0-0
Timestamp: 2026-04-30T03:25:37.606Z
Learning: Applies to autogpt_platform/frontend/**/*.{ts,tsx} : Use function declarations (not arrow functions) for components/handlers
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-15T13:44:34.273Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12797
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1991-2021
Timestamp: 2026-04-15T13:44:34.273Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (`_run_stream_attempt`), the pre-create block (PR `#12797`) intentionally does NOT call `state.transcript_builder.append_assistant(...)` when inserting the empty assistant placeholder into `ctx.session.messages`. The transcript is left ending at the `tool_result` entry (N entries) while `message_count` metadata is N+1. This mismatch is benign and deliberate: on the next `--resume`, the SDK sees the transcript ending at `tool_result` and correctly regenerates the assistant response. Pre-appending the assistant turn to the transcript would suppress regeneration while leaving `session.messages[-1].content = ""` permanently (worse outcome). On the gap-fallback path, `transcript_msg_count (N+1) >= msg_count-1 (N)` means no gap is injected for the empty placeholder, which is correct because injecting an empty assistant message as context would mislead the SDK. Do NOT flag this transcript/message_count discrepancy as a bug.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-24T02:23:31.305Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12526
File: autogpt_platform/frontend/src/app/(platform)/copilot/components/RateLimitResetDialog/RateLimitResetDialog.tsx:0-0
Timestamp: 2026-03-24T02:23:31.305Z
Learning: In the Copilot platform UI code, follow the established Orval hook `onError` error-handling convention: first explicitly detect/handle `ApiError`, then read `error.response?.detail` (if present) as the primary message; if not available, fall back to `error.message`; and finally fall back to a generic string message. This convention should be used for generated Orval hooks even if the custom Orval mutator already maps details into `ApiError.message`, to keep consistency across hooks/components (e.g., `useCronSchedulerDialog.ts`, `useRunGraph.ts`, and rate-limit/reset flows).
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-01T18:54:16.035Z
Learnt from: Bentlybro
Repo: Significant-Gravitas/AutoGPT PR: 12633
File: autogpt_platform/frontend/src/app/(platform)/library/components/AgentFilterMenu/AgentFilterMenu.tsx:3-10
Timestamp: 2026-04-01T18:54:16.035Z
Learning: In the frontend, the legacy Select component at `@/components/__legacy__/ui/select` is an intentional, codebase-wide visual-consistency pattern. During code reviews, do not flag or block PRs merely for continuing to use this legacy Select. If a migration to the newer design-system Select is desired, bundle it into a single dedicated cleanup/migration PR that updates all Select usages together (e.g., avoid piecemeal replacements).
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-07T09:24:16.582Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12686
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/__tests__/PainPointsStep.test.tsx:1-19
Timestamp: 2026-04-07T09:24:16.582Z
Learning: In Significant-Gravitas/AutoGPT’s `autogpt_platform/frontend` (Vite + `vitejs/plugin-react` with the automatic JSX transform), do not flag usages of React types/components (e.g., `React.ReactNode`) in `.ts`/`.tsx` files as missing `React` imports. Since the React namespace is made available by the project’s TS/Vite setup, an explicit `import React from 'react'` or `import type { ReactNode } ...` is not required; only treat it as missing if typechecking (e.g., `pnpm types`) would actually fail.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-02T05:43:49.128Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12640
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/WelcomeStep.tsx:13-13
Timestamp: 2026-04-02T05:43:49.128Z
Learning: Do not flag `import { Question } from "phosphor-icons/react"` as an invalid import. `Question` is a valid named export from `phosphor-icons/react` (as reflected in the package’s generated `.d.ts` files and re-exports via `dist/index.d.ts`), so it should be treated as a supported named export during code reviews.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
- delete_stale_cli_session_file: drop exists() TOCTOU; catch
FileNotFoundError; log basename + strerror only on unexpected OSError.
- useCopilotPendingChips: unify bubble id across auto-continue and
mid-turn promotion paths via `bubbleIdFor(chip) = pending-chip-{uuid}`,
so a poll resolving after auto-continue already promoted the same chip
no longer renders it twice.
- usePeekOnBoundary: capture sessionId at request time and guard the
.then() callback against a stale response that resolves after the user
switched sessions (prevents old-session chips bleeding into the new
session).
|
Addressed all three CodeRabbit findings in
Tests still green (216 backend SDK + 29 focused frontend) and pre-commit checks ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts (1)
3-3: ⚡ Quick winRemove
useMemo/useCallbackhere to match frontend hook conventions.This introduces optimization hooks where the repo guideline says to avoid them unless explicitly requested.
Suggested fix
-import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; @@ - const queuedMessages = useMemo(() => chips.map((c) => c.text), [chips]); + const queuedMessages = chips.map((c) => c.text); @@ - const appendChip = useCallback((text: string) => { + function appendChip(text: string) { setChips((prev) => [...prev, { id: crypto.randomUUID(), text }]); - }, []); + }As per coding guidelines, “Do not use
useCallbackoruseMemounless asked to optimize a given function.”Also applies to: 58-61, 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@autogpt_platform/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts at line 3, The file's hook imports and usages of useMemo/useCallback should be removed to follow the repo convention: drop useMemo and useCallback from the import list and replace any memoized values and callbacks inside useCopilotPendingChips with plain functions/values (e.g., convert memoized selectors and callbacks at the locations noted—previously using useCallback/useMemo around the logic at the ~58-61 and ~81-83 areas—into regular functions or computed values inside the hook), ensuring you adjust any references and remove unnecessary dependency arrays; keep useRef/useState/useEffect as needed.
🤖 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/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts:
- Around line 284-286: pollBackendAndPromote may apply results from an old
session after an async wait; guard against session switches by checking that the
active session still matches before calling state setters. Specifically, after
awaiting pollBackendAndPromote (or inside pollBackendAndPromote before calling
setMessages/setChips), compare the original sessionId param with the current
session identifier (e.g., from the hook or getCurrentSession function) and bail
out if they differ so promoted chips/messages are not applied to a new session;
update the interval callback and any other async paths in useCopilotPendingChips
(including the block spanning the pollBackendAndPromote usage around lines
291–344) to perform this post-await session check.
---
Nitpick comments:
In
`@autogpt_platform/frontend/src/app/`(platform)/copilot/useCopilotPendingChips.ts:
- Line 3: The file's hook imports and usages of useMemo/useCallback should be
removed to follow the repo convention: drop useMemo and useCallback from the
import list and replace any memoized values and callbacks inside
useCopilotPendingChips with plain functions/values (e.g., convert memoized
selectors and callbacks at the locations noted—previously using
useCallback/useMemo around the logic at the ~58-61 and ~81-83 areas—into regular
functions or computed values inside the hook), ensuring you adjust any
references and remove unnecessary dependency arrays; keep
useRef/useState/useEffect as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4292532d-b157-4a05-ab85-0294ec755ae5
📒 Files selected for processing (2)
autogpt_platform/backend/backend/copilot/sdk/service.pyautogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
🚧 Files skipped from review as they are similar to previous changes (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). (13)
- GitHub Check: integration_test
- GitHub Check: lint
- GitHub Check: check API types
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: Analyze (python)
- GitHub Check: end-to-end tests
- GitHub Check: Analyze (typescript)
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (8)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Use Node.js 21+ with pnpm package manager for frontend development
Always run 'pnpm format' for formatting and linting code in frontend developmentFormat frontend code using
pnpm format
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Fully capitalize acronyms in symbols, e.g.graphID,useBackendAPI
No linter suppressors (//@ts-ignore``,// eslint-disable) — fix the actual issue
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{tsx,ts}: Use function declarations for components and handlers (not arrow functions) in React components
Only use arrow functions for small inline lambdas (map, filter, etc.) in React components
Use PascalCase for component names and camelCase with 'use' prefix for hook names in React
Use Tailwind CSS utilities only for styling in frontend components
Use design system components from 'src/components/' (atoms, molecules, organisms) in frontend development
Never use 'src/components/legacy/' in frontend code
Only use Phosphor Icons (@phosphor-icons/react) for icons in frontend components
Use generated API hooks from '@/app/api/generated/endpoints/' instead of deprecated 'BackendAPI' or 'src/lib/autogpt-server-api/'
Use React Query for server state (via generated hooks) in frontend development
Default to client components ('use client') in Next.js; only use server components for SEO or extreme TTFB needs
Use '' component for rendering errors in frontend UI; use toast notifications for mutation errors; use 'Sentry.captureException()' for manual exceptions
Separate render logic from data/behavior in React components; keep comments minimal (code should be self-documenting)
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx}: No barrel files or 'index.ts' re-exports in frontend code
Regenerate API hooks with 'pnpm generate:api' after backend OpenAPI spec changes in frontend development
autogpt_platform/frontend/**/*.{ts,tsx}: Use function declarations (not arrow functions) for components/handlers
Noanytypes unless the value genuinely can be anything
Keep render functions and hooks under ~50 lines; extract named helpers or sub-components when they grow longer
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
autogpt_platform/frontend/src/**/*.{ts,tsx}: Use generated API hooks from@/app/api/__generated__/endpoints/following the patternuse{Method}{Version}{OperationName}, and regenerate withpnpm generate:api
Separate render logic from business logic using component.tsx + useComponent.ts + helpers.ts pattern, colocate state when possible and avoid creating large components, use sub-components in local/componentsfolder
Use function declarations for components and handlers, use arrow functions only for callbacks
Do not useuseCallbackoruseMemounless asked to optimise a given function
autogpt_platform/frontend/src/**/*.{ts,tsx}: Keep files under ~200 lines; extract sub-components or hooks into their own files when a file grows beyond this
Use generated API hooks from@/app/api/__generated__/endpoints/with patternuse{Method}{Version}{OperationName}
Always import the-Icon-suffixed alias from@phosphor-icons/react(e.g.TrashIcon,PlusIcon,SquareIcon) — bare exports are deprecated
Do not useuseCallbackoruseMemounless asked to optimize a given function
Never usesrc/components/__legacy__/*— use design system components fromsrc/components/
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
No barrel files or
index.tsre-exports in the frontend
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not type hook returns, let Typescript infer as much as possible
autogpt_platform/frontend/src/**/*.ts: Extract component logic into custom hooks grouped by concern, not by component, with each hook in its own.tsfile
Do not type hook returns; let TypeScript infer as much as possible
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never type with
any, if no types available useunknown
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
autogpt_platform/frontend/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (autogpt_platform/frontend/AGENTS.md)
Avoid index and barrel files
Files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/copilot/pending_messages.py:52-64
Timestamp: 2026-04-14T14:36:25.545Z
Learning: In `autogpt_platform/backend/backend/copilot` (PR `#12773`, commit d7bced0c6): when draining pending messages into `session.messages`, each message's text is sanitized via `strip_user_context_tags` before persistence to prevent user-controlled `<user_context>` injection from bypassing the trusted server-side context prefix. Additionally, if `upsert_chat_session` fails after draining, the drained `PendingMessage` objects are requeued back to Redis to avoid silent message loss. Do NOT flag the drain-then-requeue pattern as redundant — it is the intentional failure-resilience strategy for the pending buffer.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-25T02:53:53.964Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (PR `#12918`, commit 6576bf561):
- `_flush_unresolved_tool_calls` was renamed to `flush_unresolved_tool_calls` (public); all call sites updated, `# noqa: SLF001` suppressor removed.
- `_flush_orphan_tool_uses_to_session` and `_InterruptedAttempt.finalize` both return `list[StreamBaseResponse]`; the post-loop caller yields those events directly to avoid double-flush and skipped UI cleanup events.
- The three former post-loop blocks (partial restore + redundant re-flush + two separate `yield StreamError` sites) are collapsed into a single block driven by `_classify_final_failure` returning a `_FinalFailure(display_msg, code, retryable)` dataclass, so history marker and SSE yield share one source of truth.
Do NOT flag double-flush risk or mismatched history/SSE marker as issues in the post-loop section of `stream_chat_completion_sdk`.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12445
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1071-1072
Timestamp: 2026-03-17T06:48:26.471Z
Learning: In Significant-Gravitas/AutoGPT (autogpt_platform), the AI SDK enforces `z.strictObject({type, errorText})` on SSE `StreamError` responses, so additional fields like `retryable: bool` cannot be added to `StreamError` or serialized via `to_sse()`. Instead, retry signaling for transient Anthropic API errors is done via the `COPILOT_RETRYABLE_ERROR_PREFIX` constant prepended to persisted session messages (in `ChatMessage.content`). The frontend detects retryable errors by checking `markerType === "retryable_error"` from `parseSpecialMarkers()` — no SSE schema changes and no string matching on error text. This pattern was established in PR `#12445`, commit 64d82797b.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12796
File: autogpt_platform/backend/backend/api/features/chat/routes.py:504-527
Timestamp: 2026-04-16T12:33:44.990Z
Learning: In `autogpt_platform/backend/backend/api/features/chat/routes.py`, `get_session` (PR `#12796`, commit 3771bfad9c1) closes the TOCTOU race between the initial `stream_registry.get_active_session()` pre-check and `get_chat_messages_paginated()` with a post-check re-verification: after the DB fetch, if `is_initial_load and active_session is not None`, it calls `get_active_session` a second time; if `post_active is None` (stream completed during the window), it resets `from_start=True`, `forward_paginated=True`, and re-fetches messages from sequence 0. Do NOT flag the double `get_active_session` call pattern as redundant — it is the intentional TOCTOU mitigation for pagination direction selection.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12797
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1991-2021
Timestamp: 2026-04-15T13:44:34.273Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (`_run_stream_attempt`), the pre-create block (PR `#12797`) intentionally does NOT call `state.transcript_builder.append_assistant(...)` when inserting the empty assistant placeholder into `ctx.session.messages`. The transcript is left ending at the `tool_result` entry (N entries) while `message_count` metadata is N+1. This mismatch is benign and deliberate: on the next `--resume`, the SDK sees the transcript ending at `tool_result` and correctly regenerates the assistant response. Pre-appending the assistant turn to the transcript would suppress regeneration while leaving `session.messages[-1].content = ""` permanently (worse outcome). On the gap-fallback path, `transcript_msg_count (N+1) >= msg_count-1 (N)` means no gap is injected for the empty placeholder, which is correct because injecting an empty assistant message as context would mislead the SDK. Do NOT flag this transcript/message_count discrepancy as a bug.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12814
File: autogpt_platform/backend/backend/copilot/model.py:0-0
Timestamp: 2026-04-16T13:28:28.641Z
Learning: In `autogpt_platform/backend/backend/copilot/model.py` (PR `#12814`, commit 259d37083): `append_and_save_message` uses `async with _get_session_lock(session_id)` — the same shared context manager used across the module — which internally acquires `redis-py`'s built-in `Lock` (key `copilot:session_lock:{session_id}`, timeout=10s, blocking_timeout=2s) via an atomic Lua-script. Lock release is also owner-verified via Lua so a slow pod can never delete a lock it no longer holds. On Redis failure the lock is skipped with a warning; the in-function idempotency check (`session.messages[-1].role` and `.content` comparison) still runs as a fallback. Do NOT expect a raw `redis.set(nx=True)` / `redis.delete()` pattern here — that intermediate approach was replaced in commit 259d37083.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12356
File: autogpt_platform/backend/backend/copilot/constants.py:9-12
Timestamp: 2026-03-10T08:39:22.025Z
Learning: In Significant-Gravitas/AutoGPT PR `#12356`, the `COPILOT_SYNTHETIC_ID_PREFIX = "copilot-"` check in `create_auto_approval_record` (human_review.py) is intentional and safe. The `graph_exec_id` passed to this function comes from server-side `PendingHumanReview` DB records (not from user input); the API only accepts `node_exec_id` from users. Synthetic `copilot-*` IDs are only ever created server-side in `run_block.py`. The prefix skip avoids a DB lookup for a `AgentGraphExecution` record that legitimately does not exist for CoPilot sessions, while `user_id` scoping is enforced at the auth layer and on the resulting auto-approval record.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-28T03:31:29.696Z
Learning: In Significant-Gravitas/AutoGPT PR `#12933` (`fix/stripe-checkout-link-auth-loop`), the initial approach of pinning `payment_method_types=["card"]` in `top_up_intent` and `create_subscription_checkout` (in `autogpt_platform/backend/backend/data/credit.py`) was reverted in commit `584b43a71` as it patched a symptom. The true root cause was in `update_subscription_tier()` in `v1.py`: a `current_tier_price_id is not None` guard was gating admin-granted DB-tier flips and short-circuiting them when the BUSINESS tier was pruned from the price-id LaunchDarkly flag. Do NOT flag `payment_method_types` absence in these checkout helpers as a Stripe Link bypass issue; the fix lives in the subscription tier update guard logic.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/blocks/autopilot.py:631-638
Timestamp: 2026-04-14T07:35:11.464Z
Learning: In `autogpt_platform/backend/backend/copilot/executor/utils.py`, `CoPilotExecutionEntry` includes a `permissions: CopilotPermissions | None` field (added in PR `#12773` / commit a0184c87b9). `enqueue_copilot_turn` accepts and serializes this field into the queue entry, `_enqueue_for_recovery` in `autopilot.py` accepts and forwards `permissions` to `enqueue_copilot_turn`, and `_execute_async` in `processor.py` restores `entry.permissions` and passes it into `stream_chat_completion_sdk`/`stream_chat_completion_baseline` via `set_execution_context`. This ensures recovered sub-agent turns respect the same tool/block permission ceiling as the original in-process execution (mirroring `_merge_inherited_permissions`). Do NOT flag recovered turns as losing their permission ceiling — it is now fully propagated through the queue.
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12873
File: autogpt_platform/backend/backend/copilot/baseline/reasoning.py:0-0
Timestamp: 2026-04-21T17:31:26.829Z
Learning: In `autogpt_platform/backend/backend/copilot/baseline/reasoning.py` (`BaselineReasoningEmitter`), when `render_in_ui=False`, BOTH the `StreamReasoning*` wire events AND the `ChatMessage(role="reasoning")` persistence append must be suppressed together. `convertChatSessionToUiMessages.ts` unconditionally re-renders all persisted `role="reasoning"` rows as `{type:"reasoning"}` UI parts on reload, so persisting rows while silencing live wire events would resurrect the reasoning collapse on page refresh. The audit trail is preserved through the provider transcript and `_format_sdk_content_blocks` (SDK path) instead. The baseline and SDK paths mirror each other: flag off → no live wire event, no persisted row, no hydrated collapse. This was established in PR `#12873`, commit 7ef10b26c.
📚 Learning: 2026-04-14T14:36:25.545Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/backend/backend/copilot/pending_messages.py:52-64
Timestamp: 2026-04-14T14:36:25.545Z
Learning: In `autogpt_platform/backend/backend/copilot` (PR `#12773`, commit d7bced0c6): when draining pending messages into `session.messages`, each message's text is sanitized via `strip_user_context_tags` before persistence to prevent user-controlled `<user_context>` injection from bypassing the trusted server-side context prefix. Additionally, if `upsert_chat_session` fails after draining, the drained `PendingMessage` objects are requeued back to Redis to avoid silent message loss. Do NOT flag the drain-then-requeue pattern as redundant — it is the intentional failure-resilience strategy for the pending buffer.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-11T08:40:59.673Z
Learnt from: kcze
Repo: Significant-Gravitas/AutoGPT PR: 12328
File: autogpt_platform/frontend/src/app/(platform)/copilot/useLoadMoreMessages.ts:49-61
Timestamp: 2026-03-11T08:40:59.673Z
Learning: In `autogpt_platform/frontend/src/app/(platform)/copilot/useLoadMoreMessages.ts`, clearing `olderMessages` (and resetting `oldestSequence`/`hasMore`) when `initialOldestSequence` shifts on the same session is intentional. Pages already fetched were based on a now-stale cursor; retaining them risks sequence gaps or duplicates. `ScrollPreserver` keeps the currently visible viewport intact, so only unvisited older pages are dropped. This is a deliberate safe-refetch design tradeoff.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-25T02:53:53.964Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 0
File: :0-0
Timestamp: 2026-04-25T02:53:53.964Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (PR `#12918`, commit 6576bf561):
- `_flush_unresolved_tool_calls` was renamed to `flush_unresolved_tool_calls` (public); all call sites updated, `# noqa: SLF001` suppressor removed.
- `_flush_orphan_tool_uses_to_session` and `_InterruptedAttempt.finalize` both return `list[StreamBaseResponse]`; the post-loop caller yields those events directly to avoid double-flush and skipped UI cleanup events.
- The three former post-loop blocks (partial restore + redundant re-flush + two separate `yield StreamError` sites) are collapsed into a single block driven by `_classify_final_failure` returning a `_FinalFailure(display_msg, code, retryable)` dataclass, so history marker and SSE yield share one source of truth.
Do NOT flag double-flush risk or mismatched history/SSE marker as issues in the post-loop section of `stream_chat_completion_sdk`.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-21T17:31:26.829Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12873
File: autogpt_platform/backend/backend/copilot/baseline/reasoning.py:0-0
Timestamp: 2026-04-21T17:31:26.829Z
Learning: In `autogpt_platform/backend/backend/copilot/baseline/reasoning.py` (`BaselineReasoningEmitter`), when `render_in_ui=False`, BOTH the `StreamReasoning*` wire events AND the `ChatMessage(role="reasoning")` persistence append must be suppressed together. `convertChatSessionToUiMessages.ts` unconditionally re-renders all persisted `role="reasoning"` rows as `{type:"reasoning"}` UI parts on reload, so persisting rows while silencing live wire events would resurrect the reasoning collapse on page refresh. The audit trail is preserved through the provider transcript and `_format_sdk_content_blocks` (SDK path) instead. The baseline and SDK paths mirror each other: flag off → no live wire event, no persisted row, no hydrated collapse. This was established in PR `#12873`, commit 7ef10b26c.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-17T06:48:26.471Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12445
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1071-1072
Timestamp: 2026-03-17T06:48:26.471Z
Learning: In Significant-Gravitas/AutoGPT (autogpt_platform), the AI SDK enforces `z.strictObject({type, errorText})` on SSE `StreamError` responses, so additional fields like `retryable: bool` cannot be added to `StreamError` or serialized via `to_sse()`. Instead, retry signaling for transient Anthropic API errors is done via the `COPILOT_RETRYABLE_ERROR_PREFIX` constant prepended to persisted session messages (in `ChatMessage.content`). The frontend detects retryable errors by checking `markerType === "retryable_error"` from `parseSpecialMarkers()` — no SSE schema changes and no string matching on error text. This pattern was established in PR `#12445`, commit 64d82797b.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-17T06:18:51.570Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12445
File: autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/ChatContainer.tsx:55-67
Timestamp: 2026-03-17T06:18:51.570Z
Learning: In `autogpt_platform/frontend/src/app/(platform)/copilot/components/ChatContainer/ChatContainer.tsx`, an explicit `isBusy` guard on the retry handler (`handleRetry`) is not needed. Once `onSend` is invoked, the chat status immediately transitions to "submitted", which causes the `ErrorCard` (containing the retry button) to unmount before a second click can register, making double-send impossible by design.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-13T13:11:00.401Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12764
File: autogpt_platform/frontend/src/app/(platform)/copilot/components/EmptySession/EmptySession.tsx:41-42
Timestamp: 2026-04-13T13:11:00.401Z
Learning: In Significant-Gravitas/AutoGPT `autogpt_platform/frontend`, unconditional React Query hook calls (e.g. `usePulseChips()` in `EmptySession.tsx`) are intentional when the underlying data is expected to be cached from prior page visits. The team considers the fetch cost acceptable in these cases and does not require `enabled` gating purely for feature-flag-disabled paths. Do not flag unconditional query hooks as wasteful when caching makes the cost negligible.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-15T13:44:34.273Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12797
File: autogpt_platform/backend/backend/copilot/sdk/service.py:1991-2021
Timestamp: 2026-04-15T13:44:34.273Z
Learning: In `autogpt_platform/backend/backend/copilot/sdk/service.py` (`_run_stream_attempt`), the pre-create block (PR `#12797`) intentionally does NOT call `state.transcript_builder.append_assistant(...)` when inserting the empty assistant placeholder into `ctx.session.messages`. The transcript is left ending at the `tool_result` entry (N entries) while `message_count` metadata is N+1. This mismatch is benign and deliberate: on the next `--resume`, the SDK sees the transcript ending at `tool_result` and correctly regenerates the assistant response. Pre-appending the assistant turn to the transcript would suppress regeneration while leaving `session.messages[-1].content = ""` permanently (worse outcome). On the gap-fallback path, `transcript_msg_count (N+1) >= msg_count-1 (N)` means no gap is injected for the empty placeholder, which is correct because injecting an empty assistant message as context would mislead the SDK. Do NOT flag this transcript/message_count discrepancy as a bug.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-10T08:39:22.025Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12356
File: autogpt_platform/backend/backend/copilot/constants.py:9-12
Timestamp: 2026-03-10T08:39:22.025Z
Learning: In Significant-Gravitas/AutoGPT PR `#12356`, the `COPILOT_SYNTHETIC_ID_PREFIX = "copilot-"` check in `create_auto_approval_record` (human_review.py) is intentional and safe. The `graph_exec_id` passed to this function comes from server-side `PendingHumanReview` DB records (not from user input); the API only accepts `node_exec_id` from users. Synthetic `copilot-*` IDs are only ever created server-side in `run_block.py`. The prefix skip avoids a DB lookup for a `AgentGraphExecution` record that legitimately does not exist for CoPilot sessions, while `user_id` scoping is enforced at the auth layer and on the resulting auto-approval record.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-03-24T02:23:31.305Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12526
File: autogpt_platform/frontend/src/app/(platform)/copilot/components/RateLimitResetDialog/RateLimitResetDialog.tsx:0-0
Timestamp: 2026-03-24T02:23:31.305Z
Learning: In the Copilot platform UI code, follow the established Orval hook `onError` error-handling convention: first explicitly detect/handle `ApiError`, then read `error.response?.detail` (if present) as the primary message; if not available, fall back to `error.message`; and finally fall back to a generic string message. This convention should be used for generated Orval hooks even if the custom Orval mutator already maps details into `ApiError.message`, to keep consistency across hooks/components (e.g., `useCronSchedulerDialog.ts`, `useRunGraph.ts`, and rate-limit/reset flows).
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-01T18:54:16.035Z
Learnt from: Bentlybro
Repo: Significant-Gravitas/AutoGPT PR: 12633
File: autogpt_platform/frontend/src/app/(platform)/library/components/AgentFilterMenu/AgentFilterMenu.tsx:3-10
Timestamp: 2026-04-01T18:54:16.035Z
Learning: In the frontend, the legacy Select component at `@/components/__legacy__/ui/select` is an intentional, codebase-wide visual-consistency pattern. During code reviews, do not flag or block PRs merely for continuing to use this legacy Select. If a migration to the newer design-system Select is desired, bundle it into a single dedicated cleanup/migration PR that updates all Select usages together (e.g., avoid piecemeal replacements).
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-07T09:24:16.582Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12686
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/__tests__/PainPointsStep.test.tsx:1-19
Timestamp: 2026-04-07T09:24:16.582Z
Learning: In Significant-Gravitas/AutoGPT’s `autogpt_platform/frontend` (Vite + `vitejs/plugin-react` with the automatic JSX transform), do not flag usages of React types/components (e.g., `React.ReactNode`) in `.ts`/`.tsx` files as missing `React` imports. Since the React namespace is made available by the project’s TS/Vite setup, an explicit `import React from 'react'` or `import type { ReactNode } ...` is not required; only treat it as missing if typechecking (e.g., `pnpm types`) would actually fail.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
📚 Learning: 2026-04-02T05:43:49.128Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12640
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/WelcomeStep.tsx:13-13
Timestamp: 2026-04-02T05:43:49.128Z
Learning: Do not flag `import { Question } from "phosphor-icons/react"` as an invalid import. `Question` is a valid named export from `phosphor-icons/react` (as reflected in the package’s generated `.d.ts` files and re-exports via `dist/index.d.ts`), so it should be treated as a supported named export during code reviews.
Applied to files:
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts
🔇 Additional comments (1)
autogpt_platform/frontend/src/app/(platform)/copilot/useCopilotPendingChips.ts (1)
228-234: Deterministic chip bubble IDs and dedup filtering look solid.Using
pending-chip-${chip.id}across both promotion paths cleanly prevents duplicate renders when effects re-run or poll ordering varies.Also applies to: 244-251, 328-334
/pr-test --fix resultTL;DRPASS on unit/integration coverage, UI smoke test BLOCKED by an unrelated native-backend startup bug ( What was actually verified
How the harder scenarios would be triggered synthetically (UI not run)These weren't executable today because the native backend won't start. If you re-run when the multiprocess bug is fixed: Scenario 1 (disappearing chips, live): open the copilot, send a message that triggers a slow tool call (e.g. Scenario 2 (prompt-too-long, live): simplest synthetic trigger is Alternative: pre-populate Scenario 3 (double error UI, live): force a transient backend error (kill the backend mid-stream, or block the upstream Anthropic API at the firewall). Observe the in-line ErrorCard bubble appears and the trailing red banner does NOT (was: both appeared with identical text). What blocked the live UI runThe native |
Mirror the request-time-sessionId pattern from usePeekOnBoundary into useMidTurnDrainPromotion: capture sessionId at request time, compare to a live ref on resolve, bail if the user switched sessions while the GET was in flight. Without this, a slow peek for session A could promote chips into session B's message list after a switch. Cancellation-flag was tried first but is too broad — this effect re-runs on every chip-append, which would wrongly invalidate an in-flight poll for the same session. The sessionId comparison only invalidates on actual session changes, preserving the chip-append-during-poll race guarantee. Added a regression test that holds a peek in flight, switches sessions mid-resolve, and asserts no promotion fires on the old session's setMessages.
Self-review: the projects-base guard was returning silently. Mirror the warn-shape from `_write_cli_session_to_disk` so an out-of-base resolve surfaces as a Sentry-visible warning. Unreachable in normal operation (server-generated UUID + deterministic `cli_session_path`), but a hit would indicate a config or tampering issue worth seeing.
`self._active_tasks_lock = threading.Lock()` in `__init__` (added in #12877 to make `_cleanup_completed_tasks` thread-safe) holds a `_thread.lock` that the forkserver/spawn start method cannot serialize. With it set eagerly, `Process(target=self.execute_run_command).start()` from `AppProcess.start()` raises `TypeError: cannot pickle '_thread.lock' object` and `poetry run app` aborts at startup before the REST server binds. Move the lock to a lazy `@property _active_tasks_lock` so the parent process never holds a real `threading.Lock` instance — the lock is materialized inside the forked child the first time `_cleanup_completed_tasks` runs, where pickling is no longer in play. This mirrors the existing lazy-init pattern already used for the ThreadPoolExecutor, RabbitMQ clients, and consumer threads in this class.
/pr-test --fix result (native stack)Both UI smoke scenarios PASSED end-to-end after diagnosing + fixing the unrelated native-bring-up blocker (kept on this PR — under 20% scope, same module). Stack still running on :3000 / :8006. Native bring-up
Pickle blocker root-cause + fix
Fix: convert to a lazy Smoke tests
Synthetic-trigger plan (for future re-verification of scenario 3)When you want to live-validate the prompt-too-long persistence fix:
Screenshots
Lock stateReleased — |
…12951) ## Why `/pr-polish` was prematurely emitting `CLEAN-POLL` while CI was still pending, because the polish-polling loop's CI gate parsed `gh pr checks $PR` text columns with `awk '{print $2}'`. That works fine for plain job names, but breaks on jobs with spaces or parens like `test (3.11)`, `Analyze (python)`, where column 2 is the version `(3.11)` — so `grep -q "pending"` matched on column 2 of OTHER rows but missed the actual pending entries. Real symptom on PR #12948: the orchestrator reported `ORCHESTRATOR:DONE` while `test (3.11/3.12/3.13)` and `Check PR Status` were still running. ## What Add a "Concrete CI fetch" subsection right after the polish-polling pseudocode block, showing the `--json bucket` shape that bypasses the column-parsing trap entirely. Also flag the `bucket` vs `conclusion` gotcha (the REST API uses `conclusion`; `gh pr checks --json` only exposes `bucket`). ## How Surgical additive edit — the existing pseudocode + state machine is preserved; the new subsection just translates the abstract `fetch_check_runs(PR)` into a concrete one-liner so the next implementer doesn't reach for `awk` again. ## Test plan - [x] Verified the regression against PR #12948: bucket-based polling correctly identified 4 pending checks the awk path missed - [x] Confirmed `gh pr checks {N} --json conclusion` errors with `Unknown JSON field: "conclusion"` (this gotcha is now noted in the skill)
Live UI verification - PR #12948 dedup fix on dev preview
Per-scenario resultsBackend log excerpt for the PK-collision short-circuitThat Headline: PASS on all 5 scenarios. |
…ors-and-queue-bubbles
User report: chip appears, disappears mid-turn, then shows up "merged as previous chat" only after the turn ends. Root cause is ``usePeekOnBoundary``'s idle / turn-starting branches: both unconditionally rebase ``chips`` to the server's pending-messages snapshot. A chip queued AFTER the peek GET fires but BEFORE it resolves is silently overwritten because the server's response doesn't yet include it. Fix: capture the id-set of chips that were in-flight to the server at GET-fire time (read via ``setChips`` state-getter). On resolve, the server snapshot rebases those chips, and any chip in local state NOT in the snapshot is re-attached — that's the user's queued-during-window send. The same protection is applied to the turn-starting branch (was ``setChips(() => [])`` on count==0; now filters by in-flight ids).
…eue, setChips→setQueue Internal vocabulary inside ``useCopilotPendingChips`` was the leftover UI term ``chip`` while the public API has always been ``queuedMessages``. Renamed the type, state variable, setter and loop variables to match — ``QueuedMessage[]`` / ``queue`` / ``setQueue`` / ``entry`` — so a fresh reader doesn't need UI context to track the data flow. Public API (``queuedMessages: string[]``, ``appendChip``) and the file name kept as-is to avoid touching consumers + tests + the file system. Those can come in a follow-up rename.
Public API of ``useCopilotPendingChips`` was the leftover UI vocabulary ``appendChip``. Renamed to ``queueMessage`` so the verb-form matches the existing read API ``queuedMessages: string[]``. Touches 6 files: the hook + 2 consumers (useCopilotPage, useBuilderChatPanel) + 3 test files. Hook + file name still ``useCopilotPendingChips`` — that rename is wider blast radius and should be its own change.
User-reported bug: a queued chip drained mid-turn would render as a bubble during streaming, then "merge as previous chat" once the turn ended — the follow-up user row vanished from the visible feed. Root cause: ``concatWithAssistantMerge`` blindly merged two assistant UIMessages at the page boundary whenever both ends were ``role: "assistant"``. In a hydration-race window where the user/reasoning row between two assistant DB rows was not yet visible in either page, the stitch silently swallowed the missing row. Fix: extract the trailing ``-seq-N`` from each id and only merge when ``firstSeq === lastSeq + 1``. Streaming-path ids (AI SDK uuids) and idx-fallback ids fail extraction and refuse the merge — that's the safer default since the streaming consumer handles its own assistant continuity inside the active turn. Adds 6 regression tests for ``concatWithAssistantMerge`` including the exact "seq3 + (missing seq4) + seq6" repro.
…djacency Sentry follow-up to ed0d748 (concat-merge adjacency gate): the in-page ``convertChatSessionMessagesToUiMessages`` merges consecutive assistant + reasoning DB rows into one UIMessage but kept the ``id`` of only the FIRST row in the group. ``concatWithAssistantMerge`` then extracted that first-seq from the id, and a valid cross-page merge between (seq 5+6 in page A) and seq 7 in page B failed the ``firstSeq === lastSeq + 1`` check (7 !== 5+1), splitting an ongoing turn into two bubbles. Fix: when merging in-page, advance the ``prevUI.id`` to the new row's seq, and migrate the stats key from the old id to the new one so ``durationMs`` / ``createdAt`` patches still land on the right key. Now the merged bubble's id reflects the LAST seq it contains and the adjacency check works across page boundaries. Adds regression test for the Sentry-reported scenario.
User-reported: queueing a chip mid-turn rendered correctly during streaming, then "merged as previous chat" once the turn ended — the chip's text appeared joined into the original send's bubble with a ``\n\n`` separator, not as its own bubble. Root cause: at turn-start, both the SDK and baseline services combined the routes.py-saved current user row + drained pending into one ``\n\n``-joined string and wrote that back to the *existing* user row via ``update_message_content_by_sequence``. Result: one DB row, one bubble, the chip's cardinality lost forever. Fix: persist each pending message as its own user row in the DB. The combined string is still passed to the model as the current-turn input (so the model sees the same context as before), but the DB now has one row per click and the UI renders distinct bubbles. Implementation: extend the existing ``persist_pending_as_user_rows`` helper to accept ``transcript_builder=None`` for the turn-start case. With ``None``, the helper writes only to ``session.messages`` and the DB — the combined ``current_message`` carries the texts into the transcript at turn-end via the existing ``append_user`` call, so we avoid triple-counting pending entries in the next turn's ``--resume`` context. Adds regression test for the ``None`` path.
Follow-up to 2c05345: persisting pending as separate user rows *before* ``inject_user_context`` made the helper target the wrong row. ``inject_user_context`` walks ``session.messages`` in reverse to find the "current turn's user message" and rewrites its content with ``<memory_context>`` / ``<user_context>`` envelopes + the combined turn text. When the pending rows were appended first, that reverse- walk landed on the last pending row instead of the routes.py-saved row, scrambling per-bubble content: every pending bubble rendered the entire combined-and-wrapped block. Fix: keep the combine for the model prompt, run inject as before (targets the routes.py-saved row), THEN persist pending as new rows at sequences after that row. Each pending bubble now carries its own clean text. Confirmed in dev session 3e148740-… where seq=1 was previously the full wrapped+combined string; with this ordering, the routes.py row keeps its envelopes and pending rows hold their raw chip text.
Follow-up to 2a4fc40: even after moving ``persist_pending_as_user_rows`` after ``inject_user_context``, the bubble for the routes.py-saved row still rendered the COMBINED text because we'd combined first and passed that to inject — the wrapped+combined string ended up persisted on the original row, then the chip's raw text *also* got its own row, so the chip's content appeared twice in the UI. Fix: don't combine until *after* inject runs. inject targets the routes.py-saved row and wraps the ORIGINAL turn-starting text alone. Then combine for the model's current-turn prompt and persist each pending message as its own raw-text user row. Two clean bubbles: the original (with envelopes stripped by markdown) and the chip (raw). No duplication. Baseline path: same reorder, plus append each pending as a separate user entry on ``openai_messages`` so the model's chat-completion call this round sees them — matching the mid-turn drain pattern below.
User repro: chip queued during a turn that finished its turn-start drain BEFORE the frontend's first peek arrived would silently disappear from the chip strip without ever rendering as a user bubble — the bubble only reappeared once hydration replaced in-memory messages with DB rows after the turn fully ended. Cause: the turn-start branch in ``usePeekOnBoundary`` cleared chips that were in-flight at GET time (because the backend already drained them) but didn't promote them to bubbles first. ``useMidTurnDrainPromotion``'s poll wouldn't fire either, since its effect bails out when ``chips`` is empty. Fix: extract ``promoteChipsToTrailingBubbles`` (same insertion shape as the mid-turn poll's promote) and call it from the turn-start branch BEFORE removing the drained chips from local state. The bubble is now visible during streaming, matching the mid-turn behaviour.
|
Chip-queue fix verified on dev preview (deploy Session: Repro (jumbled scenario):
Results
Key signal: bubbles appeared DURING streaming as soon as backend drained each chip - confirmed via DB verification ( Chip rows are raw text only - no envelopes, no Screenshots saved locally at LGTM - ready to merge. |
…lback Sentry caught: the new turn-start ``persist_pending_as_user_rows`` call appends each pending entry to ``openai_messages`` before the persist, but didn't pass an ``on_rollback`` callback. If the persist fails and re-queues the pending into Redis for the next turn, the appended entries would stay in ``openai_messages`` for THIS turn AND re-arrive next turn, duplicating in the model's context. Mirror the mid-turn drain pattern: capture an ``_turn_start_openai_anchor`` before appending and pass an ``on_rollback`` that trims back to the anchor.
…lication Sentry caught: at turn-start drain, ``current_message`` was combined with pending BEFORE ``persist_pending_as_user_rows`` ran. If the helper rolled back (re-queueing pending into Redis), this turn's combined ``current_message`` still went to the model, AND next turn's drain would re-combine the same pending — chips appeared in the model's context across two consecutive turns. Fix in both SDK + baseline turn-start paths: persist FIRST, then only fold pending into the model's prompt + ``openai_messages`` if persist returned ``True``. On rollback, ``current_message`` / ``message`` stay as the original turn-starting send, so this turn sends just the original to the model and the re-queued chips arrive cleanly on the next turn's drain — no double-counting. Also drops the now-unused ``_trim_openai_on_turn_start_rollback`` callback in baseline (the gate replaces it: we never append to ``openai_messages`` if persist fails).
…12951) ## Why `/pr-polish` was prematurely emitting `CLEAN-POLL` while CI was still pending, because the polish-polling loop's CI gate parsed `gh pr checks $PR` text columns with `awk '{print $2}'`. That works fine for plain job names, but breaks on jobs with spaces or parens like `test (3.11)`, `Analyze (python)`, where column 2 is the version `(3.11)` — so `grep -q "pending"` matched on column 2 of OTHER rows but missed the actual pending entries. Real symptom on PR #12948: the orchestrator reported `ORCHESTRATOR:DONE` while `test (3.11/3.12/3.13)` and `Check PR Status` were still running. ## What Add a "Concrete CI fetch" subsection right after the polish-polling pseudocode block, showing the `--json bucket` shape that bypasses the column-parsing trap entirely. Also flag the `bucket` vs `conclusion` gotcha (the REST API uses `conclusion`; `gh pr checks --json` only exposes `bucket`). ## How Surgical additive edit — the existing pseudocode + state machine is preserved; the new subsection just translates the abstract `fetch_check_runs(PR)` into a concrete one-liner so the next implementer doesn't reach for `awk` again. ## Test plan - [x] Verified the regression against PR #12948: bucket-based polling correctly identified 4 pending checks the awk path missed - [x] Confirmed `gh pr checks {N} --json conclusion` errors with `Unknown JSON field: "conclusion"` (this gotcha is now noted in the skill)
…compaction, errors, chips) (#12948) ## Why Bundled bug-fix PR for the copilot chat stream surface. Multiple user-reported regressions on dev (`chat-mode-option` LD flag) — duplicate sends persisting two user rows on the same session, "prompt too long" recurring on long sessions (SENTRY-1207), assistant turn double-error UI, lost queued chips on the in-flight poll, SDK-resume rescuing context but the post-turn upload throwing it away, SDK-mode worker crashes on lazy-init lock race, compaction failures sending the same too-long payload back into the retry loop. All share the chat stream code path so one PR. ## What ### Atomic dedup at Postgres `ChatMessage.id` The duplicate-send loophole (RMQ redelivery, browser/CDN retries, refresh+retype) is closed at the database layer: - Frontend transport (`prepareSendMessagesRequest`) generates `crypto.randomUUID()` per logical send — stable across SDK-internal retries because the prepared body is reused. - Backend `StreamChatRequest.message_id` becomes `ChatMessage.id` on insert. - Postgres' PK uniqueness constraint catches duplicate inserts. `append_and_save_message` distinguishes `ChatMessage_pkey` (dedup signal → return None, route subscribes to existing turn) from `ChatMessage_sessionId_sequence_key` (sequence race, retried internally) from other failures. - Optimistic in-memory append is rolled back on **any** save-failure path, not just PK collision. - No new column, no Redis claim store — the existing `@id @default(uuid())` PK is the atomic primitive. ### Race-safe HTTP handler Dropped the redundant second `is_turn_in_flight` check in `routes.py` that introduced a TOCTOU window where a concurrent turn could leave a user message saved to the DB but never enqueued. The first check at the top of the handler already routes the in-flight branch to `queue_pending_for_http`; anything past that point starts a fresh turn. ### Race-safe queued-message chips `useCopilotPendingChips` chips now carry frontend-only UUIDs (`{id, text}[]` instead of indexed `string[]`). Mid-turn poll's drain promotion uses a functional updater that filters by id, so chips enqueued during the in-flight `getV2GetPendingMessages` GET aren't overwritten by the stale snapshot's setState. One bubble per chip, preserving identity for the `useHydrateOnStreamEnd` substring match. ### Persist context-error retry recovery T2+ retry in `sdk/service.py` was dropping `session_id` to dodge "Session ID already in use", so the recovery CLI wrote to a random path while the post-turn upload silently grabbed the stale pre-failure file at the predictable `cli_session_path`. The rescued (compacted) transcript was thrown away every time, and the next turn `--resume`d the same bloated GCS copy. New helper `delete_stale_cli_session_file` clears the local file before the retry; `session_id` is preserved so the recovery write lands on the predictable path. ### Compression-failure fallback When `_compress_messages` fails (LLM summarize + truncate fallback both error), return `[], True` (drop history, mark compacted) instead of the originals. The originals would guarantee another `Prompt is too long` on retry — burning the retry budget for zero progress. Bare current message is the tightest possible compression without an LLM. ### Dedup error UI Backend appends a `COPILOT_ERROR_PREFIX` marker to `session.messages` AND yields a `StreamError` SSE event on every final-failure path. Frontend rendered both — same `failure.display_msg`, twice. New top-level `lastAssistantHasErrorMarker` memo gates the trailing red banner. ### Empty-tool-call circuit breaker exclusion No-arg tools (e.g. `get_agent_building_guide`) were tripping the breaker because their tool-call payload is genuinely empty. Excluded via `_no_arg_tool_names()`. ### Executor lock + pickle fixes - `CoPilotExecutor` was raising `TypeError: cannot pickle '_thread.lock'` on `forkserver` start. Lock is now lazy-property-backed so it's not bound to the parent process state. - The lazy-property pattern then introduced a TOCTOU race that Sentry caught — fixed by materialising the lock pre-fork in `run()`, before workers spawn. - Foreground execution (no detached process) is restored so the helm chart's terminationGracePeriodSeconds applies cleanly to in-flight turns. ### SSR-safe persist storage `copilotStreamStore` `persist` middleware factory now returns a no-op `Storage` stub when `window` is undefined (Next.js SSR / vitest), instead of `undefined!`. Browser path is unchanged: still uses `window.sessionStorage`. ## How - `idempotency_key` field on `StreamChatRequest` was the original Stripe-style design but reverted to using `ChatMessage.id` directly — Postgres PK is the simpler authoritative dedup. - Transport-tier integration test (`copilotStreamTransport.test.ts`) added to plug the gap that allowed the AI SDK `messageId` regression (replace-mode semantics, broke optimistic render) to slip past unit tests. - `delete_stale_cli_session_file` reuses the same `projects_base()` traversal guard as `read_cli_session_from_disk`. - `useCopilotPendingChips` keeps its public API (`queuedMessages: string[]`, `appendChip(text)`) — only internal state shape changed. ## Test plan - [x] Backend pytest — `model_test`, `routes_test`, `executor` (utils, processor, manager), `sdk` (service_helpers, retry_scenarios, prompt_too_long, session_persistence, context_fallback): 154+216 passed - [x] Frontend vitest — `useCopilotPendingChips`, `ChatMessagesContainer`, `useSendMessage`, `copilotStreamTransport` (new), `copilotStreamStore`: all green - [x] `pnpm types`, `pnpm lint`, `pnpm format` clean - [x] `poetry run ruff format` + `black` on changed backend files clean - [x] /pr-test --fix locally (native), 2 independent runs on different HEADs: PASS — concurrent identical-`message_id` POSTs subscribe-only, distinct clicks fresh turn, DB row counts match - [x] /pr-test on dev preview: PASS — 5 scenarios with screenshots, [comment 4353117872](#12948 (comment)) - [x] Sentry threads addressed: 4 fixed (TOCTOU race HIGH, SSR storage HIGH, optimistic-pop MEDIUM, ack-on-success), 1 documented false positive






Why
Bundled bug-fix PR for the copilot chat stream surface. Multiple user-reported regressions on dev (
chat-mode-optionLD flag) — duplicate sends persisting two user rows on the same session, "prompt too long" recurring on long sessions (SENTRY-1207), assistant turn double-error UI, lost queued chips on the in-flight poll, SDK-resume rescuing context but the post-turn upload throwing it away, SDK-mode worker crashes on lazy-init lock race, compaction failures sending the same too-long payload back into the retry loop. All share the chat stream code path so one PR.What
Atomic dedup at Postgres
ChatMessage.idThe duplicate-send loophole (RMQ redelivery, browser/CDN retries, refresh+retype) is closed at the database layer:
prepareSendMessagesRequest) generatescrypto.randomUUID()per logical send — stable across SDK-internal retries because the prepared body is reused.StreamChatRequest.message_idbecomesChatMessage.idon insert.append_and_save_messagedistinguishesChatMessage_pkey(dedup signal → return None, route subscribes to existing turn) fromChatMessage_sessionId_sequence_key(sequence race, retried internally) from other failures.@id @default(uuid())PK is the atomic primitive.Race-safe HTTP handler
Dropped the redundant second
is_turn_in_flightcheck inroutes.pythat introduced a TOCTOU window where a concurrent turn could leave a user message saved to the DB but never enqueued. The first check at the top of the handler already routes the in-flight branch toqueue_pending_for_http; anything past that point starts a fresh turn.Race-safe queued-message chips
useCopilotPendingChipschips now carry frontend-only UUIDs ({id, text}[]instead of indexedstring[]). Mid-turn poll's drain promotion uses a functional updater that filters by id, so chips enqueued during the in-flightgetV2GetPendingMessagesGET aren't overwritten by the stale snapshot's setState. One bubble per chip, preserving identity for theuseHydrateOnStreamEndsubstring match.Persist context-error retry recovery
T2+ retry in
sdk/service.pywas droppingsession_idto dodge "Session ID already in use", so the recovery CLI wrote to a random path while the post-turn upload silently grabbed the stale pre-failure file at the predictablecli_session_path. The rescued (compacted) transcript was thrown away every time, and the next turn--resumed the same bloated GCS copy. New helperdelete_stale_cli_session_fileclears the local file before the retry;session_idis preserved so the recovery write lands on the predictable path.Compression-failure fallback
When
_compress_messagesfails (LLM summarize + truncate fallback both error), return[], True(drop history, mark compacted) instead of the originals. The originals would guarantee anotherPrompt is too longon retry — burning the retry budget for zero progress. Bare current message is the tightest possible compression without an LLM.Dedup error UI
Backend appends a
COPILOT_ERROR_PREFIXmarker tosession.messagesAND yields aStreamErrorSSE event on every final-failure path. Frontend rendered both — samefailure.display_msg, twice. New top-levellastAssistantHasErrorMarkermemo gates the trailing red banner.Empty-tool-call circuit breaker exclusion
No-arg tools (e.g.
get_agent_building_guide) were tripping the breaker because their tool-call payload is genuinely empty. Excluded via_no_arg_tool_names().Executor lock + pickle fixes
CoPilotExecutorwas raisingTypeError: cannot pickle '_thread.lock'onforkserverstart. Lock is now lazy-property-backed so it's not bound to the parent process state.run(), before workers spawn.SSR-safe persist storage
copilotStreamStorepersistmiddleware factory now returns a no-opStoragestub whenwindowis undefined (Next.js SSR / vitest), instead ofundefined!. Browser path is unchanged: still useswindow.sessionStorage.How
idempotency_keyfield onStreamChatRequestwas the original Stripe-style design but reverted to usingChatMessage.iddirectly — Postgres PK is the simpler authoritative dedup.copilotStreamTransport.test.ts) added to plug the gap that allowed the AI SDKmessageIdregression (replace-mode semantics, broke optimistic render) to slip past unit tests.delete_stale_cli_session_filereuses the sameprojects_base()traversal guard asread_cli_session_from_disk.useCopilotPendingChipskeeps its public API (queuedMessages: string[],appendChip(text)) — only internal state shape changed.Test plan
model_test,routes_test,executor(utils, processor, manager),sdk(service_helpers, retry_scenarios, prompt_too_long, session_persistence, context_fallback): 154+216 passeduseCopilotPendingChips,ChatMessagesContainer,useSendMessage,copilotStreamTransport(new),copilotStreamStore: all greenpnpm types,pnpm lint,pnpm formatcleanpoetry run ruff format+blackon changed backend files cleanmessage_idPOSTs subscribe-only, distinct clicks fresh turn, DB row counts match