fix(llm): stop leaking instruction prompts instead of processed text
Running a custom instruction (translate, summarise, rewrite, explain code,
free prompt) inserted the instruction's own wording instead of the result.
Two faults stacked:
The instruction was passed as the text to process, leaving the system-prompt
argument empty. `BASE_SYSTEM_PROMPTS` has no `custom` key, so resolution fell
back to `refine` without saying so, and the model dutifully polished the
instruction it had been handed. The transcript never reached it.
And only `{{text}}` was substituted, which none of the five built-in
instructions use — they carry `{{targetLanguage}}`, `{{userPrompt}}`, or no
placeholder at all. The substitution was a no-op from the day it was written:
the presets landed ten hours before the code that expected them.
- Instruction prompts now go to the system-prompt argument and the transcript
to the text argument. Instructions that spell out `{{text}}` keep their old
meaning, so hand-written ones still work.
- `renderInstructionPrompt` resolves `{{text}}`, `{{userPrompt}}` and
`{{targetLanguage}}` in one place, and warns by name when a placeholder is
left standing rather than letting it reach the model.
- `resolveSystemPrompt` no longer drops silently to `refine` for `custom`.
- Voice shortcuts no longer die at the `defaultLLMAction === 'none'` gate; an
explicitly named instruction outranks the default. Without one, `none` still
passes the transcript through untouched.
- `translate` receives its target language instead of relying on a default two
call frames away. It is still always English — `AppConfig` has no key for it,
and neither `language` (UI locale) nor `sttLanguage` (source language) can
stand in. Choosing a target language needs a setting and is not in this fix.
- Chains ran instructions with placeholders intact; they share the same
resolution now.
- The command screen's pipeline bench called `llm.generate`, which preload does
not expose, so every run threw and the catch showed the input back as if it
had succeeded. It uses `llm.process` now, over the same path production
takes, and a failure reads as a failure.
Present since the feature shipped: the custom-instruction path has never
worked. Plain actions (refine, summarise, grammar, expand) were unaffected and
are now covered by tests so they stay that way.
This commit is contained in:
parent
30d51c952f
commit
99f06c253c
14 changed files with 992 additions and 44 deletions
|
|
@ -48,7 +48,7 @@ Singleton + `EventEmitter` pattern (`getXService()` accessors).
|
|||
| `LocalLLMService` | Ollama REST (models, pull w/ progress, server start, NDJSON streaming) |
|
||||
| `PremiumLLMService` | Claude via Supabase `llm-proxy`, local fallback |
|
||||
| `OnlineLLMService` | JWT-authenticated .NET backend client |
|
||||
| `llm-prompts.ts` | `resolveSystemPrompt` SSOT for action prompts |
|
||||
| `llm-prompts.ts` | **SSOT for prompt resolution, placeholder substitution, and argument placement.** `resolveSystemPrompt` (`:118`) maps an `LLMAction` to its base prompt and handles `custom` explicitly instead of dropping silently to `refine`. `renderInstructionPrompt` (`:78`) substitutes `{{text}}` / `{{userPrompt}}` / `{{targetLanguage}}` and **warns by name** for any placeholder left standing rather than letting it reach the model. `buildInstructionInvocation` (`:101`) decides where an instruction goes in `processText(text, action, targetLanguage, customPrompt)`: the instruction becomes the **system prompt** and the transcript the **text**, except for instructions that spell out `{{text}}`, which keep the old meaning for backward compatibility. `resolveTargetLanguage` (`:50`) is the one place translate targets are decided (still `English`, see `11` GAP-LLM-01). **All three LLM entry paths call the same functions** — `VoiceModeService` (`:838`), `ChainService` (`:196`), and the `LLM.PROCESS` IPC handler (`llm-handlers.ts:96`) — so no caller re-implements the rules |
|
||||
|
||||
### Memory & knowledge
|
||||
| Service | Purpose |
|
||||
|
|
@ -59,7 +59,7 @@ Singleton + `EventEmitter` pattern (`getXService()` accessors).
|
|||
| `RAGService` | Local RAG: `nomic-embed-text` embeddings, cosine search over `rag_chunks` |
|
||||
| `CustomInstructionService` | User LLM commands (5 built-ins) |
|
||||
| `VoiceCommandService` | Keyword → command rule matching |
|
||||
| `ChainService` | Multi-step LLM pipelines (LLMChain) |
|
||||
| `ChainService` | Multi-step LLM pipelines (LLMChain). Each step resolves its instruction through `llm-prompts.ts` (`ChainService.ts:196`); before that, chain steps sent placeholders through unsubstituted |
|
||||
| `ScreenContextService` | Active-window + selected-text context |
|
||||
|
||||
### Phase 10+ features
|
||||
|
|
@ -140,6 +140,8 @@ Registry: `src/main/ipc/index.ts` calls 29 `registerXHandlers()` in fixed order.
|
|||
|
||||
The **`KEYBINDING`** group replaced the old per-action `HOTKEY` group. `HOTKEY` had 14 channels — a get/set pair per action plus three that were never implemented — so every new action meant new channels. `KEYBINDING` is 9 channels that take the action **as a parameter**: `getMap`, `setBindings`, `resetAction`, `resetAll`, `validate`, `isEnabled`, `setEnabled`, plus the `triggered` / `changed` events (`packages/core/src/ipc-channels.ts:104`). Adding an action now costs zero channels.
|
||||
|
||||
**`LLM.PROCESS` normalizes at the IPC boundary.** The handler runs `buildInstructionInvocation` itself when `action === 'custom'` with a `customPrompt` (`llm-handlers.ts:94-108`), so the renderer passes the **raw instruction text** and never duplicates the substitution or argument-placement rules. This is what makes `VoiceModeService`, `ChainService`, and `LLM.PROCESS` literally share one implementation. No channel or type changed for this; `LLMProcessParams` is unchanged.
|
||||
|
||||
Preload exposes **`window.electronAPI`** with 33 namespaces: `platform, audio, config, voice, stt, keybinding, llm (incl. premium), history, dictionary, stats, window, system, instruction, app, memo, voiceCommand, context, chain, caption, license, fileTranscription, meetingSummary, dictationTemplate, rag, voiceAction, voiceConversation, meetingMode, meetingChat, meetingDocTemplate, cloudSync, onlineAuth, ads, support, payment`. The `keybinding` bridge is 9 methods mirroring the channels above (`src/preload/index.ts:323`), replacing the 11-method `hotkey` bridge. Envelope: `IPCResult<T>` (success/error); `app.onDataChanged` is the global refresh channel.
|
||||
|
||||
---
|
||||
|
|
@ -191,12 +193,16 @@ DB schema (`src/main/db/schema.ts`, drizzle SQLite): `history`, `dictionary`, `s
|
|||
|
||||
## 6. Desktop status summary
|
||||
|
||||
- Core dictation/LLM/history pipeline: **implemented + tested**. Measured 2026-09-21: 1314 vitest cases in `apps/desktop`, 1311 passing; playwright e2e is separate. The failures are environment-dependent rather than regressions — two need a local sidecar venv or embedding server, one pins an error message that has since changed (`11` GAP-QA-02). These numbers hold with `better-sqlite3` built for the host Node ABI; rebuilding it for Electron to run the app invalidates them until you rebuild back (`11` GAP-INFRA-06).
|
||||
- Core dictation/LLM/history pipeline: **implemented + tested**. The vitest case count in `apps/desktop` is **1360** after the 2026-09-21 LLM fix added 46 cases; playwright e2e is separate. Read the pass numbers together with the `better-sqlite3` ABI the tree is built for (`11` GAP-INFRA-06) — they are not comparable across configurations:
|
||||
- **Host Node ABI** (2026-09-21, before the LLM fix): 1311 / 1314 passing. The three failures are environment-dependent rather than regressions — two need a local sidecar venv or embedding server, one pins an error message that has since changed (`11` GAP-QA-02). **This configuration has not been re-measured since the LLM fix.**
|
||||
- **Electron ABI** (2026-09-21, after the LLM fix): `366 failed | 994 passed (1360)`, against a clean-tree baseline of `366 failed | 948 passed (1314)` in the same configuration — identical failure count, +46 passed, **zero new failures**. 365 of those 366 are `tests/red/*.usecase.test.ts` files dying at DB creation because of the ABI mismatch, not assertions.
|
||||
- Cross-platform packaging: Windows NSIS (signed, `forceCodeSigning`), macOS DMG/ZIP arm64 (ad-hoc signing); auto-update via canonical Forgejo feed with update policy (`release/update-policy.json`).
|
||||
- Local-first AI (SoX + faster-whisper sidecar + bundled Ollama) and cloud paths both present.
|
||||
- **Local STT is packaged** (`1.3.0`): `electron-builder.yml` `extraResources` copies `sidecar-dist/sidecar` → `resources/sidecar` and `resources/ffmpeg` → `resources/ffmpeg`; `scripts/ci/verify-sidecar-bundle.mjs` gates packaging. Build locally with `npm --prefix apps/desktop run sidecar:setup && npm --prefix apps/desktop run sidecar:build`. The sidecar stays in console mode so `stdout`/`stderr` reach the app log (UTF-8, line-buffered); a packaged sidecar **must** exist or startup fails loudly instead of silently falling back to a system Python.
|
||||
- All local engine URLs (`LocalSTTService`, `LocalLLMService`, `RAGService`, `OnlineLLMService`, `STTManager`) pass through `src/main/utils/loopback.ts`, which rewrites `localhost` to `127.0.0.1`, because some Windows hosts resolve `localhost` to IPv6 only and local engines bind IPv4.
|
||||
- Meeting intelligence, RAG, voice conversation (local + Realtime), captions, file transcription: implemented.
|
||||
- **LLM instruction prompts: fixed 2026-09-21 (`9c2b4d4`), not yet verified in a running app.** Running a custom instruction inserted the instruction's own wording instead of the processed result. Two faults stacked: the instruction was passed in the `text` argument with the system-prompt argument left empty, and `BASE_SYSTEM_PROMPTS` has no `custom` key so resolution fell back to `refine` **silently** — the model polished the instruction and the transcript never reached it; separately, only `{{text}}` was substituted and none of the five built-in presets use it (`{{targetLanguage}}`, `{{userPrompt}}`, or no placeholder), so the substitution was a no-op from the day it was written. Introduced in `fea923d` (2026-04-05) and present in every release `v0.1.0-alpha`..`v1.4.0` — **the path never worked; this is not a regression.** Plain actions (`refine`/`summarize`/`grammar`/`expand`) were unaffected and are now pinned by regression cases. The fix routes all three entry paths through `llm-prompts.ts` (see §2) and additionally corrects two things found alongside it: a voice shortcut naming an instruction was nullified by the `defaultLLMAction === 'none'` gate (`VoiceModeService.ts:779`), and the commands-page pipeline bench called `llm.generate`, which preload does not expose, so every run threw and the `catch` displayed the **input** as if it had succeeded — a fail-closed violation that is the reason the bug went unnoticed for five months (`CommandsPage.tsx:180-205`, now on `llm.process` with failures rendered as failures).
|
||||
- **Verification limits — do not read this as verified.** Unit tests pass (`llm-prompts.test.ts` 21, `llm-handlers.test.ts` 7, `VoiceModeService.test.ts` 22, `ChainService.test.ts` 5), and each of the four fixes was reverted individually to confirm the tests actually fail without it. `npm run lint` (apps/desktop scope) passes; `tsconfig.check.json` errors went 36 → 35 (the `llm.generate` error is gone) with no errors in the touched files. But there is **no running-app run**, and `tests/red/{instruction,chain,voice,config}.usecase.test.ts` — precisely the related paths — never executed because of the `better-sqlite3` ABI mismatch. That range is neither passing nor failing; it is untested (`11` GAP-LLM-02, GAP-INFRA-06).
|
||||
- **Key bindings: implemented and verified on Windows.** Every global shortcut now comes from one contract (`@d3ro/core/keybinding`) with multiple bindings per action, mouse-button support, and no hardcoded accelerators left in `bootstrap.ts`. A manual run on 2026-09-21 confirmed legacy migration (custom values preserved), 6 actions loaded, the uiohook keyboard **and** mouse hook active with zero boot errors, and multi-binding working; contract side is `packages/core` 117 tests GREEN with no type errors in the key-binding files (`11` GAP-KEY-01 `[x]`). Two things remain open: `KeyBindingService` has no unit test of its own, and macOS/Linux mouse behavior is unconfirmed (`11` GAP-KEY-02). The rewrite also fixed a dead hands-free double-press path, an order-dependent reserved-combo check, a `globalShortcut.unregisterAll()` that wiped the popup accelerators, and a `setEnabled(true)` that re-enabled hooking with an empty binding set.
|
||||
- The same pass fixed an unrelated pre-existing dashboard bug: `caption.onStateChanged` delivers `{ state }`, but `DashboardPage` passed the whole object into `setCaptionState`, so the caption status readout never showed the right value (`DashboardPage.tsx:148`).
|
||||
- **Ad mediation**: `DirectHouseSponsorAdapter` performs real configurable REST bids; the other 9 adapters remain fail-closed stubs pending official SDKs (see `11-gap-backlog.md` GAP-ADS-01/02).
|
||||
|
|
@ -218,6 +224,7 @@ DB schema (`src/main/db/schema.ts`, drizzle SQLite): `history`, `dictionary`, `s
|
|||
| Preload API | `src/preload/index.ts` |
|
||||
| Windows | `src/main/windows/WindowManager.ts` |
|
||||
| Voice orchestrator | `src/main/services/VoiceModeService.ts` |
|
||||
| LLM prompt / placeholder SSOT | `src/main/services/llm-prompts.ts` (shared by `VoiceModeService`, `ChainService`, `ipc/llm-handlers.ts`) |
|
||||
| DB schema | `src/main/db/schema.ts` |
|
||||
| Renderer shell / routes | `src/renderer/components/AppLayout.tsx` |
|
||||
| Update feed SSOT | `src/main/update-feed.ts` |
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue