fix tool approval lifecycle to match SSE connection, not keep-alives #2693
Conversation
|
Warning Rate limit exceeded@sawka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request refactors RPC usage in several CLI command files to use new wshclient wrappers, removes periodic keepalive polling from frontend AI tool components, and replaces a timer-based tool-approval lifecycle with an SSE-driven on-close model. It adds a generic IdList registry, on-close registration/unregistration to the SSE handler, new global RegisterToolApproval/UnregisterToolApproval functions, and updates types by removing keepalive fields and adding telemetry properties. Telemetry handling is extended to capture additional terminal actions (Ctrl+R, SSH, editors, tail -f). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/utilds/idlist.go (1)
46-53: Consider using Go naming convention for internal method.The method
unregister_nolockuses snake_case, which is not idiomatic Go. Consider renaming tounregisterNoLockfor consistency with Go conventions.🔎 Suggested rename
-func (il *IdList[T]) unregister_nolock(id string) { +func (il *IdList[T]) unregisterNoLock(id string) { for i, entry := range il.entries { if entry.id == id { il.entries = append(il.entries[:i], il.entries[i+1:]...) return } } }Also update the callers at lines 35 and 43.
pkg/web/sse/ssehandler.go (1)
314-327: Double-locking on Register/Unregister is unnecessary but harmless.
RegisterOnCloseandUnregisterOnCloseholdh.lockwhile calling intoIdList.Register/Unregister, which acquires its own internal lock. This results in double-locking that's safe (no deadlock risk since the locks are different) but unnecessary.Consider whether
h.lockis needed here, or ifIdList's internal locking is sufficient. Ifh.lockis needed to guardhandlersRunchecks elsewhere, the current approach is fine.pkg/aiusechat/toolapproval.go (2)
36-40: Consider cleaning uponCloseUnregFninUnregisterToolApproval.If this function is called while the SSE connection is still open, the
onClosecallback remains registered and will fire when the connection eventually closes. WhileUpdateToolApprovalhandles missing requests gracefully, explicitly unregistering the callback would be cleaner and avoid unnecessary callback invocations.🔎 Proposed fix
func UnregisterToolApproval(toolCallId string) { globalApprovalRegistry.mu.Lock() - defer globalApprovalRegistry.mu.Unlock() + req, exists := globalApprovalRegistry.requests[toolCallId] delete(globalApprovalRegistry.requests, toolCallId) + globalApprovalRegistry.mu.Unlock() + + if exists && req.onCloseUnregFn != nil { + req.onCloseUnregFn() + } }
101-103: UseUnregisterToolApprovalfor consistency.Direct registry manipulation duplicates logic from
UnregisterToolApproval. Using the helper improves maintainability and ensures any future cleanup logic inUnregisterToolApprovalapplies here too.🔎 Proposed fix
req.mu.Lock() approval := req.approval req.mu.Unlock() - globalApprovalRegistry.mu.Lock() - delete(globalApprovalRegistry.requests, toolCallId) - globalApprovalRegistry.mu.Unlock() + UnregisterToolApproval(toolCallId) return approval
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.vscode/settings.json(0 hunks)cmd/wsh/cmd/wshcmd-connserver.go(1 hunks)cmd/wsh/cmd/wshcmd-deleteblock.go(2 hunks)cmd/wsh/cmd/wshcmd-editconfig.go(2 hunks)cmd/wsh/cmd/wshcmd-notify.go(2 hunks)cmd/wsh/cmd/wshcmd-secret.go(1 hunks)cmd/wsh/cmd/wshcmd-setmeta.go(2 hunks)cmd/wsh/cmd/wshcmd-view.go(2 hunks)frontend/app/aipanel/aitooluse.tsx(0 hunks)frontend/app/store/global.ts(2 hunks)frontend/app/view/term/term-model.ts(2 hunks)frontend/app/view/term/termwrap.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/aiusechat/toolapproval.go(3 hunks)pkg/aiusechat/uctypes/uctypes.go(0 hunks)pkg/aiusechat/usechat.go(2 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(1 hunks)pkg/utilds/idlist.go(1 hunks)pkg/web/sse/ssehandler.go(9 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
💤 Files with no reviewable changes (3)
- pkg/aiusechat/uctypes/uctypes.go
- frontend/app/aipanel/aitooluse.tsx
- .vscode/settings.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Applied to files:
pkg/wshrpc/wshrpctypes.gopkg/wshrpc/wshserver/wshserver.gofrontend/types/gotypes.d.tspkg/aiusechat/usechat.gopkg/aiusechat/toolapproval.go
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
cmd/wsh/cmd/wshcmd-view.gocmd/wsh/cmd/wshcmd-connserver.gocmd/wsh/cmd/wshcmd-deleteblock.gocmd/wsh/cmd/wshcmd-secret.gocmd/wsh/cmd/wshcmd-notify.gocmd/wsh/cmd/wshcmd-setmeta.gocmd/wsh/cmd/wshcmd-editconfig.go
📚 Learning: 2025-10-14T06:30:54.763Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2430
File: frontend/app/aipanel/aimessage.tsx:137-144
Timestamp: 2025-10-14T06:30:54.763Z
Learning: In `frontend/app/aipanel/aimessage.tsx`, the `AIToolUseGroup` component splits file operation tool calls into separate batches (`fileOpsNeedApproval` and `fileOpsNoApproval`) based on their approval state before passing them to `AIToolUseBatch`. This ensures each batch has homogeneous approval states, making group-level approval handling valid.
Applied to files:
pkg/aiusechat/usechat.go
🧬 Code graph analysis (13)
frontend/app/store/global.ts (1)
frontend/app/store/jotaiStore.ts (1)
globalStore(3-3)
pkg/wshrpc/wshserver/wshserver.go (1)
pkg/aiusechat/toolapproval.go (1)
UpdateToolApproval(65-87)
cmd/wsh/cmd/wshcmd-view.go (5)
frontend/app/store/wshclientapi.ts (1)
CreateBlockCommand(111-113)pkg/wshrpc/wshclient/wshclient.go (1)
CreateBlockCommand(141-144)pkg/remote/fileshare/wshfs/wshfs.go (1)
RpcClient(20-20)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)
cmd/wsh/cmd/wshcmd-deleteblock.go (4)
frontend/app/store/wshclientapi.ts (1)
DeleteBlockCommand(126-128)pkg/wshrpc/wshclient/wshclient.go (1)
DeleteBlockCommand(159-162)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)
frontend/app/view/term/termwrap.ts (1)
frontend/app/store/global.ts (1)
recordTEvent(884-884)
frontend/app/view/term/term-model.ts (1)
frontend/app/store/global.ts (2)
readAtom(876-876)recordTEvent(884-884)
cmd/wsh/cmd/wshcmd-secret.go (3)
frontend/app/store/wshclientapi.ts (1)
CreateBlockCommand(111-113)pkg/wshrpc/wshclient/wshclient.go (1)
CreateBlockCommand(141-144)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)
cmd/wsh/cmd/wshcmd-notify.go (5)
frontend/app/store/wshclientapi.ts (1)
NotifyCommand(401-403)pkg/wshrpc/wshclient/wshclient.go (1)
NotifyCommand(486-489)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)pkg/wshutil/wshrouter.go (1)
ElectronRoute(26-26)
pkg/aiusechat/usechat.go (2)
pkg/aiusechat/uctypes/uctypes.go (1)
ApprovalNeedsApproval(178-178)pkg/aiusechat/toolapproval.go (2)
RegisterToolApproval(49-63)UnregisterToolApproval(36-40)
pkg/web/sse/ssehandler.go (1)
pkg/utilds/idlist.go (1)
IdList(17-20)
pkg/aiusechat/toolapproval.go (2)
pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(66-80)pkg/aiusechat/uctypes/uctypes.go (1)
ApprovalTimeout(181-181)
cmd/wsh/cmd/wshcmd-setmeta.go (4)
frontend/app/store/wshclientapi.ts (1)
SetMetaCommand(536-538)pkg/wshrpc/wshclient/wshclient.go (1)
SetMetaCommand(644-647)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)
cmd/wsh/cmd/wshcmd-editconfig.go (4)
frontend/app/store/wshclientapi.ts (1)
CreateBlockCommand(111-113)pkg/wshrpc/wshclient/wshclient.go (1)
CreateBlockCommand(141-144)cmd/wsh/cmd/wshcmd-root.go (1)
RpcClient(32-32)pkg/wshrpc/wshrpctypes.go (1)
RpcOpts(371-377)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (23)
cmd/wsh/cmd/wshcmd-connserver.go (1)
194-194: LGTM: Clean refactoring to use wshclient wrapper.The change simplifies the RPC invocation by using the high-level
wshclient.ConnUpdateWshCommandwrapper, eliminating the need for manual type assertion of the response. The timeout value (60000ms) and error handling are preserved correctly.cmd/wsh/cmd/wshcmd-deleteblock.go (1)
11-11: LGTM: Consistent adoption of wshclient wrapper.The refactoring correctly replaces the direct RPC call with
wshclient.DeleteBlockCommand, maintaining the same timeout and error handling behavior. The pattern aligns with the broader refactoring across other command files.Also applies to: 39-39
cmd/wsh/cmd/wshcmd-secret.go (1)
14-14: LGTM: Correct wrapper usage for secrets UI.The change correctly uses
wshclient.CreateBlockCommandto open the secrets UI. The ORef return value is appropriately ignored since the command's purpose is to trigger UI display rather than capture the reference.Also applies to: 190-190
cmd/wsh/cmd/wshcmd-notify.go (1)
11-11: LGTM: Notification wrapper correctly preserves routing.The refactoring properly uses
wshclient.NotifyCommandwhile preserving the criticalRoute: wshutil.ElectronRouteoption, ensuring notifications are routed to the Electron frontend as intended.Also applies to: 42-42
cmd/wsh/cmd/wshcmd-setmeta.go (1)
16-16: LGTM: Straightforward wrapper adoption.The change correctly replaces the direct RPC call with
wshclient.SetMetaCommand, maintaining the existing timeout and error handling behavior.Also applies to: 196-196
cmd/wsh/cmd/wshcmd-view.go (1)
16-16: LGTM: View command wrapper correctly applied.The refactoring appropriately uses
wshclient.CreateBlockCommandto create the preview/view block. The ORef return value is intentionally unused since the command's purpose is to display content rather than capture the block reference.Also applies to: 103-103
cmd/wsh/cmd/wshcmd-editconfig.go (1)
12-12: LGTM: Config editor wrapper correctly implemented.The change properly uses
wshclient.CreateBlockCommandto open the configuration editor. The ORef return value is appropriately ignored since the command's goal is to open the editor UI rather than track the block reference.Also applies to: 52-52
frontend/types/gotypes.d.ts (2)
529-533: LGTM!The removal of the
keepaliveproperty aligns with the PR's shift from keepalive-based polling to SSE connection lifecycle for tool approvals.
1237-1237: LGTM!The addition of
"block:controller"?: stringproperly supports telemetry tracking for block controllers, consistent with the backend telemetry structure.pkg/telemetry/telemetrydata/telemetrydata.go (1)
30-30: LGTM!The addition of
"action:term": truecorrectly enables validation for the new terminal-related telemetry events used throughout the frontend (Ctrl+r, SSH connections, editor usage, and tail -f).frontend/app/store/global.ts (1)
461-469: LGTM!The
readAtomhelper provides a safe way to read atom values with null checking. The implementation is clean, well-documented, and properly typed.frontend/app/view/term/term-model.ts (1)
483-490: LGTM!The Ctrl+r telemetry handling is implemented correctly:
- Safely reads the shell integration status using
readAtom- Only records telemetry when shell integration is ready
- Properly returns
falseto allow the key event to propagate to the terminal for its native behaviorpkg/wshrpc/wshrpctypes.go (1)
819-822: LGTM! Simplified struct aligns with SSE lifecycle-based approval.Removing the
KeepAlivefield is consistent with the PR's objective to tie tool approvals to SSE connection lifecycle rather than frontend keepalive packets.pkg/utilds/idlist.go (1)
17-64: Well-designed thread-safe generic registry.The
IdListimplementation is clean and appropriate for the on-close handler use case. The linear scan for removal is acceptable given the expected small number of handlers per SSE connection. TheGetListmethod correctly returns a copy to prevent external mutations.pkg/wshrpc/wshserver/wshserver.go (1)
1259-1261: LGTM! Updated call signature matches the refactored API.The simplified call to
UpdateToolApprovalcorrectly passes onlytoolCallIdandapproval, aligning with the new SSE lifecycle-based approval mechanism that no longer requires keepalive tracking.pkg/aiusechat/usechat.go (2)
343-345: Correct SSE-based registration for tool approval.The registration correctly ties tool approval lifecycle to the SSE connection. The on-close callback in
RegisterToolApproval(pertoolapproval.golines 48-62) ensures that if the SSE connection closes before user action, the approval times out gracefully.
350-354: Good cleanup pattern with unconditional unregistration.Calling
UnregisterToolApprovalunconditionally after each tool call is correct. It's idempotent (safely handles cases where registration didn't occur) and ensures the on-close handler is removed once the tool call completes, regardless of whether it needed approval.pkg/web/sse/ssehandler.go (4)
329-343: Correct once-only handler execution with proper lock management.The
runOnCloseHandlersimplementation correctly:
- Uses
handlersRunflag to ensure handlers execute exactly once- Releases
h.lockbefore callingGetList()and executing handlers, preventing deadlocks if handlers call back intoSSEHandlerChmethods
305-312: Err() with side-effect is pragmatic but worth noting.The
Err()method now updatesh.errif context is cancelled. While having side-effects in a getter is unconventional, it pragmatically consolidates error state and ensures context errors are captured. The lock makes this thread-safe.
243-265: Good non-blocking queue implementation.The
queueMessagemethod correctly:
- Checks closed state and existing errors before attempting to queue
- Uses a non-blocking select with default case to return an error if the channel is full, preventing goroutine blocks
- Handles context cancellation gracefully
The buffered channel (size 10) provides reasonable headroom for bursty writes.
130-162: writerLoop correctly ties on-close handlers to connection lifecycle.The
defer h.runOnCloseHandlers()at line 132 ensures that registered handlers (including tool approval timeouts) execute when the SSE connection closes for any reason: normal close, context cancellation, or write errors. This achieves the PR's goal of tying tool approval lifecycle to SSE connection rather than keepalives.pkg/aiusechat/toolapproval.go (2)
49-63: LGTM — SSE lifecycle binding is well-structured.The approach of tying tool approval to the SSE connection lifecycle (via
RegisterOnClose) is a solid improvement over timer-based keepalives. The cleanup function stored inonCloseUnregFnensures proper resource management when approvals are resolved.
81-83: No deadlock risk exists. The code safely callsonCloseUnregFn()while holdingreq.mubecauseSSEHandlerCh.runOnCloseHandlers()releasesh.lockbefore invoking callbacks, preventing any reverse lock ordering scenario (SSE lock →req.mu).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/view/term/termwrap.ts(2 hunks)pkg/aiusechat/toolapproval.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/view/term/termwrap.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
📚 Learning: 2025-10-15T03:21:02.229Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2433
File: pkg/aiusechat/tools_readfile.go:197-197
Timestamp: 2025-10-15T03:21:02.229Z
Learning: In Wave Terminal's AI tool definitions (pkg/aiusechat/tools_*.go), the Description field should not mention approval requirements even when ToolApproval returns ApprovalNeedsApproval. This prevents the LLM from asking users for approval before calling the tool, avoiding redundant double-approval prompts since the runtime will enforce approval anyway.
Applied to files:
pkg/aiusechat/toolapproval.go
🧬 Code graph analysis (1)
pkg/aiusechat/toolapproval.go (2)
pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(66-80)pkg/aiusechat/uctypes/uctypes.go (1)
ApprovalTimeout(181-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
pkg/aiusechat/toolapproval.go (5)
10-10: LGTM: SSE import added for lifecycle integration.The new import is necessary for the SSE on-close callback mechanism.
13-19: LGTM: Cleanup function added to ApprovalRequest.The
onCloseUnregFnfield properly supports the new SSE lifecycle by storing the cleanup function that unregisters the on-close callback when approval completes.
69-91: LGTM: Idempotent approval update with proper cleanup.The function correctly handles multiple completion paths (normal approval, on-close timeout, or explicit unregister) through idempotent logic (lines 78-80). The order of operations ensures the SSE callback is unregistered before signaling completion via
doneChan.
93-110: LGTM: Proper wait and cleanup semantics.The function correctly waits for approval completion, reads the result with appropriate locking, and cleans up the registry entry. The registry deletion happens here rather than in
UpdateToolApproval, which allowsUpdateToolApprovalto remain idempotent whileWaitForToolApprovalconsumes the result.
58-64: This concern is unfounded. The implementation is thread-safe and handles self-unregistration correctly.The
runOnCloseHandlers()method uses a snapshot pattern:
- It acquires
h.lockand sets thehandlersRunflag- It explicitly releases
h.lockbefore executing handlers- It calls
h.onCloseHandlers.GetList()which creates a new slice copy of the handlers- Handlers execute from this immutable snapshot
When
UnregisterOnClose()is called from within a handler, it acquiresh.lockwithout contention (since the lock was released), and safely modifies the original handlers list. The snapshot being iterated is unaffected by concurrent modifications to the list.No deadlock, concurrent modification, or undefined behavior can occur.
| } | ||
|
|
||
| func RegisterToolApproval(toolCallId string) { | ||
| func RegisterToolApproval(toolCallId string, sseHandler *sse.SSEHandlerCh) { |
There was a problem hiding this comment.
Add nil check for sseHandler parameter.
The function will panic with a nil pointer dereference at line 58 if sseHandler is nil. Add a nil check or document that the parameter must not be nil.
🔎 Proposed fix
func RegisterToolApproval(toolCallId string, sseHandler *sse.SSEHandlerCh) {
+ if sseHandler == nil {
+ panic("sseHandler must not be nil")
+ }
req := &ApprovalRequest{
doneChan: make(chan struct{}),
}Alternatively, document the requirement in a comment if panic is not desired.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/aiusechat/toolapproval.go around line 53, RegisterToolApproval currently
accepts sseHandler and immediately dereferences it which will panic if nil; add
an early nil check at the top of the function (e.g. if sseHandler == nil { log a
warning/error and return } or return an error if you change the signature) so
the function exits without dereferencing a nil pointer, and update any callers
or comments to reflect the non-nil requirement if you choose to keep panics
instead.
tie the tool approvals to the lifecycle of the SSE connection, NOT FE keepalive packets. this prevents timeouts when the FE hasn't rendered the tools (or FE tool rendering lifecycle issues). also allows immediate timeouts on refresh.
other small fixes: convert to use wshclient in wshcmd-*.go files, cleanup in SSE code