feat(desktop,access): embed the agent — a chat pane that reads the project and writes wiki/ through the gate - #35
Conversation
…oject and writes wiki/ through the gate Implements specs/embedded-agent, enabled by adr:0019. The desktop app now runs an embedded agent in the main process, behind a chat pane: it reads the project the way a harness does and writes only wiki/, only through the validated store, and only after a human approves the specific change. The guardrail is scope, not prompting. `WikiGateBackend` implements deepagents' `BackendProtocolV2`: reads confined to the project with `assertWithin`, writes accepted only under `wiki/` and routed through `gateWrite` + `writePage` with a new origin, `agent`. `execute` is not implemented, so the backend is not a sandbox backend and the shell tool is never offered. The stack is assembled from `createAgent` plus explicit deepagents middleware rather than `createDeepAgent`, because the latter makes subagent middleware required — building it by hand is what makes `task` genuinely absent rather than present-but-dead. Every write tool interrupts before it touches disk. The card the human sees carries the whole effect of the call, including — for `edit_file` — every match site and the resulting page, computed from disk at interrupt time: the HITL interrupt fires on the model's raw arguments, so a short `old_string` with `replace_all` would otherwise render exactly like a single-site edit. A page-guard middleware hashes the target at proposal time and refuses a stale approval, re-proposing instead of clobbering. Also adds gated, undoable `renamePage`/`deletePage` primitives to @open-wiki/access (rename rolls both pages back if any step fails), the Groq model picker fed by the /models list captured at credential-save time, and the three chat IPC channels plus the buffered push channel. Tracing is disabled before any langchain module loads. That is a property of the whole module graph, not one file, so the guard is the first import of every module that reaches langchain and a source sweep in the suite enforces it. Verified: pnpm run typecheck, pnpm test (1274), pnpm lint, the 76% coverage floor on all five packages, scc validate (0 findings). code-review and security-review both run and their findings fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBfVmmLyJamvwmJVRVNZsE
Delivery gives each unit of work its own worktree, and the harness places them under .claude/worktrees/ — inside the repository. Untracked and unignored, a `git add .` in the primary checkout would commit an entire second checkout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBfVmmLyJamvwmJVRVNZsE
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds an embedded Groq-backed agent with gated wiki access, approval interrupts, edit previews, stale-page protection, project-scoped model preferences, desktop IPC, and a renderer chat pane. ChangesEmbedded agent
Estimated code review effort: 5 (Critical) | ~120 minutes 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)
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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
docs/stack.md-21-21 (1)
21-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
real-timein the performance description.Change
~228x real timeto~228x real-time, or rewrite the phrase as “about 228 times real time.” The current wording is difficult to parse.🤖 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 `@docs/stack.md` at line 21, Update the performance description in the documentation entry mentioning Groq whisper-large-v3-turbo to use the grammatical form “~228x real-time” or an equivalent “about 228 times real time” phrasing.Source: Linters/SAST tools
specs/embedded-agent/design.md-71-80 (1)
71-80: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the
task-tool explanation consistent.This section correctly states that
taskis absent becauseSubAgentMiddlewareis not included. The laterAlternatives consideredsection still says that a Groq harness-profile switch removestask. Replace that stale explanation with the explicitcreateAgentand no-subagent-middleware design.🤖 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 `@specs/embedded-agent/design.md` around lines 71 - 80, Update the later “Alternatives considered” discussion to remove the claim that a Groq harness-profile switch removes the task tool. Describe the explicit langchain createAgent assembly using the filesystem, skills, summarization, patch-tool-calls, and human-in-the-loop middleware, while omitting SubAgentMiddleware so task is genuinely absent.docs/glossary.md-23-24 (1)
23-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the non-entity page exception.
apps/desktop/src/main/agent/wiki-gate-backend.ts:24-58allowswiki/index.md,wiki/changelog.md, andwiki/log.mdwithout content validation. The glossary currently states that every agent write passes the group 5 validations. Limit that statement to entity pages, or name the approval-gated exception.Proposed wording
-- **write gate** — whatever makes an agent's write to `wiki/` pass through the group 5 validations now that MCP no longer writes — `adr:0013-the-project-directory-is-the-unit`. For the embedded agent the chosen mechanism is the **wiki-gate backend**; the term remains for the constraint itself, independent of the mechanism that enforces it. +- **write gate** — whatever makes an agent's write to `wiki/` pass through the gate; entity pages use the group 5 validations, while `index.md`, `changelog.md`, and `log.md` are approval-gated but exempt from content validation — `adr:0013-the-project-directory-is-the-unit`. For the embedded agent the chosen mechanism is the **wiki-gate backend**; the term remains for the constraint itself, independent of the mechanism that enforces it.🤖 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 `@docs/glossary.md` around lines 23 - 24, Update the “write gate” and “wiki-gate backend” glossary entries to clarify that group 5 content validation applies to entity pages only, while the approved non-entity pages wiki/index.md, wiki/changelog.md, and wiki/log.md are exempt. Preserve the existing description of WikiGateBackend routing and terminology.specs/embedded-agent/tasks.md-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
pageGuardMiddlewarein task 2.1.The completed middleware list omits
pageGuardMiddleware({ projectRoot }). The implementation appends it after human-in-the-loop middleware to capture the page hash and revalidate immediately before a write. Record this middleware so the task list documents the R5.5 protection.Proposed task-list update
-- `middleware = [createFilesystemMiddleware({ backend: wikiGateBackend, toolTokenLimitBeforeEvict: null }), createSkillsMiddleware({ backend, sources: [".claude/skills/"] }), createSummarizationMiddleware({ backend }), createPatchToolCallsMiddleware(), humanInTheLoopMiddleware({ interruptOn })]` +- `middleware = [createFilesystemMiddleware({ backend: wikiGateBackend, toolTokenLimitBeforeEvict: null }), createSkillsMiddleware({ backend, sources: [".claude/skills/"] }), createSummarizationMiddleware({ backend }), createPatchToolCallsMiddleware(), humanInTheLoopMiddleware({ interruptOn }), pageGuardMiddleware({ projectRoot })]`🤖 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 `@specs/embedded-agent/tasks.md` at line 18, Update task 2.1’s documented middleware stack to include pageGuardMiddleware({ projectRoot }) after humanInTheLoopMiddleware({ interruptOn }), and add R5.5 to the listed requirements. Keep the existing middleware ordering and configuration unchanged.apps/desktop/src/renderer/Settings.tsx-152-162 (1)
152-162: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGive the model
selectan accessible name.The
selecthas no label and noaria-label. The<h2>is not programmatically associated with it. A screen reader announces an unlabeled combobox.♿ Proposed fix
<select className="editor__source" + aria-label="Chat agent model" value={agent.selectedModel} onChange={(e) => void pickModel(e.target.value)} >🤖 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 152 - 162, Add an accessible name to the model select in the agent settings UI by associating it with a visible label or supplying an appropriate aria-label. Update the select rendered from agent.models while preserving its existing value and pickModel onChange behavior.apps/desktop/tests/chat.spec.ts-342-380 (1)
342-380: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test does not reach the abort branch it names.
The fake
streamEventsracesreleaseagainst theabortevent and then returns normally. It never throws. SorunStreamtakes the success path, callsgetState, and pushesdonefrom Line 175 ofchat-control.ts. Thecontroller.signal.abortedbranch at Lines 180-181 is never executed. The assertions pass either way, so a regression that turned an abort into anerrorevent would not be caught.A real aborted stream rejects. Make the fake reject after the abort.
💚 Proposed change
const aborted = new Promise<void>((r2) => signal.addEventListener("abort", () => r2()), ); await Promise.race([release, aborted]); + if (signal.aborted) throw new Error("The operation was aborted");🤖 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/chat.spec.ts` around lines 342 - 380, Update the fake streamEvents implementation in the cancellation test so its abort path rejects after the abort event instead of returning normally. Keep the release path resolving normally, ensuring runStream exercises its controller.signal.aborted handling while preserving the existing done-without-error assertions.apps/desktop/src/renderer/Chat.tsx-53-59 (1)
53-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the subscription with
hasBridge().
bridge()throwsNoBridgeErrorwhenwindow.owis absent. This effect calls it unguarded. If the preload failed to load, the throw escapes the effect and React unmounts the tree, so the window goes blank.
App.tsxalready handles that case: it reports the failure throughsay(failure("shell", new NoBridgeError()))and guards each of its own subscriptions withif (!hasBridge()) return;. This pane should follow the same rule so the shell notice stays on screen.🛡️ Proposed fix
-import { bridge } from "./bridge.js"; +import { bridge, hasBridge } from "./bridge.js"; @@ - useEffect( - () => - bridge().onChatEvent((event) => - dispatchRef.current({ type: "event", event: event as ChatEvent }), - ), - [], - ); + useEffect(() => { + if (!hasBridge()) return; + return bridge().onChatEvent((event) => + dispatchRef.current({ type: "event", event: event as ChatEvent }), + ); + }, []);The
checkKeycallback at Line 64 already tolerates the throw through itscatch, sohasKeybecomesfalseand the empty state renders.🤖 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/Chat.tsx` around lines 53 - 59, Guard the chat-event subscription in the useEffect with hasBridge() before calling bridge().onChatEvent, returning early when the bridge is unavailable. Keep the existing dispatchRef.current event handling unchanged and preserve the checkKey callback’s existing error tolerance.packages/access/src/write/rename-delete.ts-86-90 (1)
86-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the comment about the config-write check.
The comment states that a config write "is checked before classification".
classifyperforms noisConfigWritecall. A config target is refused at line 104 instead, which the comment at lines 102-103 already describes. The behavior is correct; only the first comment is misleading.📝 Proposed comment fix
- // A config write (`.claude/`, `.mcp.json`, `CLAUDE.md`) reached as a rename or - // delete target — refused outright by 9.6, and `gatedPageRel` would not see - // it, so it is checked before classification. + // A config write (`.claude/`, `.mcp.json`, `CLAUDE.md`) reached as a rename or + // delete target is never a gated wiki page, so `gatedPageRel` returns null for + // it and the fallback refusal below names it. const rel = gatedPageRel(realRoot, landsAt);🤖 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/write/rename-delete.ts` around lines 86 - 90, Update the comment immediately above the gatedPageRel call to remove the claim that config writes are checked before classification. Keep the implementation unchanged and describe only that config-write targets are refused by the later check referenced by the existing comment near the refusal logic.apps/desktop/tests/agent.spec.ts-224-235 (1)
224-235: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test does not prove the system prompt is used.
The assertion is only that construction does not throw. It passes whether or not
createEmbeddedAgentpassesharness.contenttocreateAgentassystemPrompt.resolveHarnessEntryis covered separately (lines 142-148), so the untested link is the wiring between the two.A scripted model records the messages it receives, so
ScriptedChatModelcan capture them and the test can assert that theCLAUDE.mdtext reaches the model.🤖 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/agent.spec.ts` around lines 224 - 235, Update the “uses CLAUDE.md as the system prompt when present” test around createEmbeddedAgent to use ScriptedChatModel, invoke the created agent, and inspect the recorded messages. Assert that the CLAUDE.md content reaches the model as the system prompt, verifying the wiring from resolveHarnessEntry through createAgent rather than only asserting construction succeeds.apps/desktop/tests/wiki-gate-backend.spec.ts-243-249 (1)
243-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a case for an empty
old_string.The suite covers an absent
old_stringbut not an empty one.WikiGateBackend.editcurrently accepts""and rewrites the whole page (see the comment onapps/desktop/src/main/agent/wiki-gate-backend.ts:254-267). A test asserting thatbackend.edit(page, "", "y")returns an error and leaves the page unchanged would have caught it, and it locks in the fix.🤖 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/wiki-gate-backend.spec.ts` around lines 243 - 249, Add a test alongside the existing absent-old_string case that calls WikiGateBackend.edit with an empty old_string, asserts an error is returned, and verifies the wiki page content remains unchanged.apps/desktop/tests/helpers/fake-model.ts-39-47 (1)
39-47: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGenerated tool-call ids collide when one response carries two tool calls.
id: tc.id ?? \call_${idx}`uses the response index, not the tool-call index. A single scripted response with two tool calls gives both the idcall_0. LangChain pairs eachToolMessageto its tool call bytool_call_id`, so the duplicate breaks the loop.Every current test scripts one tool call per response, so the fault is latent. Include the inner index so the helper stays correct for a parallel-tool-call test.
🐛 Proposed fix
- const toolCalls = r.toolCalls?.map((tc) => ({ + const toolCalls = r.toolCalls?.map((tc, j) => ({ name: tc.name, args: tc.args, - id: tc.id ?? `call_${idx}`, + id: tc.id ?? `call_${idx}_${j}`, type: "tool_call" as const, }));🤖 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/helpers/fake-model.ts` around lines 39 - 47, Update the toolCalls mapping in the fake-model response builder to include the tool-call index when generating fallback IDs, while preserving explicitly provided tc.id values. Ensure multiple tool calls within one response receive distinct IDs that remain consistent with ToolMessage tool_call_id matching.apps/desktop/tests/agent-loop.spec.ts-176-189 (1)
176-189: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDerive the preview path from the interrupt args, not a hardcoded literal.
The scripted tool call passes an absolute path (line 149), but line 183 calls
previewEditwith the literal"wiki/alpha.md". The test therefore proves the preview works for a relative path while the interrupt payload carries an absolute one. Production reads the path out of the sameaction.argsthat this test ignores, so the absolute form is never exercised here.Take the path from
action!.argsso the test covers what the interrupt actually carries.💚 Proposed change
- const preview = previewEdit(root, "wiki/alpha.md", action!.args); + const target = (action!.args["file_path"] ?? action!.args["path"]) as string; + const preview = previewEdit(root, target, action!.args);🤖 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/agent-loop.spec.ts` around lines 176 - 189, Update the previewEdit invocation in the interrupt test to derive the file path from action!.args, matching the absolute path carried by the scripted tool call instead of using the hardcoded relative path "wiki/alpha.md". Preserve the existing preview assertions.apps/desktop/src/main/agent/wiki-gate-backend.ts-293-296 (1)
293-296: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign date computation across write paths.
today()uses UTC, whilepackages/cli/src/date.tsuses the local calendar date. Writes near local midnight can stamp differentupdateddates. Share one date helper across all write 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 `@apps/desktop/src/main/agent/wiki-gate-backend.ts` around lines 293 - 296, Update the today() helper to use the shared date utility from packages/cli/src/date.ts instead of computing the date with toISOString(), and ensure all write paths reuse that single helper so local calendar dates remain consistent near midnight.
🧹 Nitpick comments (21)
apps/desktop/tests/agent-prefs.spec.ts (1)
112-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name promises a file-mode assertion the body does not make.
The title states "is written without restrictive file mode". The body checks existence and content only. Assert the mode, so a later change that copies the secrets file's
0600is caught.💚 Proposed change
+import { statSync } from "node:fs";writeAgentPrefs(root, { models: ["a"], selectedModel: "a" }, appData); expect(existsSync(agentPrefsFile(root, appData))).toBe(true); + // Not 0600: the model list is public. + expect(statSync(agentPrefsFile(root, appData)).mode & 0o077).not.toBe(0); // The secrets file would be a sibling; neither contains the other. const body = readFileSync(agentPrefsFile(root, appData), "utf8"); expect(body).toContain('"models"');Note: the exact mode depends on the process umask, so assert only that the group and other bits are not fully cleared.
🤖 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/agent-prefs.spec.ts` around lines 112 - 119, Update the test case “is written without restrictive file mode” to read the created agent preferences file’s mode and assert that group and other permission bits are not both cleared, while preserving the existing existence and content checks.apps/desktop/tests/settings.spec.ts (2)
244-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test passes for a different reason than it states.
checkCredentialrefuseswhispercppunconditionally, sosaveCredentialreturns beforewriteSecretsand before theinput.provider === "groq"branch. The assertion holds because nothing at all was written, not because the prefs branch was skipped. Add an assertion that the secrets file is also absent, so the test records the actual outcome.💚 Proposed addition
await saveCredential(root, { provider: "whispercpp" }, { appDataDir: appData }); expect(readAgentPrefs(root, appData)).toBeUndefined(); + // whisper.cpp is refused before anything is written at all. + expect(readSecrets(root, appData)).toBeUndefined();🤖 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 244 - 249, Update the test case around saveCredential to assert that the secrets file is also absent for the whispercpp provider, using the existing secrets-file path or helper. Keep the agent-prefs absence assertion, so the test records that checkCredential prevents both files from being written.
88-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a 200 response with an unparseable body.
checkCredentialwrapsresponse.json()in atry/catchand documents that a body which does not parse still validates the key. No test covers that branch.💚 Proposed test
+ it("still validates the key when the /models body does not parse (5.4)", async () => { + const doFetch: FetchLike = async () => new Response("not json", { status: 200 }); + await expect( + checkCredential({ provider: "groq", apiKey: "gsk_good" }, { fetch: doFetch }), + ).resolves.toEqual({ ok: true, models: [] }); + });🤖 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 88 - 97, Add a test alongside the existing checkCredential cases in settings.spec.ts for a successful 200 response whose body cannot be parsed as JSON. Use the existing fetch-mocking helpers, call checkCredential with valid Groq credentials, and assert that validation succeeds without requiring a models list, covering the response.json() failure branch.apps/desktop/src/main/settings.ts (1)
199-208: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve an existing model selection when the catalogue is refreshed.
saveCredentialalways writesresolveModel({ models: check.models, selectedModel: "" }). If a user re-checks the same key after picking a model, the pick reverts to the default. Pass the stored selection soresolveModelkeeps it when Groq still offers it.♻️ Proposed refactor
if (input.provider === "groq") { + const stored = readAgentPrefs(projectRoot, deps.appDataDir ?? defaultAppDataDir()); writeAgentPrefs( projectRoot, { models: check.models, - selectedModel: resolveModel({ models: check.models, selectedModel: "" }), + selectedModel: resolveModel({ + models: check.models, + selectedModel: stored?.selectedModel ?? "", + }), }, deps.appDataDir ?? defaultAppDataDir(), ); }🤖 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 199 - 208, Update the Groq branch in saveCredential to read the existing agent preferences and pass their stored selectedModel to resolveModel instead of an empty string, preserving that selection when it remains in check.models while retaining the existing default resolution otherwise.apps/desktop/src/renderer/bridge.ts (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the shared chat and prefs types out of
src/main.These are
import typeonly, so nothing fromnode:fsreaches the renderer bundle today. The contract is still declared inside main-process modules.apps/desktop/src/main/agent/agent-prefs.tsimportsnode:fs,node:path, and@open-wiki/access/secretsat module scope. If any later edit turns one of these into a value import, the renderer build pulls Node modules in. Asrc/shared/module forAgentPrefsand theChat*types removes that risk.apps/desktop/src/renderer/Settings.tsx(Line 4) imports across the same boundary.🤖 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/bridge.ts` around lines 14 - 21, Move the shared AgentPrefs and Chat* type declarations out of the main-process modules into an appropriate src/shared module, then update bridge.ts and Settings.tsx to import those types from the shared location. Preserve the existing type names and contracts, and keep node:fs, node:path, and `@open-wiki/access/secrets` dependencies isolated to main-process implementation modules.apps/desktop/src/main/agent/chat-control.ts (2)
125-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an unambiguous separator in the cache key.
`${creds.modelName} ${creds.apiKey}`joins two free-form strings with a space. Two different pairs can produce the same key. The consequence is the one the comment above says must not happen: the control keeps the agent that was built with the previous key.Groq model ids and API keys do not contain spaces today, so this is defensive rather than an active defect.
♻️ Proposed change
- const key = `${creds.modelName} ${creds.apiKey}`; + const key = JSON.stringify([creds.modelName, creds.apiKey]);🤖 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/agent/chat-control.ts` around lines 125 - 129, Update the cache key construction in the agent control flow to use a separator that cannot occur in either free-form credential value, ensuring distinct modelName/apiKey pairs never collide. Keep the existing embedded and builtFor comparison and rebuild behavior unchanged.
195-205: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider refusing a second run on a thread that already has one live.
sendandresumeboth callstartRunwithout checking whether a run is already active forthreadId. Two concurrentstreamEventscalls against the samethread_idwrite to the same checkpointer. The result is interleaved state for that conversation.The renderer prevents this today: it disables the composer while
state.runningis true, and it clears the interrupt on resume. The renderer is not a boundary. A duplicatechat:sendfrom a stuck retry or a reloaded pane reaches the control directly.Track the live run per
threadIdand refuse or abort the previous one.♻️ Sketch
+ const liveByThread = new Map<string, string>(); + const startRun = (input: unknown, threadId: string): ChatRunStarted => { const runId = randomUUID(); + const previous = liveByThread.get(threadId); + if (previous) runs.get(previous)?.abort(); const agent = ensure();Remember to clear
liveByThreadin thefinallyblock ofrunStream.🤖 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/agent/chat-control.ts` around lines 195 - 205, Update the chat control around send, resume, and startRun to track active runs by threadId, rejecting or aborting a new run when that thread already has a live run. Ensure runStream always removes the thread’s entry in a finally block, while preserving independent concurrent runs for different threads.apps/desktop/src/renderer/Chat.tsx (1)
428-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the import instead of re-exporting the type.
ChatStateis imported at Line 9 and never used in this module. The export at Line 429 exists only to silence the lint rule. Deleting the import removes both the warning and the misleading public re-export.♻️ Proposed change
proposalOf, - type ChatState, type EditableField,- -// Silence the unused-import lint for the state type re-exported through props. -export type { ChatState };If another module imports
ChatStatefromChat.tsx, point it atchat-model.tsinstead.🤖 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/Chat.tsx` around lines 428 - 429, Remove the unused ChatState import and delete the export type { ChatState } statement in Chat.tsx. If any module imports ChatState from Chat.tsx, update it to import the type from chat-model.ts instead.apps/desktop/src/main/index.ts (1)
120-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider stopping in-flight agent runs when the window closes.
createChatControltracks each run in anAbortController, butChatControlexposes no dispose method. Theclosedhandler at Line 199 closes the watcher, the inbox, the query server, and the recorder session. It does not stop a running agent stream. After the window closes, the stream keeps consuming the Groq quota and keeps the process work alive until it finishes on its own. The bufferedsenddrops the events, so the work is wasted.Add a
disposeAll()(orcancelAll()) toChatControlinapps/desktop/src/main/agent/chat-control.tsthat aborts every entry inruns, then call it from theclosedhandler.🤖 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/index.ts` around lines 120 - 134, Add a public disposeAll() or cancelAll() method to ChatControl that aborts every active controller in its runs collection, then invoke that method from the window closed handler in index.ts alongside the existing watcher, inbox, query-server, and recorder cleanup.apps/desktop/tests/chat-model.spec.ts (1)
132-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
editableOf.
editableOfis exported fromchat-model.tsand drives the inline edit control inChat.tsx. It decides which args key the human's edited text replaces:contentforwrite_file,new_stringforedit_file,toforrename_page, and nothing fordelete_page. That value is what the resumededitedActioncarries into a gated write. No test covers it.Add cases for each tool, and assert that
argsWithpreserves the other args. Adelete_pagecase should assertnull, which is what makes the pane offer only approve and reject.🤖 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/chat-model.spec.ts` around lines 132 - 170, Add tests for the exported editableOf flow covering write_file, edit_file, rename_page, and delete_page, asserting the expected editable argument key or null for delete_page. For each editable tool, also verify argsWith replaces only the selected text argument while preserving all other arguments.apps/desktop/src/renderer/chat-model.ts (1)
92-115: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a
defaultbranch that returns the state unchanged.
applyEventrelies on the union being exhaustive.Chat.tsxcasts the IPC payload withevent as ChatEventat Line 56, so the type is asserted rather than checked. An event with an unrecognizedkindfalls through the switch, the function returnsundefined, and the next read ofstate.messagesthrows.The producer is the main process, so a skew is unlikely. The guard costs one line.
🛡️ Proposed change
case "error": return { ...state, running: false, error: event.message }; + default: + return state; }If the project relies on TypeScript exhaustiveness checks to catch a forgotten
kind, useassertNeverin the default branch instead so the compile-time check survives.🤖 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/chat-model.ts` around lines 92 - 115, Add a default branch to applyEvent that returns the current state unchanged for unrecognized event kinds, preventing the function from returning undefined when IPC payloads do not match ChatEvent. If the project has an established assertNever pattern for exhaustive unions, use it while preserving compile-time exhaustiveness checking.apps/desktop/tests/chat.spec.ts (1)
137-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
control.resumein the run-loop suite.
fakeAgentdiscards theinputargument, and no test increateChatControl run loopcallscontrol.resume. The only resume coverage is the dispatch routing test at Line 62, which uses a stub control. So nothing asserts thatresumereachesresumeCommand(decisions)and that the decisions survive the trip.Capture the input in the fake and add a resume assertion.
💚 Sketch
-function fakeAgent(frames: unknown[], state: unknown): BuildAgent { +function fakeAgent(frames: unknown[], state: unknown, seen?: unknown[]): BuildAgent { return () => ({ agent: { - async *streamEvents() { + async *streamEvents(input: unknown) { + seen?.push(input); for (const f of frames) yield f; },🤖 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/chat.spec.ts` around lines 137 - 149, Update fakeAgent to retain the input passed to the generated agent, then extend the createChatControl run-loop tests with a control.resume case that invokes resumeCommand(decisions) and asserts the same decisions reach the fake agent. Preserve existing frame and state behavior.apps/desktop/tests/agent.spec.ts (1)
114-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe detector can merge a side-effect import with the next statement.
The pattern requires
from, but a side-effect import has none.import "./tracing.js";therefore has nofromof its own, so the lazy[\s\S]*?runs past it and pairs it with the next statement's specifier. Every file under review starts with exactly that import, so the first statement is always merged with the second.The direction is safe: it can classify a type-only import as a runtime one and make the sweep stricter, never the reverse. Consider anchoring the specifier to the same statement by excluding
;and newlines betweenimportandfrom.♻️ Tighter statement boundary
- const statements = /^import\s+(?:type\s+)?[\s\S]*?from\s+["']([^"']+)["']/gm; + const statements = /^import\s+(?:type\s+)?[^;]*?from\s+["']([^"']+)["']/gm;🤖 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/agent.spec.ts` around lines 114 - 124, Update the statements pattern in reachesLangchain so matching cannot cross a semicolon or newline between import and from, preventing a side-effect import from being merged with the next statement. Preserve the existing import type filtering and LangChain/deepagents specifier checks.apps/desktop/src/main/agent/edit-preview.ts (3)
157-164: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBound the read by file size before loading the page.
readFileSyncloads the whole page into the Electron main process at interrupt time.MAX_RESULTINGcaps only the payload that crosses the bridge, not the read. A very large file underwiki/blocks the main process and allocates its full size before the cap applies. Add astatSyncsize check and returnnullabove a limit.♻️ Proposed size guard
- if (!existsSync(abs)) return null; + if (!existsSync(abs)) return null; + try { + if (statSync(abs).size > MAX_READ) return null; + } catch { + return null; + } let content: string;🤖 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/agent/edit-preview.ts` around lines 157 - 164, In the file-read flow surrounding the existsSync/readFileSync calls, add a statSync size check before loading the file and return null when its size exceeds the established maximum limit. Keep the existing missing-file and read-error handling, and ensure readFileSync is only called after the size guard passes.
113-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one
insideWikiimplementation with the backend.This duplicates
WikiGateBackend.insideWikiand itsrelhelper (apps/desktop/src/main/agent/wiki-gate-backend.ts:73-82), including thetoLowerCase()comparison. The two must stay identical: this check decides whether the preview reads a file and ships its content over the IPC bridge, and the backend check decides whether the write can land. If one changes, the preview either discloses a file for a change that can never happen, or refuses to preview a legal edit.Export a single helper (for example
insideWiki(projectRoot, abs)) and call it from both modules.🤖 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/agent/edit-preview.ts` around lines 113 - 121, Remove the duplicate insideWiki implementation from edit-preview.ts and export a single shared insideWiki helper from wiki-gate-backend.ts, including its existing path-resolution and case-normalization behavior. Update both the preview logic and WikiGateBackend to import and call this shared helper so read and write checks remain identical.
78-89: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCompute line numbers in one pass instead of per site.
countLinesscanscontentfrom index 0 for each site. With the site cap of 200 and a large page, this repeats up to 200 full-prefix scans on the main process while the interrupt is being pushed. Becauseappliedis already sorted ascending, you can carry a cursor and count only the characters between consecutive matches.♻️ Single-pass line counting
+ let scanned = 0; + let lineAt = 1; const sites: ChatEditSite[] = applied.slice(0, MAX_SITES).map((at) => { - const line = countLines(content, at); + for (; scanned < at; scanned++) if (content[scanned] === "\n") lineAt++; + const line = lineAt;🤖 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/agent/edit-preview.ts` around lines 78 - 89, Update the site mapping around countLines so line numbers are computed in a single pass: maintain a cursor and running line count while iterating through the already ascending applied matches, counting only content between consecutive match offsets. Preserve the existing MAX_SITES limit and text extraction behavior, and remove the per-site full-prefix countLines calls.apps/desktop/src/main/agent/page-guard.ts (1)
52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated catch branches, or make them differ.
Both branches of the
catchreturn{ unconfined: true }, so theinstanceof OutsideProjectErrortest has no effect. As written, an unexpected error (for example a permission failure during real-path resolution) is classified as "outside the project" and the guard steps aside. If that is intended, drop the test and add a comment. If a non-confinement error should instead fail closed, the two branches must return different values.♻️ If stepping aside is intended
- } catch (e) { - if (e instanceof OutsideProjectError) return { unconfined: true }; - return { unconfined: true }; - } + } catch { + // Outside the project, or unreadable at resolution time. Either way the + // guard has no view of the page; the backend refuses on its own terms. + return { unconfined: true }; + }🤖 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/agent/page-guard.ts` around lines 52 - 62, Update the catch block in hashAt so the OutsideProjectError path is distinguished from unexpected resolution errors: retain the unconfined result only for confirmed confinement failures, and make other errors fail closed using the function’s existing return contract. Remove the redundant branch behavior while preserving normal hashing and missing-file handling.apps/desktop/tests/wiki-gate-backend.spec.ts (1)
186-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the record pages and for a write through a symlink.
Two documented behaviours have no test here:
WikiGateBackend's doc states thatwiki/index.md,wiki/changelog.mdandwiki/log.mdpass through with no content validation (apps/desktop/src/main/agent/wiki-gate-backend.ts:44-49). That is the one write path where the gate does not check the form, and the design names it as a risk. A test should assert that a write towiki/index.mdsucceeds with arbitrary content and is still logged with originagent.The junction test at line 72 covers reads only. The write path calls
confineand theninsideWikion the resolved path, so a symlink atwiki/link.mdpointing outside the project should be refused. Assert it.🤖 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/wiki-gate-backend.spec.ts` around lines 186 - 202, Add tests alongside the existing WikiGateBackend write tests for both documented cases: verify arbitrary content written to wiki/index.md succeeds and records one agent-origin operation, and create an outside-target symlink at wiki/link.md then verify backend.write rejects it without creating the target or recording an operation. Use the existing backend, root, filesystem, and listOperations setup rather than changing implementation code.apps/desktop/src/main/agent/agent.ts (2)
244-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
streamAgentexport. No tracked caller references it.🤖 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/agent/agent.ts` around lines 244 - 258, Remove the unused exported streamAgent function and its export from the module, including any now-unneeded related references. Do not alter other agent streaming behavior.
127-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn structured errors from both write tools.
Catch exceptions from
renamePageanddeletePage, including filesystem and path-resolution errors, and return{ ok: false, reason: String(error) }.RenameResult.reasonsandDeleteResult.reasonare correct.🤖 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/agent/agent.ts` around lines 127 - 163, Wrap the `renamePage` call in `renamePageTool` and the `deletePage` call in `deletePageTool` with exception handling. Preserve the existing success and typed-result error handling, but catch filesystem or path-resolution exceptions and return the structured failure shape `{ ok: false, reason: String(error) }` from each tool.apps/desktop/src/main/agent/wiki-gate-backend.ts (1)
106-106: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftExpose stable virtual absolute paths in tool results.
deepagentsexpects absolute paths for filesystem tools. Do not replacefullwiththis.rel(full). If host paths are sent to Groq, map the project root to a stable virtual prefix and translate it beforeconfine; apply this tols,glob,grep, and read/edit inputs.🤖 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/agent/wiki-gate-backend.ts` at line 106, Keep filesystem tool results such as the entry built by the directory-listing flow on stable virtual absolute paths, preserving full rather than converting it with rel. In the wiki-gate backend’s ls, glob, grep, and read/edit input handling, map the project root to a stable virtual prefix when exposing paths externally, then translate virtual paths back to host paths before confine validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68a25eae-7f5d-42a5-b31c-7f6002506a61
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.gitignoreapps/desktop/package.jsonapps/desktop/src/main/agent/agent-prefs.tsapps/desktop/src/main/agent/agent.tsapps/desktop/src/main/agent/chat-control.tsapps/desktop/src/main/agent/chat-events.tsapps/desktop/src/main/agent/edit-preview.tsapps/desktop/src/main/agent/page-guard.tsapps/desktop/src/main/agent/tracing.tsapps/desktop/src/main/agent/wiki-gate-backend.tsapps/desktop/src/main/channels.tsapps/desktop/src/main/index.tsapps/desktop/src/main/ipc.tsapps/desktop/src/main/preload.tsapps/desktop/src/main/settings.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/Chat.tsxapps/desktop/src/renderer/Rail.tsxapps/desktop/src/renderer/Settings.tsxapps/desktop/src/renderer/bridge.tsapps/desktop/src/renderer/chat-model.tsapps/desktop/src/renderer/globals.cssapps/desktop/src/renderer/navigation.tsapps/desktop/tests/agent-loop.spec.tsapps/desktop/tests/agent-prefs.spec.tsapps/desktop/tests/agent.spec.tsapps/desktop/tests/chat-model.spec.tsapps/desktop/tests/chat.spec.tsapps/desktop/tests/edit-preview.spec.tsapps/desktop/tests/helpers/fake-model.tsapps/desktop/tests/settings.spec.tsapps/desktop/tests/sources.spec.tsapps/desktop/tests/wiki-gate-backend.spec.tsdocs/glossary.mddocs/stack.mdpackages/access/src/gate/gate.tspackages/access/src/index.tspackages/access/src/write/log.tspackages/access/src/write/rename-delete.tspackages/access/tests/rename-delete.spec.tspackages/access/tests/write.spec.tsspecs/embedded-agent/design.mdspecs/embedded-agent/requirements.mdspecs/embedded-agent/tasks.md
| /** The variables that activate LangSmith/LangChain tracing. */ | ||
| const TRACING_VARS = [ | ||
| "LANGCHAIN_API_KEY", | ||
| "LANGCHAIN_ENDPOINT", | ||
| "LANGCHAIN_PROJECT", | ||
| "LANGCHAIN_CALLBACKS_BACKGROUND", | ||
| "LANGSMITH_API_KEY", | ||
| "LANGSMITH_PROJECT", | ||
| "LANGSMITH_ENDPOINT", | ||
| ] as const; | ||
|
|
||
| /** | ||
| * Set `LANGCHAIN_TRACING_V2=false` and clear every `LANGSMITH_*` / LangChain | ||
| * credential var, so no tracing client initializes. Idempotent. | ||
| */ | ||
| export function disableTracing(): void { | ||
| process.env.LANGCHAIN_TRACING_V2 = "false"; | ||
| for (const name of TRACING_VARS) delete process.env[name]; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
LangSmith JS environment variables LANGSMITH_TRACING LANGSMITH_OTEL_ENABLED disable tracing
💡 Result:
To disable or manage LangSmith tracing in a JavaScript/TypeScript environment, you should primarily rely on the LANGSMITH_TRACING environment variable and SDK-specific programmatic overrides [1][2][3]. Environment Variables The primary global control is the LANGSMITH_TRACING environment variable [1][3]. - Setting LANGSMITH_TRACING="true" enables automatic tracing [4][3]. - Setting LANGSMITH_TRACING="false" (or simply leaving it unset) disables global automatic tracing [4][2][3]. Programmatic Overrides If you need to disable or enable tracing for specific function calls while keeping global settings active, you can use programmatic overrides [2][5][3]. 1. Using traceable: When using the traceable decorator or wrapper, you can pass the tracingEnabled option to explicitly override the global environment setting [2][3]. import { traceable } from "langsmith/traceable"; // Explicitly disable tracing for this specific call const myFunc = traceable( (data: string) => { return data.toUpperCase; }, { name: "process_data", tracingEnabled: false }); await myFunc("sensitive"); // Not traced, even if LANGSMITH_TRACING is "true" 2. RunTree and OTel: - The LANGSMITH_OTEL_ENABLED variable is generally used for OpenTelemetry integration. If you are specifically trying to disable standard LangSmith tracing, LANGSMITH_TRACING is the correct variable to target [2][5]. - Note that RunTree objects created manually with the LangSmith SDK are generally not affected by the global LANGSMITH_TRACING environment variable; they will send data to LangSmith if they are explicitly executed or posted [3]. In summary, for standard application-wide disabling, ensure LANGSMITH_TRACING is set to "false" or omitted [4][3]. For granular control, use the tracingEnabled option within your code [2][3].
Citations:
- 1: https://reference.langchain.com/javascript/langsmith
- 2: https://docs.langchain.com/langsmith/conditional-tracing
- 3: https://docs.langchain.com/langsmith/annotate-code
- 4: https://docs.langchain.com/oss/javascript/langchain/observability
- 5: https://docs.langchain.com/langsmith/trace-without-env-vars
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'tracing|package.json|lock|npmrc|yarnrc|pnpm' . | head -80
printf '%s\n' '--- tracing references ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
'disableTracing|LANGCHAIN_TRACING_V2|LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGSMITH_RUNS_ENDPOINT|OTEL_EXPORTER_OTLP_ENDPOINT|langsmith' .
printf '%s\n' '--- package versions ---'
rg -n --glob 'package.json' --glob '*lock*' --glob 'pnpm-lock.yaml' --glob 'yarn.lock' \
'"(langsmith|`@langchain`[^"]*)"|langsmith@|`@langchain`' .Repository: protonspy/open-wiki
Length of output: 7589
🏁 Script executed:
#!/bin/bash
set -eu
file=$(fd -i 'tracing' . | rg 'apps/desktop/src/main/agent/tracing\.ts$' | head -1)
printf '--- %s ---\n' "$file"
cat -n "$file"
printf '%s\n' '--- nearby package manifests ---'
find . -name package.json -not -path '*/node_modules/*' -print | sort | head -80Repository: protonspy/open-wiki
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tracing.ts ---'
cat -n apps/desktop/src/main/agent/tracing.ts
printf '%s\n' '--- agent tracing test ---'
cat -n apps/desktop/tests/agent.spec.ts | sed -n '1,80p'
printf '%s\n' '--- desktop package ---'
cat -n apps/desktop/package.json | sed -n '1,70p'
printf '%s\n' '--- resolved langsmith lock entries ---'
sed -n '2438,2460p;5964,5985p' pnpm-lock.yaml
printf '%s\n' '--- requirements/spec references ---'
sed -n '15,32p' specs/embedded-agent/tasks.md
sed -n '84,98p' specs/embedded-agent/design.mdRepository: protonspy/open-wiki
Length of output: 16333
🌐 Web query:
langsmith 0.8.9 JavaScript source LANGSMITH_TRACING LANGSMITH_OTEL_ENABLED LANGCHAIN_TRACING_V2 environment variables
💡 Result:
In the LangSmith JavaScript SDK (version 0.8.9 and related versions), environment variables are the primary method for configuring tracing [1][2]. Key Environment Variables: - LANGSMITH_TRACING: Set this to "true" to enable LangSmith tracing [3][1][2]. - LANGSMITH_API_KEY: Your LangSmith API key for authentication [1][2]. - LANGSMITH_ENDPOINT: The URL for the LangSmith API. Defaults to "https://api.smith.langchain.com"; you may need to set this if your account is in a different region (e.g., EU, APAC, or AWS US) [1][2][4]. - LANGSMITH_PROJECT: Optional; defines the project name where traces are logged. If not set, it defaults to "default" [1][2][4]. Regarding LANGSMITH_OTEL_ENABLED and LANGCHAIN_TRACING_V2: - LANGSMITH_OTEL_ENABLED: The SDK treats this as a legacy variable. It, along with the even older OTEL_ENABLED, serves as a fallback for enabling OpenTelemetry-based tracing if a newer configuration method (like LANGSMITH_TRACING_MODE) is not used [5]. - LANGCHAIN_TRACING_V2: While common in older LangChain implementations, this is generally considered a legacy variable in the context of the newer standalone LangSmith SDK. The SDK internally categorizes it as an excluded/legacy variable when gathering environment metadata [5]. Tracing Configuration Hierarchy: The SDK provides a mechanism to resolve the effective tracing mode. When both modern and legacy variables are present, the SDK may prioritize explicit configuration or newer variables over legacy ones [5]. If you need to avoid environment variables entirely (e.g., in serverless environments like Cloudflare Workers), you can configure tracing programmatically by passing settings directly to the client or via context managers like traceable [6][7].
Citations:
- 1: https://github.com/langchain-ai/langsmith-sdk/blob/e398a27a/js/README.md
- 2: https://unpkg.com/langsmith@0.8.9/README.md
- 3: https://docs.langchain.com/oss/javascript/langchain/observability
- 4: https://docs.langchain.com/langsmith/observability-quickstart
- 5: https://github.com/langchain-ai/langsmith-sdk/blob/main/js/src/utils/env.ts
- 6: https://docs.langchain.com/langsmith/trace-with-langchain
- 7: https://docs.langchain.com/langsmith/trace-without-env-vars
🌐 Web query:
site:github.com/langchain-ai/langsmith-sdk "LANGSMITH_OTEL_ENABLED"
💡 Result:
LANGSMITH_OTEL_ENABLED is an environment variable used in the LangSmith SDK (both Python and JavaScript) to control integration with OpenTelemetry (OTEL) [1][2][3]. Setting LANGSMITH_OTEL_ENABLED=true enables OpenTelemetry tracing for LangSmith operations [3][4]. Depending on your configuration, this enables the export of trace data to OpenTelemetry-compatible endpoints [3][4]. Key details regarding its behavior: 1. Hybrid Mode (Default): By default, when LANGSMITH_OTEL_ENABLED is set to true, the SDK typically operates in a hybrid mode, exporting trace data to both the standard LangSmith API and your configured OpenTelemetry endpoint [4]. 2. OTEL-Only Mode: You can restrict exports exclusively to OpenTelemetry by setting an additional environment variable, LANGSMITH_OTEL_ONLY=true, alongside LANGSMITH_OTEL_ENABLED=true [4]. 3. Dependencies: To use this functionality, the required OpenTelemetry packages must be installed. In Python, this is typically done via pip install langsmith[otel] [2][3]. If the environment variable is set but the packages are not installed, the SDK will issue a warning [2]. 4. Configuration and Precedence: - Customization: You can further configure the exporter using standard OpenTelemetry environment variables, such as OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, and OTEL_SERVICE_NAME [3]. - Precedence: In newer versions of the SDK, the LANGSMITH_TRACING_MODE environment variable may take precedence over legacy settings like LANGSMITH_OTEL_ENABLED [1][5]. Accepted values for LANGSMITH_TRACING_MODE include "langsmith" and "otel" [1][5]. The integration supports trace context propagation, allowing LangSmith-traced functions and custom OpenTelemetry spans to exist within the same trace hierarchy [6].
Citations:
- 1: https://github.com/langchain-ai/langsmith-sdk/blob/main/js/src/utils/env.ts
- 2: https://github.com/langchain-ai/langsmith-sdk/blob/e398a27a/python/langsmith/client.py
- 3: Create OTel Exporter for LangChain tracing langchain-ai/langsmith-sdk#1568
- 4: feat: OTEL_ENABLED mode sends to both otel and ls, while allowing otel only through OTEL_ONLY var langchain-ai/langsmith-sdk#1762
- 5: https://github.com/langchain-ai/langsmith-sdk/blob/84e15e8427437dd402cdf51aec669ab6262d68c3/js/src/client.ts
- 6: feat[python]: Enable OpenTelemetry trace context propagation langchain-ai/langsmith-sdk#1783
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
meta="$tmp/meta.json"
curl -fsSL https://registry.npmjs.org/langsmith/0.8.9 -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
curl -fsSL "$url" -o "$tmp/langsmith.tgz"
tar -tzf "$tmp/langsmith.tgz" | rg '(^|/)(env|client|utils|index).*\.(js|cjs|mjs|ts)$' | head -80
tar -xzf "$tmp/langsmith.tgz" -C "$tmp"
printf '%s\n' '--- environment-variable references in langsmith 0.8.9 ---'
rg -n -C 3 \
'LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGCHAIN_TRACING|LANGCHAIN_TRACING_V2|LANGSMITH_RUNS_ENDPOINT|OTEL_EXPORTER_OTLP_ENDPOINT|TRACING_MODE' \
"$tmp/package" | head -240Repository: protonspy/open-wiki
Length of output: 25240
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
for spec in 'langsmith@0.8.9' '`@langchain/core`@1.2.4' 'langchain@1.5.4'; do
name=${spec%@*}; ver=${spec##*@}
meta="$tmp/meta-${name//\//@}.json"
curl -fsSL "https://registry.npmjs.org/$name/$ver" -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
out="$tmp/${name//\//@}.tgz"
mkdir -p "${out%/*}"
curl -fsSL "$url" -o "$out"
mkdir -p "$tmp/${name//\//@}"
tar -xzf "$out" -C "$tmp/${name//\//@}"
done
printf '%s\n' '--- langsmith 0.8.9 env implementation ---'
sed -n '1,280p' "$tmp/langsmith/dist/utils/env.cjs"
printf '%s\n' '--- langsmith 0.8.9 tracing construction references ---'
rg -n -C 5 'resolveTracingMode|getEnvironmentVariable\("LANGSMITH_TRACING|LANGCHAIN_TRACING|LANGSMITH_OTEL_ENABLED|initializeOTEL|OTLP' "$tmp/langsmith/dist" | head -320
printf '%s\n' '--- core 1.2.4 tracing environment references ---'
rg -n -C 4 'LANGCHAIN_TRACING_V2|LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGCHAIN_API_KEY|LANGSMITH_API_KEY|LANGCHAIN_CALLBACKS_BACKGROUND' "$tmp/@langchain/core/dist" | head -320Repository: protonspy/open-wiki
Length of output: 292
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
fetch() {
spec=$1
name=${spec%@*}
ver=${spec##*@}
meta="$tmp/meta-${RANDOM}.json"
curl -fsSL "https://registry.npmjs.org/$name/$ver" -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
dir="$tmp/${name//\//@}-${ver}"
mkdir -p "$dir"
curl -fsSL "$url" | tar -xz -C "$dir"
find "$dir" -path '*/package' -type d -print
}
fetch 'langsmith@0.8.9'
fetch '`@langchain/core`@1.2.4'
fetch 'langchain@1.5.4'
ls -d "$tmp"/*/package
lsmith=$(find "$tmp" -path '*/langsmith-0.8.9/package' -type d | head -1)
core=$(find "$tmp" -path '*/core-1.2.4/package' -type d | head -1)
printf '%s\n' '--- langsmith 0.8.9 env implementation ---'
sed -n '1,280p' "$lsmith/dist/utils/env.cjs"
printf '%s\n' '--- langsmith 0.8.9 tracing construction references ---'
rg -n -C 5 'resolveTracingMode|getEnvironmentVariable\("LANGSMITH_TRACING|LANGCHAIN_TRACING|LANGSMITH_OTEL_ENABLED|initializeOTEL|OTLP' "$lsmith/dist" | head -320
printf '%s\n' '--- core 1.2.4 tracing environment references ---'
rg -n -C 4 'LANGCHAIN_TRACING_V2|LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGCHAIN_API_KEY|LANGSMITH_API_KEY|LANGCHAIN_CALLBACKS_BACKGROUND' "$core/dist" | head -320Repository: protonspy/open-wiki
Length of output: 43706
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
meta="$tmp/meta.json"
curl -fsSL https://registry.npmjs.org/@langchain%2fcore/1.2.4 -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
mkdir "$tmp/core"
curl -fsSL "$url" | tar -xz -C "$tmp/core"
core="$tmp/core/package"
printf '%s\n' '--- core package files ---'
find "$core/dist" -maxdepth 3 -type f | rg 'trac|env|callback|manager' | head -100
printf '%s\n' '--- core tracing environment references ---'
rg -n -C 5 \
'LANGCHAIN_TRACING_V2|LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGCHAIN_API_KEY|LANGSMITH_API_KEY|LANGCHAIN_CALLBACKS_BACKGROUND|LANGCHAIN_TRACING' \
"$core/dist" | head -360
printf '%s\n' '--- langchain package tracing references ---'
meta="$tmp/langchain.json"
curl -fsSL https://registry.npmjs.org/langchain/1.5.4 -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
mkdir "$tmp/langchain"
curl -fsSL "$url" | tar -xz -C "$tmp/langchain"
rg -n -C 4 \
'LANGCHAIN_TRACING_V2|LANGSMITH_TRACING|LANGSMITH_OTEL_ENABLED|LANGCHAIN_TRACING' \
"$tmp/langchain/package/dist" | head -240Repository: protonspy/open-wiki
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp -d)
trap 'rm -rf "$tmp"' EXIT
fetch() {
spec=$1
encoded=${spec/@/%2f}
meta="$tmp/meta-${RANDOM}.json"
curl -fsSL "https://registry.npmjs.org/$encoded/${spec##*@}" -o "$meta"
url=$(node -e 'const p=require(process.argv[1]); console.log(p.dist.tarball)' "$meta")
dir="$tmp/${spec//\//@}"
mkdir -p "$dir"
curl -fsSL "$url" | tar -xz -C "$dir"
find "$dir" -path '*/package' -type d -print
}
fetch 'langsmith@0.8.9'
fetch '`@langchain/core`@1.2.4'
lsmith=$(find "$tmp" -path '*/langsmith@0.8.9/package' -type d | head -1)
core=$(find "$tmp" -path '*/@langchain/core@1.2.4/package' -type d | head -1)
printf '%s\n' '--- core callback activation implementation ---'
cat "$core/dist/utils/callbacks.js"
printf '%s\n' '--- core manager activation context ---'
sed -n '490,540p' "$core/dist/callbacks/manager.js"
printf '%s\n' '--- LangChain tracer construction and client resolution ---'
rg -n -C 8 \
'new LangSmith|LangSmithTracer|LangChainTracer|isTracingEnabled|getEnvironmentVariable\("LANGCHAIN_TRACING' \
"$core/dist/tracers" "$core/dist/callbacks" | head -220
printf '%s\n' '--- LangSmith endpoint variable names ---'
rg -n 'LANGSMITH_RUNS_ENDPOINT' "$lsmith/dist" | head -40
printf '%s\n' '--- LangSmith OTEL setup activation ---'
rg -n -C 8 \
'resolveTracingMode|initializeOTEL|OTEL_EXPORTER_OTLP_ENDPOINT|LANGSMITH_OTEL_ENABLED' \
"$lsmith/dist/experimental/otel" "$lsmith/dist/client.js" | head -220Repository: protonspy/open-wiki
Length of output: 208
Disable all tracing switches before importing LangChain.
@langchain/core@1.2.4 enables tracing when any of LANGSMITH_TRACING_V2, LANGCHAIN_TRACING_V2, LANGSMITH_TRACING, or LANGCHAIN_TRACING is "true". langsmith@0.8.9 also recognizes LANGSMITH_OTEL_ENABLED, OTEL_ENABLED, and LANGSMITH_TRACING_MODE=otel. Force the boolean switches to "false" and remove the tracing-mode override. If replica endpoints are cleared, use LANGSMITH_RUNS_ENDPOINTS (plural). Add tests for each pre-set activation variable.
🤖 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/agent/tracing.ts` around lines 33 - 51, Update
disableTracing() and its TRACING_VARS cleanup list to cover every supported
tracing activation: set LANGSMITH_TRACING_V2, LANGCHAIN_TRACING_V2,
LANGSMITH_TRACING, LANGCHAIN_TRACING, LANGSMITH_OTEL_ENABLED, and OTEL_ENABLED
to "false"; remove LANGSMITH_TRACING_MODE and clear the plural
LANGSMITH_RUNS_ENDPOINTS variable when applicable. Add tests verifying tracing
is disabled for each pre-set activation variable before LangChain imports.
Eleven findings from the review on PR #35, each verified against the code before it was changed. The two critical ones and the tracing hole were real holes rather than polish: - `readRaw` and `edit` raised `EISDIR` out of the backend on a directory path the model can find with `ls`, where every other failure in that class is an `{ error }` result. Every read now goes through one guarded helper — stat, refuse a directory or non-regular file, refuse a file over an 8 MiB limit, never throw. `grep` skips an unreadable file instead of discarding every match already collected, and says so when it caps. - `edit` accepted an empty `old_string`, which is a whole-page rewrite: `split("").join(s)` inserts between every character. `previewReplace` renders nothing for it, so the human would have approved the most destructive edit there is with no preview shown. Refused at the write. - Only `LANGCHAIN_TRACING_V2` was disabled. `@langchain/core` enables tracing when any of four variables reads "true", so three of them still switched it back on. All four are forced to "false", and the alternative transports — `LANGSMITH_TRACING_MODE`, `LANGSMITH_OTEL_ENABLED`, `OTEL_ENABLED`, and the replica list `LANGSMITH_RUNS_ENDPOINTS` — are cleared. - The page guard keyed its expectation map by path alone. One agent is cached per window and every thread shares that closure, so two threads writing one page swapped hashes and left the guard off for both. Keyed by thread and path now; the thread id is read off the runtime both hooks receive (`configurable.thread_id` in langgraph 1.4.8, `executionInfo.threadId` when a later version populates it). - `renamePage` rolled back the two pages and left `wiki/log.md`, `wiki/changelog.md` and `wiki/index.md` announcing a rename that never happened. Those are backed up separately from the operation's snapshot — putting them in it would make a later `undo` roll the changelog back over everything written since. A rollback that itself throws is now named in the reasons instead of being swallowed under "rolled back". - The chat pane was unmounted on every pane switch, resetting the transcript and the one-per-window `threadId` — and the new id addressed a thread the main process had never checkpointed, so the conversation was unrecoverable, not merely off screen. It stays mounted and hidden. - `proposalOf` read `path` but not `file_path`, the name the deepagents tools actually use, so every real proposal fell through to the card's nothing-renderable fallback. `InterruptCard` had no key, so a replacement proposal kept the superseded one's edited text in the textarea. - `readAgentPrefs` threw on a file that is not JSON; the throw reached the renderer as an unhandled rejection. - The preview-vs-backend suite seeded a page the gate refused, so its cross-check never ran and the only live assertion compared the preview to a re-implementation of itself. It now seeds a page the gate accepts and compares the count and the written body unconditionally. Spec deltas in the same branch: R3.3 and R4.8 added, R4.7 modified to cover every file a rename writes, and tasks 1.11-1.13, 2.11-2.13, 4.8-4.9, 6.17-6.20. Verified: `pnpm run typecheck` · `pnpm test:coverage` (the 76% floor on all five packages) · `pnpm lint` · `prettier --check` · `scc validate` (0 findings). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NevmGVqcA4RGFzU1SWt2YK
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
specs/embedded-agent/tasks.md (1)
72-78: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not mark the eviction test complete while eviction is disabled.
Task 2.1 sets
toolTokenLimitBeforeEvict: null, so task 6.11 cannot trigger middleware eviction. Task 6.16 correctly acknowledges this limitation, but task 6.11 remains marked complete. Add a finite-threshold test, or rewrite 6.11 as a direct backend-path test and keep 6.16 as the companion coverage.🤖 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 `@specs/embedded-agent/tasks.md` around lines 72 - 78, The task 6.11 checklist incorrectly claims eviction coverage while tool eviction is disabled by toolTokenLimitBeforeEvict: null. Add a separate test configuration with a finite threshold that actually triggers middleware eviction and verifies no file is created, or rewrite 6.11 to cover the direct backend path and retain 6.16 as companion coverage; only mark the task complete when its test exercises the claimed behavior.specs/embedded-agent/requirements.md (1)
52-63: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSpecify when model selection changes are persisted.
R2.7 only requires persistence during Groq key validation. It does not require saving a model selected after validation. Align this requirement with
specs/embedded-agent/tasks.md5.2 by requiring every model-selection change to persist in application data. Also define the fallback when a refreshed/modelslist removes the saved model.🤖 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 `@specs/embedded-agent/requirements.md` around lines 52 - 63, Update requirement R2.7 to persist every model-selection change in the application data directory keyed by project, not only during Groq key validation, and keep the data out of the project directory and repository. Define the behavior when a refreshed Groq /models response no longer contains the saved model, including the fallback model-selection outcome.specs/embedded-agent/design.md (2)
71-116: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake every approved write compare before committing and serialize the change.
pageGuardMiddlewareonly rechecks the single confirmedwrite_file/edit_filetarget and cannot stop a stale edit after its source page changes insideWikiGateBackend.edit. Pass an expected hash or version into the write path, perform the comparison and write under one per-path lock or transaction, and require the same protection fordelete_pageandrename_page, including destination state for the rename.🤖 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 `@specs/embedded-agent/design.md` around lines 71 - 116, Update pageGuardMiddleware and the WikiGateBackend write paths so each approved write carries the expected page hash or version into the backend. Compare that state and commit the write under the same per-path lock or transaction, and apply identical protection to delete_page and rename_page; rename_page must also validate the destination’s expected state before committing.
174-192: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix the rename write-path description.
renamePagecallsgateWriteplus non-loggingatomicWriteand records one operation, notgateWrite+writePage. Update this sentence so the spec does not describe a nested operation-log entry for the new page.🤖 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 `@specs/embedded-agent/design.md` around lines 174 - 192, Update the renamePage description to state that it writes the new page via gateWrite and non-logging atomicWrite, then marks the old page superseded and records one operation. Remove the reference to writePage so the specification accurately describes a single operation-log entry.
🧹 Nitpick comments (2)
apps/desktop/src/main/agent/wiki-gate-backend.ts (1)
192-205: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove the redundant
statSynccall beforereadGuarded.
readGuardedalready callsstatSyncand checksisDirectory()internally, and returns{ error }for both. Lines 192-198 duplicate that same stat and directory check before callingreadGuardedon line 203. Every candidate file in agrepscan pays for twostatsyscalls instead of one.Remove the first
statSync/isDirectoryblock and letreadGuardeddo this work once.♻️ Proposed fix to remove the duplicate stat
- let st: Stats; - try { - st = statSync(full); - } catch { - continue; - } - if (st.isDirectory()) continue; - // One unreadable or oversized file must not discard every match already + // One unreadable or oversized file must not discard every match already // collected — a scan of the project will meet a locked file, a dangling // link, or a recording sooner or later, and losing the whole result to // one of them turns a working tool into an intermittent failure. const r = readGuarded(full, full); if ("error" in r) continue;🤖 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/agent/wiki-gate-backend.ts` around lines 192 - 205, Remove the local Stats declaration and try/catch around statSync, including the separate isDirectory check, in the scan loop before readGuarded. Let readGuarded perform the single stat and directory validation, retaining its existing error and binary-content handling.apps/desktop/tests/wiki-gate-backend.spec.ts (1)
310-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the grep size-guard assertion.
The oversized file
raw/huge.txtis written empty, then extended withtruncateSync. The extended bytes are zero-filled, so the file never contains "needle" regardless of whether the size guard actually fires. If the size check inreadGuardedwere removed, this test would still pass, because the file would still not match "needle" when read.Write "needle" content into the file before truncating it past
MAX_READ_BYTES, then assert that the match from that file is absent. This proves the size guard, not content mismatch, is what excludes the file.✅ Proposed strengthened test
writeFileSync(join(root, "wiki", "a.md"), "needle here\n"); const big = join(root, "raw", "huge.txt"); - writeFileSync(big, ""); + writeFileSync(big, "needle in oversized file\n"); truncateSync(big, MAX_READ_BYTES + 1); writeFileSync(join(root, "wiki", "b.md"), "needle again\n"); const r = backend.grep("needle"); expect(r.error).toBeUndefined(); expect(r.matches?.map((m) => m.text)).toEqual(["needle here", "needle again"]); + expect(r.matches?.some((m) => m.text.includes("needle in oversized"))).toBe(false);🤖 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/wiki-gate-backend.spec.ts` around lines 310 - 324, Strengthen the test around backend.grep by writing content containing “needle” to raw/huge.txt before extending it beyond MAX_READ_BYTES, then assert the oversized file’s match is excluded while the two valid wiki matches remain. Keep the existing no-error assertion and verify the expected match texts so the test specifically validates readGuarded’s size guard rather than a content mismatch.
🤖 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 `@specs/embedded-agent/design.md`:
- Around line 228-231: Define lifecycle cleanup for page-guard entries around
pageGuardMiddleware: key expected writes by (runId, thread, path), then remove
the corresponding entries on chat:cancel, tool-error, and agent-error exits, not
only after successful tool execution. Preserve the existing replacement behavior
when a new proposal supersedes an entry.
---
Outside diff comments:
In `@specs/embedded-agent/design.md`:
- Around line 71-116: Update pageGuardMiddleware and the WikiGateBackend write
paths so each approved write carries the expected page hash or version into the
backend. Compare that state and commit the write under the same per-path lock or
transaction, and apply identical protection to delete_page and rename_page;
rename_page must also validate the destination’s expected state before
committing.
- Around line 174-192: Update the renamePage description to state that it writes
the new page via gateWrite and non-logging atomicWrite, then marks the old page
superseded and records one operation. Remove the reference to writePage so the
specification accurately describes a single operation-log entry.
In `@specs/embedded-agent/requirements.md`:
- Around line 52-63: Update requirement R2.7 to persist every model-selection
change in the application data directory keyed by project, not only during Groq
key validation, and keep the data out of the project directory and repository.
Define the behavior when a refreshed Groq /models response no longer contains
the saved model, including the fallback model-selection outcome.
In `@specs/embedded-agent/tasks.md`:
- Around line 72-78: The task 6.11 checklist incorrectly claims eviction
coverage while tool eviction is disabled by toolTokenLimitBeforeEvict: null. Add
a separate test configuration with a finite threshold that actually triggers
middleware eviction and verifies no file is created, or rewrite 6.11 to cover
the direct backend path and retain 6.16 as companion coverage; only mark the
task complete when its test exercises the claimed behavior.
---
Nitpick comments:
In `@apps/desktop/src/main/agent/wiki-gate-backend.ts`:
- Around line 192-205: Remove the local Stats declaration and try/catch around
statSync, including the separate isDirectory check, in the scan loop before
readGuarded. Let readGuarded perform the single stat and directory validation,
retaining its existing error and binary-content handling.
In `@apps/desktop/tests/wiki-gate-backend.spec.ts`:
- Around line 310-324: Strengthen the test around backend.grep by writing
content containing “needle” to raw/huge.txt before extending it beyond
MAX_READ_BYTES, then assert the oversized file’s match is excluded while the two
valid wiki matches remain. Keep the existing no-error assertion and verify the
expected match texts so the test specifically validates readGuarded’s size guard
rather than a content mismatch.
🪄 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: a54b4818-0952-4f8d-8562-73faedc005f6
📒 Files selected for processing (21)
apps/desktop/src/main/agent/agent-prefs.tsapps/desktop/src/main/agent/edit-preview.tsapps/desktop/src/main/agent/page-guard.tsapps/desktop/src/main/agent/tracing.tsapps/desktop/src/main/agent/wiki-gate-backend.tsapps/desktop/src/renderer/App.tsxapps/desktop/src/renderer/Chat.tsxapps/desktop/src/renderer/Settings.tsxapps/desktop/src/renderer/chat-model.tsapps/desktop/src/renderer/globals.cssapps/desktop/tests/agent-loop.spec.tsapps/desktop/tests/agent-prefs.spec.tsapps/desktop/tests/agent.spec.tsapps/desktop/tests/chat-model.spec.tsapps/desktop/tests/edit-preview.spec.tsapps/desktop/tests/wiki-gate-backend.spec.tspackages/access/src/write/rename-delete.tspackages/access/tests/rename-delete.spec.tsspecs/embedded-agent/design.mdspecs/embedded-agent/requirements.mdspecs/embedded-agent/tasks.md
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/desktop/src/renderer/App.tsx
- apps/desktop/tests/chat-model.spec.ts
- apps/desktop/src/renderer/Settings.tsx
- apps/desktop/src/main/agent/page-guard.ts
- packages/access/tests/rename-delete.spec.ts
- apps/desktop/src/renderer/globals.css
- apps/desktop/src/main/agent/edit-preview.ts
- apps/desktop/src/renderer/Chat.tsx
- packages/access/src/write/rename-delete.ts
- apps/desktop/tests/agent-prefs.spec.ts
- apps/desktop/src/main/agent/agent-prefs.ts
- apps/desktop/src/renderer/chat-model.ts
The follow-up review's one finding. `pageGuardMiddleware` deletes an entry when the guarded tool executes, and only then — but `chat:cancel` aborts a run while it is paused, and a run error ends it the same way, so a turn can end without ever reaching the tool. The map then grows for the window's whole life. The entry is dropped at the start of the thread's next `afterModel`, which is the moment it is provably dead: a thread's runs are sequential, so anything still standing when that hook fires belongs to a turn that is over. Scoped to the thread that is running, never another's. A new `tests/page-guard.spec.ts` drives the two hooks directly, which is the only way to reach a turn that never executes its tool — a real run cannot produce one. Watched red on both new cases with the sweep commented out; the existing stale-edit and two-thread proofs stay green, which is what says the sweep does not clear an expectation still in use. Task 2.14 added. Verified: `pnpm run typecheck` · `pnpm test:coverage` · `pnpm lint` · `prettier --check` · `scc validate` (0 findings). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NevmGVqcA4RGFzU1SWt2YK
1 participant
Implements
specs/embedded-agent, enabled byadr:0019-an-embedded-agent-that-reads-freely-and-writes-through-the-gate. All 54 tasks inspecs/embedded-agent/tasks.mdare ticked and traceable.What changed
The desktop app now runs an embedded agent in the main process, behind a chat pane. It reads the project the way a harness does — carrying in
CLAUDE.mdand.claude/skills/unchanged, never a hand-written prompt — and writes onlywiki/, only through the validated store, and only after a human approves the specific change.The guardrail is scope, not prompting.
WikiGateBackendimplements deepagents'BackendProtocolV2: reads confined to the project withassertWithin, writes accepted only underwiki/and routed throughgateWrite+writePagewith a newOriginvariant,agent.executeis not implemented, so the backend is not a sandbox backend and the shell tool is never offered.taskis absent, not disabled. The stack is assembled from langchain'screateAgentplus explicit deepagents middleware rather thancreateDeepAgent, because the latter makesSubAgentMiddlewarerequired and its harness-profile switch never reaches aChatGroqinstance. Building the stack by hand is what makes the subagent tool genuinely absent.Every write pauses first.
write_file,edit_file,rename_pageanddelete_pageinterrupt before touching disk. A page-guard middleware hashes the target at proposal time and refuses a stale approval, re-proposing rather than clobbering a page that changed in the pause window.Also here: gated, undoable
renamePage/deletePageprimitives in@open-wiki/access; the Groq model picker fed by the/modelslist captured at credential-save time; the three chat IPC channels plus the buffered push channel.How it was verified
pnpm run typecheck·pnpm test(1274) ·pnpm lint· the 76% coverage floor on all five packages ·scc validate(0 findings).code-reviewandsecurity-reviewwere both run on the diff and their findings fixed. Four are worth naming here because each was a real hole:chat-control.tsimported@langchain/langgraphbefore the module that disables LangSmith — ES modules evaluate imports in written order, soagent.tsholding the guard on line 1 did nothing for a sibling that reached langchain on its own account. The guard is now the first import of every module that reaches langchain, and a source sweep inagent.spec.tsenforces it. That sweep found a fourth unguarded importer (page-guard.ts) the reviews had not named.replace_alldisclosure was never implemented. The interrupt fires on the model's raw arguments, so nothing in the stack had counted the occurrences — a shortold_stringrendered exactly like a single-site edit, the smuggling casedesign.mdnames as the risk it exists to close. Newedit-preview.tscomputes every match site and the resulting page from disk at interrupt time."aa"twice in"aaa") where the backend'ssplit().join()counts one — it would have promised a replacement that never happened. Fixed, with a test running the preview andWikiGateBackend.editside by side over five cases so they cannot drift.renamePagehad no rollback. Two writes, and a failure between them left the new page created while the old still readactive— one entity live twice with nothing saying which is current. Now wrapped, rolling back through the existingundoprimitive, with a test that forces the failure.Fixed but not separately called out: a rotated API key was cached until restart; task 6.11's test was trivially true; the preview read and shipped files outside
wiki/with no size cap.Nothing outstanding at
minor/low.Note for the reviewer
A second commit,
chore(git): ignore .claude/worktrees/, is unrelated to the feature. The harness places worktrees inside the repository and the path was neither tracked nor ignored, so agit add .in the primary checkout would have committed an entire second checkout. Split out so it can be dropped independently.Spec deltas in the same branch:
R4.7added, and tasks 1.10 / 2.9 / 2.10 / 4.6 / 4.7 / 6.15 / 6.16 for the review fixes.🤖 Generated with Claude Code
https://claude.ai/code/session_01LBfVmmLyJamvwmJVRVNZsE
Summary by CodeRabbit