Skip to content

fix(ui-web): read the opened conversation's tier, not the one being left - #1122

Open
Handsome-wzw wants to merge 6 commits into
mainfrom
fix/tier_reads_opened_session
Open

Handsome-wzw wants to merge 6 commits into
mainfrom
fix/tier_reads_opened_session

Conversation

@Handsome-wzw

@Handsome-wzw Handsome-wzw commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The Mode row of the WebUI's model picker (the sub-agent tier) read the
conversation being left rather than the one being opened. A switch asks for
the conversation being opened before the page's current session moves over
to it, and the tier read built its session.set_mode request from "the
current session": opening B from A asked for A's tier and painted its rung
on B, and picking that very rung on B was taken as already in force, so no
request went out. #1113 fixed the same shape for the reasoning-effort row
and left the tier, which predates it, for this follow-up.

What changes, in ui-web only:

  • TierSource.read(sid?, gen?) takes the conversation it is about and the
    switch it was started under, the shape EffortSource.read and
    loadPermMode already have. The live source builds the request from the
    given key (an empty key for a draft, which the handler answers with the
    catalogue and its default) and answers null when the switch ticket has
    been overtaken, so a late answer about a view the reader has left is
    dropped rather than painted. The catalogue such an answer carries is still
    kept for the draft's staged echo, being the deployment's and not the
    conversation's.
  • tier.load(sid?, gen?) passes them through and absorbs null as nothing:
    the chip keeps the last answer about the view on screen, and the
    sub-agent instance pane's watcher is not told of a move that did not
    happen.
  • The three call sites say what they read: the switch reads the opened
    conversation under its ticket, leaving for a draft reads with no key under
    its ticket, and the staged-tier write after a draft is minted re-reads
    that session. The boot's first read is unchanged (the view as it stands);
    its comment, which still credited session.onChange with the later reads,
    now names the switch.
  • The re-read after a draft's staged rung is written to the conversation its
    first message minted (applyStagedTier) carries the ticket the promotion
    began under, as applyStagedModel already does. A ticket taken after the
    awaited write is whichever view the reader has opened since, and under
    that one the minted session's rung was painted over that view's own read
    (the review bot's nonblocking finding; reopening recovered). The same
    hole exists in applyStagedEffort (on the feat(agent): per-conversation reasoning effort (picker row, /effort, session.set_effort) #1113 branch) and
    applyStagedPerm (on main) and is left for their own PRs.
  • No wire change: session.set_mode already takes session_key. The
    offline fixture, the picker row and the instance-pane watcher need no
    change.

Stacked on #1113: until that PR merges, this branch carries its commits and
the diff here shows them as well; this PR's own change is its last two
commits. It will be brought onto main once #1113 lands.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • ui-web vitest run -- 258 files, 4423 tests, the gates included, pass

  • ui-web type-check -- clean; lint -- 0 errors (4 pre-existing
    react-hooks/exhaustive-deps warnings in files this PR does not touch)

  • Before/after for the switch read: with the four source files put back to
    their previous version and the new tests kept, 6 cases fail (the draft
    read keyed to no conversation; the read keyed to the given conversation
    and switch; the overtaken answer dropped, and its catalogue kept; the
    switch reading the opened conversation before the current session moves
    over); all pass with the change

  • Before/after for the staged re-read: with the previous runtime.ts and the
    two new cases kept, both fail (the recorded ticket is undefined); pass
    with the change

  • New and amended cases: state/tier.test.ts (the store's read names the
    conversation and the switch; a dropped read paints nothing and tells no
    listener), state/session/registry.test.ts (opening B from A reads B's
    key before sessionSet('b'); leaving for a draft reads with null under
    the switch), features/model/source.test.ts (an overtaken read resolves
    to null; a standing one answers; a draft reads with session_key: '';
    the catalogue of a dropped answer is kept; the staged write's re-read is
    keyed to the minted session under the promotion's ticket, which a switch
    during the write has spent, and under a standing ticket when the reader
    stayed)

  • CHANGELOG: an Unreleased / Fixed entry

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

Risk

  • Security impact considered -- none: the read sends the same method
    with a key the page already holds, and nothing new is written
  • Backward compatibility considered -- no wire change; an older gateway
    answers session.set_mode as before; a reply that lands after a later
    switch is now dropped instead of painted
  • Rollback path is clear for risky changes -- revert the PR; the
    previous read is one line per call site

Related Issues

N/A

Handsome-wzw and others added 4 commits October 26, 2026 15:56
…session.set_effort)

One conversation can now ask for more or less thinking than the configured
default without moving that default. The pick rides the session's metadata
beside `model` (`raven.agent.session_effort`), so it outlives the gateway and
a resumed conversation runs at what it was left at; the turn entrance hands
it to `_run_agent_loop`, which ranks it over the mode's effort and the
configured `agents.defaults.reasoningEffort`, under a hook's rollback
override. Raven's own calls only: sub-agents keep the configured default.

Surfaces: `session.set_effort` (report, set or clear; every answer names what
the default is on this install and the words), `session.info` carries
`reasoning_effort` for the status bar, the WebUI's model picker gains an
effort row under the tier row, and the TUI gains `/effort`.

The settings page drew an unset `agents.defaults.reasoningEffort` as Low; the
unset state is now its own rung, Provider default, and the key accepts null
so that rung clears it. The using-raven guide documents both levels.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
… left

A switch asks for the conversation being opened before the page's current
session moves over to it, and the effort read built its request from "the
current session": opening B from A asked for A's effort and painted it on
B -- and a pick of that very rung on B was taken as already in force. The
read now names the conversation and the switch it belongs to, the way the
permission-mode read does, and the source drops an answer whose switch has
been overtaken rather than painting a view the reader has left.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
A switch asks for the conversation being opened before the page's current
session moves over to it, and the tier read built its request from "the
current session": opening B from A asked for A's tier and painted its rung
on B -- and a pick of that very rung on B was taken as already in force.
The read now names the conversation and the switch it belongs to, the way
the effort and permission-mode reads do, and the source drops an answer
whose switch has been overtaken rather than painting a view the reader has
left; the catalogue such an answer carries is still kept, being the
deployment's and not the conversation's.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw
Handsome-wzw requested a review from LivXue as a code owner October 27, 2026 03:40
@Handsome-wzw
Handsome-wzw requested a review from 0xKT October 27, 2026 03:40
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

Test counts for this change (ui-web only):

  • vitest run: 258 files, 4421 tests, pass (206 test files under src/ plus the 52 gates under scripts/gates/)
  • the five files closest to the change (state/tier.test.ts, state/session/registry.test.ts, features/model/source.test.ts, features/model/ModelPicker.test.tsx, state/effort.test.ts): 180 tests, pass
  • type-check: clean; lint: 0 errors, 4 pre-existing react-hooks/exhaustive-deps warnings in files this PR does not touch
  • before/after: with the four source files (state/tier.ts, features/model/source.ts, state/session/registry.ts, state/session/runtime.ts) put back to their previous version and the new tests kept, 6 cases fail (the draft read keyed to no conversation; the read keyed to the given conversation and switch; the overtaken answer dropped, and its catalogue kept; the switch reading the opened conversation before the current session moves over); all pass with the change

The branch sits on top of #1113, so the diff here shows that PR's commits until it merges; this PR's own change is the last commit.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; suggestions only, and they are marked inline.

I reviewed git diff github/main...HEAD, then isolated the final commit because this branch is stacked on #1113. I also checked the tier source/store seam, every loadTier caller, session-switch and promotion ordering, relevant history, and the Web UI architecture terms in CONTEXT-MAP.md / ui-web/CONTEXT.md. The main A-to-B switch fix is sound: reads now name the opened session and carry the switch generation, while superseded successful replies cannot repaint the current view. The optional source arguments preserve existing callers, and the tests strengthen rather than weaken the prior coverage.

Verification: npm exec -- vitest run src/state/tier.test.ts src/state/session/registry.test.ts src/features/model/source.test.ts src/features/model/ModelPicker.test.tsx src/state/effort.test.ts passed 5 files / 180 tests; npm run type-check passed. git diff --check github/main...HEAD also passed. I found one narrow staged-promotion race and left it inline as a nonblocking follow-up; reopening the conversation recovers, so it does not hold this fix.

Comment thread ui-web/src/state/session/runtime.ts Outdated
@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member

Windows compatibility review — PR #1122 @ bcbdabe33f69dc0da589112d630e09981b23679b

Scope: full base 4718421324 → this head (49 files). This PR is stacked on #1113: its raven/** portion (the six effort files, +246) and the effort UI are identical to the 516c7e7d head reviewed there, and the PR's own delta on top is 9 files (+169/−31) in ui-web/src/state/tier.ts, state/session/registry.ts, features/model/source.ts, boot.ts, and their tests — the tier read keyed to the conversation being opened and the switch generation, plus the boot wiring.

Result: no Windows-specific issue. The delta is browser state logic (same pattern as the effort fix, applied to the reasoning tier); no path, filesystem, subprocess, signal, encoding, or platform primitive. The stacked effort code was already cleared for Windows in the #1113 review.

Verification (native Windows 11, isolated worktree D:\tmp\rv-1122 at this head; scratch HOME/USERPROFILE, PYTHONUTF8=1; node_modules borrowed read-only from the main checkout):

vitest run src/state/tier.test.ts src/state/session/registry.test.ts \
            src/features/model/source.test.ts src/state/effort.test.ts
→ 4 files, 109 passed

pytest tests/test_session_effort.py tests/test_rpc_session.py \
       tests/test_agent_loop_usage_sink.py tests/test_rpc_settings.py \
       tests/test_rpc_session_init_bundle.py -q
→ 1 failed, 415 passed

The single Python failure is the inherited POSIX /somewhere/theirs regex test (test_a_write_to_a_root_the_user_manages_is_refused_readably), identical at base 4718421324 and unchanged by this PR.

Limitations: TUI tests not run (harness unavailable in this checkout); no real-browser walkthrough; the stacked #1113 code is covered by that PR's review rather than re-measured line by line here.

The re-read after a draft's staged tier is written to the session its first
message minted took its switch ticket after the awaited write, so a reader
who opened another conversation while the write was in flight got the
minted session's rung painted over that conversation's own read. The
promotion's ticket now rides into `applyStagedTier` the way it already rides
into `applyStagedModel`, and the source drops the late answer. Reopening the
conversation recovered before; now there is nothing to recover from.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

On the nonblocking thread at ui-web/src/state/session/runtime.ts (the staged tier's re-read taking its switch ticket after the awaited write), fixed in 321ac37:

  • promote now hands the ticket it began under to applyStagedTier(rt, sessionId, gen), which re-reads with loadTier(sessionId, gen) the way applyStagedModel already did, so a reader who opened another conversation while session.set_mode was in flight gets the minted session's answer dropped by the source rather than painted over the view they moved to.
  • Pinned in features/model/source.test.ts ("the staged tier write"): with the write deferred and a switch made before it lands, the re-read is keyed to the minted session under the promotion's ticket, which that switch has spent, and a read under that ticket answers null; when the reader stayed, the ticket still stands. With the previous runtime.ts and the two cases kept, both fail (the recorded ticket is undefined).
  • ui-web vitest run: 258 files, 4423 tests, pass; type-check clean; lint 0 errors. CHANGELOG entry extended by one sentence.

The same shape exists in applyStagedEffort (on the #1113 branch) and applyStagedPerm (on main); both are left for their own PRs rather than widened into this one.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

I reviewed the three-file delta from bcbdabe33f69 to this head against the full github/main...HEAD change, including the promotion caller, generation-ticket contract, source seam, relevant history, compatibility of the changed function signature, and the new tests. The prior staged-tier race is fixed without weakening coverage, and I found no new issue. The thread I opened is resolved.

Verification: the focused five-file Vitest run passed 182 tests, npm run type-check passed, and git diff --check github/main...HEAD passed.

@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review — PR #1122 @ 321ac37886317f4bdbbdfcb54d5be9061668e5c1

Scope: delta from the previously reviewed head bcbdabe3 — ui-web/src/state/session/runtime.ts, features/model/source.test.ts, CHANGELOG (+61/−6). The rest of the PR is unchanged.

Result: no Windows-specific issue.

  • The delta extends the same switch-ticket fix to the staged tier write: applyStagedTier now takes the promotion's gen and re-reads the tier under it, so a reader who opens another conversation while the session.set_mode write is in flight no longer paints this session's rung on the new view. It is browser state logic only — no path, filesystem, subprocess, signal, encoding, or platform primitive.

Verification (native Windows 11, isolated worktree D:\tmp\rv-1122-n at this head; node_modules borrowed read-only from the main checkout):

vitest run src/features/model/source.test.ts src/state/tier.test.ts \
            src/state/session/registry.test.ts src/state/effort.test.ts
→ 4 files, 111 passed

The new cases assert both halves: the re-read carries { sid: 'a', gen: <promotion ticket> } after a switch, and a spent ticket makes the source answer null.

Limitations: TUI tests not run; no real-browser walkthrough.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: carried from #1113. This PR targets main and contains #1113's head 516c7e7d (git merge-base --is-ancestor 516c7e7d 321ac378 holds). Merging it as it stands therefore merges #1113 too, including the defect I filed there as blocking: the Provider default rung clears agents.defaults.reasoningEffort, but a running gateway keeps sending the effort it booted with while session.info / session.set_effort report the clear (thread on #1113, anchored to raven/agent/loop/turn_path.py).

This PR's own two commits are fine. I read 516c7e7d..321ac378 in full. tierSource.read(sid, gen) names the conversation being opened and drops an answer whose switch was overtaken (ui-web/src/features/model/source.ts). The catalogue is still kept from every answer. switchTo, switchToDraft and promote pass the conversation and the switch ticket (ui-web/src/state/session/registry.ts, ui-web/src/state/session/runtime.ts), and a dropped read paints nothing (ui-web/src/state/tier.ts). This matches the effort and permission-mode reads.

This clears when #1113's fix lands and this branch is rebased onto it, or once #1113 merges and this PR's diff against main is only its own commits. Read only; local tests not run.

@0xKT

0xKT commented Oct 27, 2026

Copy link
Copy Markdown
Member

Merge conflict: @Handsome-wzw this PR no longer merges cleanly into main.

Measured just now with git merge-tree --write-tree of main 26352da0 and this head 321ac378. Main moved with #1110 (Agent Plugins loader), #1064 (conditional branches) and #1066 (DAG file references), each of which changed CHANGELOG.md. Conflicting files: CHANGELOG.md.

After a rebase onto current main I re-check what moved before the grade comes back.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: this branch carries #1113's unresolved Provider default defect and conflicts with current main.

My prior clean stance is withdrawn based on the new record. This PR contains #1113's still-open blocker: clearing agents.defaults.reasoningEffort can leave a running gateway sending its boot-time effort while the RPC surfaces report provider default. I am not re-grading that reviewer's finding; I confirmed #1113 remains open at 516c7e7d4284 with that thread unresolved and with an independent reproduction already recorded there. This PR's own tier commits remain fine.

I also fetched current github/main (26352da09) and reproduced the reported merge conflict: git merge-tree --write-tree HEAD github/main conflicts in CHANGELOG.md. The branch therefore cannot merge as it stands.

Verification: the first Python run did not reach tests because the fresh environment lacked raven_everos (225 setup errors). After uv sync --extra dev, uv run pytest tests/test_agent_loop_usage_sink.py tests/test_rpc_session.py tests/test_rpc_settings.py -q passed 384 tests.

@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

Merged origin/main (26352da) into the branch as 44442a4, no rebase. Main had moved by five PRs since the branch's base (#1110, #1064, #1119, #1066, #1120); the one conflict was CHANGELOG.md, where both sides had added entries at the top of Unreleased / Fixed. Resolved by keeping this branch's entries first and main's #1066 entry after them, so the file's diff against main is only this branch's own entries (this PR's Fixed entry and #1113's).

Verification on the merged tree (local):

  • ui-web vitest run: 260 files, 4447 tests, pass; type-check clean; lint 0 errors (the same 4 pre-existing warnings); gen:check: generated.ts matches the contract (249 methods)
  • ui-tui vitest run: 146 files, 2285 tests, pass; type-check clean; lint:rpc and lint:i18n in sync
  • pytest on the effort files this branch carries from feat(agent): per-conversation reasoning effort (picker row, /effort, session.set_effort) #1113 (test_session_effort, test_rpc_session, test_agent_loop_usage_sink, test_rpc_settings, test_rpc_session_init_bundle): 431 passed
  • uv.lock and ui-web/package-lock.json are unchanged on main; pyproject.toml only gained import-linter entries for raven.agent_plugins

The stacking is unchanged: the branch still carries #1113's head 516c7e7, so the Provider default thread carried from #1113 stays open here until #1113's fix lands. This PR's own change is still its two tier commits (bcbdabe, 321ac37).

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: the branch still carries #1113's unresolved Provider default defect.

I reviewed the merge commit from 321ac3788631 to this head, the effective github/main...HEAD diff, the --remerge-diff conflict resolution, relevant callers, and the existing review record. The former CHANGELOG.md conflict is resolved correctly: both sides' entries are retained, git diff --check github/main...HEAD passes, and git merge-tree --write-tree HEAD github/main now succeeds. I found no merge-resolution regression, backward-compatibility issue, weakened test, or new architecture/rules problem.

The stance remains blocked because this branch still contains #1113 head 516c7e7d4284, and its Provider-default defect remains unresolved; the author explicitly confirms that stacking and open thread are unchanged. This is not a new finding or a re-grade of another reviewer's blocker.

Verification on the merged tree: the focused five-file Web Vitest run passed 182 tests, and uv run pytest tests/test_session_effort.py tests/test_rpc_session.py tests/test_agent_loop_usage_sink.py tests/test_rpc_settings.py tests/test_rpc_session_init_bundle.py -q passed 431 tests.

@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review — PR #1122 @ 44442a4e485be54120b9e4abb86081a1c45cbbca

Scope: the head moved from the previously reviewed 321ac378. The PR's own tier/effort files (state/tier.ts+test, state/effort.ts+test, state/session/registry.ts+test, features/model/source.ts+test, state/session/runtime.ts, app/boot.ts) are unchanged — the diff between the two heads over exactly those files is empty. The large rest of the delta is main's merges (#1120's composer/TurnNav work, #1110, #1119, #1066, etc.), not this PR.

Result: no Windows-specific issue; the previous conclusion holds.

Verification (native Windows 11, isolated worktree D:\tmp\rv-1122-n2 at this head; scratch HOME/USERPROFILE, PYTHONUTF8=1; node_modules borrowed read-only from the main checkout):

vitest run src/state/tier.test.ts src/state/session/registry.test.ts \
            src/features/model/source.test.ts src/state/effort.test.ts
→ 4 files, 111 passed

pytest tests/test_session_effort.py tests/test_rpc_session.py \
       tests/test_agent_loop_usage_sink.py tests/test_rpc_settings.py \
       tests/test_rpc_session_init_bundle.py -q
→ 1 failed, 430 passed

The single failure is the inherited POSIX /somewhere/theirs regex test, unchanged by this PR and failing at base.

Limitations: TUI tests not run; no real-browser walkthrough.

This branch has not been deployed

No deployments
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.

4 participants