Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
📝 WalkthroughWalkthroughAdds Skill Runtime Hardening: per-skill sidecar configs for runtimes/env/scripts, binary-read guardrails, close-based process completion with output offload, SkillExecutionService and skill_run tooling, presenter/file security checks, renderer UI for runtime/env/scripts, and tests for discovery, execution, and read behavior. Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent (Tool Caller)
participant ToolMgr as AgentToolManager
participant SkillExec as SkillExecutionService
participant SkillPres as SkillPresenter
participant SessionMgr as BackgroundExecSessionManager
participant Process as Shell Process
Agent->>ToolMgr: callTool('skill_run', {skill, script, args})
ToolMgr->>SkillExec: execute(request, {conversationId})
SkillExec->>SkillPres: listSkillScripts(skill)
SkillPres-->>SkillExec: script descriptors
SkillExec->>SkillPres: getSkillExtension(skill)
SkillPres-->>SkillExec: extension (env, runtimePolicy)
SkillExec->>SkillExec: resolveRuntimeCommand()/buildSpawnPlan()
alt background
SkillExec->>SessionMgr: start background session with spawn plan
SessionMgr->>Process: spawn process
Process-->>SessionMgr: stdout/close events
SessionMgr-->>SkillExec: {status: "running", sessionId}
SkillExec-->>ToolMgr: running response
else foreground
SkillExec->>Process: spawn foreground process
Process-->>SkillExec: stdout stream
SkillExec->>SkillExec: buffer and maybe offload to file
Process-->>SkillExec: close event
SkillExec-->>ToolMgr: final output or offload path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
src/renderer/src/i18n/ru-RU/settings.json-1277-1282 (1)
1277-1282:⚠️ Potential issue | 🟡 MinorMissing Russian translations for skill card strings.
The new
cardobject contains English strings that need Russian translations.🌐 Example translations (verify with native speaker)
"card": { - "scripts": "{count} scripts", - "env": "{count} env", - "pythonShort": "Py", - "nodeShort": "Node" + "scripts": "{count} скриптов", + "env": "{count} перем.", + "pythonShort": "Py", + "nodeShort": "Node" }Note:
pythonShortandnodeShortmay intentionally remain as "Py" and "Node" since these are commonly recognized technical abbreviations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` around lines 1277 - 1282, The "card" localization block contains English strings; update the values for the keys "scripts" and "env" inside the card object to Russian (e.g., replace "{count} scripts" with "{count} скрипт(ов)" or a proper pluralized form and "{count} env" with "{count} окружение(й)" or similar pluralized Russian phrasing), and confirm whether to keep "pythonShort" as "Py" and "nodeShort" as "Node" (if not, replace with Russian-appropriate abbreviations); ensure the keys remain "card", "scripts", "env", "pythonShort", and "nodeShort" exactly.src/renderer/src/i18n/ru-RU/settings.json-1143-1159 (1)
1143-1159:⚠️ Potential issue | 🟡 MinorMissing Russian translations for new skill editor strings.
All newly added strings in this section are in English instead of Russian. This will result in a mixed-language UI for Russian-speaking users. These keys need Russian translations:
runtimeTitle,runtimeHintpythonRuntime,nodeRuntimeenvTitle,envWarningscriptsTitle,scriptsHint,noScriptsscriptEnabled,scriptDescription,scriptDescriptionPlaceholderruntime.auto,runtime.system,runtime.builtin🌐 Example translations (verify with native speaker)
- "runtimeTitle": "Runtime", - "runtimeHint": "Select how bundled scripts resolve Python and Node runtimes.", - "pythonRuntime": "Python Runtime", - "nodeRuntime": "Node Runtime", - "envTitle": "Environment Variables", - "envWarning": "Masked in the UI only. Values are stored as plain text in the skill sidecar file.", - "scriptsTitle": "Bundled Scripts", - "scriptsHint": "Only scripts under scripts/ can be run via skill_run.", - "noScripts": "No runnable scripts detected", - "scriptEnabled": "Enabled", - "scriptDescription": "Description Override", - "scriptDescriptionPlaceholder": "Optional description shown to the agent", - "runtime": { - "auto": "Auto fallback", - "system": "System runtime", - "builtin": "Built-in runtime" - } + "runtimeTitle": "Среда выполнения", + "runtimeHint": "Выберите, как скрипты разрешают среды выполнения Python и Node.", + "pythonRuntime": "Среда выполнения Python", + "nodeRuntime": "Среда выполнения Node", + "envTitle": "Переменные окружения", + "envWarning": "Скрыты только в интерфейсе. Значения хранятся в открытом виде в файле навыка.", + "scriptsTitle": "Встроенные скрипты", + "scriptsHint": "Только скрипты в папке scripts/ могут запускаться через skill_run.", + "noScripts": "Исполняемые скрипты не обнаружены", + "scriptEnabled": "Включено", + "scriptDescription": "Переопределение описания", + "scriptDescriptionPlaceholder": "Необязательное описание для агента", + "runtime": { + "auto": "Автовыбор", + "system": "Системная среда", + "builtin": "Встроенная среда" + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ru-RU/settings.json` around lines 1143 - 1159, The listed UI strings are still in English; update the JSON object in src/renderer/src/i18n/ru-RU/settings.json by providing Russian translations for the keys runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the nested runtime keys runtime.auto, runtime.system, runtime.builtin; replace the English values with appropriate Russian text (ensure proper escaping and keep the same key names and JSON structure) so the ru-RU locale is fully localized for the skill editor.src/renderer/src/i18n/fr-FR/settings.json-1143-1159 (1)
1143-1159:⚠️ Potential issue | 🟡 MinorTranslate the new skill editor strings in the French locale.
src/renderer/settings/components/skills/SkillEditorSheet.vuerenders these keys directly, so the current block will show English labels and hints in the French UI.🌐 Suggested translation pass
- "runtimeTitle": "Runtime", - "runtimeHint": "Select how bundled scripts resolve Python and Node runtimes.", - "pythonRuntime": "Python Runtime", - "nodeRuntime": "Node Runtime", - "envTitle": "Environment Variables", - "envWarning": "Masked in the UI only. Values are stored as plain text in the skill sidecar file.", - "scriptsTitle": "Bundled Scripts", - "scriptsHint": "Only scripts under scripts/ can be run via skill_run.", - "noScripts": "No runnable scripts detected", - "scriptEnabled": "Enabled", - "scriptDescription": "Description Override", - "scriptDescriptionPlaceholder": "Optional description shown to the agent", + "runtimeTitle": "Environnement d’exécution", + "runtimeHint": "Choisissez comment les scripts intégrés résolvent les environnements Python et Node.", + "pythonRuntime": "Environnement Python", + "nodeRuntime": "Environnement Node", + "envTitle": "Variables d’environnement", + "envWarning": "Masquées uniquement dans l’interface. Les valeurs sont stockées en clair dans le fichier sidecar de la compétence.", + "scriptsTitle": "Scripts intégrés", + "scriptsHint": "Seuls les scripts sous scripts/ peuvent être exécutés via skill_run.", + "noScripts": "Aucun script exécutable détecté", + "scriptEnabled": "Activé", + "scriptDescription": "Remplacement de description", + "scriptDescriptionPlaceholder": "Description facultative affichée à l’agent", "runtime": { - "auto": "Auto fallback", - "system": "System runtime", - "builtin": "Built-in runtime" + "auto": "Bascule automatique", + "system": "Environnement système", + "builtin": "Environnement intégré" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fr-FR/settings.json` around lines 1143 - 1159, The French locale JSON block is missing translations for the new skill editor keys (runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the runtime.auto/system/builtin entries); open the fr-FR settings JSON and replace the English values with appropriate French strings for each key so SkillEditorSheet.vue renders localized labels/hints (ensure grammar and accents are correct and keep placeholders like "Optional description shown to the agent" as a short French placeholder).src/renderer/src/i18n/fr-FR/settings.json-1277-1281 (1)
1277-1281:⚠️ Potential issue | 🟡 MinorLocalize the new skill card badge labels as well.
These keys feed
src/renderer/settings/components/skills/SkillCard.vue, so French users will currently get English badge text.🌐 Suggested translation pass
"card": { - "scripts": "{count} scripts", - "env": "{count} env", + "scripts": "{count} scripts", + "env": "{count} variable(s) d’environnement", "pythonShort": "Py", "nodeShort": "Node" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fr-FR/settings.json` around lines 1277 - 1281, Add French translations for the new skill card badge keys used by SkillCard.vue: update the "card.scripts", "card.env", "card.pythonShort", and "card.nodeShort" entries under the "card" object in the fr-FR settings JSON so strings are localized (e.g., replace English labels with appropriate French equivalents), ensuring keys remain identical so SkillCard.vue picks them up.src/renderer/src/i18n/he-IL/settings.json-1143-1147 (1)
1143-1147:⚠️ Potential issue | 🟡 MinorFinish localizing the new skill runtime labels.
Unlike the surrounding entries, these new
skills.editandskills.cardvalues are still English, so the Hebrew settings page will render mixed-language UI.🌍 Suggested fix
- "runtimeTitle": "Runtime", - "runtimeHint": "Select how bundled scripts resolve Python and Node runtimes.", - "pythonRuntime": "Python Runtime", - "nodeRuntime": "Node Runtime", - "envTitle": "Environment Variables", - "scriptsTitle": "Bundled Scripts", - "scriptsHint": "Only scripts under scripts/ can be run via skill_run.", - "noScripts": "No runnable scripts detected", - "scriptEnabled": "Enabled", - "scriptDescription": "Description Override", - "scriptDescriptionPlaceholder": "Optional description shown to the agent", + "runtimeTitle": "סביבת הרצה", + "runtimeHint": "בחר כיצד סקריפטים מצורפים יאתרו את סביבות ההרצה של Python ו-Node.", + "pythonRuntime": "סביבת הרצה של Python", + "nodeRuntime": "סביבת הרצה של Node", + "envTitle": "משתני סביבה", + "scriptsTitle": "סקריפטים מצורפים", + "scriptsHint": "רק סקריפטים תחת scripts/ ניתנים להרצה דרך skill_run.", + "noScripts": "לא זוהו סקריפטים ניתנים להרצה", + "scriptEnabled": "מופעל", + "scriptDescription": "תיאור חלופי", + "scriptDescriptionPlaceholder": "תיאור אופציונלי שיוצג לסוכן", "runtime": { - "auto": "Auto fallback", - "system": "System runtime", - "builtin": "Built-in runtime" + "auto": "בחירה אוטומטית", + "system": "סביבת הרצה מערכתית", + "builtin": "סביבת הרצה מובנית" } ... - "scripts": "{count} scripts", - "env": "{count} env", + "scripts": "{count} סקריפטים", + "env": "{count} משתני סביבה",Also applies to: 1149-1159, 1278-1279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/he-IL/settings.json` around lines 1143 - 1147, The new UI strings are still in English; update the Hebrew localization by translating the following keys into Hebrew: "runtimeTitle", "runtimeHint", "pythonRuntime", "nodeRuntime", "envTitle" and also the related "skills.edit" and "skills.card" entries referenced in the comment. Locate those keys in the settings.json and replace the English values with their proper Hebrew equivalents, keeping punctuation and surrounding JSON structure intact.src/renderer/src/i18n/ja-JP/settings.json-1142-1159 (1)
1142-1159:⚠️ Potential issue | 🟡 MinorLocalize the new ja-JP strings before release.
Lines 1142-1159 and Lines 1277-1282 are still English, so the new runtime editor and skill card badges will render as mixed-language UI for Japanese users.
Also applies to: 1277-1282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ja-JP/settings.json` around lines 1142 - 1159, Translate the untranslated English strings into Japanese: replace the values for the keys runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the nested runtime.auto/system/builtin entries with appropriate Japanese text, and also localize the corresponding skill-card badge keys referenced later in the file (the English badge strings around the skill card section) so the runtime editor and skill card badges are fully Japanese.src/renderer/src/i18n/fa-IR/settings.json-1143-1159 (1)
1143-1159:⚠️ Potential issue | 🟡 MinorTranslate the new
fa-IRstrings.These added runtime/editor/card entries are still English, so Persian users will see mixed-language labels in the new skill management UI.
Also applies to: 1277-1282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/fa-IR/settings.json` around lines 1143 - 1159, Translate the newly added English i18n keys in fa-IR by replacing the English values with Persian translations for the keys runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the nested runtime values runtime.auto, runtime.system and runtime.builtin; also ensure the corresponding editor/card-related i18n entries added elsewhere in this file are translated the same way so the skill management UI is fully localized for Persian users.src/renderer/src/i18n/ko-KR/settings.json-1143-1159 (1)
1143-1159:⚠️ Potential issue | 🟡 MinorLocalize the new skill runtime labels.
These new
ko-KRentries are still English, so the skill editor and summary badges will show mixed-language UI for Korean users.Also applies to: 1277-1281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/ko-KR/settings.json` around lines 1143 - 1159, The new Korean locale entries for the skill runtime UI are still in English; update the values for keys like "runtimeTitle", "runtimeHint", "pythonRuntime", "nodeRuntime", "envTitle", "envWarning", "scriptsTitle", "scriptsHint", "noScripts", "scriptEnabled", "scriptDescription", "scriptDescriptionPlaceholder" and the nested "runtime" object ("auto","system","builtin") to their proper Korean translations so the skill editor and summary badges display fully localized text (also apply the same translations to the similar entries around the later block referenced at 1277-1281).src/renderer/src/i18n/da-DK/settings.json-1089-1104 (1)
1089-1104:⚠️ Potential issue | 🟡 MinorTranslate the new
da-DKstrings.The runtime/editor/card entries added here are still English, so Danish users will get mixed-language labels in the new skill UI.
Also applies to: 1223-1228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/da-DK/settings.json` around lines 1089 - 1104, The new Danish translation file still contains English labels: translate all the new keys such as "runtimeTitle", "runtimeHint", "pythonRuntime", "nodeRuntime", "envTitle", "envWarning", "scriptsTitle", "scriptsHint", "noScripts", "scriptEnabled", "scriptDescription", "scriptDescriptionPlaceholder" and the nested "runtime" values ("auto", "system", "builtin") into Danish; also locate and translate the additional English entries for the new editor/card strings mentioned in the review (the other added keys in the same i18n file) so the skill UI shows Danish labels consistently.src/renderer/src/i18n/pt-BR/settings.json-1143-1159 (1)
1143-1159:⚠️ Potential issue | 🟡 MinorTranslate the new
pt-BRentries.These additions are still English, so the runtime editor and skill card summary will render mixed-language UI for Brazilian Portuguese users.
Also applies to: 1277-1282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/pt-BR/settings.json` around lines 1143 - 1159, The new pt-BR localization entries are still in English; please translate the keys runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the nested runtime values (auto, system, builtin) into Brazilian Portuguese so the runtime editor and skill card summary are fully localized; also apply the same translation fixes to the duplicated entries referenced around lines 1277-1282.
🧹 Nitpick comments (3)
test/main/presenter/agentPresenter/agentToolManagerRead.test.ts (1)
157-168: Tighten the “no prompt pollution” assertion.This test only proves
prepareFileCompletely()was skipped. Add a negative assertion forpresenter.llmproviderPresenter.generateCompletionStandalonetoo, otherwise a future OCR/vision fallback could still inject binary-derived content while this test keeps passing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/agentPresenter/agentToolManagerRead.test.ts` around lines 157 - 168, The test currently only asserts presenter.filePresenter.prepareFileCompletely was not called; also assert that presenter's LLM fallback was not invoked by adding a negative expectation for presenter.llmproviderPresenter.generateCompletionStandalone (i.e., ensure generateCompletionStandalone was not called) after calling manager.callTool('read', ...) so binary reads cannot pollute the prompt via OCR/vision fallbacks; reference presenter.llmproviderPresenter.generateCompletionStandalone and presenter.filePresenter.prepareFileCompletely when adding the assertion.src/main/presenter/skillPresenter/skillExecutionService.ts (1)
384-391: Add a comment explaining why kill errors are ignored.The empty catch block at line 388-390 silently ignores errors but the comment "ignore kill errors" could be more descriptive about why this is safe (e.g., process may have already exited).
timeoutId = setTimeout(() => { timedOut = true try { child.kill('SIGTERM') } catch { - // ignore kill errors + // Process may have already exited; safe to ignore } }, timeoutMs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/skillPresenter/skillExecutionService.ts` around lines 384 - 391, Add a brief explanatory comment above the timeout kill block (around timeoutId/timedOut and child.kill('SIGTERM') using timeoutMs) stating that kill errors are intentionally ignored because the child process may have already exited, the OS may have reaped it, or signal delivery can fail due to race conditions/permissions; mention that ignoring the error is safe here because we only attempt best-effort termination and do not rely on kill throwing for control flow.src/main/presenter/skillPresenter/index.ts (1)
1158-1191: Consider adding a depth limit to prevent stack overflow on deeply nested directories.Unlike
buildFolderTreewhich usesSKILL_CONFIG.FOLDER_TREE_MAX_DEPTH,collectScriptDescriptorsrecursively traverses without a depth limit. While symlinks are skipped, a maliciously crafted skill directory could still cause stack overflow.♻️ Proposed fix: Add depth limit parameter
private collectScriptDescriptors( currentDir: string, skillRoot: string, - acc: SkillScriptDescriptor[] = [] + acc: SkillScriptDescriptor[] = [], + depth: number = 0 ): SkillScriptDescriptor[] { + if (depth >= SKILL_CONFIG.FOLDER_TREE_MAX_DEPTH) { + return acc + } + const entries = fs.readdirSync(currentDir, { withFileTypes: true }) for (const entry of entries) { if (entry.isSymbolicLink()) { continue } const fullPath = path.join(currentDir, entry.name) if (entry.isDirectory()) { - this.collectScriptDescriptors(fullPath, skillRoot, acc) + this.collectScriptDescriptors(fullPath, skillRoot, acc, depth + 1) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/skillPresenter/index.ts` around lines 1158 - 1191, The collectScriptDescriptors function currently recurses without a depth limit; modify collectScriptDescriptors to accept a depth parameter (e.g., currentDepth = 0) and stop recursion when currentDepth >= SKILL_CONFIG.FOLDER_TREE_MAX_DEPTH (same limit used by buildFolderTree), passing currentDepth + 1 when recursing into subdirectories; ensure callers that initiate the traversal call the revised collectScriptDescriptors with the default starting depth (0) so the stack overflow risk on deeply nested directories is mitigated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/skill-runtime-hardening/plan.md`:
- Around line 9-10: The runtime resolution must occur before applying
skill-provided environment variables: update SkillExecutionService so that
ResolveRuntime/selectRuntime (or the method that “selects runtime”) uses a
trusted base environment (or absolute paths) to locate interpreters, then only
after the runtime binary is resolved merge the skill-provided env into the child
process env; ensure SkillExecutionService’s flow (the method that currently
"merges env and then selects runtime") is reordered to resolve runtime first and
then apply SkillPresenter-provided env for process spawn.
In `@docs/specs/skill-runtime-hardening/spec.md`:
- Around line 10-13: The spec currently requires storing skill env vars as
plaintext sidecar metadata and avoiding SKILL.md frontmatter updates; update the
spec to forbid persisting secrets in plaintext sidecars and instead mandate
either storing secret values in OS-backed secure storage (e.g.,
keychain/credential store) or limiting the feature to non-secret env vars only;
replace the bullet "Env vars are stored outside the skill folder as plaintext
sidecar metadata." with a statement that secret env vars must be persisted to
secure OS-backed storage (or marked as non-secret and never written to
.deepchat-meta/*.json), and clarify the behavior referenced by "Editing a skill
does not rewrite external `SKILL.md` frontmatter to persist env vars." to ensure
SKILL.md frontmatter is not used for secret persistence and that tooling will
read/write non-secret env vars only to the approved store.
In `@src/main/lib/binaryReadGuard.ts`:
- Around line 25-34: ALWAYS_BINARY_MIMES currently contains
'application/octet-stream', which causes shouldRejectAgentBinaryRead to return
true early and skip the intended isLikelyTextFile fallback; remove
'application/octet-stream' from the ALWAYS_BINARY_MIMES Set so that
shouldRejectAgentBinaryRead will fall through to the isLikelyTextFile check
(retain all other MIME entries), and run tests or adjust any logic that expects
octet-stream to be classified by isLikelyTextFile in the
shouldRejectAgentBinaryRead flow.
In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts`:
- Around line 200-223: appendOutput's catch path re-appends buffered data to the
in-memory output but leaves the offloaded flag true, causing the close handler
(which prefers readLastCharsFromFile(...)) to return an empty preview on offload
failures; update the catch handler in appendOutput (and the similar block at
280-290) to reset offloaded to false (or set a fallback flag) when appendFile
fails so the close/cleanup logic will prefer the in-memory output variable
(output) and ensure outputWriteQueue chaining still resolves after the failure.
In `@src/main/presenter/filePresenter/FilePresenter.ts`:
- Around line 52-56: The readFile method currently allows absolute paths,
enabling arbitrary local reads via IPC; change readFile (and the code paths that
call it such as useChatInputFiles/filePresenter.readFile) to reject or sanitize
absolute paths by resolving the candidate path with fs.realpath or path.resolve
and ensuring the resulting path is inside this.userDataPath (or an explicit
allowlist); if the resolved path is outside the allowed base, throw an error and
do not read the file. Also remove or gate the path.isAbsolute branch so only
relative paths under userDataPath (or explicitly allowed external directories)
are permitted.
In `@src/main/presenter/skillPresenter/skillExecutionService.ts`:
- Around line 432-437: The Windows quoting in quoteForShell only escapes double
quotes and allows cmd.exe %...% env var expansion; update quoteForShell (the
branch where process.platform === 'win32') to additionally escape percent signs
by replacing every '%' with '%%' (and still escape double quotes) before
wrapping in double quotes so tokens cannot inject environment variable
references; ensure the percent-replacement occurs prior to wrapping and that
both replacements target the token parameter.
In `@src/renderer/settings/components/skills/SkillCard.vue`:
- Around line 17-20: The component defines a local computed named scripts that
conflicts with the auto-exposed scripts prop from <script setup>; rename the
local computed (for example to computedScripts or scriptsList) and update all
internal usages (e.g., the computed declaration and any references like
scripts.length in the template and script) so the template uses the prop as
intended and the local computed no longer collides with the prop binding.
In `@src/renderer/settings/components/skills/SkillEditorSheet.vue`:
- Around line 403-445: loadSkill can complete out-of-order and overwrite state
for a different open skill; guard against stale async completions by tracking a
per-call token and only mutating refs if the token still matches the latest
request. At the start of loadSkill (and before each await), capture a unique id
(e.g. incrementing loadCounter or localRequestId stored in a ref) and after each
await (especially after filePresenter.readFile and skillsStore.loadSkillRuntime)
verify that props.open is still true and props.skill?.name (or the stored token)
matches the current request id; if not, bail out without calling
hydrateRuntimeForm or mutating
editName/editDescription/editContent/editAllowedTools. Also ensure the watchers
create a new token when triggering loadSkill so late completions are ignored.
- Around line 153-210: The template removed the editable skill body, so although
loadSkill() hydrates editContent and handleSave() writes it back, nothing is
bound to editContent; restore an input bound to that ref (e.g., re-add a
<Textarea> or Markdown editor component with v-model="editContent" or
:model-value="editContent" `@update`:model-value to set editContent) in the
SkillEditorSheet template where the skill body should be edited (around the
sections referenced by lines ~153-210 and also where 403-410, 485-486 relate),
ensuring the component uses the same reactive ref name (editContent) and that
handleSave() continues to persist its value.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Line 1148: The "envWarning" key in the he-IL locale currently contains an
English disclosure; replace its value with a clear Hebrew translation conveying
that environment values are only masked in the UI and are stored as plain text
in the skill sidecar file (e.g., "הערכים מוסתרים רק בממשק המשתמש; הם נשמרים
כטקסט רגיל בקובץ ה-sidecar של הכישרון."), updating the "envWarning" entry so
Hebrew users see the same security disclosure.
In `@src/renderer/src/stores/skillsStore.ts`:
- Around line 184-187: The saveSkillExtension helper currently swallows
main-process write failures because usePresenter defaults to safeCall: true;
change the call so failures surface: either invoke
skillPresenter.saveSkillExtension with safeCall disabled (or the presenter
option that forces throws) or capture the returned value and throw an error if
it is null/undefined before calling loadSkillRuntime; update saveSkillExtension
to ensure any persistence error from skillPresenter.saveSkillExtension is
propagated (or rethrown) so the UI does not assume success.
In `@test/main/presenter/skillPresenter/skillPresenter.test.ts`:
- Around line 643-675: The test and implementation are persisting secret runtime
env values into the sidecar JSON (extension.env) which must not be written to
/.deepchat-meta/<skill>.json; update skillPresenter.saveSkillExtension to
extract and remove or mask extension.env before writing the sidecar, and instead
store the actual env values into a secure store (e.g., a keychain/encrypted app
store) via a new or existing helper (e.g., secureStore.saveSkillSecrets or
keychain.setSkillSecrets), and update skillPresenter.getSkillExtension to
rehydrate the extension by reading the sidecar and then merging in secrets
loaded from the secure store (secureStore.getSkillSecrets /
keychain.getSkillSecrets) so callers still receive the full extension object
without secrets ever being written to disk; also update tests to mock the
secure-store calls and assert the sidecar write contains no raw secrets while
secure-store.save/get are invoked with the env map.
---
Minor comments:
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 1089-1104: The new Danish translation file still contains English
labels: translate all the new keys such as "runtimeTitle", "runtimeHint",
"pythonRuntime", "nodeRuntime", "envTitle", "envWarning", "scriptsTitle",
"scriptsHint", "noScripts", "scriptEnabled", "scriptDescription",
"scriptDescriptionPlaceholder" and the nested "runtime" values ("auto",
"system", "builtin") into Danish; also locate and translate the additional
English entries for the new editor/card strings mentioned in the review (the
other added keys in the same i18n file) so the skill UI shows Danish labels
consistently.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 1143-1159: Translate the newly added English i18n keys in fa-IR by
replacing the English values with Persian translations for the keys
runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime, envTitle, envWarning,
scriptsTitle, scriptsHint, noScripts, scriptEnabled, scriptDescription,
scriptDescriptionPlaceholder and the nested runtime values runtime.auto,
runtime.system and runtime.builtin; also ensure the corresponding
editor/card-related i18n entries added elsewhere in this file are translated the
same way so the skill management UI is fully localized for Persian users.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 1143-1159: The French locale JSON block is missing translations
for the new skill editor keys (runtimeTitle, runtimeHint, pythonRuntime,
nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts,
scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the
runtime.auto/system/builtin entries); open the fr-FR settings JSON and replace
the English values with appropriate French strings for each key so
SkillEditorSheet.vue renders localized labels/hints (ensure grammar and accents
are correct and keep placeholders like "Optional description shown to the agent"
as a short French placeholder).
- Around line 1277-1281: Add French translations for the new skill card badge
keys used by SkillCard.vue: update the "card.scripts", "card.env",
"card.pythonShort", and "card.nodeShort" entries under the "card" object in the
fr-FR settings JSON so strings are localized (e.g., replace English labels with
appropriate French equivalents), ensuring keys remain identical so SkillCard.vue
picks them up.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 1143-1147: The new UI strings are still in English; update the
Hebrew localization by translating the following keys into Hebrew:
"runtimeTitle", "runtimeHint", "pythonRuntime", "nodeRuntime", "envTitle" and
also the related "skills.edit" and "skills.card" entries referenced in the
comment. Locate those keys in the settings.json and replace the English values
with their proper Hebrew equivalents, keeping punctuation and surrounding JSON
structure intact.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 1142-1159: Translate the untranslated English strings into
Japanese: replace the values for the keys runtimeTitle, runtimeHint,
pythonRuntime, nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint,
noScripts, scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and
the nested runtime.auto/system/builtin entries with appropriate Japanese text,
and also localize the corresponding skill-card badge keys referenced later in
the file (the English badge strings around the skill card section) so the
runtime editor and skill card badges are fully Japanese.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Around line 1143-1159: The new Korean locale entries for the skill runtime UI
are still in English; update the values for keys like "runtimeTitle",
"runtimeHint", "pythonRuntime", "nodeRuntime", "envTitle", "envWarning",
"scriptsTitle", "scriptsHint", "noScripts", "scriptEnabled",
"scriptDescription", "scriptDescriptionPlaceholder" and the nested "runtime"
object ("auto","system","builtin") to their proper Korean translations so the
skill editor and summary badges display fully localized text (also apply the
same translations to the similar entries around the later block referenced at
1277-1281).
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 1143-1159: The new pt-BR localization entries are still in
English; please translate the keys runtimeTitle, runtimeHint, pythonRuntime,
nodeRuntime, envTitle, envWarning, scriptsTitle, scriptsHint, noScripts,
scriptEnabled, scriptDescription, scriptDescriptionPlaceholder and the nested
runtime values (auto, system, builtin) into Brazilian Portuguese so the runtime
editor and skill card summary are fully localized; also apply the same
translation fixes to the duplicated entries referenced around lines 1277-1282.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 1277-1282: The "card" localization block contains English strings;
update the values for the keys "scripts" and "env" inside the card object to
Russian (e.g., replace "{count} scripts" with "{count} скрипт(ов)" or a proper
pluralized form and "{count} env" with "{count} окружение(й)" or similar
pluralized Russian phrasing), and confirm whether to keep "pythonShort" as "Py"
and "nodeShort" as "Node" (if not, replace with Russian-appropriate
abbreviations); ensure the keys remain "card", "scripts", "env", "pythonShort",
and "nodeShort" exactly.
- Around line 1143-1159: The listed UI strings are still in English; update the
JSON object in src/renderer/src/i18n/ru-RU/settings.json by providing Russian
translations for the keys runtimeTitle, runtimeHint, pythonRuntime, nodeRuntime,
envTitle, envWarning, scriptsTitle, scriptsHint, noScripts, scriptEnabled,
scriptDescription, scriptDescriptionPlaceholder and the nested runtime keys
runtime.auto, runtime.system, runtime.builtin; replace the English values with
appropriate Russian text (ensure proper escaping and keep the same key names and
JSON structure) so the ru-RU locale is fully localized for the skill editor.
---
Nitpick comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 1158-1191: The collectScriptDescriptors function currently
recurses without a depth limit; modify collectScriptDescriptors to accept a
depth parameter (e.g., currentDepth = 0) and stop recursion when currentDepth >=
SKILL_CONFIG.FOLDER_TREE_MAX_DEPTH (same limit used by buildFolderTree), passing
currentDepth + 1 when recursing into subdirectories; ensure callers that
initiate the traversal call the revised collectScriptDescriptors with the
default starting depth (0) so the stack overflow risk on deeply nested
directories is mitigated.
In `@src/main/presenter/skillPresenter/skillExecutionService.ts`:
- Around line 384-391: Add a brief explanatory comment above the timeout kill
block (around timeoutId/timedOut and child.kill('SIGTERM') using timeoutMs)
stating that kill errors are intentionally ignored because the child process may
have already exited, the OS may have reaped it, or signal delivery can fail due
to race conditions/permissions; mention that ignoring the error is safe here
because we only attempt best-effort termination and do not rely on kill throwing
for control flow.
In `@test/main/presenter/agentPresenter/agentToolManagerRead.test.ts`:
- Around line 157-168: The test currently only asserts
presenter.filePresenter.prepareFileCompletely was not called; also assert that
presenter's LLM fallback was not invoked by adding a negative expectation for
presenter.llmproviderPresenter.generateCompletionStandalone (i.e., ensure
generateCompletionStandalone was not called) after calling
manager.callTool('read', ...) so binary reads cannot pollute the prompt via
OCR/vision fallbacks; reference
presenter.llmproviderPresenter.generateCompletionStandalone and
presenter.filePresenter.prepareFileCompletely when adding the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ceb4a8bd-74f0-469f-9acc-4480fafb2ed3
📒 Files selected for processing (35)
docs/specs/skill-runtime-hardening/plan.mddocs/specs/skill-runtime-hardening/spec.mddocs/specs/skill-runtime-hardening/tasks.mdsrc/main/lib/binaryReadGuard.tssrc/main/presenter/agentPresenter/acp/acpFsHandler.tssrc/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/acp/backgroundExecSessionManager.tssrc/main/presenter/filePresenter/FilePresenter.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/renderer/settings/components/skills/SkillCard.vuesrc/renderer/settings/components/skills/SkillEditorSheet.vuesrc/renderer/settings/components/skills/SkillsSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/stores/skillsStore.tssrc/shared/types/skill.tstest/main/presenter/agentPresenter/agentToolManagerRead.test.tstest/main/presenter/agentPresenter/agentToolManagerSettings.test.tstest/main/presenter/llmProviderPresenter/acpFsHandler.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/skillSyncPresenter/index.test.ts
Comment on lines
+643
to
+675
| it('should save and load sidecar runtime config', async () => { | ||
| const extension = { | ||
| version: 1 as const, | ||
| env: { API_KEY: 'secret' }, | ||
| runtimePolicy: { python: 'builtin' as const, node: 'system' as const }, | ||
| scriptOverrides: { | ||
| 'scripts/run.py': { | ||
| enabled: false, | ||
| description: 'Run OCR' | ||
| } | ||
| } | ||
| } | ||
|
|
||
| await skillPresenter.saveSkillExtension('test-skill', extension) | ||
| ;(fs.existsSync as Mock).mockImplementation( | ||
| (target: string) => | ||
| !target.includes('/scripts') || target.endsWith('/.deepchat-meta/test-skill.json') | ||
| ) | ||
| ;(fs.readFileSync as Mock).mockImplementation((target: string) => { | ||
| if (target.endsWith('/.deepchat-meta/test-skill.json')) { | ||
| return JSON.stringify(extension) | ||
| } | ||
| return 'test' | ||
| }) | ||
|
|
||
| const loaded = await skillPresenter.getSkillExtension('test-skill') | ||
|
|
||
| expect(fs.writeFileSync).toHaveBeenCalledWith( | ||
| expect.stringContaining('/.deepchat-meta/test-skill.json'), | ||
| JSON.stringify(extension, null, 2), | ||
| 'utf-8' | ||
| ) | ||
| expect(loaded).toEqual(extension) |
Contributor
There was a problem hiding this comment.
Don’t persist skill env secrets as raw sidecar JSON.
These lines codify writing extension.env directly into /.deepchat-meta/<skill>.json. In practice those values will include API keys, and skills are shareable/exportable, so this turns secret material into easy-to-leak plaintext on disk, in backups, and in synced folders. Keep non-secret runtime metadata in the sidecar, but move secret values to OS keychain or another encrypted app-local store.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/main/presenter/skillPresenter/skillPresenter.test.ts` around lines 643 -
675, The test and implementation are persisting secret runtime env values into
the sidecar JSON (extension.env) which must not be written to
/.deepchat-meta/<skill>.json; update skillPresenter.saveSkillExtension to
extract and remove or mask extension.env before writing the sidecar, and instead
store the actual env values into a secure store (e.g., a keychain/encrypted app
store) via a new or existing helper (e.g., secureStore.saveSkillSecrets or
keychain.setSkillSecrets), and update skillPresenter.getSkillExtension to
rehydrate the extension by reading the sidecar and then merging in secrets
loaded from the secure store (secureStore.getSkillSecrets /
keychain.getSkillSecrets) so callers still receive the full extension object
without secrets ever being written to disk; also update tests to mock the
secure-store calls and assert the sidecar write contains no raw secrets while
secure-store.save/get are invoked with the env map.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts (1)
201-215:⚠️ Potential issue | 🟠 MajorDrain pending offload writes before serving file-backed reads.
Line 646 returns immediately for running sessions, but offloaded chunks are appended asynchronously after
outputBufferhas already been cleared. That letspoll()/log()read the file before the newest chunk lands on disk, so callers can get stale or truncated output while the process is still running.💡 Suggested fix
- await this.waitForSessionDrain(session) - const config = getConfig() const isOffloaded = !session.offloadFailed && session.outputFilePath !== null && session.totalOutputLength > config.offloadThresholdChars + await this.waitForSessionDrain(session, isOffloaded)- await this.waitForSessionDrain(session) - const config = getConfig() const isOffloaded = !session.offloadFailed && session.outputFilePath !== null && session.totalOutputLength > config.offloadThresholdChars + await this.waitForSessionDrain(session, isOffloaded)- private async waitForSessionDrain(session: BackgroundSession): Promise<void> { - if (session.status === 'running') { - return - } - - await session.closePromise + private async waitForSessionDrain( + session: BackgroundSession, + waitForPendingWrites = false + ): Promise<void> { + if (session.status !== 'running') { + await session.closePromise + return + } + + if (waitForPendingWrites) { + await session.outputWriteQueue.catch((error) => { + logger.warn('[BackgroundExec] Failed while draining output queue before read:', error) + }) + } }Also applies to: 237-255, 645-651
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts` around lines 201 - 215, The offloaded-file read can happen before pending async offload writes finish, causing stale/truncated output; ensure you await session drain right before any file-backed reads. Specifically, in poll() (and the other locations noted) call await this.waitForSessionDrain(session) immediately before invoking readLastCharsFromFile(session.outputFilePath, ...) or any file reads (and before returning offloaded output), so that outputBuffer is flushed to disk first; update the branches that check isOffloaded and any early-return paths in poll()/log() to perform this await when session.outputFilePath is present.
♻️ Duplicate comments (1)
src/main/presenter/skillPresenter/index.ts (1)
866-878:⚠️ Potential issue | 🟠 MajorDon't persist skill env secrets in plaintext sidecars.
extension.envis written directly into/.deepchat-meta/<skill>.json. These values are likely API keys, and the sidecar lives next to shareable/syncable skills, so backups, exports, and folder syncs will leak them. Keep non-secret runtime metadata in the sidecar, but move env secrets to a secure app-local/keychain store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/skillPresenter/index.ts` around lines 866 - 878, The saveSkillExtension method currently writes extension.env secrets into the sidecar JSON; instead, strip sensitive env entries before persisting by using sanitizeSkillExtensionConfig to produce a non-secret payload for fs.writeFileSync(this.getSidecarPath(name), ...), and move the removed secret values into a secure app-local/keychain store via a new or existing credential API (e.g., setSkillSecret(name, key, value) — create if missing). Also update delete/update flows (contentCache.delete(name) and any discoverSkills/load paths) to read secrets back from the secure store when constructing the runtime config and to avoid writing secrets back into the sidecar.
🧹 Nitpick comments (3)
test/main/presenter/FilePresenter.readFile.test.ts (2)
55-82: Consider adding edge-case tests for more comprehensive coverage.The current tests cover the main scenarios well. Consider adding tests for:
- Empty string input (should throw "File path is required")
- Whitespace-only input (should throw "File path is required")
- Path with embedded null bytes or control characters
- Realpath fallback behavior when file doesn't exist
📝 Suggested additional tests
it('rejects empty paths', async () => { const presenter = new FilePresenter(mockConfigPresenter) await expect(presenter.readFile('')).rejects.toThrow('File path is required') expect(fs.readFile).not.toHaveBeenCalled() }) it('rejects whitespace-only paths', async () => { const presenter = new FilePresenter(mockConfigPresenter) await expect(presenter.readFile(' ')).rejects.toThrow('File path is required') expect(fs.readFile).not.toHaveBeenCalled() }) it('handles realpath failure gracefully for non-existent files inside userDataPath', async () => { vi.mocked(fs.realpath).mockImplementation(async (targetPath: string) => { if (targetPath === '/mock/user/data') { return '/mock/user/data' } // Simulate ENOENT for non-existent file throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) }) vi.mocked(fs.readFile).mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) const presenter = new FilePresenter(mockConfigPresenter) // Should attempt to read, path validation should pass, but read fails await expect(presenter.readFile('nonexistent.txt')).rejects.toThrow('ENOENT') expect(fs.readFile).toHaveBeenCalledWith('/mock/user/data/nonexistent.txt', 'utf-8') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/FilePresenter.readFile.test.ts` around lines 55 - 82, Add edge-case unit tests for FilePresenter.readFile: (1) verify it rejects an empty string and a whitespace-only string by throwing "File path is required" and not calling fs.readFile (create two tests that instantiate FilePresenter with mockConfigPresenter and call readFile('') and readFile(' ')); (2) add a test that passes a path containing embedded null bytes or control characters and assert it rejects (ensure fs.readFile is not called); (3) add a test for realpath/read fallback behavior by mocking fs.realpath to return the userDataPath for parent dir but throw ENOENT for the target file, mock fs.readFile to reject with ENOENT, then call presenter.readFile('nonexistent.txt') and assert it propagates the ENOENT error and that fs.readFile was called with the resolved '/mock/user/data/nonexistent.txt'. Use vi.mocked for fs.realpath and fs.readFile to simulate the errors and responses.
41-52: Add a comment clarifying the symlink escape simulation.The mock for
/mock/user/outside.txtreturning/mock/outside.txtsimulates a symlink pointing outside the user data directory, which may not be immediately obvious to readers.📝 Suggested clarification
beforeEach(() => { vi.clearAllMocks() vi.mocked(fs.realpath).mockImplementation(async (targetPath: string) => { if (targetPath === '/mock/user/data') { return '/mock/user/data' } if (targetPath === '/mock/user/data/notes/today.md') { return '/mock/user/data/notes/today.md' } + // Simulates a symlink inside userDataPath that resolves outside it if (targetPath === '/mock/user/outside.txt') { return '/mock/outside.txt' } return targetPath }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/FilePresenter.readFile.test.ts` around lines 41 - 52, Add an inline comment above the vi.mocked(fs.realpath).mockImplementation block explaining that the branch where targetPath === '/mock/user/outside.txt' returns '/mock/outside.txt' is intentionally simulating a symlink that resolves outside the user data directory; reference the mocked function name fs.realpath and the specific paths '/mock/user/outside.txt' and '/mock/outside.txt' so future readers understand this escape-case test scenario.src/main/presenter/agentPresenter/acp/agentBashHandler.ts (1)
323-352: Consider async file operations for consistency.The method uses synchronous file operations (
statSync,openSync,readSync,closeSync) within an async close handler. While the read size is bounded (~48KB max), async operations would be more consistent with the rest of the codebase and avoid blocking the event loop.This is a minor point given the bounded read size.
♻️ Optional async refactor
- private readLastCharsFromFile(filePath: string, maxChars: number): string { + private async readLastCharsFromFile(filePath: string, maxChars: number): Promise<string> { try { - const stats = fs.statSync(filePath) + const stats = await fs.promises.stat(filePath) const fileSize = stats.size const bytesToRead = Math.min(maxChars * 4, fileSize) const startPosition = Math.max(0, fileSize - bytesToRead) - const fd = fs.openSync(filePath, 'r') + const handle = await fs.promises.open(filePath, 'r') try { const buffer = Buffer.alloc(bytesToRead) - const bytesRead = fs.readSync(fd, buffer, 0, bytesToRead, startPosition) + const { bytesRead } = await handle.read(buffer, 0, bytesToRead, startPosition) // ... rest of logic } finally { - fs.closeSync(fd) + await handle.close() }Note: This would require updating line 283 to
await this.readLastCharsFromFile(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts` around lines 323 - 352, The readLastCharsFromFile function currently uses synchronous fs calls and should be converted to an async implementation: change signature to async readLastCharsFromFile(filePath: string, maxChars: number): Promise<string>, use fs.promises.stat to get size, use fs.promises.open to obtain a FileHandle, allocate a Buffer, call handle.read(buffer, 0, bytesToRead, startPosition) to get bytesRead, and finally call handle.close() in a try/finally; preserve the same logic to trim to the first newline and return '' on errors but throw/catch as needed and update any callers (e.g., the code that calls readLastCharsFromFile) to await the result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts`:
- Around line 191-194: The offload logic currently uses session.offloadFailed to
determine isOffloaded, which hides already-persisted file data when
offloadFailed is set; change the condition so existing persisted output remains
readable: compute offloaded based on session.outputFilePath !== null &&
session.totalOutputLength > getConfig().offloadThresholdChars (ignore
offloadFailed for read-time decisions), and introduce a separate flag (e.g.,
offloadDisabled or offloadAttemptFailed) to prevent future offload attempts
without affecting reads. Update all isOffloaded checks (used by poll() and
log()) to use the new read-safe predicate and ensure the code that flips
session.offloadFailed (the offload attempt handler) only sets the new “disable
future offloads” flag while leaving outputFilePath-driven reads intact or
migrating file contents back into memory before flipping read behavior.
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 372-377: The buildRuntimeInstructions method currently injects an
absolute path via metadata.skillRoot into the runtime prompt, which leaks local
paths; update buildRuntimeInstructions to avoid embedding metadata.skillRoot
directly (in function buildRuntimeInstructions and where loadSkillContent uses
its output) — replace the literal absolute path with a generic placeholder like
"<skill_root>" or a recommended relative base_directory, or remove the path line
entirely and document that the actual path should be resolved server-side by
skill_run (refer to deepchatAgentPresenter and loadSkillContent to ensure the
system prompt no longer contains metadata.skillRoot).
- Around line 768-779: readSkillFile currently sync-reads the skill file without
enforcing the SKILL_FILE_MAX_SIZE guard, allowing a large SKILL.md to be
returned over IPC; update readSkillFile (and the path where it uses
metadataCache and metadata.path) to perform the same size check as
loadSkillContent: stat the file first (or use fs.promises.stat) and throw an
error if size > SKILL_FILE_MAX_SIZE, then read the file (prefer async readFile)
and return its contents, matching loadSkillContent's behavior and error
messages.
In `@src/main/presenter/skillPresenter/skillExecutionService.ts`:
- Around line 339-355: When createForegroundOutputPath() returns null the code
currently sets offloaded and then buffers everything into outputBuffer or
re-adds data on write failure; change queueOutputWrite and appendOutput logic so
that we never leave offloaded=true when outputFilePath is falsy or when a write
fails: ensure queueOutputWrite checks outputFilePath before treating data as
offloaded and on any fs.promises.appendFile error it sets offloaded=false
(instead of just pushing data back into outputBuffer) so future appends follow
the non-offload path; also ensure appendOutput respects the offloaded flag and
applies any existing buffer-size caps or rotation policy rather than allowing
unbounded growth.
- Around line 384-391: The current timeout handler sets timedOut and sends
SIGTERM but still waits indefinitely for the child's 'close', so the timeout
isn't hard; update the handler around timeoutId/timeoutMs to start a short
hard-kill escalation after a grace period (e.g., graceMs) if the child ignores
SIGTERM: on initial timeout call child.kill('SIGTERM'), start a second timer
that after graceMs calls child.kill('SIGKILL') (or force-kills), clears
listeners and ensures any pending promise/await for the child's 'close' is
rejected/resolved so the function doesn't hang; reference timeoutId, timedOut,
child.kill(...) and the 'close' wait logic to implement the escalation and
cleanup.
In `@src/renderer/settings/components/skills/SkillEditorSheet.vue`:
- Around line 526-546: The current two-step save (calling
skillsStore.updateSkillFile(...) then skillsStore.saveSkillExtension(...)) can
leave partial state if the second call fails; replace these with a single atomic
presenter call (e.g., implement and call a new presenter method like
skillsStore.saveSkillWithExtension(skillName, skillContent, extension)) that
runs in the main process and performs both writes together, or if you prefer a
quicker patch implement rollback: read and stash the previous SKILL.md before
calling updateSkillFile, then if saveSkillExtension(...) throws, restore the
previous SKILL.md by calling skillsStore.updateSkillFile(skillName,
previousContent) and propagate the error; include buildSkillContent, buildEnv,
buildScriptOverrides and runtime values (pythonRuntime.value, nodeRuntime.value)
when building the combined payload for the new presenter.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 1143-1158: The review found untranslated English labels for the
skill-runtime UI in the he-IL locale; update the he-IL JSON to provide Hebrew
translations for the new keys shown (e.g., "runtimeTitle", "runtimeHint",
"pythonRuntime", "nodeRuntime", "envTitle", "envWarning", "scriptsTitle",
"scriptsHint", "noScripts", "scriptEnabled", "scriptDescription",
"scriptDescriptionPlaceholder", and the nested
"runtime.auto"/"runtime.system"/"runtime.builtin"), and also translate the newly
added "skills.edit" and "skills.card" entries referenced around lines 1278-1279
so the UI is fully localized; replace the English values with appropriate Hebrew
strings while keeping the same JSON keys.
---
Outside diff comments:
In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts`:
- Around line 201-215: The offloaded-file read can happen before pending async
offload writes finish, causing stale/truncated output; ensure you await session
drain right before any file-backed reads. Specifically, in poll() (and the other
locations noted) call await this.waitForSessionDrain(session) immediately before
invoking readLastCharsFromFile(session.outputFilePath, ...) or any file reads
(and before returning offloaded output), so that outputBuffer is flushed to disk
first; update the branches that check isOffloaded and any early-return paths in
poll()/log() to perform this await when session.outputFilePath is present.
---
Duplicate comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 866-878: The saveSkillExtension method currently writes
extension.env secrets into the sidecar JSON; instead, strip sensitive env
entries before persisting by using sanitizeSkillExtensionConfig to produce a
non-secret payload for fs.writeFileSync(this.getSidecarPath(name), ...), and
move the removed secret values into a secure app-local/keychain store via a new
or existing credential API (e.g., setSkillSecret(name, key, value) — create if
missing). Also update delete/update flows (contentCache.delete(name) and any
discoverSkills/load paths) to read secrets back from the secure store when
constructing the runtime config and to avoid writing secrets back into the
sidecar.
---
Nitpick comments:
In `@src/main/presenter/agentPresenter/acp/agentBashHandler.ts`:
- Around line 323-352: The readLastCharsFromFile function currently uses
synchronous fs calls and should be converted to an async implementation: change
signature to async readLastCharsFromFile(filePath: string, maxChars: number):
Promise<string>, use fs.promises.stat to get size, use fs.promises.open to
obtain a FileHandle, allocate a Buffer, call handle.read(buffer, 0, bytesToRead,
startPosition) to get bytesRead, and finally call handle.close() in a
try/finally; preserve the same logic to trim to the first newline and return ''
on errors but throw/catch as needed and update any callers (e.g., the code that
calls readLastCharsFromFile) to await the result.
In `@test/main/presenter/FilePresenter.readFile.test.ts`:
- Around line 55-82: Add edge-case unit tests for FilePresenter.readFile: (1)
verify it rejects an empty string and a whitespace-only string by throwing "File
path is required" and not calling fs.readFile (create two tests that instantiate
FilePresenter with mockConfigPresenter and call readFile('') and readFile('
')); (2) add a test that passes a path containing embedded null bytes or control
characters and assert it rejects (ensure fs.readFile is not called); (3) add a
test for realpath/read fallback behavior by mocking fs.realpath to return the
userDataPath for parent dir but throw ENOENT for the target file, mock
fs.readFile to reject with ENOENT, then call
presenter.readFile('nonexistent.txt') and assert it propagates the ENOENT error
and that fs.readFile was called with the resolved
'/mock/user/data/nonexistent.txt'. Use vi.mocked for fs.realpath and fs.readFile
to simulate the errors and responses.
- Around line 41-52: Add an inline comment above the
vi.mocked(fs.realpath).mockImplementation block explaining that the branch where
targetPath === '/mock/user/outside.txt' returns '/mock/outside.txt' is
intentionally simulating a symlink that resolves outside the user data
directory; reference the mocked function name fs.realpath and the specific paths
'/mock/user/outside.txt' and '/mock/outside.txt' so future readers understand
this escape-case test scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 255c50f8-55c3-45cb-9315-5ca933acd73a
📒 Files selected for processing (17)
src/main/lib/binaryReadGuard.tssrc/main/presenter/agentPresenter/acp/agentBashHandler.tssrc/main/presenter/agentPresenter/acp/backgroundExecSessionManager.tssrc/main/presenter/filePresenter/FilePresenter.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/renderer/settings/components/skills/SkillCard.vuesrc/renderer/settings/components/skills/SkillEditorSheet.vuesrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/stores/skillsStore.tssrc/shared/types/skill.tstest/main/lib/binaryReadGuard.test.tstest/main/presenter/FilePresenter.readFile.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/skillSyncPresenter/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/main/presenter/skillSyncPresenter/index.test.ts
- src/shared/types/skill.ts
- test/main/presenter/skillPresenter/skillExecutionService.test.ts
- src/main/lib/binaryReadGuard.ts
src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/presenter/skillPresenter/index.ts (1)
1239-1272: Consider adding a depth limit tocollectScriptDescriptorsto match folder tree traversal.This method recursively traverses the
scripts/directory without a depth limit. While skills are typically shallow, a maliciously crafted skill with deep nesting could cause stack overflow. The folder tree traversal usesFOLDER_TREE_MAX_DEPTHfor protection.🛡️ Optional fix to add depth limiting
private collectScriptDescriptors( currentDir: string, skillRoot: string, - acc: SkillScriptDescriptor[] = [] + acc: SkillScriptDescriptor[] = [], + depth: number = 0 ): SkillScriptDescriptor[] { + if (depth >= SKILL_CONFIG.FOLDER_TREE_MAX_DEPTH) { + return acc + } + const entries = fs.readdirSync(currentDir, { withFileTypes: true }) for (const entry of entries) { if (entry.isSymbolicLink()) { continue } const fullPath = path.join(currentDir, entry.name) if (entry.isDirectory()) { - this.collectScriptDescriptors(fullPath, skillRoot, acc) + this.collectScriptDescriptors(fullPath, skillRoot, acc, depth + 1) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/skillPresenter/index.ts` around lines 1239 - 1272, The recursive collectScriptDescriptors currently traverses directories without checking depth, risking stack overflow on deeply nested folders; update collectScriptDescriptors to accept a depth parameter (e.g., depth = 0) and on each recursive call pass depth + 1, and before recursing check against FOLDER_TREE_MAX_DEPTH to skip entering directories when depth >= FOLDER_TREE_MAX_DEPTH; reference the collectScriptDescriptors function and ensure all calls (initial invocation and the recursive call inside the entry.isDirectory() branch) are updated to supply the initial depth so traversal is bounded.src/renderer/src/stores/skillsStore.ts (1)
63-86: Bulk load replaces entire state—stale entries for removed skills won't be cleared incrementally.
loadSkillRuntimeDatabuilds new objects only for the provideditemsand assigns them wholesale. This is correct when called with a fresh skill list, but ifitemsis a subset, extensions/scripts for skills not initemswill be lost. Currently this is only called fromloadSkillswith the full list, so it's safe, but consider documenting this invariant or merging instead of replacing if partial refreshes are ever needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/skillsStore.ts` around lines 63 - 86, loadSkillRuntimeData currently replaces skillExtensions and skillScripts entirely with entries only for the provided items, which will drop data for skills not included in a partial refresh; update loadSkillRuntimeData so it merges into existing skillExtensions and skillScripts instead of overwriting them (e.g., start from copies of skillExtensions.value and skillScripts.value, populate/override only entries for each skill in items using createDefaultSkillExtension() when needed, then assign the merged objects back to skillExtensions.value and skillScripts.value), or add a clear comment documenting that callers must always pass the full skill list; refer to loadSkillRuntimeData, skillExtensions, skillScripts, createDefaultSkillExtension, skillPresenter.getSkillExtension and skillPresenter.listSkillScripts to locate the logic to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/skillPresenter/index.ts`:
- Around line 1239-1272: The recursive collectScriptDescriptors currently
traverses directories without checking depth, risking stack overflow on deeply
nested folders; update collectScriptDescriptors to accept a depth parameter
(e.g., depth = 0) and on each recursive call pass depth + 1, and before
recursing check against FOLDER_TREE_MAX_DEPTH to skip entering directories when
depth >= FOLDER_TREE_MAX_DEPTH; reference the collectScriptDescriptors function
and ensure all calls (initial invocation and the recursive call inside the
entry.isDirectory() branch) are updated to supply the initial depth so traversal
is bounded.
In `@src/renderer/src/stores/skillsStore.ts`:
- Around line 63-86: loadSkillRuntimeData currently replaces skillExtensions and
skillScripts entirely with entries only for the provided items, which will drop
data for skills not included in a partial refresh; update loadSkillRuntimeData
so it merges into existing skillExtensions and skillScripts instead of
overwriting them (e.g., start from copies of skillExtensions.value and
skillScripts.value, populate/override only entries for each skill in items using
createDefaultSkillExtension() when needed, then assign the merged objects back
to skillExtensions.value and skillScripts.value), or add a clear comment
documenting that callers must always pass the full skill list; refer to
loadSkillRuntimeData, skillExtensions, skillScripts,
createDefaultSkillExtension, skillPresenter.getSkillExtension and
skillPresenter.listSkillScripts to locate the logic to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d2de7dc-1a16-4ec3-83e6-9f4b9b437831
📒 Files selected for processing (12)
src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.tssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/skillPresenter/skillExecutionService.tssrc/renderer/settings/components/skills/SkillEditorSheet.vuesrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/stores/skillsStore.tssrc/shared/types/skill.tstest/main/presenter/agentPresenter/acp/backgroundExecSessionManager.test.tstest/main/presenter/skillPresenter/skillExecutionService.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/skillPresenter/skillTools.test.tstest/main/presenter/skillSyncPresenter/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/types/skill.ts
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.
fix #1342 #1327
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests