Skip to content

fix(native-chat): stop an unsettleable dispatch from blocking /clear and /compact - #20791

Closed
brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/sta7416-cmd-admission
Closed

brennanb2025 wants to merge 2 commits into
mainfrom
brennanb2025/sta7416-cmd-admission

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​66 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​66
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​10 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4

ELI5

If Orca could not confirm whether one of your messages reached the agent, it refused /clear and /compact for the rest of the session, telling you to "resolve pending or unconfirmed messages" — something you had no way to do. Now it only refuses while this session genuinely still owes an answer on a message.

What Changed

structured-conversation-command-admission.ts no longer runs its own inline scan over submissions. It reuses the existing hasUnansweredStructuredAgentSessionDispatch predicate from structured-agent-session-projection.ts, scoped to the caller's runtime fence.

Why

Failure mechanism. Conversation-command admission refused every command when any submission was pending or unknown:

.some((entry) => entry.dispatchState === 'pending' || entry.dispatchState === 'unknown')

unknown has two very different meanings, and this collapsed them. A live unknown is a send still outstanding — refusing on it is right. A recovered unknown is terminal and unsettleable:

  • markJournalPendingSubmissionsUnknown skips rows already marked recovered, so crash recovery never revisits one.
  • Restart reconciliation only ever narrows to accepted/rejected; anything it leaves unknown stays exactly as the crash boundary wrote it.
  • The renderer's Retry drops the outbox entry without touching the journal row.

Nothing re-derives the row and no user action settles it, so the command stayed refused for the life of the session behind an instruction nobody could follow. The refusal was a durable obligation with no way to die.

This is pre-existing, but #20723 makes a recovered unknown the normal terminal outcome for a restart-stranded send, which takes this from rare to routine.

The predicate being reused already encodes exactly the right distinction: it excludes recovered rows and carries a fallback for older hosts that publish the recovery reason without the optional marker.

Fence decision — the admission check now passes ctx.fence, deliberately.

Every submission this generation appends carries fence: ctx.fence, and the journal write guard refuses any fence below highestFence, so submission.fence >= ctx.fence selects exactly the rows the current generation owns.

A stale-fence unanswered dispatch is work owed by a host generation that is gone. No live provider will answer it, and no user action settles it — so blocking on one reproduces the very same unfollowable-instruction bug, just one state over (for example a pending row from a dead generation whose crash-boundary settle write failed, or a legacy-host row with no marker). Fence-scoping does not weaken the real guard: a send genuinely in flight in this generation has fence == ctx.fence and still refuses the command. It also brings this call site in line with the helper's other callers, which all pass a fence; passing undefined would have left this the only caller that still strands on dead-generation rows.

Linked Issue

STA-7416. No GitHub issue for this one; the admission defect was found while reviewing the plan for #20723.

Visual Proof

N/A — no UI or layout change. The only user-visible difference is that a refusal string stops appearing in a state the user could not exit; there is no rendered surface that differs to capture side by side.

Testing

Guard is a vitest test collected by pnpm test (structured-conversation-command-admission.test.ts), not e2e.

Failing-first evidence. Source change reverted to the base file with the final test in place, then restored:

  • Base source, final test: Tests 3 failed | 7 passed (10) — the three failures are exactly the new cases (recovered unknown, legacy-host reason without the marker, dead-generation fence), each expected 'Resolve pending or unconfirmed messag…' to be null.
  • With the fix: Tests 10 passed (10).

The ablation was instrumented rather than assumed: the reverted file was asserted to contain the old inline predicate (1 match) and none of the new helper (0 matches), and to carry the helper again after restore.

Tests cover both directions — the three admitted cases above, plus that a live pending and a live unknown in the current generation still refuse, and that fully settled submissions admit.

Gates: pnpm tc exit 0, oxlint exit 0, changed-code quality gate passed with 0 new findings across both files.

Platforms: pure decision logic in the execution host with no platform-dependent behavior, so it carries across macOS / Linux / Windows and the SSH execution host identically. No wire shape, journal schema, or published content changed, so there is no mixed-version client concern.

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Reuse over reimplementation: the inline scan is deleted in favour of the predicate that already existed, so there is no second copy of this rule to drift.

Checked every other dispatchState === 'unknown' site in src/ for the same class of defect. No other site blocks a user action on an unsettleable row:

  • journal-submission-reconciler.ts / journal-restart-reconciliation.ts include recovered rows on purpose — that inclusion is how a recovered unknown can still be narrowed to accepted. Settlement machinery, not a user gate.
  • structured-agent-session-dead-generation-settlement.ts already excludes recovered.
  • journal-pending-submission-recovery.ts skips recovered for idempotence.
  • journal-store.ts pendingSubmissions reads pending only.
  • Renderer outbox and send-disposition treat a recovered unknown by dropping or parking the entry; neither blocks.
  • structured-agent-session-send-settlement.ts waits on pending only, per message id and with a bounded timeout.

Checklist

  • This PR is small and focused
  • I explained what changed and why (including ELI5)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…and /compact

Conversation-command admission refused every command while any submission was
`pending` or `unknown`. A recovered `unknown` is terminal: crash recovery skips
rows it already marked, restart reconciliation only narrows accepted outcomes,
and Retry drops the outbox entry without touching the journal. Nothing settles
it, so the refusal named a step the user could not take for the rest of the
session.

Reuse `hasUnansweredStructuredAgentSessionDispatch`, which already excludes
recovered rows and keeps the legacy-host fallback for hosts that publish the
recovery reason without the marker. Scope it to the caller's runtime fence: a
dispatch from a dead generation is owed by a host that is gone, so no live
provider answers it and no user action settles it either.
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f6ed50e8-2304-4363-ac6b-d18dee3462c9

📥 Commits

Reviewing files that changed from the base of the PR and between 18d0afc and dc1dadd.

📒 Files selected for processing (2)
  • src/main/native-chat/agent-session-wire/structured-conversation-command-admission.test.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command-admission.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

conversationCommandBlocked now uses hasUnansweredStructuredAgentSessionDispatch with the journal submissions and current fence. Tests cover recovered unknown submissions, legacy host doubt reasons, prior-fence pending submissions, terminal submissions, and current-generation pending or unknown submissions.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to dc1da

The change continues blocking commands for live unresolved dispatches while allowing stale recovered records, so it is ready to merge.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing unsettleable dispatches from blocking /clear and /compact.
Description check ✅ Passed The description is complete and relevant. It includes the required explanation, change summary, rationale, issue reference, visual-proof justification, testing details, review notes, and completed che…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Admission reuses the shared unanswered-dispatch predicate — structured-conversation-command-admission.ts:58 now calls hasUnansweredStructuredAgentSessionDispatch(ctx.journal.submissions(), ctx.fence), replacing the inline pending || unknown scan. A recovered unknown and a dead-generation row no longer refuse /clear, /compact, or rewind for the life of the session.
  • Fence-scoped to the current generation — a live in-generation pending/unknown still refuses; only rows at an older fence, or marked recovered / carrying the legacy host_restarted_before_acknowledgement reason, are admitted. This matches the fence semantics the same helper already applies to session status.
  • Coverage — five new cases in structured-conversation-command-admission.test.ts: three admitted states (recovered unknown, legacy reason without the marker, stale fence) and two refusal guards (live pending/unacknowledged, terminally settled). The three admitted cases fail against the base predicate, so they pin the fix rather than the status quo; all 10 pass here, and the related command/projection/rewind suites pass 59/59.

I independently checked the two directions that matter and both hold. A current-generation send is appended with fence: ctx.fence, and runtimeFence only advances with proven prior-owner death or an unused reservation, so no live dispatch can be excluded by the fence comparison; the handoffStage window is refused earlier at structured-conversation-command-admission.ts:29-31. The recovered and legacy-reason relaxations are written only by crash/restart/provider-exit recovery, never on a normal-operation path, and /compact remains protected by the unresolved-turn check.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing as obsolete: main no longer has the code this PR changes, or already behaves as it intended. Main structured-conversation-command-admission.ts:110 refuses /clear and /compact only for sends work.owesSend() says the live generation still holds (current-work.ts:228; #28103 3bfb114, #29905), not for any recovered pending/unknown

(Stale-PR cleanup 2026-10-20, approved by Brennan. Reopen if something here is still missing on main.)

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