Skip to content

fix(desktop): skip failed turns in regenerate/edit truncation ordinal - #41275

Closed
vondelomlo wants to merge 1 commit into
NousResearch:mainfrom
vondelomlo:fix/desktop-regenerate-ordinal-failed-turn
Closed

fix(desktop): skip failed turns in regenerate/edit truncation ordinal#41275
vondelomlo wants to merge 1 commit into
NousResearch:mainfrom
vondelomlo:fix/desktop-regenerate-ordinal-failed-turn

Conversation

@vondelomlo

Copy link
Copy Markdown
Contributor

What does this PR do?

Regenerate stayed silently broken after any failed turn — this realigns
the client-side truncation ordinal with backend history.

A user turn whose prompt.submit fails keeps its optimistic bubble in the
transcript, but that turn is never written to backend history. The desktop
visibleUserOrdinal helper still counted those failed turns, so every later
truncate_before_user_ordinal overshot the gateway's persisted-history index.
The backend then rejected the request with error 4018 ("target user message is
no longer in session history"). editMessage recovers via a plain resend, but
reloadFromMessage (regenerate) has no fallback — it just surfaces
"regenerate failed", so the feature is unusable for the rest of the session.

The fix excludes failed/optimistic user turns (a user message immediately
followed by an assistant error) from the count, mirroring the existing
isFailedTurn heuristic already used in editMessage, so the client and
backend ordinals stay in lockstep.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/session/hooks/use-prompt-actions.tsvisibleUserOrdinal now skips failed/optimistic user turns so the ordinal matches the gateway's persisted-history count; exported for direct unit testing.
  • apps/desktop/src/app/session/hooks/use-prompt-actions.test.tsx — added coverage for the counter: normal turns, a failed turn (must be skipped), and hidden branched-away turns.

How to Test

  1. In the desktop app, send a message while a provider is unreachable — an inline assistant error appears and the user bubble stays.
  2. Send a second message that succeeds.
  3. Hover the successful turn and click Regenerate. Before: it fails with "regenerate failed". After: it regenerates correctly.
  4. Unit test: cd apps/desktop && npx vitest run --environment jsdom src/app/session/hooks/use-prompt-actions.test.tsx (14 passing).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (vitest + tsc + eslint)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

## What does this PR do?

Regenerate stayed silently broken after any failed turn — this realigns
the client-side truncation ordinal with backend history.

A user turn whose `prompt.submit` fails keeps its optimistic bubble in the
transcript, but that turn is never written to backend history. The desktop
`visibleUserOrdinal` helper still counted those failed turns, so every later
`truncate_before_user_ordinal` overshot the gateway's persisted-history index.
The backend then rejected the request with error 4018 ("target user message is
no longer in session history"). `editMessage` recovers via a plain resend, but
`reloadFromMessage` (regenerate) has no fallback — it just surfaces
"regenerate failed", so the feature is unusable for the rest of the session.

The fix excludes failed/optimistic user turns (a user message immediately
followed by an assistant error) from the count, mirroring the existing
`isFailedTurn` heuristic already used in `editMessage`, so the client and
backend ordinals stay in lockstep.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)
- [ ] ✨ New feature (non-breaking change that adds functionality)
- [ ] 🔒 Security fix
- [ ] 📝 Documentation update
- [ ] ✅ Tests (adding or improving test coverage)
- [ ] ♻️ Refactor (no behavior change)
- [ ] 🎯 New skill (bundled or hub)

## Changes Made

- `apps/desktop/src/app/session/hooks/use-prompt-actions.ts` — `visibleUserOrdinal` now skips failed/optimistic user turns so the ordinal matches the gateway's persisted-history count; exported for direct unit testing.
- `apps/desktop/src/app/session/hooks/use-prompt-actions.test.tsx` — added coverage for the counter: normal turns, a failed turn (must be skipped), and hidden branched-away turns.

## How to Test

1. In the desktop app, send a message while a provider is unreachable — an inline assistant error appears and the user bubble stays.
2. Send a second message that succeeds.
3. Hover the successful turn and click Regenerate. Before: it fails with "regenerate failed". After: it regenerates correctly.
4. Unit test: `cd apps/desktop && npx vitest run --environment jsdom src/app/session/hooks/use-prompt-actions.test.tsx` (14 passing).

## Checklist

### Code

- [x] I've read the [Contributing Guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md)
- [x] My commit messages follow [Conventional Commits](https://www.conventionalcommits.org/) (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for [existing PRs](https://github.com/NousResearch/hermes-agent/pulls) to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: macOS 15 (vitest + tsc + eslint)

### Documentation & Housekeeping

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the [compatibility guide](https://github.com/NousResearch/hermes-agent/blob/main/CONTRIBUTING.md#cross-platform-compatibility) — or N/A
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary (PR #41275)

Verdict: Approved

Looks Good

  • Fixes ordinal misalignment between desktop UI and backend when a failed turn (assistant error) is visible to the user but not persisted in gateway history
  • visibleUserOrdinal now skips failed assistant turns, preventing error 4018 on regenerate
  • Tests cover: failed turn skip, hidden (branched) turn skip, normal counting
  • Good inline documentation explaining the root cause

Reviewed by Hermes Agent

@daimon-nous daimon-nous Bot added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 7, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the client/backend ordinal mismatch; current main still has the naive counter in apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts:202-204.

Problems

  • The proposed assistant.error adjacency predicate conflates two different states. A rejected prompt.submit appends an error in submit.ts:405-421, but a user prompt that already reached the backend can also receive an adjacent assistant error from the streaming path in use-message-stream/index.ts:380-400 and 470-510. Skipping both undercounts persisted history. The gateway uses the supplied ordinal to truncate session['history'] (tui_gateway/server.py:8465-8479), so this can rewind before the intended user turn.
  • The original hook file has since been split; the helper now lives in apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts:202.

Suggested changes

  • Carry explicit local provenance for a pre-persistence submit failure, then make the ordinal helper skip only that category.
  • Test both a rejected submit and a persisted turn with a later provider/stream error.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
teknium1 pushed a commit that referenced this pull request Aug 15, 2026
A user turn whose submit failed keeps its optimistic bubble but never
reached the gateway, so counting it makes every later
truncate_before_user_ordinal overshoot the backend index (refused 4018,
regenerate dead for the rest of the session). Skip failed turns in the
one shared visible-user ordinal space (visibleUserMessageIndices) used by
truncate ordinals, ordinal->index resolution, and survivor-rowId
rebinding.

Based on #41275 by @vondelomlo, relocated onto the split
use-prompt-actions/ modules and widened from visibleUserOrdinal to the
shared index helper.
@teknium1

Copy link
Copy Markdown
Contributor

Salvaged into #87294 (merged) — your failed-turn ordinal fix now lives in visibleUserMessageIndices() in use-prompt-actions/utils.ts, relocated onto the split modules and widened so the same failed-turn-aware ordinal space is shared by truncate ordinals, ordinal→index resolution, and survivor-rowId rebinding. Your commit is preserved with authorship in the merge (f60477e → rebased into main via #87294).

Thanks for the fix and the clean test coverage — closing since the work has landed.

@teknium1 teknium1 closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants