Conversation
|
Codex review: found issues before merge. Summary Reproducibility: yes. The review findings are reproducible by comparing the PR diff against current-main source/docs/tests for Next step before merge Security Review findings
Review detailsBest possible solution: Keep the focused latency fixes, but restore shipped dictation, Arcee Trinity no-tools compatibility, and catalog-derived official npm plugin trust before changed-gate validation. Do we have a high-confidence way to reproduce the issue? Yes. The review findings are reproducible by comparing the PR diff against current-main source/docs/tests for Is this the best way to solve the issue? No. The session reload, transcript-key, lazy media-provider, and descriptor-cache directions are maintainable, but the current patch mixes them with unrelated removals that break existing product and plugin behavior. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9ef7f241f605. |
There was a problem hiding this comment.
Pull request overview
Reduces Control WebUI/Gateway session-related latency by avoiding unnecessary full session-list reloads, carrying session keys through transcript update events to skip file→session lookup, and making media generation provider discovery lazy when explicit model config is present.
Changes:
- Control UI: skip
loadSessions()onsessions.changedwhen a message-phase patch was successfully applied locally. - Gateway/session plumbing: include
sessionKeyin file-only transcript update emissions to avoid hot-path transcript-file key resolution. - Media tools: make provider listing lazy in
resolveCapabilityModelConfigForTool, and update image/video/music generation tools + tests to verify explicit model paths don’t enumerate providers.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/ui/app-gateway.ts | Avoids full session reload when a message-phase sessions.changed patch was applied. |
| ui/src/ui/app-gateway.sessions.node.test.ts | Adds coverage for the new message-phase no-reload behavior. |
| src/config/sessions/transcript.ts | Passes sessionKey through file-only transcript update events. |
| src/agents/tools/media-tool-shared.ts | Allows lazy provider sources (thunk) and memoizes resolution. |
| src/agents/tools/image-generate-tool.ts | Uses lazy provider listing for explicit image model config. |
| src/agents/tools/image-generate-tool.test.ts | Tests explicit-config path does not list runtime providers; refactors env stubbing. |
| src/agents/tools/video-generate-tool.ts | Uses lazy provider listing for explicit video model config. |
| src/agents/tools/video-generate-tool.test.ts | Adds explicit-config “no provider listing” test; improves env isolation. |
| src/agents/tools/music-generate-tool.ts | Makes provider selection lazy when no reference images; threads providerId via parsed model ref. |
| src/agents/tools/music-generate-tool.test.ts | Verifies explicit-config execution doesn’t list runtime providers. |
| src/agents/pi-embedded-runner/transcript-rewrite.ts | Emits transcript update with {sessionFile, sessionKey}. |
| src/agents/pi-embedded-runner/transcript-rewrite.test.ts | Updates assertions for new transcript update payload shape. |
| src/agents/pi-embedded-runner/tool-result-truncation.ts | Emits transcript update with {sessionFile, sessionKey}. |
| src/agents/pi-embedded-runner/tool-result-truncation.test.ts | Adds listener assertion including sessionKey. |
| src/agents/pi-embedded-runner/compaction-hooks.ts | Emits transcript update with {sessionFile, sessionKey}. |
| src/agents/pi-embedded-runner/compact.hooks.test.ts | Updates assertions for new transcript update payload shape. |
| src/agents/command/attempt-execution.ts | Emits transcript update with {sessionFile, sessionKey} after persisting turn transcript. |
| CHANGELOG.md | Documents the performance-related fixes and references relevant issues. |
|
@copilot resolve the merge conflicts in this pull request |
Done — merged latest |
37a2367 to
5ed7f1f
Compare
1f5a34c to
a1054fb
Compare
c1c34ee to
ad0e1a0
Compare
Rewrite PR branch onto current main with verified history and changelog attribution for @BunsDev.
Summary
sessions.listreload when a message-phasesessions.changedpayload already patched local session statesessionKeythrough file-only transcript update events so Gateway event handling can skip transcript-file-to-session lookup on hot pathsRoot Cause
The latency cluster mixed several hot-path costs: UI-side full list invalidation on message updates, Gateway transcript update events without session keys, and eager provider/tool materialization even when explicit generation model config already selected the provider/model.
Validation
pnpm test ui/src/ui/app-gateway.sessions.node.test.tspnpm test src/gateway/session-transcript-key.test.ts src/gateway/session-message-events.test.tspnpm test src/agents/tools/media-tool-shared.test.ts src/agents/tools/image-generate-tool.test.ts src/agents/tools/video-generate-tool.test.ts src/agents/tools/music-generate-tool.test.tspnpm test ui/src/ui/app-gateway.sessions.node.test.ts src/agents/pi-embedded-runner/compact.hooks.test.ts src/agents/pi-embedded-runner/transcript-rewrite.test.ts src/agents/pi-embedded-runner/tool-result-truncation.test.ts src/agents/tools/image-generate-tool.test.ts src/agents/tools/video-generate-tool.test.ts src/agents/tools/music-generate-tool.test.tspnpm exec oxfmt --check --threads=1 ...pnpm check:changelog-attributionsOPENCLAW_TESTBOX=1 pnpm check:changedTriage Notes
prtagsauth is valid, butprtags group list/searchreturned 502 during triage, so no duplicate group or gated@clawsweepercomment was written yet.Refs #76236
Refs #76203
Refs #76188
Refs #76107
Refs #76166