big improvements to waveapp builder #2550
Conversation
sawka
commented
Nov 14, 2025
sawka
commented
- build manifest
- working on secrets injection (secretstore + secret-bindings.json)
- tool progress indicators
- build output and errors injected as the result of the edit calls so AI gets instant feedback on edits
- change edits to not be atomic (allows AI to make better progress)
- updated binary location for waveapps
- publish button
- new partial json parser (for sending incremental tool progress indication)
- updated tsunami view to use new embedded scaffold + config vars
- lots of work on cleaning up the output so it is more useful to users + AI agents
…tead of rolling back everything
…oval, parse partial tool json
…ead of just apppath)
WalkthroughThis PR implements app publishing and secret-bindings end-to-end, adds tool-progress streaming and partial-file/JSON repair utilities, and updates multiple UI elements. Frontend: PublishAppModal, modal registry entry, new RPC client methods, tool-progress rendering/components, and UI position/style tweaks. Backend: RPC handlers for publish and secret bindings, manifest generation during build, builder controller and builder-run signature changes to load manifests and inject secret env, waveappstore manifest/secret read-write and partial file edits, OpenAI tool-progress emission, partial JSON parser, and numerous type and API surface changes (app manifest, tsunami:appid, command types). Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-21T05:09:26.916ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/builder/tabs/builder-previewtab.tsx (1)
129-129: Apply the new accent button styling consistently.The "Start App" button still uses the old styling pattern (
bg-accent hover:bg-accent/80), which is inconsistent with the updated "Ask AI to Fix" button at line 76 and the new guideline in rules.md.Apply this diff to align with the new guideline:
- className="px-6 py-2 bg-accent text-primary font-semibold rounded hover:bg-accent/80 transition-colors cursor-pointer" + className="px-6 py-2 bg-accent/80 text-primary font-semibold rounded hover:bg-accent transition-colors cursor-pointer"Based on coding guidelines
tsunami/build/build.go (1)
808-835: Add timeout and improve error handling for manifest generation.The manifest generation command has no timeout and only captures stdout. If the binary hangs or writes errors to stderr, the build will hang or lose diagnostic information.
Apply this diff to add timeout and capture combined output:
func generateManifest(tempDir, exePath string, opts BuildOpts) error { oc := opts.OutputCapture + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() - manifestCmd := exec.Command(exePath, "--manifest") + manifestCmd := exec.CommandContext(ctx, exePath, "--manifest") manifestCmd.Dir = tempDir if opts.Verbose { oc.Printf("[debug] Running: %s --manifest", exePath) oc.Printf("Generating manifest...") } - manifestOutput, err := manifestCmd.Output() + manifestOutput, err := manifestCmd.CombinedOutput() if err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("manifest generation timed out after 30s") + } - return fmt.Errorf("manifest generation failed: %w", err) + return fmt.Errorf("manifest generation failed: %w\nOutput: %s", err, string(manifestOutput)) }
🧹 Nitpick comments (10)
pkg/buildercontroller/buildercontroller.go (1)
26-26: Status enrichment works, but repeated manifest/secret reads under lock may be heavySwitching
GetStatusto returnwshrpc.BuilderStatusDataand enriching it withAppManifest,SecretBindings, andSecretBindingsCompleteis functionally sound and the wshrpc struct mapping looks correct.However, this method now:
- Reads the manifest and secret‑bindings files on every
GetStatuscall.- Calls
BuildAppSecretEnvjust to computeSecretBindingsComplete, which entails resolving all secrets viasecretstore.GetSecret.- Performs all of that while holding
bc.statusLock.In a UI that polls builder status frequently, this can mean:
- Extra filesystem traffic and secret‑store calls on the hot path.
- Longer holds on
statusLock, delaying other status updates.It might be worth (later) refactoring to compute manifest/secret‑binding info outside the lock and/or cache it (invalidated when
bc.appIdchanges or a relevant write occurs), and use a cheaper check for “bindings complete” that doesn’t require re‑fetching all secret values on every status read.Also applies to: 498-542
pkg/aiusechat/tools_builder.go (2)
94-115: Dynamic write-app descriptions and progress look good; optional line-count tweakUsing
parseBuilderWriteAppFileInputin bothToolCallDescandToolProgressDesckeeps behavior consistent and gracefully falls back when parsing fails. The line-count-based messages are also very readable for users/AI. If you ever care about micro-allocations or want to reuse this pattern elsewhere, you could factor out a small helper that counts'\n'withoutstrings.Split, but that’s strictly an optional optimization.
277-291: Static 'listing files' ToolCallDesc is fine; optional progress hook for consistencyReturning a simple
"listing files"fromToolCallDesckeeps this tool aligned with the newer, more descriptive patterns. If you later want uniform progress reporting across builder tools, you could add a trivialToolProgressDeschere that just returns[]string{"listing files"}, but it’s not required for correctness.frontend/builder/app-selection-modal.tsx (1)
122-143: Title updates on app select/create look correct; consider centralizing formattingSetting
document.titletoWaveApp Builder (${appId})on both select and create keeps the builder window title in sync with the active app and matches the wave.ts behavior. You might later want a small helper (e.g.,setBuilderTitle(appId?: string)) shared withinitBuilder/reinitBuilderto avoid three separate call sites drifting, but the current change is functionally sound.frontend/builder/builder-buildpanel.tsx (1)
7-8: Restart button and filtered output behavior look good
- Wiring
handleRestarttoBuilderAppPanelModel.getInstance().restartBuilder()cleanly reuses the existing restart path used by saves/env-var updates.- Updated
filteredLineslogic to hide[debug]lines and blank lines whenshowDebugis false makes the non-debug view much easier to read.- Header layout with the debug toggle and “Restart App” button is straightforward and matches the rest of the builder UI.
If you want to be explicit about ignoring the async return in the click handler, you could wrap the call as
void BuilderAppPanelModel.getInstance().restartBuilder();, but it’s not required.Also applies to: 81-86, 91-107
pkg/wshrpc/wshserver/wshserver.go (1)
1030-1035: New app RPCs and builder status handling are sound
WriteAppSecretBindingsCommandcorrectly rejects emptyAppIdand delegates towaveappstore.WriteAppSecretBindings, which already validates the appId and handles nil bindings. This mirrors the patterns used by the app file commands above.GetBuilderStatusCommandnow returns&statuswherestatus := bc.GetStatus(). SinceGetStatusreturns a value, this gives callers an isolated snapshot of the status without exposing the controller’s internal struct; Go will safely escape this to the heap.PublishAppCommandis a straightforward wrapper aroundwaveappstore.PublishDraft, with errors wrapped in context and the resultingPublishedAppIdpassed back to the client. For consistency with the other commands you might optionally add an explicitif data.AppId == ""guard, but functionally it’s already safe becausePublishDraftvalidates the appId.Also applies to: 1089-1096, 1121-1129
frontend/builder/builder-apppanel.tsx (2)
103-120: Consider improving error handling in the modal.Currently, errors are only logged to the console (line 118), leaving the user without visible feedback if publishing fails. Consider displaying the error message within the modal UI.
Example enhancement:
+ const [error, setError] = useState<string | null>(null); + const handlePublish = async () => { if (!builderAppId) { console.error("No builder app ID found"); modalsModel.popModal(); return; } + setError(null); try { const result = await RpcApi.PublishAppCommand(TabRpcClient, { appid: builderAppId }); console.log("App published successfully:", result.publishedappid); modalsModel.popModal(); } catch (error) { console.error("Failed to publish app:", error); + setError(error instanceof Error ? error.message : "Failed to publish app"); } };Then display the error in the modal UI if present.
219-221: Remove unusedhandleRestartfunction.The
handleRestartfunction is defined but never called, as the UI now useshandlePublishClickinstead (line 283).Apply this diff:
- const handleRestart = useCallback(() => { - model.restartBuilder(); - }, [model]); - const handlePublishClick = useCallback(() => {pkg/util/utilfn/partial_test.go (1)
92-96: Remove or document the commented-out test case.This commented test case appears to demonstrate a known limitation where partial keywords (like
fafor incompletefalse) are not repaired. Either remove this commented code or add a comment explaining why this case is not supported.Apply this diff to remove the commented code:
- // { - // name: "unclosed object with partial value", - // input: `{"a": fa`, - // expected: `{"a": fa}`, - // },Alternatively, if documenting the limitation, add an explanatory comment above it.
pkg/util/utilfn/partial.go (1)
88-94: Consider adding a comment to clarify the valueStart logic.The boolean expression at line 91 checks for all possible starting characters of JSON values but is complex and could benefit from a brief comment explaining what each character/range represents (e.g.,
n/t/ffor null/true/false, digits for numbers, etc.).Apply this diff to add clarifying comments:
+ // Check if byte can start a JSON value: + // {/[ for objects/arrays, n/t/f for null/true/false, " for strings, 0-9/- for numbers valueStart := b == '{' || b == '[' || b == 'n' || b == 't' || b == 'f' || b == '"' || (b >= '0' && b <= '9') || b == '-'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.roo/rules/rules.md(1 hunks)emain/emain-menu.ts(1 hunks)frontend/app/aipanel/aimessage.tsx(3 hunks)frontend/app/aipanel/aipanel.tsx(1 hunks)frontend/app/aipanel/aipanelmessages.tsx(1 hunks)frontend/app/aipanel/aitooluse.tsx(6 hunks)frontend/app/aipanel/aitypes.ts(1 hunks)frontend/app/aipanel/thinkingmode.tsx(1 hunks)frontend/app/modals/modalregistry.tsx(2 hunks)frontend/app/store/wshclientapi.ts(2 hunks)frontend/app/view/tsunami/tsunami.tsx(1 hunks)frontend/builder/app-selection-modal.tsx(2 hunks)frontend/builder/builder-app.tsx(2 hunks)frontend/builder/builder-apppanel.tsx(5 hunks)frontend/builder/builder-buildpanel.tsx(2 hunks)frontend/builder/tabs/builder-previewtab.tsx(1 hunks)frontend/types/gotypes.d.ts(6 hunks)frontend/wave.ts(4 hunks)pkg/aiusechat/openai/openai-backend.go(3 hunks)pkg/aiusechat/tools_builder.go(6 hunks)pkg/aiusechat/uctypes/usechat-types.go(2 hunks)pkg/aiusechat/usechat.go(1 hunks)pkg/blockcontroller/tsunamicontroller.go(3 hunks)pkg/buildercontroller/buildercontroller.go(7 hunks)pkg/util/fileutil/fileutil.go(2 hunks)pkg/util/utilfn/partial.go(1 hunks)pkg/util/utilfn/partial_test.go(1 hunks)pkg/waveappstore/waveappstore.go(4 hunks)pkg/waveobj/metaconsts.go(1 hunks)pkg/waveobj/wtypemeta.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(2 hunks)pkg/wshrpc/wshrpctypes.go(5 hunks)pkg/wshrpc/wshserver/wshserver.go(3 hunks)tsunami/build/build.go(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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.gopkg/aiusechat/openai/openai-backend.gofrontend/app/aipanel/aitypes.tspkg/aiusechat/uctypes/usechat-types.gopkg/aiusechat/tools_builder.gofrontend/app/aipanel/aitooluse.tsx
📚 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.gofrontend/app/aipanel/aitypes.tsfrontend/app/aipanel/aimessage.tsxfrontend/app/aipanel/aitooluse.tsx
📚 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/wshrpc/wshclient/wshclient.gopkg/buildercontroller/buildercontroller.go
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/wave.ts
🧬 Code graph analysis (19)
frontend/builder/builder-buildpanel.tsx (1)
frontend/builder/store/builder-apppanel-model.ts (1)
BuilderAppPanelModel(21-294)
pkg/aiusechat/usechat.go (2)
pkg/aiusechat/uctypes/usechat-types.go (1)
ApprovalNeedsApproval(140-140)pkg/aiusechat/toolapproval.go (1)
RegisterToolApproval(48-58)
pkg/aiusechat/openai/openai-backend.go (2)
pkg/util/utilfn/partial.go (1)
ParseParialJson(167-175)pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolProgress(166-170)
frontend/builder/builder-apppanel.tsx (5)
frontend/app/store/global.ts (1)
atoms(816-816)frontend/app/store/modalmodel.ts (1)
modalsModel(45-45)frontend/app/store/wshclientapi.ts (1)
RpcApi(662-662)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/modals/modal.tsx (1)
Modal(116-116)
emain/emain-menu.ts (1)
emain/emain-platform.ts (1)
unamePlatform(273-273)
pkg/wshrpc/wshclient/wshclient.go (3)
frontend/app/store/wshclientapi.ts (2)
PublishAppCommand(386-388)WriteAppSecretBindingsCommand(631-633)pkg/wshutil/wshrpc.go (1)
WshRpc(47-61)pkg/wshrpc/wshrpctypes.go (4)
CommandPublishAppData(1072-1074)RpcOpts(357-363)CommandPublishAppRtnData(1076-1078)CommandWriteAppSecretBindingsData(1022-1025)
pkg/aiusechat/tools_builder.go (3)
pkg/util/fileutil/fileutil.go (1)
EditSpec(259-263)pkg/aiusechat/uctypes/usechat-types.go (2)
ToolDefinition(80-95)UIMessageDataToolUse(148-159)pkg/waveappstore/waveappstore.go (1)
ReplaceInAppFilePartial(359-375)
frontend/app/store/wshclientapi.ts (1)
pkg/wshrpc/wshrpctypes.go (4)
CommandPublishAppData(1072-1074)RpcOpts(357-363)CommandPublishAppRtnData(1076-1078)CommandWriteAppSecretBindingsData(1022-1025)
pkg/util/fileutil/fileutil.go (1)
pkg/aiusechat/tools_writefile.go (1)
MaxEditFileSize(18-18)
frontend/app/aipanel/aimessage.tsx (1)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(36-36)
frontend/app/aipanel/aitooluse.tsx (1)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(36-36)
pkg/wshrpc/wshrpctypes.go (2)
frontend/app/store/wshclientapi.ts (2)
WriteAppSecretBindingsCommand(631-633)PublishAppCommand(386-388)pkg/wshrpc/wshclient/wshclient.go (2)
WriteAppSecretBindingsCommand(753-756)PublishAppCommand(468-471)
frontend/types/gotypes.d.ts (1)
pkg/wshrpc/wshrpctypes.go (5)
AppManifest(1046-1052)SecretMeta(1041-1044)CommandPublishAppData(1072-1074)CommandPublishAppRtnData(1076-1078)CommandWriteAppSecretBindingsData(1022-1025)
pkg/waveappstore/waveappstore.go (3)
pkg/util/fileutil/fileutil.go (3)
EditSpec(259-263)EditResult(265-269)ReplaceInFilePartial(380-406)pkg/wshrpc/wshrpctypes.go (1)
AppManifest(1046-1052)pkg/secretstore/secretstore.go (1)
GetSecret(251-263)
frontend/app/modals/modalregistry.tsx (1)
frontend/builder/builder-apppanel.tsx (1)
PublishAppModal(345-345)
frontend/builder/builder-app.tsx (1)
frontend/app/modals/modalsrenderer.tsx (1)
ModalsRenderer(57-57)
pkg/buildercontroller/buildercontroller.go (3)
pkg/wavebase/wavebase.go (1)
TryMkdirs(255-271)pkg/waveappstore/waveappstore.go (3)
ReadAppManifest(672-694)ReadAppSecretBindings(696-725)BuildAppSecretEnv(754-792)pkg/wshrpc/wshrpctypes.go (3)
BuilderStatusData(1054-1063)AppManifest(1046-1052)SecretMeta(1041-1044)
pkg/wshrpc/wshserver/wshserver.go (3)
pkg/wshrpc/wshclient/wshclient.go (2)
WriteAppSecretBindingsCommand(753-756)PublishAppCommand(468-471)pkg/wshrpc/wshrpctypes.go (3)
CommandWriteAppSecretBindingsData(1022-1025)CommandPublishAppData(1072-1074)CommandPublishAppRtnData(1076-1078)pkg/waveappstore/waveappstore.go (2)
WriteAppSecretBindings(727-752)PublishDraft(118-148)
pkg/blockcontroller/tsunamicontroller.go (5)
pkg/wconfig/filewatcher.go (1)
GetWatcher(33-57)pkg/wavebase/wavebase.go (2)
GetWaveAppPath(113-115)ExpandHomeDir(141-155)tsunami/cmd/main-tsunami.go (1)
TsunamiSdkVersion(17-17)pkg/waveobj/metaconsts.go (2)
MetaKey_TsunamiAppPath(129-129)MetaKey_TsunamiAppId(130-130)pkg/waveappstore/waveappstore.go (1)
GetAppDir(79-86)
⏰ 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 (47)
emain/emain-menu.ts (1)
128-132: Verify the keyboard shortcut choice.
Command+Shift+Bon macOS is the standard shortcut for toggling the bookmarks bar in most browsers (Chrome, Firefox, Safari). While this is a dev-only feature, developers using Wave Terminal may have muscle memory for this shortcut, potentially causing confusion or unintended behavior.Consider whether an alternative shortcut might be more appropriate, or confirm that this conflict is acceptable for a dev-only builder window feature.
frontend/app/view/tsunami/tsunami.tsx (1)
238-238: LGTM!The appId retrieval follows the same pattern as appPath and correctly uses optional chaining.
pkg/buildercontroller/buildercontroller.go (1)
101-116: Executable path helper and build wiring look correct
GetBuilderAppExecutablePathcorrectly targets<appPath>/bin/app(.exe)and ensures the bin directory exists. The subsequent usage inbuildAndRun(asOutputFile, withMoveFileBackand the post-buildos.Stat+ executable bit check) is consistent and should work across platforms.Also applies to: 193-203, 223-238, 249-258, 275-285
pkg/aiusechat/tools_builder.go (4)
171-191: formatEditDescriptions helper produces clear multi-edit summariesThe helper’s “header + per-edit bullet” format is easy to scan, and the
descfallback toedit #Navoids empty lines. The (+newLines -oldLines) notation should give both users and the UI a nice approximate sense of the edit impact without extra complexity. No issues spotted here.
197-222: Edit-tool description and schema accurately document sequential semanticsThe updated
Descriptionandedits/old_str/descfield docs clearly explain the “exactly once” constraint and the new sequential behavior (partial success with later edits skipped). This should make the tool much easier for the model to use correctly, while keeping human-facing text concise and not mixing in approval semantics, which matches how ToolApproval is handled. Based on learnings.
231-244: No action needed—multi-line handling already implementedThe frontend's
ToolDesccomponent (frontend/app/aipanel/aitooluse.tsx:63–75) is explicitly designed to handle multi-line descriptions. It splits strings on"\n"and renders each line throughToolDescLine. Both current usages (toolData.tooldescat line 420 andprogressData.statuslinesat line 450) flow through this component, which correctly handles newline-separated output without wrapping or truncation issues.
245-273: Implementation matches documented semantics; consumer verification requires manual confirmationThe
ReplaceInFilePartialimplementation is correct:
- Applies edits sequentially and stops on first failure (lines 322–345 in
pkg/util/fileutil/fileutil.go)- Returns per-edit results with success/error status for each edit
- Partial modifications are written to disk even on failure
The callback at lines 245–273 correctly returns
{"edits": editResults, ...}matching this contract.However, I could not locate frontend or integration tests in this codebase that validate consumers expect the new
"edits"payload format. The result shape may differ from previous versions—verify that any client consumingbuilder_edit_app_fileresults has been updated to handle this structure..roo/rules/rules.md (1)
53-53: LGTM!The styling guideline for accent buttons is clear and addresses the hover behavior issue. This will help maintain consistent button styling across the codebase.
pkg/waveobj/metaconsts.go (1)
130-130: LGTM!The new constant follows the established naming convention and aligns with the tsunami metadata expansion described in the PR objectives.
pkg/waveobj/wtypemeta.go (1)
134-134: LGTM!The new field follows the JSON naming convention (lowercase, no underscores) and integrates properly with the tsunami metadata infrastructure.
frontend/builder/tabs/builder-previewtab.tsx (1)
76-76: LGTM!The button styling correctly implements the new accent button guideline from rules.md.
frontend/app/aipanel/aipanelmessages.tsx (1)
50-50: LGTM!The repositioning of the ThinkingLevelDropdown to the left side is consistent with the alignment change in thinkingmode.tsx.
frontend/app/aipanel/thinkingmode.tsx (1)
78-78: LGTM!The dropdown alignment change from right to left is consistent with the repositioning of the trigger button container in aipanelmessages.tsx and aipanel.tsx.
frontend/builder/builder-app.tsx (1)
6-6: LGTM!The integration of ModalsRenderer is clean and enables the publish modal functionality described in the PR objectives.
Also applies to: 50-50
frontend/app/aipanel/aipanel.tsx (1)
500-500: LGTM!The repositioning of the ThinkingLevelDropdown to the left side is consistent with the changes in aipanelmessages.tsx and thinkingmode.tsx.
pkg/aiusechat/usechat.go (1)
373-383: Early approval registration wiring is correctRegistering approval with
chatOpts.RegisterToolApprovalimmediately after sending the initialdata-toolusepacket ensures the approval workflow is set up beforeprocessToolCallInternalblocks onWaitForToolApproval. The nil check keeps this safe for callers that don’t use approvals. No issues from a correctness or concurrency standpoint.frontend/app/modals/modalregistry.tsx (1)
7-18: PublishAppModal registration is consistent with existing modal patternsImporting
PublishAppModaland registering it underPublishAppModal.displayName || "PublishAppModal"matches the existing registry convention and gives a stable key even ifdisplayNameisn’t set. No issues here.frontend/app/aipanel/aitypes.ts (1)
27-33: Tool progress typing matches backend payloadsThe new
toolprogressentry onWaveUIDataTypes(toolcallid/toolname/statuslines) lines up withUIMessageDataToolProgresson the Go side, sodata-toolprogresspackets from the backend will deserialize cleanly into these types. This is a minimal, correct extension of the existing message data union.pkg/aiusechat/openai/openai-backend.go (1)
379-386: Streaming tool-progress implementation is well-structured; just ensure behavior matches OpenAI’s event semanticsThe new logic to surface tool progress from streaming function-call arguments looks solid:
openaiBlockState.partialJSONtracks the accumulated arguments for each tool-use block, keyed by item_id, which is exactly what you want for handling...arguments.deltaevents.- In the delta handler, you:
- Append the latest
DeltatopartialJSON.- Look up the tool’s definition and, if
ToolProgressDescis provided, run it against a tolerant parse (utilfn.ParseParialJson) and emitdata-toolprogresspackets withUIMessageDataToolProgress. Parse/descriptor errors are swallowed, which prevents flaky progress parsing from impacting the main stream.- In the done handler, you still create
ToolUseDataviacreateToolUseDatafrom the fullArgumentsstring, and then, ifToolProgressDescexists, parse the final JSON withjson.Unmarshaland emit a finaldata-toolprogressupdate.This keeps progress updates strictly derived from the arguments JSON without interfering with the existing tool-use or approval flow.
Please double-check against the current OpenAI Responses API streaming docs that:
response.function_call_arguments.deltaand.doneevents are emitted exactly once per tool-call item and in the order assumed here, andDelta/Argumentscontain a contiguous JSON string for arguments, so that accumulating intopartialJSONand parsing via your partial-JSON repair logic remains safe.Also applies to: 855-880, 899-912
pkg/wshrpc/wshclient/wshclient.go (1)
467-471: LGTM! Generated RPC client wrappers follow the established pattern.The new
PublishAppCommandandWriteAppSecretBindingsCommandwrappers are consistent with the existing RPC command pattern in this generated file.Also applies to: 752-756
frontend/app/store/wshclientapi.ts (1)
385-388: LGTM! Generated TypeScript RPC wrappers are consistent.The new
PublishAppCommandandWriteAppSecretBindingsCommandmethods follow the established pattern and have appropriate return types.Also applies to: 630-633
pkg/aiusechat/uctypes/usechat-types.go (2)
94-94: LGTM! New tool progress callback follows the pattern.The
ToolProgressDesccallback field is appropriately excluded from JSON serialization with thejson:"-"tag, consistent with other callback fields likeToolAnyCallbackandToolApproval.
165-170: LGTM! Tool progress data type is well-structured.The
UIMessageDataToolProgresstype provides a clean interface for tracking tool execution progress with status lines.frontend/builder/builder-apppanel.tsx (1)
223-227: LGTM! Publish button integration is clean.The new publish functionality is well-integrated, using the modal system appropriately and deriving the app name from the builderAppId.
Also applies to: 280-287
pkg/util/fileutil/fileutil.go (2)
320-345: Verify partial write behavior on failure is intentional.
ApplyEditsPartialcontinues to write the file even when some edits fail (line 344), which means partially modified content is persisted. Confirm this is the desired behavior, as it could leave files in an inconsistent state if later edits depend on earlier ones.For example, if edit 1 succeeds but edit 2 fails:
- File is written with only edit 1 applied
- Edit 2's failure might be due to content that edit 1 should have set up
- File now contains incomplete changes
If this behavior is intentional (e.g., for AI tools that make incremental progress), consider documenting this clearly in the function's godoc comment.
265-269: LGTM! Edit result tracking is well-structured.The
EditResulttype andapplyEdithelper provide clear per-edit status tracking with descriptive error messages.Also applies to: 271-302
pkg/waveappstore/waveappstore.go (4)
754-792: LGTM! Secret environment builder has robust validation.The
BuildAppSecretEnvfunction properly handles:
- Required vs. optional secrets (lines 768-770, 782-786)
- Missing bindings (line 769)
- Non-existent secrets in the store (lines 781-784)
- Error propagation from secretstore (lines 777-778)
The logic correctly skips unbound optional secrets while failing on unbound or missing required secrets.
672-694: LGTM! Manifest reading follows consistent patterns.The
ReadAppManifestfunction properly validates the app ID, constructs the path, and handles JSON parsing errors with descriptive messages.
696-725: LGTM! Secret bindings I/O is well-implemented.Both
ReadAppSecretBindingsandWriteAppSecretBindingshandle the optional nature of secret bindings appropriately:
- Reading returns an empty map if the file doesn't exist (lines 709-711)
- Writing ensures a valid map before marshaling (lines 737-739)
- Pretty-printed JSON for readability (line 741)
Also applies to: 727-752
359-375: LGTM! Partial edit wrapper maintains consistency.The
ReplaceInAppFilePartialfunction follows the same validation pattern asReplaceInAppFile, ensuring path safety before delegating tofileutil.ReplaceInFilePartial.frontend/types/gotypes.d.ts (1)
65-72: LGTM! Generated TypeScript types are accurate.The new types (
AppManifest,SecretMeta, command types) and BuilderStatusData extensions properly reflect the corresponding Go types, maintaining type safety across the frontend/backend boundary.Also applies to: 136-138, 340-348, 507-511, 982-986
frontend/app/aipanel/aimessage.tsx (2)
143-149: LGTM! Tool progress grouping is well-integrated.The changes properly extend the message part grouping to include
data-toolprogressalongsidedata-tooluse, enabling progress updates to be displayed within tool execution groups. The implementation maintains type safety and follows the existing pattern.Also applies to: 152-177
226-226: Note: User message width adjusted for better spacing.The max-width change from
calc(100%-20px)tocalc(90%-10px)provides more whitespace around user messages, improving visual balance. This is a subjective UI adjustment.frontend/app/aipanel/aitooluse.tsx (4)
16-77: LGTM! Clean implementation of highlighted signed numbers.The regex pattern correctly identifies isolated signed numbers using negative lookbehind/lookahead, and the color coding (+/green, -/red) provides clear visual feedback. The multi-line support via
ToolDescis well-structured.
436-456: LGTM! Clean progress indicator component.The
AIToolProgresscomponent follows the established patterns and correctly usesToolDescfor status lines, maintaining consistency with the rest of the codebase.
420-420: LGTM! Consistent use of ToolDesc component.Refactoring to use
ToolDescensures consistent rendering and applies the signed number highlighting to all tool descriptions.
459-553: LGTM! Sound deduplication logic for tool progress.The extension to handle
toolprogressparts alongsidetooluseparts is well-implemented. The deduplication logic (line 477) correctly filters out progress indicators for tools that have completed, preventing redundant display.pkg/wshrpc/wshrpctypes.go (3)
160-173: LGTM! Builder command constants properly organized.The formatting and organization of builder command constants is clean and follows the established naming conventions.
338-338: LGTM! New RPC interface methods follow established conventions.Both
WriteAppSecretBindingsCommandandPublishAppCommandfollow the standard RPC method signature pattern and align with the client implementations shown in the codebase.Also applies to: 345-345
1022-1078: LGTM! Well-structured types for app publishing infrastructure.The new data types for secret bindings, manifests, and app publishing are well-designed:
- Appropriate use of pointers for optional fields
map[string]anycorrectly handles arbitrary JSON schemas- JSON tags follow lowercase conventions
- Field names are clear and descriptive
pkg/util/utilfn/partial_test.go (1)
11-135: Comprehensive test coverage for JSON repair.The test cases thoroughly cover incomplete JSON scenarios including unclosed brackets, strings, trailing commas, and nested structures. The dual validation (exact string match + JSON validity) ensures both correctness and usability of the repair output.
pkg/util/utilfn/partial.go (1)
10-57: Stack implementation looks solid.The stack operations correctly handle JSON structure tracking with appropriate state transitions. Note that
stackQuoteis defined but doesn't appear to be used in therepairJsonfunction—this may be intentional for future use or could be removed if not needed.tsunami/build/build.go (2)
746-806: LGTM on the signature change and path handling.The updated signature correctly returns the absolute path to the built binary, and the logic properly handles both custom output paths and default temp directory paths. The distinction between
outputPath(for the build command) andabsOutputPath(for the return value) is well-handled.
731-741: LGTM on manifest copy logic.The manifest.json copy logic follows the same pattern as go.sum (only copies if exists) and includes appropriate debug logging. The implementation is consistent with the existing code.
pkg/blockcontroller/tsunamicontroller.go (3)
139-159: LGTM on app path resolution logic.The app path resolution correctly handles both
tsunami:apppathandtsunami:appidmetadata keys. The logic properly:
- Derives the app directory from the app ID when
apppathis not provided- Expands home directory references in provided paths
- Validates that paths are absolute
- Provides clear error messages for each failure case
188-190: Code under review is correct; however, inconsistency exists in other callers.The
SdkVersion,NodePath, andGoPathfields inpkg/blockcontroller/tsunamicontroller.go(lines 188-190) are correctly provided. However, verification reveals an inconsistency:
SdkVersionis present at all 4 BuildOpts construction sites (tsunamicontroller.go, buildercontroller.go, and 2 sites in main-tsunami.go) ✓NodePathandGoPathare provided in tsunamicontroller.go and buildercontroller.go, but not intsunami/cmd/main-tsunami.go(lines 71 and 100)Both
NodePathandGoPathare optional with sensible defaults (NodePathdefaults to"node"viagetNodePath(),GoPathdefaults to systemgoexecutable viaFindGoExecutable()). The missing fields in main-tsunami.go will simply use these defaults, so there is no compilation or runtime error. However, verify whether this inconsistency is intentional or if main-tsunami.go callers should also be updated for consistency.
26-26: golang.org/x/mod v0.27.0 requires Go 1.23.0, incompatible with minimum supported Go 1.22.golang.org/x/mod@v0.27.0 specifies
go 1.23.0as its minimum requirement, which exceeds the codebase's minimum supported version of Go 1.22. This will cause the build to fail for users on Go 1.22. Either upgrade the minimum Go version requirement or downgrade golang.org/x/mod to a version compatible with Go 1.22.⛔ Skipped due to learnings
Learnt from: esimkowitz Repo: wavetermdev/waveterm PR: 1998 File: go.mod:37-37 Timestamp: 2025-02-20T00:10:31.048Z Learning: The golang.org/x/sync package should remain at version v0.11.0 in go.mod, as explicitly confirmed by the maintainer.
frontend/wave.ts
Outdated
| await WOS.reloadWaveObject<Client>(WOS.makeORef("client", savedBuilderInitOpts.clientId)); | ||
| document.title = `Tsunami Builder - ${savedBuilderInitOpts.appId}`; | ||
| document.title = savedBuilderInitOpts.appId ? `WaveApp Builder (${savedBuilderInitOpts.appId})` : "WaveApp Builder"; | ||
| getApi().setWindowInitStatus("wave-ready"); |
There was a problem hiding this comment.
Builder window title can drift from the actual appId resolved from RTInfo
Right now:
initBuildersetsdocument.titlefrominitOpts.appId(Line 276), then potentially overrides the active app via RTInfo (builder:appid) intoappIdToUseandatoms.builderAppId(Lines 292–303).reinitBuildersetsdocument.titlefromsavedBuilderInitOpts.appId(Line 250), which never sees the RTInfo-based override.
If RTInfo has a different appId than initOpts.appId (e.g., user previously selected a different app in the builder and it was persisted), the builder window title can show one appId while the builder actually operates on another.
Consider driving the title from the resolved appIdToUse instead of the raw init opts, e.g.:
- document.title = initOpts.appId ? `WaveApp Builder (${initOpts.appId})` : "WaveApp Builder";
+ let appIdToUse = initOpts.appId;
+ try {
+ const oref = WOS.makeORef("builder", initOpts.builderId);
+ const rtInfo = await RpcApi.GetRTInfoCommand(TabRpcClient, { oref });
+ if (rtInfo && rtInfo["builder:appid"]) {
+ appIdToUse = rtInfo["builder:appid"];
+ }
+ } catch (e) {
+ console.log("Could not load saved builder appId from rtinfo:", e);
+ }
+
+ document.title = appIdToUse ? `WaveApp Builder (${appIdToUse})` : "WaveApp Builder";
+ globalStore.set(atoms.builderAppId, appIdToUse);and (optionally) mirroring the RTInfo lookup in reinitBuilder so the title stays consistent across re-inits.
Also applies to: 276-277, 292-303
🤖 Prompt for AI Agents
In frontend/wave.ts around lines 250-252 (also relevant at 276-277 and 292-303):
the window title is being set from the raw init opts (savedBuilderInitOpts.appId
/ initOpts.appId) which can differ from the resolved appId coming from RTInfo
(atoms.builderAppId / appIdToUse). Update reinitBuilder and initBuilder to set
document.title from the resolved appIdToUse (or atoms.builderAppId after RTInfo
resolution) instead of the raw init options; if reinitBuilder does not run the
RTInfo resolution, mirror the RTInfo lookup there or read atoms.builderAppId and
use that to compose the title so the UI title always reflects the actual appId
in use.
| settings := wconfig.GetWatcher().GetFullConfig().Settings | ||
| scaffoldPath := settings.TsunamiScaffoldPath | ||
| if scaffoldPath == "" { | ||
| return fmt.Errorf("tsunami:scaffoldpath is required") | ||
| scaffoldPath = filepath.Join(wavebase.GetWaveAppPath(), "tsunamiscaffold") | ||
| } | ||
| scaffoldPath, err := wavebase.ExpandHomeDir(scaffoldPath) | ||
| if err != nil { | ||
| return fmt.Errorf("tsunami:scaffoldpath invalid: %w", err) | ||
| } | ||
| if !filepath.IsAbs(scaffoldPath) { | ||
| return fmt.Errorf("tsunami:scaffoldpath must be absolute: %s", scaffoldPath) | ||
| } | ||
|
|
||
| sdkReplacePath := blockMeta.GetString(waveobj.MetaKey_TsunamiSdkReplacePath, "") | ||
| if sdkReplacePath == "" { | ||
| return fmt.Errorf("tsunami:sdkreplacepath is required") | ||
| } | ||
| sdkReplacePath, err = wavebase.ExpandHomeDir(sdkReplacePath) | ||
| if err != nil { | ||
| return fmt.Errorf("tsunami:sdkreplacepath invalid: %w", err) | ||
| } | ||
| if !filepath.IsAbs(sdkReplacePath) { | ||
| return fmt.Errorf("tsunami:sdkreplacepath must be absolute: %s", sdkReplacePath) | ||
| sdkReplacePath := settings.TsunamiSdkReplacePath | ||
| sdkVersion := settings.TsunamiSdkVersion | ||
| if sdkVersion == "" { | ||
| sdkVersion = "v0.12.2" | ||
| } | ||
| goPath := settings.TsunamiGoPath |
There was a problem hiding this comment.
Add nil checks and use constant for SDK version.
The settings retrieval lacks defensive nil checks and uses a hardcoded SDK version string.
Apply this diff to improve safety and maintainability:
+ watcher := wconfig.GetWatcher()
+ if watcher == nil {
+ return fmt.Errorf("config watcher not initialized")
+ }
+ fullConfig := watcher.GetFullConfig()
+ if fullConfig.Settings == nil {
+ return fmt.Errorf("settings not initialized")
+ }
+
- settings := wconfig.GetWatcher().GetFullConfig().Settings
+ settings := fullConfig.Settings
scaffoldPath := settings.TsunamiScaffoldPath
if scaffoldPath == "" {
scaffoldPath = filepath.Join(wavebase.GetWaveAppPath(), "tsunamiscaffold")
}
sdkReplacePath := settings.TsunamiSdkReplacePath
sdkVersion := settings.TsunamiSdkVersion
if sdkVersion == "" {
- sdkVersion = "v0.12.2"
+ // TODO: Import and use constant from tsunami/cmd package
+ sdkVersion = "v0.12.2" // Matches TsunamiSdkVersion in tsunami/cmd/main-tsunami.go
}
goPath := settings.TsunamiGoPathCommittable suggestion skipped: line range outside the PR's diff.
| func (bc *BuilderController) runBuilderApp(ctx context.Context, appId string, appBinPath string, builderEnv map[string]string) (*BuilderProcess, error) { | ||
| manifest, err := waveappstore.ReadAppManifest(appId) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read app manifest: %w", err) | ||
| } | ||
|
|
||
| secretBindings, err := waveappstore.ReadAppSecretBindings(appId) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read secret bindings: %w", err) | ||
| } | ||
|
|
||
| secretEnv, err := waveappstore.BuildAppSecretEnv(appId, manifest, secretBindings) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build secret environment: %w", err) | ||
| } | ||
|
|
||
| if builderEnv == nil { | ||
| builderEnv = make(map[string]string) | ||
| } | ||
| for k, v := range secretEnv { | ||
| builderEnv[k] = v | ||
| } | ||
|
|
There was a problem hiding this comment.
Running apps now hard‑require a manifest; consider treating “no manifest” as optional
The new runBuilderApp path fails fast on any ReadAppManifest error:
manifest, err := waveappstore.ReadAppManifest(appId)
if err != nil {
return nil, fmt.Errorf("failed to read app manifest: %w", err)
}Given ReadAppManifest wraps os.ReadFile errors, this will break running existing apps that don’t yet ship a manifest (e.g., missing manifest.json), whereas BuildAppSecretEnv already handles manifest == nil gracefully.
If manifests are not guaranteed to exist for all apps at this point, this is a backward‑compatibility issue and will cause builder runs to fail unnecessarily.
You can keep missing manifests non‑fatal while still surfacing real errors by allowing the “file not found” case:
- manifest, err := waveappstore.ReadAppManifest(appId)
- if err != nil {
- return nil, fmt.Errorf("failed to read app manifest: %w", err)
- }
+ manifest, err := waveappstore.ReadAppManifest(appId)
+ if err != nil && !os.IsNotExist(err) {
+ return nil, fmt.Errorf("failed to read app manifest: %w", err)
+ }This leaves manifest as nil when the file doesn’t exist, so BuildAppSecretEnv will no-op and you still get strict behavior for malformed or otherwise broken manifest files.
🤖 Prompt for AI Agents
In pkg/buildercontroller/buildercontroller.go around lines 298 to 320, change
the ReadAppManifest error handling so a missing manifest file is non‑fatal: call
ReadAppManifest, and if it returns an error check if the error is a
file-not-found (os.IsNotExist or wrapped equivalent); if so leave manifest as
nil and continue, otherwise return the wrapped error as today. This preserves
backward compatibility by allowing BuildAppSecretEnv to handle a nil manifest
while still surfacing real I/O or parse errors.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| editStr = "edit" | ||
| } | ||
|
|
||
| result := make([]string, len(edits)+1) |
Check failure
Code scanning / CodeQL
Size computation for allocation may overflow High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
The best fix is to validate the size of the input before it is used to allocate memory, and reject unreasonably large input with a clear error.
- The limit should be defined at a sensible maximum for your use-case. For an array of edit specifications, a value like 1000 is almost certainly enough for all normal operation and protects against both overflow and OOM attacks.
- Change
parseBuilderEditAppFileInputintools_builder.goto check thatlen(result.Edits)does not exceed this maximum (e.g.,1000). - If exceeded, return a user-friendly error such as "too many edits requested (max 1000)".
- No other regions need adjustment, since after this check, all downstream code can safely assume a reasonable length.
| @@ -165,6 +165,11 @@ | ||
| return nil, fmt.Errorf("missing edits parameter") | ||
| } | ||
|
|
||
| const maxEdits = 1000 | ||
| if len(result.Edits) > maxEdits { | ||
| return nil, fmt.Errorf("too many edits requested (maximum %d allowed)", maxEdits) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
| // ReplaceInFilePartial applies edits incrementally up to the first failure. | ||
| // Returns the results for each edit and writes the partially modified content. | ||
| func ReplaceInFilePartial(filePath string, edits []EditSpec) ([]EditResult, error) { | ||
| fileInfo, err := os.Stat(filePath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 5 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| return nil, fmt.Errorf("file too large for editing: %d bytes (max: %d)", fileInfo.Size(), MaxEditFileSize) | ||
| } | ||
|
|
||
| contents, err := os.ReadFile(filePath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 5 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
|
|
||
| modifiedContents, results := ApplyEditsPartial(contents, edits) | ||
|
|
||
| if err := os.WriteFile(filePath, modifiedContents, fileInfo.Mode()); err != nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 5 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| } | ||
|
|
||
| manifestPath := filepath.Join(appDir, ManifestFileName) | ||
| data, err := os.ReadFile(manifestPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fully mitigate path traversal risks, the code should enforce that any path constructed from user input is always strictly contained within a designated safe directory. In this situation, the home directory + "waveapps" is intended as the root for all app directories. The fix involves updating GetAppDir so that, after constructing the path using the validated appId components, it resolves the absolute path and checks that it starts with the absolute safe root directory. If the check fails, the function should return an error, rejecting the input no matter what. This complements existing regex and length validation, and makes the containment explicit and robust.
To implement this:
- Update the
GetAppDirfunction to use the absolute path for both the computed app directory and the apps root, and compare prefixes (usingstrings.HasPrefixor similar, taking care to avoid false matches for e.g./home/foo/barvs/home/foo/barbaz). - Return an error if the check fails.
- No new imports are needed—existing imports suffice.
- All changes are confined to
pkg/waveappstore/waveappstore.go, specifically in theGetAppDirfunction.
| @@ -82,7 +82,21 @@ | ||
| } | ||
| appNS, appName, _ := ParseAppId(appId) | ||
| homeDir := wavebase.GetHomeDir() | ||
| return filepath.Join(homeDir, "waveapps", appNS, appName), nil | ||
| appsRoot := filepath.Join(homeDir, "waveapps") | ||
| appDir := filepath.Join(appsRoot, appNS, appName) | ||
| absAppsRoot, err := filepath.Abs(appsRoot) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get absolute apps root: %w", err) | ||
| } | ||
| absAppDir, err := filepath.Abs(appDir) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to get absolute app dir: %w", err) | ||
| } | ||
| // Ensure absAppDir is strictly inside absAppsRoot | ||
| if absAppDir == absAppsRoot || !strings.HasPrefix(absAppDir, absAppsRoot+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("app directory resolves outside of apps root") | ||
| } | ||
| return absAppDir, nil | ||
| } | ||
|
|
||
| func copyDir(src, dst string) error { |
| } | ||
|
|
||
| bindingsPath := filepath.Join(appDir, SecretBindingsFileName) | ||
| data, err := os.ReadFile(bindingsPath) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To guarantee that paths constructed from a user-supplied appId are always contained within the desired app home, defensive validation should be applied after constructing the absolute path. After creating the appDir in GetAppDir, normalize it to absolute form and check that it is strictly a subdirectory of the intended apps root directory (e.g. $HOME/waveapps/). If the resolved path does not start with the canonical apps root prefix, reject it. This guards against any flaws in predecessor validation or possible future modifications.
Specifically, edit the function GetAppDir in pkg/waveappstore/waveappstore.go:
- After constructing
appDir, resolve it to an absolute path. - Also resolve the app home root itself to an absolute path.
- Check that the absolute
appDirbegins with the root prefix and is not equal to the root itself. - Return an error if the check fails.
Add a testable helper for this containment check if desired.
No new imports are required; the necessary filepath and strings packages are available.
| @@ -82,7 +82,23 @@ | ||
| } | ||
| appNS, appName, _ := ParseAppId(appId) | ||
| homeDir := wavebase.GetHomeDir() | ||
| return filepath.Join(homeDir, "waveapps", appNS, appName), nil | ||
| appsRootDir := filepath.Join(homeDir, "waveapps") | ||
| appDir := filepath.Join(appsRootDir, appNS, appName) | ||
| absAppsRootDir, err := filepath.Abs(appsRootDir) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to resolve apps root dir: %w", err) | ||
| } | ||
| absAppDir, err := filepath.Abs(appDir) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to resolve app dir: %w", err) | ||
| } | ||
| absAppsRootDir = filepath.Clean(absAppsRootDir) | ||
| absAppDir = filepath.Clean(absAppDir) | ||
| // Ensure absAppDir is strictly a subdirectory; equal is not allowed | ||
| if !strings.HasPrefix(absAppDir, absAppsRootDir+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("resolved app dir escapes app root") | ||
| } | ||
| return absAppDir, nil | ||
| } | ||
|
|
||
| func copyDir(src, dst string) error { |
| func generateManifest(tempDir, exePath string, opts BuildOpts) error { | ||
| oc := opts.OutputCapture | ||
|
|
||
| manifestCmd := exec.Command(exePath, "--manifest") |
Check failure
Code scanning / CodeQL
Command built from user-controlled sources Critical
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/wave.ts (1)
251-251: Builder window title can still drift from actual appId on reinit.While
initBuildernow correctly sets the title from the resolvedappIdToUse(line 301),reinitBuilderstill sets the title directly fromsavedBuilderInitOpts.appIdwithout checking RTInfo. If the user previously selected a different app in the builder and it was persisted tobuilder:appidin RTInfo, the title will be stale after reinit.Consider mirroring the RTInfo lookup from
initBuilderor reading fromatoms.builderAppId:async function reinitBuilder() { console.log("Reinit Builder"); getApi().sendLog("Reinit Builder"); // We use this hack to prevent a flicker of the previously-hovered tab when this view was last active. document.body.classList.add("nohover"); requestAnimationFrame(() => setTimeout(() => { document.body.classList.remove("nohover"); }, 100) ); await WOS.reloadWaveObject<Client>(WOS.makeORef("client", savedBuilderInitOpts.clientId)); - document.title = savedBuilderInitOpts.appId ? `WaveApp Builder (${savedBuilderInitOpts.appId})` : "WaveApp Builder"; + + let appIdToUse = savedBuilderInitOpts.appId; + try { + const oref = WOS.makeORef("builder", savedBuilderInitOpts.builderId); + const rtInfo = await RpcApi.GetRTInfoCommand(TabRpcClient, { oref }); + if (rtInfo && rtInfo["builder:appid"]) { + appIdToUse = rtInfo["builder:appid"]; + } + } catch (e) { + console.log("Could not load saved builder appId from rtinfo:", e); + } + + document.title = appIdToUse ? `WaveApp Builder (${appIdToUse})` : "WaveApp Builder"; + globalStore.set(atoms.builderAppId, appIdToUse); + getApi().setWindowInitStatus("wave-ready"); globalStore.set(atoms.reinitVersion, globalStore.get(atoms.reinitVersion) + 1); globalStore.set(atoms.updaterStatusAtom, getApi().getUpdaterStatus());pkg/blockcontroller/tsunamicontroller.go (1)
129-139: Add nil checks to prevent potential panics.The configuration retrieval chain lacks defensive nil checks, which could lead to panics if the watcher or config is not properly initialized.
Apply this diff to add defensive checks:
+ watcher := wconfig.GetWatcher() + if watcher == nil { + return fmt.Errorf("config watcher not initialized") + } + fullConfig := watcher.GetFullConfig() + if fullConfig.Settings == nil { + return fmt.Errorf("settings not initialized") + } + - settings := wconfig.GetWatcher().GetFullConfig().Settings + settings := fullConfig.Settings
🧹 Nitpick comments (2)
pkg/aiusechat/openai/openai-backend.go (1)
861-879: Consider extracting progress emission into a helper function.The progress data construction and emission logic (lines 870-875) is duplicated in the
donehandler (lines 904-909). Extracting this into a helper reduces duplication and improves maintainability.Example helper function you can add before
handleOpenAIEvent:// emitToolProgress sends tool progress data via SSE if parsing succeeds func emitToolProgress(sse *sse.SSEHandlerCh, toolCallID, toolName string, toolDef *uctypes.ToolDefinition, jsonData any) { if toolDef == nil || toolDef.ToolProgressDesc == nil { return } statusLines, err := toolDef.ToolProgressDesc(jsonData) if err == nil { progressData := &uctypes.UIMessageDataToolProgress{ ToolCallId: toolCallID, ToolName: toolName, StatusLines: statusLines, } _ = sse.AiMsgData("data-toolprogress", "progress-"+toolCallID, progressData) } }Then simplify the delta handler:
if st := state.blockMap[ev.ItemId]; st != nil && st.kind == openaiBlockToolUse { st.partialJSON = append(st.partialJSON, []byte(ev.Delta)...) - toolDef := state.chatOpts.GetToolDefinition(st.toolName) - if toolDef != nil && toolDef.ToolProgressDesc != nil { - parsedJSON, err := utilfn.ParsePartialJson(st.partialJSON) - if err == nil { - statusLines, err := toolDef.ToolProgressDesc(parsedJSON) - if err == nil { - progressData := &uctypes.UIMessageDataToolProgress{ - ToolCallId: st.toolCallID, - ToolName: st.toolName, - StatusLines: statusLines, - } - _ = sse.AiMsgData("data-toolprogress", "progress-"+st.toolCallID, progressData) - } - } - } + if parsedJSON, err := utilfn.ParsePartialJson(st.partialJSON); err == nil { + toolDef := state.chatOpts.GetToolDefinition(st.toolName) + emitToolProgress(sse, st.toolCallID, st.toolName, toolDef, parsedJSON) + } }And the done handler:
- if toolDef != nil && toolDef.ToolProgressDesc != nil { - var parsedJSON any - if err := json.Unmarshal([]byte(ev.Arguments), &parsedJSON); err == nil { - statusLines, err := toolDef.ToolProgressDesc(parsedJSON) - if err == nil { - progressData := &uctypes.UIMessageDataToolProgress{ - ToolCallId: st.toolCallID, - ToolName: st.toolName, - StatusLines: statusLines, - } - _ = sse.AiMsgData("data-toolprogress", "progress-"+st.toolCallID, progressData) - } - } - } + var parsedJSON any + if err := json.Unmarshal([]byte(ev.Arguments), &parsedJSON); err == nil { + toolDef := state.chatOpts.GetToolDefinition(st.toolName) + emitToolProgress(sse, st.toolCallID, st.toolName, toolDef, parsedJSON) + }pkg/blockcontroller/tsunamicontroller.go (1)
32-33: Consider centralizing the SDK version constant.The version string is now duplicated between this package and
tsunami/cmd/main-tsunami.go. While importing from acmdpackage is not ideal, consider moving this constant to a shared package (e.g.,pkg/tsunamiutilor a newpkg/tsunamiversion) that both locations can import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/view/tsunami/tsunami.tsx(2 hunks)frontend/wave.ts(3 hunks)pkg/aiusechat/openai/openai-backend.go(3 hunks)pkg/blockcontroller/tsunamicontroller.go(3 hunks)pkg/util/utilfn/partial.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/app/view/tsunami/tsunami.tsx
- pkg/util/utilfn/partial.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-21T05:09:26.916Z
Learnt from: sawka
Repo: wavetermdev/waveterm PR: 2465
File: frontend/app/onboarding/onboarding-upgrade.tsx:13-21
Timestamp: 2025-10-21T05:09:26.916Z
Learning: In the waveterm codebase, clientData is loaded and awaited in wave.ts before React runs, ensuring it is always available when components mount. This means atoms.client will have data on first render.
Applied to files:
frontend/wave.ts
📚 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/openai/openai-backend.go
🧬 Code graph analysis (2)
pkg/aiusechat/openai/openai-backend.go (2)
pkg/util/utilfn/partial.go (1)
ParsePartialJson(167-175)pkg/aiusechat/uctypes/usechat-types.go (1)
UIMessageDataToolProgress(166-170)
pkg/blockcontroller/tsunamicontroller.go (5)
pkg/wconfig/filewatcher.go (1)
GetWatcher(33-57)pkg/wavebase/wavebase.go (2)
GetWaveAppPath(113-115)ExpandHomeDir(141-155)tsunami/cmd/main-tsunami.go (1)
TsunamiSdkVersion(17-17)pkg/waveobj/metaconsts.go (2)
MetaKey_TsunamiAppPath(129-129)MetaKey_TsunamiAppId(130-130)pkg/waveappstore/waveappstore.go (1)
GetAppDir(79-86)
⏰ 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 (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (6)
pkg/aiusechat/openai/openai-backend.go (2)
385-385: LGTM!The new
partialJSONfield is appropriately typed and well-documented for tracking accumulated JSON arguments during streaming.
898-912: Progress emission logic is correct.This segment correctly emits final progress data using the complete arguments. The code duplication with the delta handler has been flagged in the previous comment.
frontend/wave.ts (1)
289-302: Good fix: title now uses resolved appId from RTInfo.The implementation correctly resolves
appIdToUsefrom RTInfo (builder:appid) before setting the document title. This ensures the title reflects the actual app being edited rather than the initialinitOpts.appId, addressing the concern from the previous review forinitBuilder.pkg/blockcontroller/tsunamicontroller.go (3)
141-161: LGTM! Well-structured appPath resolution with proper validation.The logic correctly handles both
tsunami:apppathandtsunami:appidmetadata with appropriate fallback behavior, home directory expansion, and absolute path validation. Error handling is comprehensive.
182-193: LGTM! BuildOpts correctly propagates new configuration fields.The addition of
SdkVersionandGoPathfields properly integrates the centralized configuration into the build flow.
226-229: LGTM! Appropriate async pattern for schema retrieval.The asynchronous fetch ensures schema retrieval doesn't block the Start() method, and errors are properly logged within
fetchAndSetSchemas.
…ion. fix builder initial-load/refresh flow
