Repository navigation
Keep mid-turn user exchanges visible after a turn completes - #1657
Conversation
| const isUserInputBoundary = isTimelineUserInputBoundaryMessage(message); | ||
| const isMidTurnUserInput = | ||
| isUserInputBoundary && | ||
| (items.length > 0 || groupedMessages.length > 0); |
There was a problem hiding this comment.
🚨 slopcop/review — This position check treats a second initial input group as mid-turn input.
A new-turn request can create several initial user rows through inputGroups. The second row makes items.length positive.
This arms both flags for an ordinary turn. It can expose the first and penultimate assistant messages after completion.
Please require turnRequest.kind === "steer" for user boundaries. Keep answered questions and external boundaries as separate cases.
Add an unsteered inputGroups test. It must expect only the terminal assistant row at the top level.
| itemId: "a1", | ||
| text: "Audit complete. Which changes should I include?", | ||
| }), | ||
| answeredUserQuestionEvent(6), |
There was a problem hiding this comment.
🚨 slopcop/review — The question fixture creates two events with the same sequence.
answeredUserQuestionEvent(6) uses sequence 6. The next factory event also uses sequence 6.
Persisted events require unique, increasing sequence values. This fixture also skips the pending-to-resolved merge used in production.
Please add a typed factory method. Emit pending and resolved events through the factory.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This change keeps mid-turn user replies, nearby assistant replies, and answered questions visible after a turn ends.
The goal is correct. The current input check also changes ordinary turns that contain several starting input groups.
Findings
-
Blocking correctness issue: A second initial
inputGroupsrow looks like mid-turn input. This can expose interim assistant rows in an ordinary turn. See the inline comment. -
Test defect: The question test creates two events with sequence 6. It also skips the real pending-to-resolved question lifecycle. See the inline comment.
Other checks
- I found no security issue.
- The grouping pass stays O(n).
- Many answered questions can exceed the 200-row cache limit. This is a non-blocking performance risk.
- The completed-turn module is the correct policy layer.
- I found no duplicate helper for this rule.
- The existing
turnRequest.kindfield gives the required semantic distinction. - GitHub build, type, lint, package, app, server, and integration checks passed.
- An unrelated plugin SDK version guard caused the aggregate check failure.
- I opened the trusted base app and viewed a local steer flow in a headless browser.
- I did not run the fork code. Therefore, the browser smoke test did not verify this pull request head.
Please fix both inline findings before merge. I left a comment-only review, as required.
Completed-turn grouping buried the direct reply to a mid-turn user message, interim assistant messages in the final segment, and answered user questions inside the collapsed turn summary. Preserve the first assistant/error message after user input, preserve the final segment's last terminal message, and treat answered user questions as user-input boundaries.
The turn-starting user row lives inside turn.messages, so treating every user-input boundary as preservation-worthy surfaced the first reply and the penultimate message of ordinary unsteered turns, defeating the completed-turn fold. Gate preservation on the boundary having prior messages in the turn, and only preserve the final segment's terminal when a mid-turn boundary actually occurred.
…ixture Address SlopCop review on get-bb#1657: - A grouped `inputGroups` new-turn request projects several initial user rows. The position check (`items.length > 0`) treated the second row as mid-turn input and armed the preserve flags for an ordinary turn, exposing its interim assistant messages after completion. Track whether the turn has produced any assistant/tool output and only classify user input that arrives after output as mid-turn. - The answered-question fixture hand-built a single resolved event whose seq collided with the next factory event and skipped the pending-to-resolved lifecycle. Add `userQuestionLifecycle` to the timeline event factory and emit pending + resolved events through it. - Add a regression test: an unsteered turn with two initial input groups keeps only the terminal assistant row at the top level. Co-Authored-By: Claude <noreply@anthropic.com>
544c163 to
b106134
Compare
|
Addressed both SlopCop findings (rebased onto
|
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this change for security, code quality, performance, architecture, and user-visible behavior. |
| if (message.kind === "assistant-text") { | ||
| return message.isLegacyUserMessage === true; | ||
| } | ||
| if (message.kind === "user-question-lifecycle") { |
There was a problem hiding this comment.
🚨 slopcop/review — The conversation outline lacks question lifecycle rows
This new grouping rule needs system/userQuestion/lifecycle events. The full conversation-outline query does not load that type, so its projection can fold assistant rows that the main timeline exposes. Add the lifecycle type to listStoredConversationOutlineEventRows. Add a server test that compares outline and timeline conversation rows for an answered question.
| message: EventProjectionMessage, | ||
| ): boolean { | ||
| if (message.kind === "user") { | ||
| return message.initiator === "user"; |
There was a problem hiding this comment.
🚨 slopcop/review — Rejected steers activate the preservation policy
This predicate accepts every user-initiated row. A rejected human steer also returns true. The provider never received that input, but later unrelated assistant rows can become visible. Require an accepted user steer here. Add a completed-turn test with a rejected steer.
| message.kind === "user" && message.initiator === "user", | ||
| ); | ||
| const isUserInputBoundary = isTimelineUserInputBoundaryMessage(message); | ||
| const isMidTurnUserInput = isUserInputBoundary && sawTurnOutput; |
There was a problem hiding this comment.
🚨 slopcop/review — A first-output question does not start reply preservation
When an answered question is the first provider output, sawTurnOutput is still false. The answer does not become a mid-turn boundary. A direct acknowledgment can then fold into the summary if more assistant output follows. Treat an answered question as both provider output and user input. Add a test with the question before all tool and assistant rows, plus two later assistant messages.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change keeps a live user exchange visible after the turn completes. It keeps the follow-up, direct reply, and final segment reply outside the folded work summary.
I found three medium correctness issues:
- An answered question does not become a boundary when it is the first provider output. A later direct reply can disappear into the summary.
- A rejected user steer also becomes a boundary. It can expose assistant replies that do not answer accepted user input.
- The conversation-outline query omits question lifecycle events. Its rows can differ from the main timeline after an answered question.
The display policy also has two boundary definitions. A typed shared classification could cover the input source, request status, and turn phase. The test fixture repeats an existing question builder. The new as never cast is also unnecessary.
I found no security issue. The extra visible rows can increase response and cache sizes, but existing limits keep this risk low.
Verification passed:
- All 382
@bb/thread-viewtests passed across 22 files. - The
@bb/thread-viewtype check passed. - A live Codex turn received a steer and completed in the dev app. Chromium showed the stored follow-up and assistant acknowledgment after completion.
- A separate GPT-5.6 review gate confirmed the three correctness findings.
This is a comment-only review. I did not approve the pull request or request changes.
…stions Address the second SlopCop round on get-bb#1657: - An answered user question that is the turn's first provider output now starts reply preservation. Questions are provider-initiated, so the "after output" rule for user requests does not apply to them; the answered question also counts as turn output for later requests. - Only accepted user requests are boundaries. Rejected (or still pending) steers never reached the provider, so the assistant rows after them no longer get exposed. - Both rules live in one typed helper (`getTimelineUserInputSource` + `isMidTurnUserInputBoundaryMessage`) used by the completed-turn grouping. - The conversation-outline query loads `system/userQuestion/lifecycle` so the outline exposes the same conversation rows as the timeline. - Drop the `as never` request id cast and reuse the harness question builder in build-thread-timeline tests. Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed 9f82501 addressing the second SlopCop round:
Verified:
|
…1657)" (#1981) ## What was wrong Main CI is red in `Checks` and `Tests (server)` since d9a5934 (#1657). #1976 made `parentToolCallId` required in `InsertEventInput`; #1657 was written before that change and merged after it, so its new test inserts events without the field. Typecheck fails with TS2741 on six literals and the SQLite insert fails with `near ",": syntax error`. Main runs 32314735170 and 32314935793 show it. ## What changed Reverts d9a5934 ("Keep mid-turn user exchanges visible after a turn completes (#1657)") with `git revert`. The revert applies cleanly on current main. #1657 can be re-landed after a rebase that adds `parentToolCallId` to its event inserts. Supersedes #1980, which was a forward fix of the test only. ## How you verified - `pnpm exec turbo run typecheck --filter=@bb/server --filter=@bb/thread-view --filter=@bb/app` passes. - `pnpm exec turbo run test --filter=@bb/thread-view --filter=@bb/db` passes. - CI on this PR. > AGENT GENERATED: by Claude Opus 5
Stack layer 2 of 4 for the bb mobile app (stack #1990). Prerequisite: #1986 (`@bb/client-core`). Next layer: #1988 (the app). ## What was wrong A native client needs a few small, additive pieces the server, cloud, SDK, and test harness do not have today: a way to tell the server it is a mobile client, SDK types that survive React Native's global `Response`/`FormData` declarations, a shared contract for the two plugin-rendered pending interactions so the phone can render them natively, a deterministic approval path in the fake provider for e2e, a harness that can run on a fixed port behind a stub gate, a way to mint a connect pairing code for a phone, and app-link association files so universal links can work. ## What changed - `packages/config` + `apps/server`: request-side `RequestAppSurface` (`desktop | web | mobile`) for `x-bb-app-surface` + telemetry; the server config union stays `desktop | web`. Test added. - `packages/sdk`: `SdkResponseLike` structural constraint in `response.ts`/`transport.ts` (Hono client responses stay assignable under RN globals); `projects.sidebarBootstrap()`; public-types test updates. - New `packages/plugin-interaction-contracts`: the ask-user-question and secret-request payload/resolution schemas moved from the two plugins' private `contracts.ts`; both plugins re-export them (their tests unchanged). - `packages/agent-runtime` fake provider: `approve:<kind>` control token (`command | file_change | permission_grant | plan`) emits an approval interactive request; adapter decodes/encodes approval payloads; 5 tests. - `tests/integration`: harness `serverPort`/`bindHost` options; `mobile-e2e/backend.ts` (seeded long-lived harness) and `connect-stub.ts` (TLS stub gate in front of the harness) + scripts + turbo tasks. - `plugins/connect`: "Add mobile device" QR/code in Settings → Remote access and `bb connect machine-code [--json]` (uses the existing `createMachineCode` RPC; 409 machine-limit surfaced). Guide template, bb-cli SKILL, `docs/multiple-devices.md`, `docs/configuration.md` pairing section updated per `docs/cli-guide-and-skill.md`. - **Hidden behind the new `mobileApp` experiment** (off by default; Settings → Experiments → "Mobile app", or `bb settings experiment mobileApp true`) until the app is generally available. `@bb/domain` gains the key; the connect plugin reads it through its loopback SDK (`bb.sdk.system.config()`) on every call — a new `mobilePairing` rpc tells the panel whether to render the section, and `createMachineCode` / `bb connect machine-code` refuse with a pointer to the toggle. No new plugin-SDK API. With the experiment off, a paired install sees no visual change from this PR. - `packages/connect-client`: pairing helpers used by the phone. - `apps/connect` (gate) + `apps/web` (apex) + `packages/connect-db`: serve `/.well-known/apple-app-site-association` and `/.well-known/assetlinks.json` before the session gate (static JSON only; Android fingerprints from `ASSETLINKS_SHA256_FINGERPRINTS`, empty if unset). No auth change. No daemon contract change → `HOST_DAEMON_PROTOCOL_VERSION` untouched. ## How you verified - `pnpm exec turbo run typecheck lint` for config, server, sdk, plugin-interaction-contracts, ask-user-question, secrets, connect plugin, agent-runtime, integration-tests, connect-client, templates, @bb/connect, @bb/web, connect-db, cli — pass (the only failure is the pre-existing `conversation-outline-parity.test.ts` typecheck on main from #1657). - Tests: sdk 96, agent-runtime 413, connect plugin 88, templates 43, connect/connect-db/connect-client, server telemetry + public route tests (342), `@bb/integration-tests test:smoke` 28 — all pass. Part of the bb mobile app plan (`plans/bb-mobile-expo.md`, lands in #1988). > AGENT GENERATED: by Claude Opus 5 --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Fixes #1656.
Completed-turn grouping buried messages the user had already read once
turn/completedre-grouped the turn:Changes in
packages/thread-view:user-question-lifecyclemessages are ungroupable and act as user-input boundaries.Turns without mid-turn user input keep the existing single-summary collapse. Regression tests in
completed-turn-user-input-visibility.test.ts; one existing expectation updated (the trailing segment after a legacy user message now keeps its last terminal visible).