feat(desktop): the credential, the launcher, the language, and the run (8.3, 8.4, 8.12, 6.3) - #15
Merged
Merged
Conversation
Plan 8.3, 8.4, 8.12 and 6.3.
**6.3 is here because this is where the credential is.** 4.15 left the wiring
deliberately unbuilt: `config/secrets.ts` forbids the CLI, the hooks and the
MCP process from reading the Groq key, because their stderr is consumed by an
agent and travels to a model provider. The orchestrator that reads it had to
be the desktop application, and it needed 8.3 to have stored one. So group 4's
pipeline finally has something that starts it — preprocess if it has not been,
transcribe what the journal says is left, then finish. "Redo only what failed"
needs no flag: a resume sends exactly what did not succeed, which is
`adr:0012`'s default and the whole point of the journal.
The key is checked **on the spot**, against the models endpoint rather than by
sending audio — it costs nothing, and a 401 there means what a 401 from the
real call would. A wrong key is discovered at the settings screen rather than
an hour later with a meeting already recorded. A key that could not be
*checked* is told apart from one that is *wrong*: refusing to store it because
the user is on a train would be this screen inventing a policy nobody asked
for.
The key never crosses the bridge in either direction. `credentialState`
answers whether one is stored, never what it is — a field pre-filled with it
would put the application's one secret in the DOM of a window that renders
markdown an agent wrote. It is reached through a named
`@open-wiki/access/secrets` subpath rather than the barrel, so the one place
in the product that reads it stays visible in a single grep.
8.12 regenerates `CLAUDE.md`, because it is generated and carries the
language; the skills are not and are left alone, which is the distinction 9.4
draws. The generator moved out of the CLI into `@open-wiki/access` beside
`scaffoldSkills` — 9.3 and 9.4 are one act, and leaving it in the CLI would
have meant the desktop application either reaching into a package whose `.`
export points at a file that does not exist, or growing a second generator
that drifts.
The launcher creates a project through the scaffolder of 2.1, the same one
`ow init` and the first run use, so a project is the same project whichever
door it came through. A project whose directory moved is shown rather than
hidden: the registry is a cache and never truth (2.2), and hiding the entry
would leave the user wondering where their project went. Forgetting removes
the entry and never the directory.
One thing this found about itself: the "every channel dispatches" test called
`createProject("x", "y")` along with everything else, and `createProject`
actually scaffolds — so it left a real project directory inside the repository.
A relative path resolves against whatever working directory the Electron
process happens to have, which is not a place any user chose. It is refused
now, and the test no longer invokes the channels that write.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
Two that lose something the user cannot get back:
- **`deleteWavAfterTranscription: false` was ignored.** `runTranscription` was
the only production caller of `finishRecording` and never passed the
setting, so the seal deleted the WAVs regardless — ~690 MB per hour of "the
only copy of a meeting that already happened", for a user who had explicitly
said to keep it. It is a documented key in a closed schema and it was dead.
- **`timemap.json` was cast, not checked.** It is committed by design — the
managed gitignore covers the audio and `.state/`, not this — so it arrives
with a clone, and `{}` casts to `TimeMap` as happily as a real map does.
`timemap.ts` exports `isTimeMap` and says in as many words that every
consumer goes through it; this one did not. What flows out of it decides how
many *paid* provider requests one click makes and what wall-clock instant
every citation of that recording resolves to. A map that does not check out
is now treated as absent, so the recording is preprocessed again.
**whisper.cpp could be chosen, was accepted, was stored, and could never
transcribe anything.** `createProvider` needs a binary and a model, neither of
which exists anywhere in the product — so the sequence was: pick it, be told
it saved, record an hour, click Transcribe, get `MissingWhisperPathError`. No
*credential* is not the same as nothing to check, and this screen had no
business making that promise.
**8.4's launcher was ticked and unreachable.** `Launcher.tsx` was imported by
nothing and `index.ts` still printed to stderr and quit. A window may now have
no project: `project()` answers null, the renderer shows the launcher, and
every channel that needs a project refuses by saying there is none — which is
a better answer than a window wired to a directory nobody chose.
**Changing the content language bricked an in-flight transcription.** 4.17
refuses a journal whose language moved and says "start again from the
beginning, or put the previous settings back" — and the only caller in the
product could not take the first half of that offer, because `restart` was
never threaded through. A user who recorded, transcribed half, then switched
to pt-BR had a permanently unfinishable recording and its 690 MB WAV.
**`createProject` scaffolded before it validated the name.** `ProjectRegistry`
validates inside `register`, which is the last statement — so "My Project", an
ordinary thing to type into a prompt, created the whole tree and then threw,
leaving an orphan on disk nothing knew about.
**`saveCredential` cast its input where every other channel coerces.** A
`provider` of `"bogus"` took the Groq branch and was stored as `"bogus"`; a
`whispercpp` request persisted whatever `apiKey` came with it, unchecked. It
is parsed at the boundary now, and a key sent alongside whisper.cpp is dropped
rather than stored.
And in `secrets.ts`, which this diff is the first to write a real key through:
`defaultAppDataDir` fell back to `process.cwd()`, which for the desktop
process is the project — the exact "`git init` a week later turns a
conditional rule into a leak" outcome the module exists to prevent. It refuses
now. The file is written 0600 in a 0700 directory, which matters on a
developer machine falling back to `$HOME`.
Smaller: `checkCredential` asserts https before attaching the credential, the
same assertion `groq.ts` makes for the same written-down reason; an id that
escapes `raw/` and a missing manifest come back inside the outcome type rather
than rejecting past it; and `Deps.onProgress` is gone — nothing supplied it,
and progress already reaches the screen through the 8.10 watcher re-deriving
it from the journal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
📝 WalkthroughWalkthroughThe desktop app now supports launcher windows without a selected project, credential and language settings, project registry management, generated ChangesDesktop settings and transcription
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant Preload
participant MainIPC
participant Settings
Renderer->>Preload: request project or settings data
Preload->>MainIPC: invoke desktop IPC channel
MainIPC->>Settings: resolve project registry or credential state
Settings-->>MainIPC: return operation result
MainIPC-->>Preload: return IPC response
Preload-->>Renderer: update launcher or settings view
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
apps/desktop/src/renderer/Sources.tsx (1)
73-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard state updates in
transcribeagainst unmount.
transcribecallssetNote/setBusyafter anawaitwith no check thatSourceItemis still mounted. Unlike the pre-existingload/retitlecalls, a transcription can run for minutes, and this PR stack adds project switching, which unmountsSources. If the user switches projects while a transcription is in flight, the callback updates state on an unmounted component.Track mount status with a ref and skip the state updates once unmounted.
🛡️ Proposed fix
+ const mountedRef = useRef(true); + useEffect(() => { + return () => { + mountedRef.current = false; + }; + }, []); + const transcribe = useCallback(async () => { setBusy(true); setNote(null); try { const result = await bridge().transcribe(row.id); + if (!mountedRef.current) return; setNote( result.ok ? result.sealed ? "Transcribed." : `${result.done} of ${result.total} chunks done.` : result.reason, ); onChanged(); } catch (e) { - setNote(e instanceof Error ? e.message : String(e)); + if (mountedRef.current) setNote(e instanceof Error ? e.message : String(e)); } finally { - setBusy(false); + if (mountedRef.current) setBusy(false); } }, [row.id, onChanged]);
useRefanduseEffectneed to be imported fromreactif not already.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/Sources.tsx` around lines 73 - 91, Update the transcribe callback in SourceItem to track mounted state with a ref and lifecycle effect, and guard every setNote/setBusy after the awaited bridge().transcribe(row.id) call so they are skipped after unmount. Ensure cleanup marks the component unmounted while preserving the existing transcription result and error behavior.apps/desktop/src/main/settings.ts (2)
278-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
scaffoldalready writes the ignore file and the skills.
packages/access/src/scaffold.tscallswriteSettings,writeIgnore, andscaffoldSkillsbefore it returns. Lines 280 and 281 repeat that work. The repetition contradicts the comment above this function, which states that a project must come through one door. It also hides which caller owns each file, so a change to the scaffold order will not be visible here.Keep the two calls that the scaffold does not make: the chosen language and
CLAUDE.md.♻️ Proposed refactor
scaffold(directory); + // Only what the scaffolder does not do: the chosen language, and the + // generated `CLAUDE.md` that carries it. writeSettings(directory, { language }); - writeIgnore(directory); - scaffoldSkills(directory); writeClaudeMd(directory, language); registry.register(name, directory);Remove
scaffoldSkillsandwriteIgnorefrom the import list if no other call site remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/settings.ts` around lines 278 - 282, Remove the redundant writeIgnore and scaffoldSkills calls from the scaffold setup sequence, since scaffold already performs both operations. Keep writeSettings with the selected language and writeClaudeMd, and remove writeIgnore and scaffoldSkills imports if they are no longer referenced.
222-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
PROJECT_NAMEduplicates the registry's rule.
ProjectRegistry.validateNameinpackages/access/src/registry.tsapplies its ownNAME_REplus explicit slash checks. This constant restates that rule, and the comment says so. Two copies of one rule drift: a change toNAME_REwill not change this check, and the launcher will then accept a name thatregisterrefuses after the scaffold has run — the exact failure the comment at Line 270 describes.Export the validator from
@open-wiki/accessand call it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/settings.ts` around lines 222 - 234, The local PROJECT_NAME regex and assertProjectName duplicate ProjectRegistry.validateName and can drift. Export and reuse the registry’s validator from `@open-wiki/access` in the launcher validation flow, replacing the local regex-based check while preserving InvalidProjectNameError handling and rejecting names before scaffolding or registration.apps/desktop/tests/settings.spec.ts (1)
290-299: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese tests reach the real application data directory.
createApitakes noappDataDir, soapi.knownProjects()builds aProjectRegistryondefaultAppDataDir(). The test therefore reads%APPDATA%/open-wikior$HOME/open-wikion the machine that runs it, not the temporaryappDatadirectory this file creates. Two consequences follow. The result depends on whatever projects the developer has registered. If neitherAPPDATAnorHOMEis set,defaultAppDataDirnow throwsNoAppDataDirErrorand the test fails for a reason unrelated to the launcher.Let
Depscarry an optionalappDataDirand pass the temporary directory, or assert only the behaviour that needs no registry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/tests/settings.spec.ts` around lines 290 - 299, Update the test setup around createApi and Deps so the no-project tests use the file’s temporary appData directory when calling knownProjects(), rather than defaultAppDataDir(). Add an optional appDataDir dependency and pass it through to ProjectRegistry, or remove the registry-dependent assertion while preserving the project() null behavior.apps/desktop/src/renderer/Settings.tsx (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
LANGUAGESfrom@open-wiki/accessfor the value fields.The package exports the source-of-truth list. Import it as a value and derive this array's language values from it to prevent drift.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/Settings.tsx` around lines 16 - 20, Update the LANGUAGES constant in Settings.tsx to import and use the value-exported LANGUAGES list from `@open-wiki/access` as the source for each language value, while preserving the existing labels and array shape; do not duplicate language value literals locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/settings.ts`:
- Around line 258-263: Validate the language argument in createProject using the
same LANGUAGES check as setLanguage, alongside the existing name and directory
validation. Reject unknown values before writeSettings, writeClaudeMd, or any
project scaffolding proceeds, while preserving valid-language behavior.
- Around line 89-96: Update parseCredentialInput so Groq apiKey values are
trimmed before length validation and storage, while preserving the existing
provider validation and whispercpp handling. Use the normalized key for both the
MAX_KEY_CHARS check and the returned SaveCredentialInput.
In `@apps/desktop/src/main/transcribe-run.ts`:
- Around line 71-77: Wrap the readSecrets call in runTranscription with error
handling so failures from defaultAppDataDir, file reads, or JSON parsing return
an appropriate TranscribeOutcome instead of rejecting. Preserve the existing
no-provider configuration response when secrets are successfully read without
stt credentials.
In `@apps/desktop/src/renderer/App.tsx`:
- Around line 90-98: Update the initialization effect around bridge().project()
and refreshIndex() so refreshIndex() runs only after project() resolves with a
non-null project. Do not invoke refreshIndex() unconditionally on effect
startup; preserve the existing project state updates and error handling, and
keep the no-project case from triggering the index call.
In `@apps/desktop/src/renderer/Sources.tsx`:
- Around line 77-91: Update the note rendering near the existing
`setNote`/`setBusy` flow so failure messages from `result.reason` or caught
exceptions use the `error` class instead of `empty`, while preserving `empty`
for neutral informational messages. Reuse the existing failure-state distinction
and keep the current note content unchanged.
In `@packages/access/src/config/secrets.ts`:
- Around line 75-82: Update the secret-writing flow around mkdirSync and
writeFileSync to explicitly enforce 0700 on the parent directory and 0600 on the
secret file after they exist, including pre-existing paths. Add chmodSync to the
node:fs imports and apply it to the corresponding directory and file while
preserving the current write behavior.
- Around line 46-48: Handle NoAppDataDirError in the desktop callers
credentialState, saveCredential, forgetProject, and runTranscription: display
the error through the existing Launcher flow, and move runTranscription’s
defaultAppDataDir call inside its try so it still returns TranscribeOutcome.
Preserve the existing knownProjects and createProject handling.
---
Nitpick comments:
In `@apps/desktop/src/main/settings.ts`:
- Around line 278-282: Remove the redundant writeIgnore and scaffoldSkills calls
from the scaffold setup sequence, since scaffold already performs both
operations. Keep writeSettings with the selected language and writeClaudeMd, and
remove writeIgnore and scaffoldSkills imports if they are no longer referenced.
- Around line 222-234: The local PROJECT_NAME regex and assertProjectName
duplicate ProjectRegistry.validateName and can drift. Export and reuse the
registry’s validator from `@open-wiki/access` in the launcher validation flow,
replacing the local regex-based check while preserving InvalidProjectNameError
handling and rejecting names before scaffolding or registration.
In `@apps/desktop/src/renderer/Settings.tsx`:
- Around line 16-20: Update the LANGUAGES constant in Settings.tsx to import and
use the value-exported LANGUAGES list from `@open-wiki/access` as the source for
each language value, while preserving the existing labels and array shape; do
not duplicate language value literals locally.
In `@apps/desktop/src/renderer/Sources.tsx`:
- Around line 73-91: Update the transcribe callback in SourceItem to track
mounted state with a ref and lifecycle effect, and guard every setNote/setBusy
after the awaited bridge().transcribe(row.id) call so they are skipped after
unmount. Ensure cleanup marks the component unmounted while preserving the
existing transcription result and error behavior.
In `@apps/desktop/tests/settings.spec.ts`:
- Around line 290-299: Update the test setup around createApi and Deps so the
no-project tests use the file’s temporary appData directory when calling
knownProjects(), rather than defaultAppDataDir(). Add an optional appDataDir
dependency and pass it through to ProjectRegistry, or remove the
registry-dependent assertion while preserving the project() null behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa3a7109-622f-43f8-b39f-c92c54427e6f
📒 Files selected for processing (35)
apps/desktop/src/main/index.tsapps/desktop/src/main/ipc.tsapps/desktop/src/main/preload.tsapps/desktop/src/main/settings.tsapps/desktop/src/main/transcribe-run.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/Launcher.tsxapps/desktop/src/renderer/Settings.tsxapps/desktop/src/renderer/Sources.tsxapps/desktop/src/renderer/bridge.tsapps/desktop/tests/settings.spec.tsapps/desktop/tests/sources.spec.tspackages/access/package.jsonpackages/access/src/claude-md.tspackages/access/src/config/secrets.tspackages/access/src/index.tspackages/cli/package.jsonpackages/cli/src/commands/consult.tspackages/cli/src/commands/gate.tspackages/cli/src/commands/init.tspackages/cli/src/commands/write.tspackages/cli/src/install.tspackages/cli/tests/consult.spec.tspackages/cli/tests/e2e.spec.tspackages/cli/tests/gate.spec.tspackages/cli/tests/init.spec.tspackages/cli/tests/install.spec.tspackages/cli/tests/main.spec.tspackages/cli/tsconfig.jsonpackages/cli/vitest.config.tspackages/mcp/package.jsonpackages/mcp/src/index.tspackages/mcp/tests/consult.spec.tspackages/mcp/vitest.config.tsplans/open-wiki.md
Comment on lines
+89
to
+96
| export function parseCredentialInput(value: unknown): SaveCredentialInput | null { | ||
| if (typeof value !== "object" || value === null) return null; | ||
| const input = value as Partial<SaveCredentialInput>; | ||
| if (input.provider !== "groq" && input.provider !== "whispercpp") return null; | ||
| if (input.provider === "whispercpp") return { provider: "whispercpp" }; | ||
| if (typeof input.apiKey !== "string" || input.apiKey.length > MAX_KEY_CHARS) return null; | ||
| return { provider: "groq", apiKey: input.apiKey }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim the key before it is checked and stored.
A user pastes the key. A paste often carries a leading or trailing space or newline. This parser accepts it, checkCredential sends it in the authorization header, Groq answers 401, and the screen reports "Groq did not accept that key". The key is correct, and the message points the user at the wrong cause.
🐛 Proposed fix
- if (typeof input.apiKey !== "string" || input.apiKey.length > MAX_KEY_CHARS) return null;
- return { provider: "groq", apiKey: input.apiKey };
+ if (typeof input.apiKey !== "string" || input.apiKey.length > MAX_KEY_CHARS) return null;
+ // Trimmed. A pasted key carries the whitespace around it, and Groq answers
+ // 401 — which the screen reports as a wrong key, blaming the user for a
+ // space they cannot see.
+ return { provider: "groq", apiKey: input.apiKey.trim() };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/settings.ts` around lines 89 - 96, Update
parseCredentialInput so Groq apiKey values are trimmed before length validation
and storage, while preserving the existing provider validation and whispercpp
handling. Use the normalized key for both the MAX_KEY_CHARS check and the
returned SaveCredentialInput.
Comment on lines
+258
to
+263
| export function createProject( | ||
| name: string, | ||
| directory: string, | ||
| language: Language = "en", | ||
| appDataDir?: string, | ||
| ): KnownProject { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate language in createProject.
setLanguage checks the value against LANGUAGES. createProject does not. apps/desktop/src/main/ipc.ts Line 343 passes String(args[2] ?? "en") as Language, so the renderer decides this value and the cast performs no check. An unknown value reaches writeSettings(directory, { language }) and writeClaudeMd(directory, language), and generateClaudeMd resolves LANGUAGE_LABEL[language] to undefined. The new project then carries an invalid language and a CLAUDE.md that states the content language is undefined.
Reuse the same check that setLanguage applies, next to the name and path checks.
🛡️ Proposed fix to reject an unknown language before the scaffold
+export class UnknownLanguageError extends Error {
+ constructor(language: string) {
+ super(`unknown language "${language}" — one of ${LANGUAGES.join(", ")}`);
+ this.name = "UnknownLanguageError";
+ }
+}
+
export function createProject(
name: string,
directory: string,
language: Language = "en",
appDataDir?: string,
): KnownProject {
@@
if (!isAbsolute(directory)) throw new RelativeProjectPathError(directory);
+ // Before the scaffold, for the same reason the name is. The renderer chooses
+ // this value, and an unchecked one reaches `ow.json` and the generated
+ // `CLAUDE.md`, which then states the content language is `undefined`.
+ if (!LANGUAGES.includes(language)) throw new UnknownLanguageError(language);Also applies to: 274-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/settings.ts` around lines 258 - 263, Validate the
language argument in createProject using the same LANGUAGES check as
setLanguage, alongside the existing name and directory validation. Reject
unknown values before writeSettings, writeClaudeMd, or any project scaffolding
proceeds, while preserving valid-language behavior.
Comment on lines
+71
to
+77
| const secrets = readSecrets(projectRoot, deps.appDataDir ?? defaultAppDataDir()); | ||
| if (!secrets?.stt) { | ||
| return { | ||
| ok: false, | ||
| reason: "no transcription provider is configured yet — set one in Settings", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether readSecrets can throw on invalid input.
fd secrets.ts packages/access/src --exec cat -n {}Repository: protonspy/open-wiki
Length of output: 3765
Catch errors from readSecrets before returning the configuration error.
readSecrets can throw from defaultAppDataDir(), readFileSync(), or JSON.parse(). The call is outside the existing try blocks, so these failures reject runTranscription instead of returning a TranscribeOutcome.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/transcribe-run.ts` around lines 71 - 77, Wrap the
readSecrets call in runTranscription with error handling so failures from
defaultAppDataDir, file reads, or JSON parsing return an appropriate
TranscribeOutcome instead of rejecting. Preserve the existing no-provider
configuration response when secrets are successfully read without stt
credentials.
Comment on lines
90
to
98
| void bridge() | ||
| .project() | ||
| .then(setProject) | ||
| .then((info) => { | ||
| setProject(info); | ||
| setHasProject(info !== null); | ||
| }) | ||
| .catch((e: unknown) => setError(message(e))); | ||
| void refreshIndex(); | ||
| }, [refreshIndex]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether index()-related IPC handlers throw NoProjectError without a project.
rg -n -B3 -A10 'NoProjectError' apps/desktop/src/main/ipc.ts apps/desktop/src/main/index.tsRepository: protonspy/open-wiki
Length of output: 1764
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- App.tsx relevant section ---'
sed -n '1,150p' apps/desktop/src/renderer/App.tsx
printf '%s\n' '--- IPC index-related handlers and API wiring ---'
rg -n -B8 -A18 'index|refreshIndex|project\(' apps/desktop/src/main/ipc.ts apps/desktop/src/main/index.ts apps/desktop/src/renderer apps/desktop/src -g '*.ts' -g '*.tsx'Repository: protonspy/open-wiki
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- App render branch ---'
sed -n '210,265p' apps/desktop/src/renderer/App.tsx
printf '%s\n' '--- preload bridge implementation ---'
sed -n '1,180p' apps/desktop/src/renderer/bridge.ts
printf '%s\n' '--- window/API construction ---'
sed -n '1,125p' apps/desktop/src/main/index.ts
printf '%s\n' '--- focused tests for App, IPC, and launcher behavior ---'
rg -n -i -B5 -A12 'launcher|refreshIndex|NoProjectError|wiki:index|project\(\)' apps/desktop -g '*test*' -g '*spec*' -g '*.tsx' -g '*.ts' | head -n 300Repository: protonspy/open-wiki
Length of output: 33504
Gate refreshIndex() on a confirmed project. bridge().index() calls the project-dependent wiki:index handler, which throws NoProjectError when projectRoot is absent. The initial effect invokes it before bridge().project() resolves, so launcher windows set error unnecessarily.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/App.tsx` around lines 90 - 98, Update the
initialization effect around bridge().project() and refreshIndex() so
refreshIndex() runs only after project() resolves with a non-null project. Do
not invoke refreshIndex() unconditionally on effect startup; preserve the
existing project state updates and error handling, and keep the no-project case
from triggering the index call.
Comment on lines
+77
to
+91
| const result = await bridge().transcribe(row.id); | ||
| setNote( | ||
| result.ok | ||
| ? result.sealed | ||
| ? "Transcribed." | ||
| : `${result.done} of ${result.total} chunks done.` | ||
| : result.reason, | ||
| ); | ||
| onChanged(); | ||
| } catch (e) { | ||
| setNote(e instanceof Error ? e.message : String(e)); | ||
| } finally { | ||
| setBusy(false); | ||
| } | ||
| }, [row.id, onChanged]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Style failure feedback with the error class, not empty.
note can hold a failure reason (Line 83: result.reason) or a caught exception message (Line 87), but Line 126 always renders it with the empty class. Elsewhere in this file, empty is reserved for neutral/informational text (Line 40) and error marks failures (Line 125). A message like "no transcription provider is configured yet — set one in Settings" needs the user's attention and should not use the same muted styling as an empty-state message.
🎨 Proposed fix
const [busy, setBusy] = useState(false);
const [note, setNote] = useState<string | null>(null);
+ const [noteIsError, setNoteIsError] = useState(false);
const transcribe = useCallback(async () => {
setBusy(true);
setNote(null);
+ setNoteIsError(false);
try {
const result = await bridge().transcribe(row.id);
setNote(
result.ok
? result.sealed
? "Transcribed."
: `${result.done} of ${result.total} chunks done.`
: result.reason,
);
+ setNoteIsError(!result.ok);
onChanged();
} catch (e) {
setNote(e instanceof Error ? e.message : String(e));
+ setNoteIsError(true);
} finally {
setBusy(false);
}
}, [row.id, onChanged]);- {note ? <p className="empty">{note}</p> : null}
+ {note ? <p className={noteIsError ? "error" : "empty"}>{note}</p> : null}Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/Sources.tsx` around lines 77 - 91, Update the note
rendering near the existing `setNote`/`setBusy` flow so failure messages from
`result.reason` or caught exceptions use the `error` class instead of `empty`,
while preserving `empty` for neutral informational messages. Reuse the existing
failure-state distinction and keep the current note content unchanged.
Comment on lines
46
to
+48
| export function defaultAppDataDir(): string { | ||
| const base = process.env["APPDATA"] ?? process.env["HOME"] ?? process.cwd(); | ||
| const base = process.env["APPDATA"] ?? process.env["HOME"]; | ||
| if (!base) throw new NoAppDataDirError(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site of defaultAppDataDir and NoAppDataDirError and show the surrounding error handling.
rg -nP -C 6 '\bdefaultAppDataDir\s*\(|\bNoAppDataDirError\b' --type=tsRepository: protonspy/open-wiki
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- matching files ---'
git ls-files | rg '(^|/)(secrets|transcribe-run|credential|known|project).*\.tsx?$|packages/access|apps/desktop/src/main'
printf '%s\n' '--- identifier references (all tracked files) ---'
rg -n -P -C 8 'defaultAppDataDir|NoAppDataDirError|credentialState|knownProjects|forgetProject|readSecrets|writeSecrets|secretsFile' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' . || trueRepository: protonspy/open-wiki
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- settings implementation ---'
sed -n '1,90p;145,180p;200,225p;255,295p' apps/desktop/src/main/settings.ts
printf '%s\n' '--- transcription implementation ---'
sed -n '1,180p' apps/desktop/src/main/transcribe-run.ts
printf '%s\n' '--- IPC routing and error handling ---'
sed -n '240,370p' apps/desktop/src/main/ipc.ts
printf '%s\n' '--- renderer consumers ---'
sed -n '1,90p' apps/desktop/src/renderer/Launcher.tsx
sed -n '1,150p' apps/desktop/src/renderer/Settings.tsxRepository: protonspy/open-wiki
Length of output: 25964
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- IPC registration ---'
rg -n -C 8 'ipcMain\.handle|dispatch\(' apps/desktop/src/main apps/desktop/src
printf '%s\n' '--- direct defaultAppDataDir callers ---'
rg -n -C 3 '\bdefaultAppDataDir\s*\(' apps/desktop/src packages/access/src
printf '%s\n' '--- caller error-boundary probe ---'
python3 - <<'PY'
from pathlib import Path
import re
targets = [
("apps/desktop/src/main/settings.ts", "credentialState"),
("apps/desktop/src/main/settings.ts", "saveCredential"),
("apps/desktop/src/main/settings.ts", "knownProjects"),
("apps/desktop/src/main/settings.ts", "createProject"),
("apps/desktop/src/main/settings.ts", "forgetProject"),
("apps/desktop/src/main/transcribe-run.ts", "runTranscription"),
]
for filename, name in targets:
text = Path(filename).read_text()
match = re.search(rf"export (?:async )?function {name}\b", text)
if not match:
print(f"{filename}:{name}: NOT FOUND")
continue
start = match.start()
brace = text.find("{", match.end())
depth = 0
end = None
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
body = text[start:end]
calls = [line.strip() for line in body.splitlines() if "defaultAppDataDir()" in line]
print(f"{filename}:{name}: calls={calls!r}; try={body.count('try {')}; catch={body.count('catch')}")
PYRepository: protonspy/open-wiki
Length of output: 13800
Handle NoAppDataDirError in all desktop callers.
When neither APPDATA nor HOME is set, credentialState, saveCredential, and forgetProject reject without showing a user-facing message. runTranscription calls defaultAppDataDir() outside its try, so it can reject instead of returning TranscribeOutcome. knownProjects and createProject already display the error through Launcher; preserve that behavior for the remaining paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/access/src/config/secrets.ts` around lines 46 - 48, Handle
NoAppDataDirError in the desktop callers credentialState, saveCredential,
forgetProject, and runTranscription: display the error through the existing
Launcher flow, and move runTranscription’s defaultAppDataDir call inside its try
so it still returns TranscribeOutcome. Preserve the existing knownProjects and
createProject handling.
Comment on lines
+75
to
+82
| // 0700/0600. On Windows the per-user ACL on %APPDATA% already covers this; | ||
| // on a developer machine falling back to $HOME it is the difference between | ||
| // the key being readable by that user and by every local account. | ||
| mkdirSync(join(file, ".."), { recursive: true, mode: 0o700 }); | ||
| writeFileSync(file, JSON.stringify(secrets, null, 2) + "\n", { | ||
| encoding: "utf8", | ||
| mode: 0o600, | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Existing secret files and directories keep their old permissions.
writeFileSync applies mode only when it creates the file. mkdirSync with recursive: true does not change the mode of a directory that already exists. An installation that wrote the secrets file before this change keeps the previous mode (typically 0644), so the key stays world-readable on a machine that falls back to $HOME. Rewriting the credential does not repair it.
Apply the mode explicitly after the write.
🔒 Proposed fix to enforce the permissions on pre-existing paths
+ const dir = join(file, "..");
// 0700/0600. On Windows the per-user ACL on %APPDATA% already covers this;
// on a developer machine falling back to $HOME it is the difference between
// the key being readable by that user and by every local account.
- mkdirSync(join(file, ".."), { recursive: true, mode: 0o700 });
+ mkdirSync(dir, { recursive: true, mode: 0o700 });
writeFileSync(file, JSON.stringify(secrets, null, 2) + "\n", {
encoding: "utf8",
mode: 0o600,
});
+ // `mode` above is honoured only on creation, so a directory or file that
+ // predates this is left at whatever it was — which for the secrets file is
+ // the leak this block exists to close.
+ if (process.platform !== "win32") {
+ chmodSync(dir, 0o700);
+ chmodSync(file, 0o600);
+ }Add chmodSync to the node:fs import.
📝 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.
Suggested change
| // 0700/0600. On Windows the per-user ACL on %APPDATA% already covers this; | |
| // on a developer machine falling back to $HOME it is the difference between | |
| // the key being readable by that user and by every local account. | |
| mkdirSync(join(file, ".."), { recursive: true, mode: 0o700 }); | |
| writeFileSync(file, JSON.stringify(secrets, null, 2) + "\n", { | |
| encoding: "utf8", | |
| mode: 0o600, | |
| }); | |
| const dir = join(file, ".."); | |
| // 0700/0600. On Windows the per-user ACL on %APPDATA% already covers this; | |
| // on a developer machine falling back to $HOME it is the difference between | |
| // the key being readable by that user and by every local account. | |
| mkdirSync(dir, { recursive: true, mode: 0o700 }); | |
| writeFileSync(file, JSON.stringify(secrets, null, 2) + "\n", { | |
| encoding: "utf8", | |
| mode: 0o600, | |
| }); | |
| // `mode` above is honoured only on creation, so a directory or file that | |
| // predates this is left at whatever it was — which for the secrets file is | |
| // the leak this block exists to close. | |
| if (process.platform !== "win32") { | |
| chmodSync(dir, 0o700); | |
| chmodSync(file, 0o600); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/access/src/config/secrets.ts` around lines 75 - 82, Update the
secret-writing flow around mkdirSync and writeFileSync to explicitly enforce
0700 on the parent directory and 0600 on the secret file after they exist,
including pre-existing paths. Add chmodSync to the node:fs imports and apply it
to the corresponding directory and file while preserving the current write
behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes 8.3, 8.4, 8.12 and 6.3 of
plans/open-wiki.md.What changed
6.3 is here because this is where the credential is. 4.15 left the wiring deliberately unbuilt:
config/secrets.tsforbids the CLI, the hooks and the MCP process from reading the Groq key, because their stderr is consumed by an agent and travels to a model provider. The orchestrator that reads it had to be the desktop application, and it needed 8.3 to have stored one. Group 4's pipeline finally has something that starts it.The key is checked on the spot, against the models endpoint rather than by sending audio — a wrong key is discovered at the settings screen, not an hour later with a meeting already recorded. A key that could not be checked is told apart from one that is wrong.
The key never crosses the bridge in either direction, and it is reached through a named
@open-wiki/access/secretssubpath rather than the barrel, so the one place in the product that reads it stays visible in a single grep.The
CLAUDE.mdgenerator moved from the CLI into@open-wiki/accessbesidescaffoldSkills— 9.3 and 9.4 are one act, and the CLI's.export points at a file that does not exist.What the reviews caught
Two that lose something unrecoverable:
deleteWavAfterTranscription: falsewas ignored. The only production caller never passed it, so the seal deleted ~690 MB per hour of "the only copy of a meeting that already happened" for a user who had explicitly said to keep it.timemap.jsonwas cast, not checked — and it is committed by design, so it arrives with a clone. It decides how many paid provider requests one click makes and what instant every citation resolves to.isTimeMapexists and says every consumer must go through it.Plus: whisper.cpp could be selected, was accepted, was stored, and could never transcribe anything; 8.4's launcher was ticked and imported by nothing; changing the language bricked an in-flight transcription with no way to take 4.17's restart offer;
createProjectscaffolded before validating the name, leaving orphans;saveCredentialcast where every other channel coerces; anddefaultAppDataDirfell back toprocess.cwd()— which for the desktop process is the project, the exact leak that module exists to prevent.How it was verified
pnpm test— 1006 passing (199 in@open-wiki/desktop)pnpm run typecheck,pnpm lint,prettier --check— cleanscc validate— no findingscode-reviewandsecurity-reviewsubagents on the diff; every finding closedStill open
9.14 (bundle the CLI, local socket) moves to the distribution PR with group 10 — both are build and packaging concerns and 9.14's socket peer is this application, which now exists.
🤖 Generated with Claude Code
https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
Summary by CodeRabbit