Add Write File Tools to WaveAI #2492
Conversation
…proval if a tool will fail input validation, cleanup of tool call func
WalkthroughReorganizes AI roadmap content and adds AI-driven file-diff and file-edit tooling end-to-end. Frontend: new AiFileDiff view/block, DiffViewer (Monaco) with editor:inlinediff setting, AI panel restore/diff UI flows, updated AI tool approval UI, modal ok/cancel disable flags, RPC client methods, block registration, and orphaned-block cleanup triggers. Backend: new pkg/filebackup (make/restore/cleanup), scheduled backup cleanup on startup, write/edit/delete text-file tools with safety checks and backups, read-file blocking, processToolCall refactor and CreateWriteTextFileDiff, BlockService CleanupOrphanedBlocks, new layout action constant, and metadata/settings/type additions across packages and schemas. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| if err := os.WriteFile(filePath, []byte(modifiedContents), fileInfo.Mode()); err != nil { | ||
| if err := os.WriteFile(filePath, modifiedContents, fileInfo.Mode()); err != nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 days ago
To mitigate this vulnerability, ensure that the ReplaceInFile function in pkg/util/fileutil/fileutil.go does not accept file paths that are either absolute or that attempt to escape the intended base safe directory. Since this utility function is generic, it is safest to add a check that its input path is not absolute and does not contain path traversal (e.g., ".."), unless documented to only accept previously-validated paths (and to panic/error otherwise).
The most robust, generic fix is to require a baseDir argument in this function so all paths are resolved relative to a safe base and checked strictly. If that is not possible, add a check at the head of ReplaceInFile that the path is not absolute, does not contain ".." or path separators (if only filenames are permitted), or, if called with a path that is meant to be restricted to a base, validate that the path is within the designated directory. At minimum, check that the provided path is not absolute and does not contain ".." after cleaning, and provide an error otherwise.
The fix requires only changes to ReplaceInFile in pkg/util/fileutil/fileutil.go. No dependencies are needed.
| @@ -288,6 +288,15 @@ | ||
| } | ||
|
|
||
| func ReplaceInFile(filePath string, edits []EditSpec) error { | ||
| // Defense-in-depth: reject absolute paths and path traversal. | ||
| cleanPath := filepath.Clean(filePath) | ||
| if filepath.IsAbs(filePath) { | ||
| return fmt.Errorf("ReplaceInFile: absolute path not allowed: %s", filePath) | ||
| } | ||
| if strings.HasPrefix(cleanPath, "..") || strings.Contains(cleanPath, string(filepath.Separator)+"..") { | ||
| return fmt.Errorf("ReplaceInFile: path traversal not allowed: %s", filePath) | ||
| } | ||
|
|
||
| fileInfo, err := os.Stat(filePath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to stat file: %w", err) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
pkg/util/fileutil/fileutil.go (1)
265-288: Document sequential edit application behavior.The function applies edits sequentially, where each edit operates on the result of all previous edits (line 284 updates
modifiedContents). This means:
- Edit N sees modifications from edits 1 through N-1
- If edit 1 adds text that edit 2's
OldStrmatches, edit 2 will replace that newly added text- If edit 1 removes text that edit 2 expects, edit 2 will fail with "not found"
This behavior may be unexpected for callers who assume edits are independent. Consider adding a doc comment clarifying this sequential application model.
Apply this diff to document the behavior:
-// ApplyEdits applies a series of edits to the given content and returns the modified content. -// Each edit's OldStr must appear exactly once in the content or an error is returned. +// ApplyEdits applies a series of edits sequentially to the given content and returns the modified content. +// Edits are applied in order; each edit operates on the result of all previous edits. +// Each edit's OldStr must be non-empty and appear exactly once in the current content, or an error is returned. func ApplyEdits(originalContent []byte, edits []EditSpec) ([]byte, error) {pkg/aiusechat/tools_readfile.go (4)
152-154: Make.secretsdirectory check more precise.Using
strings.Containsfor.secretsmay produce false positives by matching file paths like/home/user/.secrets_backup/or/home/user/project.secrets/. For consistency with other directory checks (lines 139-144), consider using prefix matching with directory separator.- if strings.Contains(cleanPath, filepath.Join(homeDir, ".secrets")) { - return true, "secrets directory" - } + secretsDir := filepath.Join(homeDir, ".secrets") + string(filepath.Separator) + if strings.HasPrefix(cleanPath, secretsDir) || cleanPath == filepath.Join(homeDir, ".secrets") { + return true, "secrets directory" + }
156-167: Consider adding directory separator for consistency.For consistency with other directory prefix checks (lines 139-144), consider appending a directory separator to
credPathto ensure you're matching directory contents rather than files with similar prefixes.if localAppData := os.Getenv("LOCALAPPDATA"); localAppData != "" { - credPath := filepath.Join(localAppData, "Microsoft", "Credentials") + credPath := filepath.Join(localAppData, "Microsoft", "Credentials") + string(filepath.Separator) if strings.HasPrefix(cleanPath, credPath) { return true, "Windows credentials" } } if appData := os.Getenv("APPDATA"); appData != "" { - credPath := filepath.Join(appData, "Microsoft", "Credentials") + credPath := filepath.Join(appData, "Microsoft", "Credentials") + string(filepath.Separator) if strings.HasPrefix(cleanPath, credPath) { return true, "Windows credentials" } }
169-171: SSH key check may produce false positives.Using
strings.Contains(cleanPath, ".ssh")could match paths like/home/user/my.ssh-config/id_notes.txtthat aren't actually SSH keys. Consider checking for.sshas a path component with directory separators.- if strings.HasPrefix(baseName, "id_") && strings.Contains(cleanPath, ".ssh") { + sshDir := filepath.Join(homeDir, ".ssh") + string(filepath.Separator) + if strings.HasPrefix(baseName, "id_") && strings.HasPrefix(cleanPath, sshDir) { return true, "SSH private key" }
193-195: Remove redundant check.The
.git-credentialsfile is already checked as an exact path at line 120. This check is redundant.- if baseName == ".git-credentials" { - return true, "Git credentials file" - } - return false, ""ROADMAP.md (1)
77-77: Minor: Add hyphen for compound adjective.Static analysis suggests "Non-AI" should be hyphenated when used as a compound adjective.
As per static analysis hints.
-## Other Platform & UX Improvements (Non AI) +## Other Platform & UX Improvements (Non-AI)frontend/app/aipanel/waveai-model.tsx (1)
437-454: Verify toolcallid validation.The validation at line 440 checks
chatIdandfileName, but does not validatetoolcallid. Consider whether an invalid or emptytoolcallidcould cause issues downstream in the diff view.Additionally, the
createBlockcall uses boolean parametersfalse, truewithout clarity on their meaning. Consider adding a brief comment explaining these parameters for maintainability.frontend/app/store/keymodel.ts (1)
155-169: Consider extracting common logic to reduce duplication.The pattern of checking for
aifilediffblocks and focusing the AI input is duplicated betweenuxCloseBlockandgenericClose(lines 202-214). Consider extracting this into a helper function to follow the DRY principle.Example refactor:
+function focusAIIfClosingDiff(blockId: string, isAIPanelOpen: boolean): void { + const blockAtom = WOS.getWaveObjectAtom<Block>(WOS.makeORef("block", blockId)); + const blockData = globalStore.get(blockAtom); + const isAIFileDiff = blockData?.meta?.view === "aifilediff"; + + if (isAIFileDiff && isAIPanelOpen) { + setTimeout(() => WaveAIModel.getInstance().focusInput(), 50); + } +}Then use it in both functions:
const layoutModel = getLayoutModelForStaticTab(); const node = layoutModel.getNodeByBlockId(blockId); if (node) { fireAndForget(() => layoutModel.closeNode(node.id)); - - if (isAIFileDiff && isAIPanelOpen) { - setTimeout(() => WaveAIModel.getInstance().focusInput(), 50); - } + focusAIIfClosingDiff(blockId, isAIPanelOpen); }pkg/filebackup/filebackup.go (1)
100-100: Document or make the retention period configurable.The 5-day retention period is hardcoded. Consider:
- Adding a constant with a descriptive comment explaining why 5 days was chosen
- Making it configurable via an environment variable or config setting
- At minimum, adding a comment here explaining the retention policy
+// BackupRetentionDays defines how many days to retain backup files before cleanup +const BackupRetentionDays = 5 + func CleanupOldBackups() error { backupBaseDir := filepath.Join(wavebase.GetWaveCachesDir(), "waveai-backups") if _, err := os.Stat(backupBaseDir); os.IsNotExist(err) { return nil } entries, err := os.ReadDir(backupBaseDir) if err != nil { return fmt.Errorf("failed to read backup directory: %w", err) } - cutoffTime := time.Now().Add(-5 * 24 * time.Hour) + cutoffTime := time.Now().Add(-BackupRetentionDays * 24 * time.Hour)pkg/aiusechat/tools_writefile.go (2)
116-118: Remove redundant binary data check.The binary data check on lines 116-118 is redundant with the check inside
validateTextFile(line 61) which is called immediately after on line 120. The validation invalidateTextFileis sufficient.func verifyWriteTextFileInput(input any) error { params, err := parseWriteTextFileInput(input) if err != nil { return err } expandedPath, err := wavebase.ExpandHomeDir(params.Filename) if err != nil { return fmt.Errorf("failed to expand path: %w", err) } - contentsBytes := []byte(params.Contents) - if utilfn.HasBinaryData(contentsBytes) { - return fmt.Errorf("contents appear to contain binary data") - } - _, err = validateTextFile(expandedPath, "write to", false) return err }
158-158: Consider security implications of 0644 file permissions.Files are written with
0644permissions (owner read/write, group/others read). For files containing sensitive data, this might be too permissive. Consider:
- Using
0600(owner read/write only) as the default, or- Preserving the original file's permissions if overwriting, or
- Documenting that users should adjust permissions if needed
+ // Use 0600 (owner-only) for new files to prevent accidental exposure + filePerms := os.FileMode(0600) + if fileInfo != nil { + // Preserve existing permissions when overwriting + filePerms = fileInfo.Mode().Perm() + } + - err = os.WriteFile(expandedPath, contentsBytes, 0644) + err = os.WriteFile(expandedPath, contentsBytes, filePerms)frontend/app/view/aifilediff/aifilediff.tsx (1)
94-94: Improve TypeScript error type safety.TypeScript catch blocks have
unknowntype for the error parameter. Accessinge.messagedirectly isn't type-safe. Consider a safer approach:} catch (e) { console.error("Error loading diff data:", e); - globalStore.set(model.errorAtom, `Error loading diff data: ${e.message}`); + const errorMessage = e instanceof Error ? e.message : String(e); + globalStore.set(model.errorAtom, `Error loading diff data: ${errorMessage}`); globalStore.set(model.loadingAtom, false); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
ROADMAP.md(1 hunks)cmd/server/main-server.go(4 hunks)docs/docs/config.mdx(1 hunks)frontend/app/aipanel/aitooluse.tsx(5 hunks)frontend/app/aipanel/aitypes.ts(2 hunks)frontend/app/aipanel/waveai-model.tsx(3 hunks)frontend/app/block/block.tsx(2 hunks)frontend/app/block/blockframe.tsx(4 hunks)frontend/app/store/keymodel.ts(2 hunks)frontend/app/store/services.ts(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/aifilediff/aifilediff.tsx(1 hunks)frontend/app/view/codeeditor/diffviewer.tsx(1 hunks)frontend/layout/lib/layoutModel.ts(4 hunks)frontend/types/gotypes.d.ts(3 hunks)pkg/aiusechat/openai/openai-convertmessage.go(1 hunks)pkg/aiusechat/tools.go(2 hunks)pkg/aiusechat/tools_readfile.go(3 hunks)pkg/aiusechat/tools_writefile.go(1 hunks)pkg/aiusechat/uctypes/usechat-types.go(3 hunks)pkg/aiusechat/usechat.go(6 hunks)pkg/filebackup/filebackup.go(1 hunks)pkg/service/blockservice/blockservice.go(2 hunks)pkg/util/fileutil/fileutil.go(3 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wconfig/metaconsts.go(1 hunks)pkg/wconfig/settingsconfig.go(1 hunks)pkg/wcore/layout.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wshutil/wshrpc.go(2 hunks)schema/settings.json(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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.gopkg/aiusechat/tools.gopkg/aiusechat/tools_writefile.gofrontend/app/aipanel/aitooluse.tsxpkg/aiusechat/usechat.gopkg/aiusechat/uctypes/usechat-types.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:
pkg/wshutil/wshrpc.gopkg/wshrpc/wshserver/wshserver.gopkg/wshrpc/wshclient/wshclient.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:
frontend/app/view/aifilediff/aifilediff.tsxfrontend/app/aipanel/waveai-model.tsxfrontend/app/aipanel/aitypes.tsfrontend/app/aipanel/aitooluse.tsx
🧬 Code graph analysis (21)
frontend/app/store/keymodel.ts (2)
pkg/waveobj/wtype.go (2)
Block(282-290)Block(292-294)frontend/layout/lib/layoutModelHooks.ts (1)
getLayoutModelForStaticTab(45-48)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (1)
WaveAIGetToolDiffCommand(546-548)pkg/wshrpc/wshclient/wshclient.go (1)
WaveAIGetToolDiffCommand(651-654)
pkg/wshrpc/wshserver/wshserver.go (4)
frontend/app/store/wshclientapi.ts (1)
WaveAIGetToolDiffCommand(546-548)pkg/wshrpc/wshclient/wshclient.go (1)
WaveAIGetToolDiffCommand(651-654)pkg/wshrpc/wshrpctypes.go (2)
CommandWaveAIGetToolDiffData(779-782)CommandWaveAIGetToolDiffRtnData(784-787)pkg/aiusechat/usechat.go (1)
CreateWriteTextFileDiff(826-892)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (1)
WaveAIGetToolDiffCommand(546-548)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (3)
CommandWaveAIGetToolDiffData(779-782)RpcOpts(325-331)CommandWaveAIGetToolDiffRtnData(784-787)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (3)
CommandWaveAIGetToolDiffData(779-782)RpcOpts(325-331)CommandWaveAIGetToolDiffRtnData(784-787)
frontend/app/view/aifilediff/aifilediff.tsx (4)
frontend/app/store/global.ts (1)
WOS(866-866)frontend/app/store/wshclientapi.ts (1)
RpcApi(597-597)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/view/codeeditor/diffviewer.tsx (1)
DiffViewer(40-69)
pkg/aiusechat/tools.go (1)
pkg/aiusechat/tools_writefile.go (3)
GetWriteTextFileToolDefinition(169-204)GetEditTextFileToolDefinition(316-378)GetDeleteTextFileToolDefinition(449-480)
pkg/filebackup/filebackup.go (1)
pkg/wavebase/wavebase.go (1)
GetWaveCachesDir(227-232)
pkg/aiusechat/tools_readfile.go (1)
pkg/remote/fileshare/fspath/fspath.go (2)
Base(17-19)Separator(10-10)
pkg/aiusechat/openai/openai-convertmessage.go (2)
pkg/aiusechat/uctypes/usechat-types.go (1)
AIChat(220-226)pkg/aiusechat/openai/openai-backend.go (2)
OpenAIFunctionCallInput(43-51)OpenAIChatMessage(30-36)
frontend/app/aipanel/waveai-model.tsx (2)
frontend/app/store/global.ts (2)
globalStore(839-839)createBlock(820-820)pkg/waveobj/wtype.go (1)
BlockDef(244-247)
pkg/aiusechat/tools_writefile.go (5)
pkg/util/utilfn/marshal.go (2)
MarshalJSONString(207-213)ReUnmarshal(36-42)pkg/util/fileutil/fileutil.go (4)
MaxEditFileSize(256-256)EditSpec(259-263)ApplyEdits(267-288)ReplaceInFile(290-319)pkg/util/utilfn/utilfn.go (1)
HasBinaryData(1010-1017)pkg/filebackup/filebackup.go (1)
MakeFileBackup(26-86)pkg/aiusechat/uctypes/usechat-types.go (2)
ToolDefinition(78-91)ApprovalNeedsApproval(130-130)
frontend/app/aipanel/aitooluse.tsx (2)
frontend/util/util.ts (1)
fireAndForget(466-466)frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(34-455)
frontend/app/block/block.tsx (1)
frontend/app/view/aifilediff/aifilediff.tsx (1)
AiFileDiffViewModel(17-45)
cmd/server/main-server.go (2)
pkg/panichandler/panichandler.go (1)
PanicHandler(25-43)pkg/filebackup/filebackup.go (1)
CleanupOldBackups(88-130)
frontend/layout/lib/layoutModel.ts (3)
frontend/layout/lib/layoutNode.ts (1)
walkNodes(164-173)frontend/app/store/services.ts (1)
BlockService(28-28)pkg/service/blockservice/blockservice.go (3)
BlockService(22-22)BlockService(39-44)BlockService(93-98)
pkg/aiusechat/usechat.go (7)
pkg/aiusechat/uctypes/usechat-types.go (7)
WaveToolCall(167-172)WaveChatOpts(423-443)ToolDefinition(78-91)AIToolResult(290-295)ToolUseStatusError(125-125)ToolUseStatusCompleted(126-126)AIMetrics(236-252)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/logutil/logutil.go (1)
DevPrintf(13-17)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/openai/openai-convertmessage.go (1)
GetFunctionCallInputByToolCallId(660-671)pkg/aiusechat/tools_writefile.go (1)
EditTextFileDryRun(250-282)
pkg/aiusechat/uctypes/usechat-types.go (1)
pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
frontend/app/block/blockframe.tsx (2)
frontend/app/store/global.ts (3)
atoms(816-816)getSettingsKeyAtom(835-835)WOS(866-866)frontend/app/block/block-model.ts (1)
BlockModel(12-51)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (2)
CommandWaveAIGetToolDiffData(779-782)CommandWaveAIGetToolDiffRtnData(784-787)
pkg/service/blockservice/blockservice.go (4)
frontend/app/store/services.ts (1)
CleanupOrphanedBlocks(12-14)pkg/waveobj/wtype.go (2)
UpdatesRtnType(12-12)LayoutActionData(206-217)pkg/waveobj/ctxupdate.go (2)
ContextWithUpdates(32-40)ContextGetUpdatesRtn(127-137)pkg/wcore/layout.go (2)
LayoutActionDataType_CleanupOrphaned(25-25)QueueLayoutActionForTab(107-114)
🪛 LanguageTool
ROADMAP.md
[grammar] ~77-~77: Use a hyphen to join words.
Context: ...## Other Platform & UX Improvements (Non AI) - 🔷 Import/Export tab layouts and ...
(QB_NEW_EN_HYPHEN)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (28)
pkg/aiusechat/openai/openai-convertmessage.go (1)
658-671: LGTM! Clean and correct implementation.The helper function correctly iterates through the chat messages, safely type-asserts to OpenAI messages, and returns the matching function call input. The logic is straightforward and appropriate for a lookup function.
pkg/util/fileutil/fileutil.go (1)
7-7: LGTM! Clean refactor extracting edit logic.The addition of the
bytesimport and refactoring ofReplaceInFileto delegate toApplyEditsimproves testability and code organization. The error handling is properly maintained.Also applies to: 309-314
pkg/aiusechat/tools_readfile.go (1)
214-216: LGTM: Security check properly integrated.The blocked file check is correctly placed after path expansion but before any file system operations (stat, open, read). The error message appropriately includes the descriptive reason from
isBlockedFile, providing clear feedback when access is denied.pkg/wcore/layout.go (1)
25-25: Constant aligns with new action flow.String value matches the backend/FE handler expectations; thanks for keeping the enum in sync.
frontend/app/store/services.ts (1)
10-14: Service wrapper looks good.Wrapper follows existing generated conventions and keeps the call surface aligned with the new backend entry point.
pkg/service/blockservice/blockservice.go (1)
93-111: Backend queuing looks solid.Good to see the action wrapped in ContextWithUpdates and queued with a fresh action ID; this lines up with the frontend handler.
pkg/wconfig/settingsconfig.go (1)
93-93: LGTM!The new
EditorInlineDifffield follows the established pattern for editor configuration settings and is properly tagged.docs/docs/config.mdx (1)
67-67: LGTM!The documentation clearly explains the new
editor:inlinediffconfiguration option and its default responsive behavior.schema/settings.json (1)
116-118: LGTM!The schema addition for
editor:inlinediffis correct and consistent with the other editor configuration properties.cmd/server/main-server.go (3)
55-56: LGTM!The backup cleanup timing constants follow the established pattern used for telemetry timing.
120-135: LGTM!The backup cleanup loop is properly implemented with panic handling, error logging, and appropriate timing logic. The function structure mirrors the existing telemetry loops, ensuring consistency.
436-436: LGTM!The cleanup loop is correctly scheduled as a goroutine, consistent with the other background loops in the application.
pkg/wconfig/metaconsts.go (1)
47-47: LGTM!The new constant follows the established naming convention and is correctly positioned with other editor configuration keys. The file header indicates this is generated code, which is appropriate for maintaining consistency across configuration constants.
frontend/app/aipanel/waveai-model.tsx (1)
415-417: LGTM!Simple getter method to retrieve the current chat ID from the store.
pkg/wshutil/wshrpc.go (1)
272-277: LGTM!The addition of pprof profiling labels for RPC handlers improves observability by allowing profiling data to be segmented by command type. The refactoring cleanly separates the profiling instrumentation from the core handler logic.
frontend/app/store/wshclientapi.ts (1)
545-548: LGTM!The new RPC command method follows the established pattern and correctly maps to the backend implementation.
pkg/aiusechat/tools.go (2)
123-124: LGTM!Returning an empty string for
aifilediffblocks is appropriate since these are internal UI views for displaying diffs, not workspace state that the AI needs to reason about.
161-163: LGTM!The three new file operation tools are properly added within the
widgetAccessconditional block, following the established pattern for other widget-related tools.pkg/wshrpc/wshrpctypes.go (1)
147-147: LGTM!The new RPC command infrastructure follows the established patterns:
- Command constant matches the expected naming convention
- Interface method signature is consistent with other RPC commands
- Data structures are well-defined with appropriate json tags
- Base64 encoding for file contents is a sensible choice for handling potentially binary data
Also applies to: 291-291, 779-787
frontend/app/block/block.tsx (1)
12-12: LGTM!The registration of the
aifilediffview type follows the established pattern for other block views in the BlockRegistry.Also applies to: 54-54
frontend/app/aipanel/aitypes.ts (1)
23-23: LGTM!The addition of the optional
inputfilenamefield to thetoolusetype is a straightforward extension that enables the UI to display file-related information for tool calls without breaking existing functionality.frontend/app/block/blockframe.tsx (1)
485-485: LGTM!The addition of a default border color for unfocused ephemeral blocks without an explicit border provides helpful visual distinction. The semi-transparent white color is subtle yet effective.
Also applies to: 512-514
pkg/waveobj/wtypemeta.go (1)
78-79: LGTM!The two new metadata fields follow the established naming conventions and struct patterns. The
omitemptytags are appropriately used for optional metadata fields.pkg/wshrpc/wshclient/wshclient.go (1)
650-654: LGTM!The generated RPC command wrapper follows the established pattern and is consistent with other commands in the file.
frontend/app/view/aifilediff/aifilediff.tsx (1)
83-84: atob() error handling is covered by outer try-catch.The
atob()calls could throw if the base64 data is malformed, but they're already inside the try-catch block that starts at line 71, so errors will be caught and handled properly. The current error handling is sufficient.pkg/aiusechat/usechat.go (3)
299-308: InputFileName extraction is redundant but acceptable.Lines 299-308 extract
InputFileNamefrom the tool input, but this is already done inprocessToolCalls(lines 411-418) before this function is called. The redundancy serves as defensive programming, ensuringInputFileNameis always set, which is acceptable for robustness.
322-333: Excellent addition of input validation hook.The
ToolVerifyInputhook provides a clean way for tool definitions to validate inputs before execution. The error handling properly sets the status and returns an error result, preventing invalid tool calls from proceeding. This is a well-designed addition to the tool execution pipeline.
880-887: Original file reading properly handles non-existent files.The logic correctly handles cases where the original file doesn't exist (line 885 checks
IsNotExist), allowingoriginalContentto be empty for newly created files. This is the correct behavior for thewrite_text_filetool creating new files.
|
|
||
| export function DiffViewer({ blockId, original, modified, language, fileName }: DiffViewerProps) { | ||
| const minimapEnabled = useOverrideConfigAtom(blockId, "editor:minimapenabled") ?? false; | ||
| const fontSize = boundNumber(useOverrideConfigAtom(blockId, "editor:fontsize"), 6, 64); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify fontSize default handling.
If useOverrideConfigAtom returns null or undefined, ensure boundNumber provides a sensible default fontSize (e.g., 12). Otherwise, opts.fontSize on line 49 might be set to undefined, which could cause Monaco to use its own default or throw an error.
Run the following script to check the boundNumber implementation:
🏁 Script executed:
#!/bin/bash
# Description: Verify boundNumber handles null/undefined inputs
# Search for boundNumber function definition
rg -A 10 'function boundNumber|export.*boundNumber'Length of output: 698
I found a critical type mismatch in the boundNumber function. Let me gather more context about how useOverrideConfigAtom and Monaco handle the potential null return value.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find useOverrideConfigAtom to see what it returns
rg -A 5 'function useOverrideConfigAtom|export.*useOverrideConfigAtom' --type=ts --type=tsx
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check line 49 and surrounding context to see how fontSize is used
cat frontend/app/view/codeeditor/diffviewer.tsx | sed -n '40,55p'Length of output: 729
🏁 Script executed:
#!/bin/bash
# Find other usages of boundNumber to see how null returns are handled
rg 'boundNumber\(' frontend/app/view/codeeditor/diffviewer.tsx -A 2 -B 2Length of output: 396
Let me check the Monaco editor options type to understand what happens when fontSize is null:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find defaultDiffEditorOptions to see the Monaco opts type
rg -B 5 -A 15 'defaultDiffEditorOptions' frontend/app/view/codeeditor/diffviewer.tsx
Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Check what Monaco expects for fontSize - search imports
rg 'from.*monaco|import.*monaco' frontend/app/view/codeeditor/diffviewer.tsx -A 2 -B 2Length of output: 221
🏁 Script executed:
#!/bin/bash
# Search for other fontSize assignments to see how they handle null
rg 'fontSize.*=' frontend/ --type=ts -B 2 -A 2 | head -50Length of output: 4275
Add null-safety default to fontSize assignment.
The boundNumber function returns null when input is null/undefined/NaN, but line 42 doesn't provide a fallback like other options do (e.g., line 41 uses ?? false). If useOverrideConfigAtom returns null, fontSize becomes null and is passed to Monaco editor, violating the type contract.
Fix: const fontSize = boundNumber(useOverrideConfigAtom(blockId, "editor:fontsize"), 6, 64) ?? 12;
Also apply the same fix in frontend/app/view/codeeditor/codeeditor.tsx which has identical code.
🤖 Prompt for AI Agents
In frontend/app/view/codeeditor/diffviewer.tsx around line 42, the fontSize
assignment uses boundNumber(...) which may return null; change it to const
fontSize = boundNumber(useOverrideConfigAtom(blockId, "editor:fontsize"), 6, 64)
?? 12; to provide a safe default; also make the identical change in
frontend/app/view/codeeditor/codeeditor.tsx where the same pattern appears so
both components never pass null to the Monaco editor.
| func isBlockedFile(expandedPath string) (bool, string) { | ||
| homeDir := os.Getenv("HOME") | ||
| if homeDir == "" { | ||
| homeDir = os.Getenv("USERPROFILE") | ||
| } | ||
|
|
||
| cleanPath := filepath.Clean(expandedPath) | ||
| baseName := filepath.Base(cleanPath) | ||
|
|
||
| exactPaths := []struct { | ||
| path string | ||
| reason string | ||
| }{ | ||
| {filepath.Join(homeDir, ".aws", "credentials"), "AWS credentials file"}, | ||
| {filepath.Join(homeDir, ".git-credentials"), "Git credentials file"}, | ||
| {filepath.Join(homeDir, ".netrc"), "netrc credentials file"}, | ||
| {filepath.Join(homeDir, ".pgpass"), "PostgreSQL password file"}, | ||
| {filepath.Join(homeDir, ".my.cnf"), "MySQL credentials file"}, | ||
| {filepath.Join(homeDir, ".kube", "config"), "Kubernetes config file"}, | ||
| {"/etc/shadow", "system password file"}, | ||
| {"/etc/sudoers", "system sudoers file"}, | ||
| } | ||
|
|
||
| for _, ep := range exactPaths { | ||
| if cleanPath == ep.path { | ||
| return true, ep.reason | ||
| } | ||
| } | ||
|
|
||
| dirPrefixes := []struct { | ||
| prefix string | ||
| reason string | ||
| }{ | ||
| {filepath.Join(homeDir, ".gnupg") + string(filepath.Separator), "GPG directory"}, | ||
| {filepath.Join(homeDir, ".password-store") + string(filepath.Separator), "password store directory"}, | ||
| {"/etc/sudoers.d/", "system sudoers directory"}, | ||
| {"/Library/Keychains/", "macOS keychain directory"}, | ||
| {filepath.Join(homeDir, "Library", "Keychains") + string(filepath.Separator), "macOS keychain directory"}, | ||
| } | ||
|
|
||
| for _, dp := range dirPrefixes { | ||
| if strings.HasPrefix(cleanPath, dp.prefix) { | ||
| return true, dp.reason | ||
| } | ||
| } | ||
|
|
||
| if strings.Contains(cleanPath, filepath.Join(homeDir, ".secrets")) { | ||
| return true, "secrets directory" | ||
| } | ||
|
|
||
| if localAppData := os.Getenv("LOCALAPPDATA"); localAppData != "" { | ||
| credPath := filepath.Join(localAppData, "Microsoft", "Credentials") | ||
| if strings.HasPrefix(cleanPath, credPath) { | ||
| return true, "Windows credentials" | ||
| } | ||
| } | ||
| if appData := os.Getenv("APPDATA"); appData != "" { | ||
| credPath := filepath.Join(appData, "Microsoft", "Credentials") | ||
| if strings.HasPrefix(cleanPath, credPath) { | ||
| return true, "Windows credentials" | ||
| } | ||
| } | ||
|
|
||
| if strings.HasPrefix(baseName, "id_") && strings.Contains(cleanPath, ".ssh") { | ||
| return true, "SSH private key" | ||
| } | ||
| if strings.Contains(baseName, "id_rsa") { | ||
| return true, "SSH private key" | ||
| } | ||
| if strings.HasPrefix(baseName, "ssh_host_") && strings.Contains(baseName, "key") { | ||
| return true, "SSH host key" | ||
| } | ||
|
|
||
| extensions := map[string]string{ | ||
| ".pem": "certificate/key file", | ||
| ".p12": "certificate file", | ||
| ".key": "key file", | ||
| ".pfx": "certificate file", | ||
| ".pkcs12": "certificate file", | ||
| ".keystore": "Java keystore file", | ||
| ".jks": "Java keystore file", | ||
| } | ||
|
|
||
| if reason, exists := extensions[filepath.Ext(baseName)]; exists { | ||
| return true, reason | ||
| } | ||
|
|
||
| if baseName == ".git-credentials" { | ||
| return true, "Git credentials file" | ||
| } | ||
|
|
||
| return false, "" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Symlinks could bypass security checks.
The function uses filepath.Clean which normalizes paths but does not resolve symlinks. A malicious user could create a symlink to a blocked file (e.g., ln -s ~/.ssh/id_rsa /tmp/innocuous.txt) and bypass these security checks entirely, since the function would evaluate the symlink path rather than the target.
Consider resolving symlinks before checking:
func isBlockedFile(expandedPath string) (bool, string) {
+ // Resolve symlinks to prevent bypass
+ resolvedPath, err := filepath.EvalSymlinks(expandedPath)
+ if err != nil {
+ // If we can't resolve, check both paths for safety
+ resolvedPath = expandedPath
+ }
+
homeDir := os.Getenv("HOME")
if homeDir == "" {
homeDir = os.Getenv("USERPROFILE")
}
- cleanPath := filepath.Clean(expandedPath)
+ cleanPath := filepath.Clean(resolvedPath)
baseName := filepath.Base(cleanPath)Verify whether symlink traversal is a concern in your threat model:
🏁 Script executed:
#!/bin/bash
# Check if there are other file operation tools that might have the same vulnerability
rg -n "os\.Open|os\.ReadFile|os\.Stat" --type go -g "pkg/aiusechat/tools_*.go" -C 2Length of output: 1576
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where expandedPath is created and how isBlockedFile is called
rg -n "expandedPath\s*:=|expandedPath\s*=" --type go -g "pkg/aiusechat/tools_*.go" -B5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find calls to isBlockedFile to see the context
rg -n "isBlockedFile" --type go -g "pkg/aiusechat/tools_*.go" -A3 -B3Length of output: 1436
🏁 Script executed:
#!/bin/bash
# Look at the full readfile function to understand the flow
rg -n "func.*Read.*\(" --type go -g "pkg/aiusechat/tools_readfile.go" -A20 | head -100Length of output: 2857
🏁 Script executed:
#!/bin/bash
# Check if filepath.EvalSymlinks is used anywhere in the codebase
rg -n "EvalSymlinks|Lstat" --type goLength of output: 76
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full ReadTextFile function implementation
rg -n "^func ReadTextFile" --type go -g "pkg/aiusechat/tools_readfile.go" -A60
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check what os.ExpandUser does and how expandedPath is created
rg -n "filepath.Abs|os.ExpandUser|ExpandUser" --type go -g "pkg/aiusechat/tools_readfile.go" -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at how validateTextFile in writefile uses Lstat after isBlockedFile
rg -n "func validateTextFile" --type go -g "pkg/aiusechat/tools_writefile.go" -A15Length of output: 1077
---
🏁 Script executed:
```shell
#!/bin/bash
# Get context around line 214 where isBlockedFile is called in readfile.go
sed -n '200,230p' $(find . -name "tools_readfile.go" -type f)
Length of output: 873
🏁 Script executed:
#!/bin/bash
# Search for the function that contains line 214
rg -n "func.*expandedPath" --type go -g "pkg/aiusechat/tools_readfile.go" -A40 | head -80Length of output: 2872
🏁 Script executed:
#!/bin/bash
# Look for all functions in readfile.go to find which one contains line 214
ast-grep --pattern 'func $_($_) {
$$$
}'Length of output: 185617
🏁 Script executed:
#!/bin/bash
# Try simpler approach - get lines 180-240 to see full function context
sed -n '180,240p' $(find . -name "tools_readfile.go" -type f 2>/dev/null | head -1)Length of output: 1580
Symlink security bypass in readTextFileCallback.
The readTextFileCallback function uses wavebase.ExpandHomeDir() which only expands ~ but does not resolve symlinks. A malicious user can create a symlink to a blocked file (e.g., ln -s ~/.ssh/id_rsa /tmp/innocuous.txt) and bypass the security checks entirely. The isBlockedFile() check examines the unresolved path (which passes), but subsequent operations like os.Stat() and os.Open() follow the symlink to the actual target file, exposing sensitive data.
Note: tools_writefile.go already uses os.Lstat() instead, which properly detects symlinks without following them—this pattern should be applied consistently here.
Replace the file operations after the security check:
if blocked, reason := isBlockedFile(expandedPath); blocked {
return nil, fmt.Errorf("access denied: potentially sensitive file: %s", reason)
}
- fileInfo, err := os.Stat(expandedPath)
+ fileInfo, err := os.Lstat(expandedPath)
+ if err == nil && fileInfo.Mode()&os.ModeSymlink != 0 {
+ return nil, fmt.Errorf("access denied: symlinks are not allowed")
+ }
if err != nil {
return nil, fmt.Errorf("failed to stat file: %w", err)
}Committable suggestion skipped: line range outside the PR's diff.
| homeDir := os.Getenv("HOME") | ||
| if homeDir == "" { | ||
| homeDir = os.Getenv("USERPROFILE") | ||
| } |
There was a problem hiding this comment.
Consider handling empty home directory.
If both HOME and USERPROFILE environment variables are unset, homeDir will be an empty string. Subsequent filepath.Join(homeDir, ...) calls will produce paths like /.aws/credentials instead of failing gracefully, potentially causing incorrect path matching logic.
Consider adding a check:
homeDir := os.Getenv("HOME")
if homeDir == "" {
homeDir = os.Getenv("USERPROFILE")
}
+ if homeDir == "" {
+ // Skip home-based checks if home directory cannot be determined
+ return false, ""
+ }Alternatively, skip home-based path checks when homeDir is empty.
🤖 Prompt for AI Agents
In pkg/aiusechat/tools_readfile.go around lines 107 to 110, the code reads HOME
then USERPROFILE but does not handle the case where both are unset, leaving
homeDir empty and causing filepath.Join(homeDir, ...) to produce invalid
absolute paths like "/.aws/credentials"; update the logic to detect an empty
homeDir and either (a) skip any home-based path checks when homeDir == "" or (b)
return an explicit error indicating no home directory found before using
filepath.Join; implement the chosen approach consistently where homeDir is used
to avoid constructing incorrect paths.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/aipanel/aitooluse.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
frontend/app/aipanel/aitooluse.tsx
📚 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:
frontend/app/aipanel/aitooluse.tsx
🧬 Code graph analysis (1)
frontend/app/aipanel/aitooluse.tsx (2)
frontend/util/util.ts (1)
fireAndForget(466-466)frontend/app/aipanel/waveai-model.tsx (1)
WaveAIModel(34-455)
⏰ 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). (5)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Build Docsite
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (3)
frontend/app/aipanel/aitooluse.tsx (3)
10-12: LGTM! Effective refactor consolidating approval logic.The helper correctly computes the effective approval status based on streaming state, reducing duplication across components.
5-5: LGTM! Diff viewer integration is well-implemented.The Show Diff button is appropriately displayed for write operations with file input, and
fireAndForgetcorrectly handles the asyncopenDiffcall without blocking the UI.Also applies to: 148-149, 213-215, 234-243
255-257: LGTM! Type definition is clear and well-structured.The discriminated union appropriately models the two rendering paths for grouped tool calls.
…ol calls by passing the toolusedata struct
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/filebackup/filebackup.go (1)
63-85: Clean up orphaned backups when metadata creation fails.If metadata marshalling or the JSON write fails we return an error but leave the freshly written
.bakfile (and possibly a partial.json) behind. These orphans can accumulate and complicate restore/debug flows. Please delete the backup artifacts before returning on those error paths.metadataJSON, err := json.MarshalIndent(metadata, "", " ") if err != nil { + _ = os.Remove(backupPath) return "", fmt.Errorf("failed to marshal backup metadata: %w", err) } metadataName := fmt.Sprintf("%s.%s.%s.json", basename, dirHash8, uuidStr) metadataPath := filepath.Join(backupDir, metadataName) err = os.WriteFile(metadataPath, metadataJSON, 0600) if err != nil { + _ = os.Remove(backupPath) + _ = os.Remove(metadataPath) return "", fmt.Errorf("failed to write backup metadata: %w", err) }pkg/aiusechat/tools_writefile.go (2)
18-18: Consider documenting the rationale for the smaller size limit.The
MaxEditFileSizeconstant here is set to 100KB, which is significantly smaller thanfileutil.MaxEditFileSize(5MB). While this appears intentional for AI safety constraints, consider adding a comment explaining why AI file operations have stricter limits than general file operations.
36-42: Symlink error message has awkward capitalization.The error message capitalizes the verb after the ellipsis:
"cannot %s symlinks... %s the target file". This creates messages like "cannot write symlinks... Write the target file" where the mid-sentence capitalization looks odd.Apply this diff to improve the message:
- return nil, fmt.Errorf("cannot %s symlinks (target: %s). %s the target file directly if needed", verb, utilfn.MarshalJSONString(target), verb) + return nil, fmt.Errorf("cannot %s symlinks (target: %s); %s the target file directly if needed", verb, utilfn.MarshalJSONString(target), verb)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pkg/aiusechat/tools.go(3 hunks)pkg/aiusechat/tools_builder.go(3 hunks)pkg/aiusechat/tools_readdir.go(1 hunks)pkg/aiusechat/tools_readdir_test.go(5 hunks)pkg/aiusechat/tools_readfile.go(3 hunks)pkg/aiusechat/tools_term.go(2 hunks)pkg/aiusechat/tools_tsunami.go(2 hunks)pkg/aiusechat/tools_web.go(1 hunks)pkg/aiusechat/tools_writefile.go(1 hunks)pkg/aiusechat/uctypes/usechat-types.go(3 hunks)pkg/aiusechat/usechat.go(7 hunks)pkg/filebackup/filebackup.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/aiusechat/tools_readfile.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/tools_web.gopkg/aiusechat/tools_writefile.gopkg/aiusechat/tools_builder.gopkg/aiusechat/tools_readdir.gopkg/aiusechat/tools_term.gopkg/aiusechat/tools.gopkg/aiusechat/tools_readdir_test.gopkg/aiusechat/usechat.gopkg/aiusechat/uctypes/usechat-types.gopkg/aiusechat/tools_tsunami.go
🧬 Code graph analysis (11)
pkg/aiusechat/tools_web.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/tools_writefile.go (5)
pkg/util/utilfn/marshal.go (2)
MarshalJSONString(207-213)ReUnmarshal(36-42)pkg/util/fileutil/fileutil.go (4)
MaxEditFileSize(256-256)EditSpec(259-263)ApplyEdits(267-288)ReplaceInFile(290-319)pkg/util/utilfn/utilfn.go (1)
HasBinaryData(1010-1017)pkg/aiusechat/uctypes/usechat-types.go (3)
UIMessageDataToolUse(138-148)ToolDefinition(78-92)ApprovalNeedsApproval(131-131)pkg/filebackup/filebackup.go (1)
MakeFileBackup(28-88)
pkg/aiusechat/tools_builder.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/tools_readdir.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/tools_term.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/tools.go (2)
pkg/aiusechat/tools_writefile.go (3)
GetWriteTextFileToolDefinition(175-210)GetEditTextFileToolDefinition(328-390)GetDeleteTextFileToolDefinition(467-498)pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/tools_readdir_test.go (1)
pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
pkg/aiusechat/usechat.go (7)
pkg/aiusechat/uctypes/usechat-types.go (6)
WaveToolCall(168-173)ToolDefinition(78-92)AIToolResult(291-296)ToolUseStatusError(126-126)ToolUseStatusCompleted(127-127)AIMetrics(237-253)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/logutil/logutil.go (1)
DevPrintf(13-17)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/openai/openai-convertmessage.go (1)
GetFunctionCallInputByToolCallId(660-671)pkg/aiusechat/tools_writefile.go (1)
EditTextFileDryRun(261-293)
pkg/aiusechat/uctypes/usechat-types.go (1)
pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
pkg/filebackup/filebackup.go (1)
pkg/wavebase/wavebase.go (1)
GetWaveCachesDir(227-232)
pkg/aiusechat/tools_tsunami.go (2)
pkg/blockcontroller/blockcontroller.go (1)
BlockControllerRuntimeStatus(54-61)pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolUse(138-148)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (17)
pkg/aiusechat/tools_term.go (1)
193-213: LGTM! Signature updates align with expanded ToolAnyCallback type.The callbacks now accept the additional
toolUseDataparameter to match the updatedToolAnyCallbacksignature inuctypes.ToolDefinition. Although the parameter is currently unused in these implementations, the signature change maintains consistency across the tool system and prepares for future use cases where tool-use metadata may be needed.Also applies to: 269-300
pkg/aiusechat/tools_builder.go (1)
61-81: LGTM! Consistent signature updates across builder tools.All three builder tool definitions (Write App File, Edit App File, List App Files) have been updated to accept the
toolUseDataparameter in theirToolAnyCallbackimplementations. The business logic remains unchanged, and the signature updates maintain consistency with the updatedToolDefinitiontype.Also applies to: 152-172, 191-198
pkg/aiusechat/uctypes/usechat-types.go (2)
79-91: LGTM! Well-designed extension to ToolDefinition.The addition of
ToolVerifyInputhook and the updatedToolAnyCallbacksignature enable proper input validation and tool-use metadata tracking. This is a clean, extensible design that supports the new file operation workflows while maintaining backward compatibility through the optional nature ofToolVerifyInput.
138-148: LGTM! UIMessageDataToolUse extension supports file operation metadata.The new
WriteBackupFileNameandInputFileNamefields enable proper tracking of file operations for diff generation and UI display. The fields are properly tagged withomitemptyfor optional serialization.pkg/aiusechat/tools_writefile.go (6)
104-173: LGTM! Write text file implementation is secure and well-structured.The implementation properly:
- Validates input in both
verifyWriteTextFileInputandwriteTextFileCallback(defense in depth)- Creates backups before overwriting existing files
- Populates
WriteBackupFileNamecorrectly for diff generation- Checks for binary data to prevent misuse
- Sets reasonable file permissions (0644)
175-210: LGTM! Tool definition follows established patterns.The tool definition properly:
- Requires approval via
ToolApproval(matching learned pattern)- Uses
ToolVerifyInputfor pre-execution validation- Provides clear descriptions without mentioning approval (per learnings)
- Uses strict schema validation
Based on learnings
239-326: LGTM! Edit text file implementation is robust and supports diff generation.The implementation properly:
- Validates input and requires file to exist
- Creates backups before editing
- Populates
WriteBackupFileNamefor diff workflows- Uses atomic edit operations via
fileutil.ReplaceInFile- Provides
EditTextFileDryRunas a public function for diff generation with backup file override support
328-390: LGTM! Tool definition with comprehensive edit specifications.The edit tool definition provides excellent guidance for the LLM:
- Clear atomicity guarantees ("if any single edit fails, the entire operation fails")
- Explicit uniqueness requirements for
old_str- Required
descfield for each edit to document intent- Proper approval and verification hooks
414-465: LGTM! Delete text file implementation with proper backup safeguards.The implementation properly:
- Validates input and requires file to exist
- Creates backups before deletion (enabling recovery)
- Populates
WriteBackupFileNamefor audit/recovery workflows- Uses straightforward
os.Removeafter validation
467-498: LGTM! Delete tool definition with appropriate safety messaging.The tool definition clearly communicates that "A backup is created before deletion", which is important for user confidence when approving destructive operations.
pkg/aiusechat/usechat.go (7)
311-322: Verify validation timing relative to approval flow.The
ToolVerifyInputvalidation now runs before the approval check (lines 324-351). This means invalid inputs will be rejected immediately without requiring user approval. While this is efficient, consider whether certain validations (like file existence checks) should happen after approval to avoid exposing filesystem information before the user approves the operation.However, the current order may be intentional for UX: catching obvious input errors early prevents users from approving operations that will fail anyway.
If the current validation timing is intentional for UX (fail-fast on bad input), the implementation is correct. If validation should happen post-approval for security reasons, the order of lines 311-322 and 324-351 should be swapped.
290-363: LGTM! Clean refactoring with proper separation of concerns.The extraction of
processToolCallInternalimproves code organization by isolating the core tool processing logic (validation, approval, execution, status updates) from the logging and metrics tracking in the wrapperprocessToolCallfunction. The error handling and status management are comprehensive.
365-389: LGTM! Well-structured wrapper with proper logging and metrics.The refactored
processToolCallfunction cleanly separates concerns:
- Delegates core logic to
processToolCallInternal- Handles logging with truncated output for readability
- Tracks detailed metrics using
ToolLogName- Ensures UI and chat state updates after processing
400-407: LGTM! Pre-execution filename extraction improves UX.Extracting the filename from tool input before execution enables the UI to display the target file while waiting for approval. The implementation handles both
filenameandfile_nameconventions with proper type checking.
812-881: LGTM! CreateWriteTextFileDiff properly implements diff generation.The function correctly:
- Validates chat existence and API type support
- Extracts
WriteBackupFileNamefromToolUseData(resolving the past review issue)- Handles both
edit_text_file(viaEditTextFileDryRun) andwrite_text_file- Falls back to reading the original file when no backup exists
- Gracefully handles new file creation (no original content)
The past review concern about
WriteBackupFileNamenot being populated is now resolved in this PR, as the tool callbacks inpkg/aiusechat/tools_writefile.goproperly capture the backup path fromfilebackup.MakeFileBackup()and assign it totoolUseData.WriteBackupFileName.
29-29: LGTM! Import addition supports path expansion in CreateWriteTextFileDiff.The
wavebaseimport is properly used forExpandHomeDirfunctionality in the newCreateWriteTextFileDifffunction (line 869).
565-565: LGTM! ToolAnyCallback invocation updated to match new signature.The callback now properly passes
toolCall.ToolUseDataas the second parameter, aligning with the updatedToolAnyCallbacktype signature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/aiusechat/usechat.go (1)
290-364: Clean refactor with input validation integration.The extraction of
processToolCallInternaleffectively separates validation/approval logic from logging/metrics. The newToolVerifyInputintegration (lines 311-322) correctly validates inputs before proceeding with the approval flow.One minor clarity improvement: the comment on line 348 could be more precise:
- // this still happens here because we need to update the FE to say the tool call was approved + // Update FE and chat store to reflect approval status before tool execution
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/aipanel/aitypes.ts(1 hunks)pkg/aiusechat/uctypes/usechat-types.go(4 hunks)pkg/aiusechat/usechat.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/aipanel/aitypes.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/uctypes/usechat-types.gopkg/aiusechat/usechat.go
🧬 Code graph analysis (1)
pkg/aiusechat/usechat.go (4)
pkg/aiusechat/uctypes/usechat-types.go (3)
WaveToolCall(171-176)ToolDefinition(79-93)AIToolResult(294-299)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/openai/openai-convertmessage.go (1)
GetFunctionCallInputByToolCallId(660-671)pkg/aiusechat/tools_writefile.go (1)
EditTextFileDryRun(261-293)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (7)
pkg/aiusechat/uctypes/usechat-types.go (2)
80-93: Well-structured extension of tool callback signatures.The updated
ToolAnyCallbacksignature and newToolVerifyInputfield provide tool implementations with richer context (*UIMessageDataToolUse), enabling better validation and processing. The breaking change appears intentional and properly propagated across tool implementations.
70-76: Frontend types verified and synchronized.The TypeScript type
WaveUIDataTypes.userfileinfrontend/app/aipanel/aitypes.ts(lines 8-13) correctly includes both new fields:
mimetype: string(maps to Go'sMimeType)previewurl?: string(maps to Go'sPreviewUrl)The backend and frontend types are in sync.
pkg/aiusechat/usechat.go (5)
29-29: LGTM!The
wavebaseimport is correctly added to support path expansion inCreateWriteTextFileDiff.
366-390: LGTM!The refactored
processToolCallwrapper cleanly handles logging, metrics, and SSE updates while delegating core logic toprocessToolCallInternal. The separation of concerns improves maintainability.
401-408: LGTM!The filename extraction logic defensively handles both
filenameandfile_namefield names, ensuring the UI can display the target file before tool execution. This enhances the user experience by providing context in the approval UI.
813-882: Well-structured diff generation with comprehensive error handling.The
CreateWriteTextFileDifffunction correctly:
- Validates API type support (OpenAI only)
- Retrieves and validates tool call data
- Handles both
write_text_fileandedit_text_fileoperations- Gracefully handles non-existent files for new file writes (line 875)
- Provides clear, specific error messages for each failure mode
The fallback logic for reading original content (backup file first, then current file) aligns with the tool execution flow.
124-124: No issues found—comment is accurate.The
CurrentAppGoFiletag is confirmed to be used. The code in bothpkg/aiusechat/anthropic/anthropic-convertmessage.go(line 83) andpkg/aiusechat/openai/openai-convertmessage.go(line 205) wrapschatOpts.AppGoFilewith exactly these tags before injecting it into the last user message. The comment accurately describes the behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/app/aipanel/waveai-model.tsx (1)
465-468: Incomplete implementation of backup restore.The
restoreBackupmethod currently only logs and closes the modal without implementing the actual file restoration logic. This appears to be a placeholder.Would you like me to help implement the restore logic? This likely requires an RPC call to restore the backup file on the backend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/aipanel/aimessage.tsx(6 hunks)frontend/app/aipanel/aitooluse.tsx(8 hunks)frontend/app/aipanel/aitypes.ts(1 hunks)frontend/app/aipanel/waveai-model.tsx(4 hunks)frontend/layout/lib/layoutModel.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/aipanel/aitypes.ts
- frontend/layout/lib/layoutModel.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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:
frontend/app/aipanel/waveai-model.tsxfrontend/app/aipanel/aimessage.tsxfrontend/app/aipanel/aitooluse.tsx
📚 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:
frontend/app/aipanel/aitooluse.tsx
🧬 Code graph analysis (2)
frontend/app/aipanel/waveai-model.tsx (2)
frontend/app/store/global.ts (2)
globalStore(839-839)createBlock(820-820)pkg/waveobj/wtype.go (1)
BlockDef(244-247)
frontend/app/aipanel/aitooluse.tsx (3)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(30-30)frontend/app/modals/modal.tsx (1)
Modal(112-112)frontend/util/util.ts (2)
fireAndForget(466-466)cn(462-462)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (9)
frontend/app/aipanel/aimessage.tsx (4)
14-22: LGTM!The props are properly typed with a sensible default value (
false), maintaining backward compatibility.
41-49: LGTM!The conditional rendering provides clear visual feedback: a clock icon when waiting for approval vs. the animated pulsing indicator during normal thinking. This improves the user experience by disambiguating the waiting state.
187-212: LGTM!The thinking message logic correctly prioritizes pending approvals and threads the
isWaitingApprovalstate through appropriately.
229-231: LGTM!The
min-w-[min(100%,500px)]styling ensures assistant messages maintain a minimum width for better readability while respecting container constraints.frontend/app/aipanel/aitooluse.tsx (5)
12-14: LGTM!The helper function correctly encapsulates the approval timeout logic: when streaming ends with a pending approval, it's treated as timed out. This centralizes the logic that was previously inline.
132-178: LGTM!The
RestoreBackupModalcomponent provides clear user communication with proper formatting and warnings about data loss. The confirmation flow is well-structured.
266-268: LGTM!Proper use of
fireAndForgetfor the async diff opening operation, correctly passing the required parameters fromtoolData.
276-299: LGTM!The conditional rendering logic is correct: "Restore Backup" only appears when a backup exists (
writebackupfilename), while "Show Diff" appears for all file write operations with a filename. The UI provides clear tooltips and appropriate icons.
322-366: LGTM!The grouping logic collects file operations by approval state and batches them for display. This approach is intentional per previous review discussions.
Based on learnings
| async openDiff(fileName: string, toolcallid: string) { | ||
| const chatId = this.getChatId(); | ||
|
|
||
| if (!chatId || !fileName) { | ||
| console.error("Missing chatId or fileName for opening diff", chatId, fileName); | ||
| return; | ||
| } | ||
|
|
||
| const blockDef: BlockDef = { | ||
| meta: { | ||
| view: "aifilediff", | ||
| file: fileName, | ||
| "aifilediff:chatid": chatId, | ||
| "aifilediff:toolcallid": toolcallid, | ||
| }, | ||
| }; | ||
| await createBlock(blockDef, false, true); | ||
| } |
There was a problem hiding this comment.
Validate the toolcallid parameter.
The validation at line 441 checks chatId and fileName but omits toolcallid, which is a required parameter and subsequently used in the block metadata. If toolcallid is empty or undefined, the diff view may not function correctly.
Apply this diff to validate all required parameters:
- if (!chatId || !fileName) {
- console.error("Missing chatId or fileName for opening diff", chatId, fileName);
+ if (!chatId || !fileName || !toolcallid) {
+ console.error("Missing required parameters for opening diff", { chatId, fileName, toolcallid });
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async openDiff(fileName: string, toolcallid: string) { | |
| const chatId = this.getChatId(); | |
| if (!chatId || !fileName) { | |
| console.error("Missing chatId or fileName for opening diff", chatId, fileName); | |
| return; | |
| } | |
| const blockDef: BlockDef = { | |
| meta: { | |
| view: "aifilediff", | |
| file: fileName, | |
| "aifilediff:chatid": chatId, | |
| "aifilediff:toolcallid": toolcallid, | |
| }, | |
| }; | |
| await createBlock(blockDef, false, true); | |
| } | |
| async openDiff(fileName: string, toolcallid: string) { | |
| const chatId = this.getChatId(); | |
| if (!chatId || !fileName || !toolcallid) { | |
| console.error("Missing required parameters for opening diff", { chatId, fileName, toolcallid }); | |
| return; | |
| } | |
| const blockDef: BlockDef = { | |
| meta: { | |
| view: "aifilediff", | |
| file: fileName, | |
| "aifilediff:chatid": chatId, | |
| "aifilediff:toolcallid": toolcallid, | |
| }, | |
| }; | |
| await createBlock(blockDef, false, true); | |
| } |
🤖 Prompt for AI Agents
In frontend/app/aipanel/waveai-model.tsx around lines 438 to 455, the openDiff
function validates chatId and fileName but omits validating the required
toolcallid parameter; add a check that toolcallid is present (non-empty/defined)
alongside chatId and fileName, log an error including the missing values if
validation fails, and return early to avoid creating a block with incomplete
metadata.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/app/aipanel/waveai-model.tsx (1)
440-457: Validate thetoolcallidparameter.The validation at line 443 checks
chatIdandfileNamebut omitstoolcallid, which is a required parameter and subsequently used in the block metadata at line 453. Iftoolcallidis empty or undefined, the diff view may not function correctly.Apply this diff to validate all required parameters:
- if (!chatId || !fileName) { - console.error("Missing chatId or fileName for opening diff", chatId, fileName); + if (!chatId || !fileName || !toolcallid) { + console.error("Missing required parameters for opening diff", { chatId, fileName, toolcallid }); return; }
🧹 Nitpick comments (1)
pkg/aiusechat/usechat.go (1)
290-364: Well-structured refactor with new input verification.The extraction of
processToolCallInternalcleanly separates core tool execution logic from logging and metrics. The newToolVerifyInputhook (lines 311-322) provides a good extension point for pre-execution validation.Consider making the error message at line 295 more descriptive:
- ErrorText: "Invalid Tool Call", + ErrorText: "Invalid Tool Call: ToolUseData is nil",This would aid debugging if this condition is ever encountered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.roo/rules/rules.md(2 hunks)frontend/app/aipanel/aitooluse.tsx(8 hunks)frontend/app/aipanel/waveai-model.tsx(4 hunks)frontend/app/modals/modal.tsx(3 hunks)frontend/app/store/wshclientapi.ts(2 hunks)frontend/types/gotypes.d.ts(4 hunks)pkg/aiusechat/usechat.go(8 hunks)pkg/filebackup/filebackup.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(2 hunks)pkg/wshrpc/wshrpctypes.go(6 hunks)pkg/wshrpc/wshserver/wshserver.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/wshrpc/wshserver/wshserver.go
- pkg/filebackup/filebackup.go
- frontend/app/aipanel/aitooluse.tsx
- frontend/types/gotypes.d.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
frontend/app/aipanel/waveai-model.tsx
📚 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:
frontend/app/aipanel/waveai-model.tsxpkg/wshrpc/wshrpctypes.gopkg/aiusechat/usechat.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:
.roo/rules/rules.mdpkg/wshrpc/wshclient/wshclient.gofrontend/app/store/wshclientapi.ts
🧬 Code graph analysis (5)
frontend/app/aipanel/waveai-model.tsx (4)
frontend/app/store/global.ts (2)
globalStore(839-839)createBlock(820-820)pkg/waveobj/wtype.go (1)
BlockDef(244-247)frontend/app/store/wshclientapi.ts (1)
RpcApi(602-602)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (2)
FileRestoreBackupCommand(251-253)WaveAIGetToolDiffCommand(551-553)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (4)
CommandFileRestoreBackupData(602-605)RpcOpts(327-333)CommandWaveAIGetToolDiffData(786-789)CommandWaveAIGetToolDiffRtnData(791-794)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (4)
CommandFileRestoreBackupData(602-605)RpcOpts(327-333)CommandWaveAIGetToolDiffData(786-789)CommandWaveAIGetToolDiffRtnData(791-794)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (2)
FileRestoreBackupCommand(251-253)WaveAIGetToolDiffCommand(551-553)pkg/wshrpc/wshclient/wshclient.go (2)
FileRestoreBackupCommand(307-310)WaveAIGetToolDiffCommand(657-660)
pkg/aiusechat/usechat.go (7)
pkg/aiusechat/uctypes/usechat-types.go (5)
WaveToolCall(171-176)WaveChatOpts(427-447)ToolDefinition(79-93)AIToolResult(294-299)AIMetrics(240-256)pkg/web/sse/ssehandler.go (1)
SSEHandlerCh(64-77)pkg/util/logutil/logutil.go (1)
DevPrintf(13-17)pkg/util/utilfn/utilfn.go (1)
TruncateString(151-159)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/openai/openai-convertmessage.go (1)
GetFunctionCallInputByToolCallId(660-671)pkg/aiusechat/tools_writefile.go (1)
EditTextFileDryRun(261-293)
⏰ 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). (5)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build Docsite
- GitHub Check: Build for TestDriver.ai
- GitHub Check: merge-gatekeeper
🔇 Additional comments (13)
frontend/app/modals/modal.tsx (1)
21-22: LGTM! Clean implementation of button disabled states.The new
okDisabledandcancelDisabledprops are correctly threaded fromModalPropsthrough to theButtoncomponents inModalFooter. The implementation is straightforward, type-safe, and follows existing patterns. The optional props ensure backward compatibility while enabling fine-grained control over modal button states for AI/diff approval flows.Also applies to: 26-26, 44-44, 67-68, 71-71, 75-75, 79-79
pkg/wshrpc/wshrpctypes.go (1)
85-85: LGTM! New RPC definitions are well-structured.The new RPC commands, data structures, and interface methods follow the established conventions and integrate cleanly with the existing RPC system.
Also applies to: 148-148, 214-214, 293-293, 602-605, 786-794
.roo/rules/rules.md (1)
38-38: LGTM! Documentation updates are clear and helpful.The new TypeScript convention guideline and RPC System documentation section provide valuable guidance that aligns with the actual system implementation.
Also applies to: 52-57
pkg/wshrpc/wshclient/wshclient.go (1)
306-310: LGTM! Generated RPC client code is consistent.Both new command wrappers follow the established pattern and correctly invoke
sendRpcRequestCallHelperwith the appropriate RPC command names.Also applies to: 656-660
frontend/app/store/wshclientapi.ts (1)
250-253: LGTM! Generated TypeScript RPC methods are consistent.Both new RPC API methods follow the established pattern and correctly invoke
client.wshRpcCallwith appropriate command names and type signatures.Also applies to: 550-553
frontend/app/aipanel/waveai-model.tsx (1)
59-61: LGTM! New state management and methods are well-implemented.The new atoms,
getChatId()method, modal control methods, andrestoreBackup()method all follow established patterns and properly useglobalStorefor state management. The error handling inrestoreBackup()is appropriate.Also applies to: 418-420, 459-485
pkg/aiusechat/usechat.go (7)
29-29: LGTM!The
wavebaseimport is correctly added to support path expansion in the newCreateWriteTextFileDifffunction.
95-97: LGTM!The system prompt updates correctly communicate the new file write/edit capabilities while appropriately setting expectations that shell execution and remote file operations are not yet available.
124-124: LGTM!The builder system prompt update appropriately documents the
<CurrentAppGoFile>tag for AI context.
366-390: LGTM!The refactored
processToolCallwrapper correctly delegates to the internal handler while managing logging, metrics, and UI synchronization. The separation of concerns improves maintainability.
401-408: LGTM!Good defensive programming to extract the filename for UI display, checking both
"filename"and"file_name"key variants. This enhances the user experience by showing which file will be affected.
566-566: LGTM!The updated
ToolAnyCallbacksignature now passestoolCall.ToolUseDatain addition totoolCall.Input, enabling callbacks to access and populate metadata likeWriteBackupFileName. This is a necessary API evolution to support the new diff generation features.
813-882: Implementation is correct; fallback logic handles expected scenarios safely.The verification confirms that
WriteBackupFileNamepopulation is working as designed:
- write_text_file (new files): No backup created since
fileInfois nil — correct. The fallback safely reads the current file (which contains the new content), returning nil fororiginalContent.- write_text_file (existing files): Backup always created before
WriteBackupFileNameassignment viafilebackup.MakeFileBackup().- edit_text_file: Backup unconditionally created in all cases.
The error handling at line 872 (
if err != nil && !os.IsNotExist(err)) properly treats file-not-found as a valid state, only returning errors for actual read failures. This is correct for new file scenarios.The past issue regarding
WriteBackupFileNamebeing unpopulated specifically concerned new files, which is expected behavior—not a bug. The RPC workflow ensures tool callbacks populate the backup beforeCreateWriteTextFileDiffretrieves it, preventing any race conditions.
…elieve it is possible for rootNode to be null at this point.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/aiusechat/usechat.go (1)
805-874: LGTM - comprehensive diff generation with appropriate fallbacks.The function properly handles both
write_text_fileandedit_text_filescenarios with comprehensive validation. The fallback behavior (lines 856-870) whenbackupFileNameis empty allows diff generation even for preview scenarios before tool execution, reading from the current file state.As an optional future refactoring, consider extracting the write_text_file diff generation (lines 850-873) into a helper function to reduce the overall function length and improve readability—but this is not essential given the current clarity of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/layout/lib/layoutModel.ts(4 hunks)pkg/aiusechat/usechat.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/layout/lib/layoutModel.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/usechat.go
🧬 Code graph analysis (1)
pkg/aiusechat/usechat.go (4)
pkg/aiusechat/uctypes/usechat-types.go (7)
WaveToolCall(171-176)WaveChatOpts(427-447)ToolDefinition(79-93)AIToolResult(294-299)ToolUseStatusError(127-127)ToolUseStatusCompleted(128-128)AIMetrics(240-256)pkg/aiusechat/chatstore/chatstore.go (1)
DefaultChatStore(18-20)pkg/aiusechat/openai/openai-convertmessage.go (1)
GetFunctionCallInputByToolCallId(660-671)pkg/aiusechat/tools_writefile.go (1)
EditTextFileDryRun(261-293)
⏰ 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). (5)
- GitHub Check: Build Docsite
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (5)
pkg/aiusechat/usechat.go (5)
29-29: LGTM - necessary import for path expansion.The wavebase package import is correctly added to support the
ExpandHomeDirfunctionality used inCreateWriteTextFileDiff.
95-97: LGTM - clear capability boundaries.The updated prompt text clearly communicates what file operations are available while setting appropriate expectations about execution limitations.
290-364: LGTM - well-structured refactoring with improved validation.The extraction of
processToolCallInternalcleanly separates the core tool execution logic from the wrapper concerns (logging, metrics). The addition of input verification viaToolVerifyInput(lines 311-322) is a valuable safety improvement that catches invalid inputs before execution.
366-390: LGTM - clean wrapper pattern.The refactored
processToolCallproperly delegates to the internal implementation while handling cross-cutting concerns like logging, metrics, and UI synchronization.
558-558: LGTM - callback signature enhancement.Passing
toolCall.ToolUseDatato the callback enables tools to access and enrich UI metadata during execution, which aligns with the updatedToolDefinition.ToolAnyCallbacksignature.
also updates ROADMAP.md, and fixes a node pruning bug on the FE, and adds a new diff viewer that we can view the write_text_file and edit_text_file diffs in.