TUI: restore hook-visible /new resets#49945
Conversation
Greptile SummaryThis PR restores hook-visible Key changes:
Confidence Score: 5/5
Last reviewed commit: "TUI: restore /new ho..." |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37d84d03f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b62dde6f7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target.canonicalKey ?? params.key, | ||
| { | ||
| sessionEntry: entry, | ||
| previousSessionEntry: entry, | ||
| sessionEntry: hookSourceEntry, | ||
| previousSessionEntry: hookSourceEntry, |
There was a problem hiding this comment.
/new hooks under the fresh tui-* key
In the TUI /new flow, hookSourceKey now makes previousSessionEntry point at the session being left, but the emitted hook still uses sessionKey=target.canonicalKey. Bundled hooks such as src/hooks/bundled/command-logger/handler.ts:52-59 and src/hooks/bundled/session-memory/handler.ts:221-235 treat event.sessionKey as the conversation being archived, so installs with those hooks enabled will now record the old transcript under the brand-new tui-* key instead of the session that actually ended.
Useful? React with 👍 / 👎.
| // to other connected TUI clients sharing the original session key. | ||
| const uniqueKey = `tui-${randomUUID()}`; | ||
| const nextSessionKey = `agent:${normalizeAgentId(state.currentAgentId)}:${uniqueKey}`; | ||
| await client.resetSession(nextSessionKey, "new", state.currentSessionKey); |
There was a problem hiding this comment.
Calling sessions.reset on a brand-new tui-* key means /new now writes a fresh SessionEntry immediately, even though no message was ever sent on that session. performGatewaySessionReset always persists a new entry for the requested key, and src/gateway/session-utils.ts:868-976 lists every store row, so repeatedly pressing /new will leave behind blank tui-* sessions that clutter the session picker/history. Before this change, /new only switched the local key and did not persist an empty session.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep this PR open but not merge-ready: it targets a real TUI hook regression, but the current branch exposes a risky reset hook-source contract, can misattribute old-session hook output to a new Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives a high-confidence reproduction path: current TUI Is this the best way to solve the issue? No. The PR targets the right regression, but resetting the future Security review: Security review needs attention: No dependency or workflow supply-chain issue was found, but the diff broadens session-context selection in a public gateway RPC.
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 6bd430ee3517. |
clawsweeper
Bot
added
P2
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
ClawSweeper applied the proposed close for this PR.
|
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
#49918reported that TUI/newstopped emitting hook-visiblecommand:newbehavior after the per-client session isolation change.command-loggerandsession-memorysilently stop running for TUI/new, even though users still expect a normal fresh-session reset./newnow resets the newly allocated per-clienttui-<uuid>session throughsessions.reset(reason="new")before switching the TUI to that unique key, and the command-handler test now locks that route in./resetbehavior stays in-place on the current session, and TUI session isolation still uses unique per-client keys.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
TUI
/newonce again emits the same hook-visible reset path as other reset flows, while still creating an isolatedtui-<uuid>session for the current TUI client.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation:Repro + Verification
Environment
Steps
command:new.openclaw tuiand run/new.sessions.reset(reason="new")or only switches local session state.Expected
/newkeeps the isolatedtui-<uuid>behavior and still emits the standard hook-visiblecommand:newreset path.Actual
/newonly called localsetSession(uniqueKey)and skipped the gateway reset path entirely.Evidence
Attach at least one:
Verification commands run locally:
pnpm test -- src/tui/tui-command-handlers.test.tspnpm test -- src/gateway/server.sessions.gateway-server-sessions-a.test.ts -t 'sessions.reset emits internal command hook with reason'pnpm exec oxfmt --check src/tui/tui-command-handlers.ts src/tui/tui-command-handlers.test.tspnpm exec oxlint src/tui/tui-command-handlers.ts src/tui/tui-command-handlers.test.tsHuman Verification (required)
What you personally verified (not just CI), and how:
/newnow routes through gateway reset with reasonnew,/resetstill resets the shared current session, and existing gateway coverage still confirms thatsessions.reset(reason="new")emits the internal command hook.agent:main:tui-<uuid>session key generation for/newand preserved/resetbehavior.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
37d84d03f8src/tui/tui-command-handlers.ts,src/tui/tui-command-handlers.test.ts,CHANGELOG.md/newno longer emitting hook-visible resets, or TUI/newfalling back to the shared main session instead of a uniquetui-<uuid>keyRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None./newcould accidentally stop isolating the TUI client if the canonical unique key routing regresses.tui-<uuid>session handoff and the gateway reset call shape.