Leonid PershinandCursor
1a03c3178f
Restructure UI with app-level tabs and chat history drawer; add dataset curation, HF import, Modelfile/QLoRA hooks, and link approved samples to the agent immediately via heard vector memory without waiting for fine-tuning. Co-authored-by: Cursor <cursoragent@cursor.com>
128 lines
10 KiB
Markdown
128 lines
10 KiB
Markdown
# Project Review — 2026-08-22 (1)
|
||
|
||
Scope: Swarm Assistent 0.11.8 (client `Assets/assistent.js`, C# pipeline, packs, HTML/CSS). Includes a verification of the 0.11.8 ship (`a5e96f7`).
|
||
|
||
## Prior Reviews Summary
|
||
|
||
> Based on the last 3 review files analysed in Phase 0.
|
||
|
||
### Still Open (carried forward)
|
||
None.
|
||
|
||
### Resolved Since Last Review
|
||
None. (no prior `docs/reviews/` files)
|
||
|
||
---
|
||
|
||
## 0.11.8 verification
|
||
|
||
Checked against the ship notes: session_exact, slim `/debug ask`, generate-only-for-frames.
|
||
|
||
**Correct**
|
||
- `ListPacks` skips `hidden: true`; `LoadPackPrompt` still loads enabled hidden packs (`debug_explain.json` has `hidden: true`, `enabled: true`).
|
||
- Slim debug skips prefs, skills, identity, RAG retrieve, tool hops, and memory/pref/persona writes; Exact still loads when `includeBase` is false.
|
||
- Client `fromDebug` sends pack `debug_explain`, `includeBase: false`, `skipAppendUser`, empty skills; dump stays a system note.
|
||
- Single `doParams` body; `shouldRememberSessionParam` remembers when the user asked **or** the value differs from Exact.
|
||
- `userImpliesGenerate` has no noun-only fallback; `userIsChatNotFrame` strips generate on thanks/trivia; `willGen` no longer ORs the auto-generate checkbox.
|
||
- Auto-critique / auto-vision HTML defaults remain unchecked (0.11.4 opt-in).
|
||
- Negative pass-through and variant-interrupt epoch guard still in place.
|
||
|
||
**Not fully correct (filed below)**
|
||
- Nested Krea-prep / empty-patch retry `sendChat` is a no-op while `state.busy` is true (vision/critique hops are exempt; these are not).
|
||
- `fromDebug` returns after `rememberLastPatch` / `interrupt` / `maybeVisionHop`.
|
||
- Model `actions:["generate"]` still runs Generate unless `userIsChatNotFrame` matches.
|
||
- `shouldHonorLookAt` still honors unsolicited `look_at` on non-generate turns.
|
||
|
||
---
|
||
|
||
## Phase 1: Code Quality
|
||
|
||
### SOLID
|
||
`Assets/assistent.js` is a single IIFE (~9600 lines) owning chat, board, patches, inventory, personas, cards, and settings. C# is split into `partial` files on `SwarmAssistentExtension`, which is appropriate for a SwarmUI extension. No extra SOLID tasks beyond the concrete bugs below.
|
||
|
||
### Performance
|
||
- Every RAG retrieve loads the full `memories` table plus embeddings, then filters in C# — `AssistentMemory.cs` line 750.
|
||
|
||
### Correctness & Bugs
|
||
- Nested `sendChat` from `handleReplySideEffects` (Krea EN prep, empty-patch retry) hits the `state.busy` gate and returns without sending — `Assets/assistent.js` lines 8622–8624, 8104–8115, 8169–8181.
|
||
- `/debug` side effects run before the Q&A early return — lines 8120–8160.
|
||
- C# `TryParsePatch` returns the **first** fence; JS `extractPatch` keeps the **last** — `AssistentPatch.cs` line 97 vs `Assets/assistent.patch.js` line 88.
|
||
- `ResolveEnabledSkills` ignores `skills: []` (`Count > 0`) so the user cannot disable all skills — `AssistentConfig.cs` line 1474. Slim debug is unaffected because it skips the skills layer.
|
||
- `doInterruptNow` bumps `chatEpoch` without `clearInFlightUi`; `finishOk` then bails and leaves `state.busy` true — `Assets/assistent.js` lines 4629–4635, 8867–8868.
|
||
- Failed `RunToolHop` (`follow == null`) `break`s the hop loop, dropping sibling tools (e.g. empty `memory_get` kills `search_civitai`) — `AssistentChatPipeline.cs` lines 237–239.
|
||
- Overlay JSON writes never take `AssistentConfig._lock` (the field is unused) — `AssistentConfig.cs` line 18.
|
||
- Wanted YAML is load–mutate–`WriteAllText` with no lock — `AssistentWanted.cs` lines 46–77.
|
||
- Patch key lists diverge: JS has `persona_clone` / `persona`, neither list has `scheduler` (but `applyPatch` writes scheduler) — `AssistentPatch.cs` lines 12–25, `Assets/assistent.patch.js` lines 8–19, `Assets/assistent.js` line 4327.
|
||
|
||
### Code Quality
|
||
No extra tasks. Duplicated help strings (JS fallback vs `ui.json`) are filed under UX.
|
||
|
||
---
|
||
|
||
## Phase 2: Logical Consistency
|
||
|
||
### Domain & Application Layer
|
||
No layered DDD. Pack/core contracts vs client behavior are the real domain rules.
|
||
|
||
### Data Flow
|
||
- Server hops parse the first JSON fence; the UI applies the last. Hops/`ApplyMemoryActions` can follow a weak fence while Generate uses a later one.
|
||
- `includeBase` only gates `core.md`; Exact/live still flow on debug turns (intended).
|
||
|
||
### State Management
|
||
- `fromDebug` can overwrite `state.lastPatch` and start a vision hop before the early return.
|
||
- `skipAppendUser` persists an assistant explanation with no matching user turn in `state.history` — next chat turns see a dangling assistant message.
|
||
|
||
### Consistency
|
||
- `write_prompt.md` always demands a JSON patch with `prompt`+`negative`; `core.md` output contract shows a generate example as mandatory, while later saying “Pure Q&A: omit the JSON patch”. Client auto-apply will still write Swarm fields if the model emits a prompt patch on chat.
|
||
- `shouldHonorLookAt` returns true whenever there is `look_at` and no generate trigger — contradicts 0.11.4 / pack “look only if asked”.
|
||
- `wantsGen` trusts model `actions:["generate"]` unless `userIsChatNotFrame` (narrow). `synthesizePatchAfterEmptyFence` can invent `actions:["generate"]` when the reply has an empty `### JSON Patch` heading even if the user did not ask for a frame.
|
||
|
||
---
|
||
|
||
## Phase 3: UI/UX
|
||
|
||
### Usability
|
||
- Loading spinner, Stop, empty chat/board states exist.
|
||
- Chat clear confirms; **Clear Init+Mask** in the board ⋯ menu does not.
|
||
- `/debug ask` dump as a system note (no second user bubble) is correct.
|
||
|
||
### Visual & Consistency
|
||
No token/theme issues filed. Status and health use text plus color.
|
||
|
||
### Interaction & Feedback
|
||
- Interrupt from the Stop button clears UI; interrupt from a model patch does not (see busy-stuck bug).
|
||
- README slash table and `/pack` error string lag behind `Config/_base/ui.json`.
|
||
|
||
### Accessibility
|
||
- Composer `#sa_input` has only a placeholder (no accessible name).
|
||
- Persona/pack/model `<select>`s use `title` only.
|
||
- Main subtabs are a `tablist` without `role="tab"` / `aria-selected` (`setView` toggles CSS class only).
|
||
- `.sa-board` / `.sa-slot` set `outline: none` with no `:focus-visible` replacement; board is `tabindex="0"`.
|
||
- `#sa_status` is not `aria-live`.
|
||
|
||
---
|
||
|
||
## Tasks
|
||
|
||
> Only actionable issues are listed here. Carried-forward items from prior reviews come first (marked with source), followed by net-new findings.
|
||
|
||
- [x] 1. [Bug] Exempt `fromPromptEnRetry` and `fromEmptyPatchRetry` from the `state.busy` gate (same as vision/critique). Nested Krea-prep and empty-patch retry currently return immediately; the “Готовлю промпт…” note is shown and Generate is skipped. — `Assets/assistent.js` line 8623 *(already resolved via `isContinuationTurn`)*
|
||
- [x] 2. [Bug] Move the `fromDebug` early return to the top of `handleReplySideEffects` (after extract). Today debug can `rememberLastPatch`, `doInterruptNow`, and `maybeVisionHop` before the Q&A return. — `Assets/assistent.js` line 8151 *(already resolved: return immediately after `extractPatch`)*
|
||
- [x] 3. [Bug] Align C# `TryParsePatch` with JS `extractPatch`: prefer the last terminal/strong fence, not the first match. Server hops otherwise act on a weak fence while the UI applies a later one. — `AssistentPatch.cs` line 97
|
||
- [x] 4. [Logic] `shouldHonorLookAt` must not `return true` for unsolicited `look_at` on ordinary/write turns. Honor only user `/look`, vision packs, or auto-critique/hop. — `Assets/assistent.js` line 1184
|
||
- [x] 5. [Bug] Do not start Generate from a model `actions:["generate"]` or from `synthesizePatchAfterEmptyFence` unless the user turn is a frame (`userImpliesGenerate` / explicit `/gen`). Empty `### JSON Patch` on chat currently synthesizes generate. — `Assets/assistent.js` lines 915 and 8164
|
||
- [x] 6. [Bug] `doInterruptNow` (patch `interrupt` action) must call `clearInFlightUi` like the Stop button. Epoch bump inside `finishOk` currently leaves `state.busy` stuck. — `Assets/assistent.js` line 4629
|
||
- [x] 7. [Bug] Honor `clientSkills` even when empty: `if (clientSkills is not null)` (null = defaults, `[]` = none). Unchecking every skill currently reloads `defaultOn`. — `AssistentConfig.cs` line 1474
|
||
- [x] 8. [Bug] On `RunToolHop` returning null, skip that tool and try the next `NextToolHop` instead of `break`. Empty `memory_get` currently aborts `search_civitai` / inventory on the same patch. — `AssistentChatPipeline.cs` line 237
|
||
- [x] 9. [Logic] Make `write_prompt.md` and `core.md` match ordinary: fenced JSON only on frame turns; Q&A is prose only. The “mandatory JSON patch with prompt+negative” line fights auto-apply on chat. — `Config/_base/packs/write_prompt.md` line 8
|
||
- [x] 10. [Bug] Sync patch key lists: add `scheduler` to JS `PATCH_KEYS` and C# `PatchKeys`; add `persona_clone` / `persona` to C#. A scheduler-only (or clone-only) fence is dropped. — `Assets/assistent.patch.js` line 8
|
||
- [x] 11. [Performance] Filter RAG `SELECT` by persona chain (and/or FTS id list) instead of scanning every memory row + embedding on each chat. — `AssistentMemory.cs` line 750
|
||
- [x] 12. [Bug] Use `AssistentConfig._lock` around overlay read-modify-write (`SaveSettings`, control shelves) or remove the unused lock. Concurrent API calls can tear JSON. — `AssistentConfig.cs` line 18
|
||
- [x] 13. [Bug] Lock Wanted YAML load/merge/save (or write atomically). Concurrent enqueues can drop entries. — `AssistentWanted.cs` line 46
|
||
- [x] 14. [Logic] For `skipAppendUser`, persist a short history marker (`/debug ask`) with the assistant reply, or omit both. Orphan assistant turns pollute the next LLM context. — `Assets/assistent.js` line 8775
|
||
- [x] 15. [Accessibility] Name the composer (`aria-label` on `#sa_input`) and header selects; give main subtabs `role="tab"` + `aria-selected` in `setView`. — `Tabs/Text2Image/Assistent.html` line 93
|
||
- [x] 16. [Accessibility] Restore a `:focus-visible` ring on `.sa-board` and `.sa-slot` (they set `outline: none`; board is keyboard-focusable). — `Assets/assistent.css` line 95
|
||
- [x] 17. [UX] Mark `#sa_status` as `role="status"` / `aria-live="polite"` so busy/error text is announced. — `Tabs/Text2Image/Assistent.html` line 108
|
||
- [x] 18. [UX] Confirm before Clear Init+Mask (chat clear already confirms). — `Assets/assistent.js` line 9425
|
||
- [x] 19. [UX] Sync README slash table and `/pack` error status with `Config/_base/ui.json` (`/new`, `/history`, `/persona *`, `ordinary|card|persona`). — `README.md` line 115
|