Skip to content

[Fix] Address review findings on the transcript convergence and question panel fixes#44

Merged
daniel-lxs merged 3 commits into
developfrom
fix/transcript-merge-review-findings
Jul 9, 2026
Merged

[Fix] Address review findings on the transcript convergence and question panel fixes#44
daniel-lxs merged 3 commits into
developfrom
fix/transcript-merge-review-findings

Conversation

@daniel-lxs

Copy link
Copy Markdown
Member

What problem this solves

Follow-up addressing the three outstanding openmote review findings on already-merged PRs #33 and #37:

  1. [Fix] Live transcript converges to persisted envelopes on every history refetch #37: acpService.setMessages after the history rebuild reset the whole protocol service, discarding the pending request_user_input lookup populated from history — a live request_user_input_response after a refetch formatted with request === null, so secret answers could render raw in the transcript.
  2. [Fix] Live transcript converges to persisted envelopes on every history refetch #37: rebuilt.todos.length > 0 ? rebuilt.todos : state.todos kept stale todos when the server history's latest plan state was intentionally empty — refetches couldn't converge todos to server truth.
  3. [Fix] Pending question panel stays answerable when the reported phase flips #33: with the question panel no longer phase-gated, a cancel/session error cleared the worker-side pending map but the client kept its pendingUserInputRequests — the aborted question stayed answerable and free-text sends could be swallowed as answers to a dead request.

What changed

  • New AcpProtocolService.rebindMessages re-points only the id/position indexes; the merge path uses it instead of setMessages (removed), so request/user/tool lookups built by loadAcpEnvelopes survive. Two service tests that emulated the old merge now call reset() + rebindMessages() explicitly.
  • loadAcpEnvelopes returns hasPlanHistory; the merge treats fetched todos as authoritative (including empty) whenever history carries plan state.
  • _setTaskStatus drops pending user-input requests (store + service map) when taskStateEvent === 'taskAborted'.

Validation

Three new regression tests (secret masking after refetch-over-pending-request, todos converging to an empty plan, abort clearing the pending question). Full src/app/(sandbox)/task/ suite green — 486 tests. Lint + typecheck clean.

🤖 Generated with Claude Code

@roomote-roomote-v1

roomote-roomote-v1 Bot commented Jul 9, 2026

Copy link
Copy Markdown

3 issues outstanding. See task

  • apps/web/src/app/(sandbox)/task/[taskId]/hooks/use-sandbox-store.ts:832 _mergeAcpHistory rebuilds with loadAcpEnvelopes(sorted) using the default trailing-completion behavior, so the live provider's immediate post-hydration refetch can flip an active trailing assistant message to isTurnCompletion even when initial hydration explicitly suppressed that for a still-running task.
  • apps/web/src/app/(sandbox)/task/[taskId]/hooks/services/acp-protocol-service.ts:1724 History todowrite events processed through the ToolCall/ToolCallUpdate branch update todos but never set hasPlanHistory, so _mergeAcpHistory treats that server plan state as absent and can keep stale live todos instead of converging to persisted history.
  • apps/web/src/app/(sandbox)/task/[taskId]/hooks/use-sandbox-store.ts:247 The merge coverage key for tool messages is only the shared toolCallId; if a refetch has the persisted tool call but not the persisted terminal result yet, the live-only tool result is considered covered and is dropped instead of being carried over.
  • apps/web/src/app/(sandbox)/task/[taskId]/hooks/use-sandbox-store.ts:906 _mergeAcpHistory treats any rebuilt tool_result for a call as terminal coverage. If history includes a persisted in-progress ToolCallUpdate but not the final terminal update yet, a live terminal result is still considered covered and dropped, leaving the transcript stuck on the stale in-progress output until the terminal envelope persists.
  • apps/api/src/handlers/ado/index.ts:100 Flat Azure DevOps comment normalization calls getAdoPullRequest before recordWebhook claims the delivery; if that external fetch fails or the delivery is retried, the failure bypasses the idempotency/error-recording path and ADO can keep retrying the same unrecorded delivery.
  • apps/web/src/components/settings/CommsProviderSection.tsx:527 Clearing a saved optional communications field enables Save, but handleSave submits only non-empty values and saveCommsAuthConfigCommand only upserts, so a successful-looking save leaves optional saved values like TELEGRAM_WEBHOOK_SECRET or TELEGRAM_BOT_USERNAME stored and active.
  • packages/cloud-agents/src/server/__tests__/opencode-runtime.test.ts:301 The signal-handler test relies on the previous test leasing a server to register process-global handlers, so running this test alone or after ordering/sharding changes can fail; the test should lease/register its own server or move that setup into shared test setup.

daniel-lxs and others added 3 commits July 9, 2026 11:51
…ion panel fixes

Three issues from the openmote reviews of #33 and #37:

- The history rebuild no longer resets the protocol service's auxiliary
  lookups: rebindMessages re-points only the id/position indexes, so the
  pending request_user_input map populated from history survives a
  refetch and a subsequent live response still masks secret answers.
- Todos now converge to the fetched history whenever it carries plan
  state, including an intentionally empty list, instead of falling back
  to stale live todos.
- A taskAborted status (cancel or terminal session error) drops the
  client-side pending user input requests, matching the worker-side
  clear, so an aborted question can no longer stay answerable and
  swallow free-text sends.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- The history rebuild now derives the trailing turn-completion decision
  from the live task status (falling back to what the transcript already
  shows), instead of always marking it - merging while a task is running
  no longer flips the active trailing assistant message to completed.
- todowrite plan state arriving through ToolCall/ToolCallUpdate history
  envelopes now counts as authoritative plan history for todo
  convergence.
- Live terminal tool results are matched by terminality rather than
  id/tool key: a persisted pending tool_call no longer swallows the live
  result (which can sit under the same envelope id after an in-place
  update), and the superseded pending call row is dropped instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lt kind

A persisted in-progress ToolCallUpdate also rebuilds as a (partial)
tool_result, so kind alone let it swallow a live terminal result and
leave the transcript stuck on stale in-progress output. Coverage now
requires a non-partial persisted result for live terminal results, and
carried terminal results supersede both pending tool_call and partial
tool_result rebuilt rows.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@daniel-lxs
daniel-lxs force-pushed the fix/transcript-merge-review-findings branch from 99dec35 to ff474c4 Compare July 9, 2026 16:52
@daniel-lxs
daniel-lxs merged commit 7e57e17 into develop Jul 9, 2026
daniel-lxs added a commit that referenced this pull request Jul 9, 2026
…ase range (#49)

* [Fix] Scope GitHub sync review to the PR's Files Changed, not the rebase range

The sync re-review computed its since-last-review delta as
compare(lastReviewSha...currentHead) — three-dot semantics, so after the
branch was rebased onto its base it also contained every commit pulled
in from the base branch. The reviewer then flagged findings in files the
PR never touched (observed on #44: 3 findings in ado/index.ts,
CommsProviderSection.tsx, opencode-runtime.test.ts inherited from
develop), and the merged PR permanently showed them as outstanding.

The sync review now fetches the PR's authoritative Files Changed
(base...head, via gh pr diff) and intersects the range diff and
changed-file list with it, dropping rebased-in base-branch files. A
rebase-only head change with no PR-relevant delta collapses to the
existing no-op path. The review-code skill also records the authoritative
file set (pull_request_changed_files) and is instructed to only report
findings within it and to disregard base-branch/rebase hunks when running
its own local delta diff.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* [Fix] Harden sync-review scoping: fetch-failure safety and two-dot rebase no-op

Addresses the two review findings on #49:

- A fetch failure or too-large diff from either gh pr diff or the compare
  API returns { diff: undefined, changedFiles: [] }, indistinguishable
  from a genuine empty result. The no-op decision now keeps the delta
  (reviewer inspects manually) on failure, and falls back to the unscoped
  range when the PR's authoritative Files Changed cannot be read, instead
  of silently suppressing the delta. Extracted the decision into a pure,
  unit-tested resolveScopedSyncReviewDelta helper.
- Filtering the three-dot range to PR file paths still re-shows the PR's
  own already-reviewed files after a rebase, so it cannot collapse to
  no-op from the API layer. The review-code skill now inspects the delta
  with a two-dot diff (git diff last..head) scoped to the PR files — the
  actual content difference between the reviewed commits, which is empty
  for a rebase-only head change — and marks no_new_delta when it is empty.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* [Fix] Review scope is the PR's base...head diff, not the two-dot range

Second review round on #49:

- knip: dropped the unused exported ScopedSyncReviewDelta interface
  (the helper now returns an inline-typed object).
- A two-dot last..head diff still leaks base-branch hunks on files the PR
  also touches: it compares the old reviewed tree with the new rebased
  tree, so a base-branch change to a shared file shows up as a reviewable
  change even though it is outside the PR's base...head Files Changed. The
  only leak-free scope is the PR's own base...head diff. resolveScoped-
  SyncReviewDelta now uses the three-dot range only to pick which PR files
  changed since the last review, and presents the PR's authoritative
  base...head hunks for those files (fed as a pull-request diff, not a
  compare range). The skill makes base...head the sole in-scope change set
  and uses two-dot only to detect the no-op/rebase-only case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@mrubens
mrubens deleted the fix/transcript-merge-review-findings branch July 12, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant