Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/specs/skill-runtime-hardening/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Add runtime-aware skill extensions without changing external `SKILL.md` formats.
- Acceptance:
- Only scripts under `<skillRoot>/scripts/` can be executed.
- The runtime picks system `uv`/`node` first, then falls back to DeepChat bundled runtimes.
- Python scripts run from the skill root and honor `pyproject.toml` via `uv run --project`.
- Skill scripts execute from the current session workdir when available, while Python still honors `pyproject.toml` via `uv run --project <skillRoot>`.

### US-3: Prevent prompt pollution from binary file reads
- As an agent, reading an image or binary file through text-file APIs does not inject raw bytes into prompt context.
Expand All @@ -28,7 +28,7 @@ Add runtime-aware skill extensions without changing external `SKILL.md` formats.
### US-4: Guide the model toward stable skill execution
- As an agent, active skill instructions clearly include absolute paths, script inventory, and the preferred execution tool.
- Acceptance:
- Active skill prompt includes `skillRoot`, recommended `base_directory`, runnable scripts, and explicit guardrails against inline `python -c` / `node -e`.
- Active skill prompt includes `skillRoot`, skill root env vars, recommended `base_directory`, runnable scripts, and explicit guardrails against inline `python -c` / `node -e`.

### US-5: Keep process output after process exit
- As an agent, command output is still available when a child process writes a large payload right before exiting.
Expand Down
24 changes: 23 additions & 1 deletion src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,25 @@ export class AcpProcessManager implements AgentProcessManager<AcpProcessHandle,
return handler
}

private resolveTerminalCwd(sessionId: string, requestedCwd?: string | null): string {
const explicitCwd = requestedCwd?.trim()
if (explicitCwd) {
return explicitCwd
}

const sessionWorkdir = this.sessionWorkdirs.get(sessionId)?.trim()
if (sessionWorkdir) {
return sessionWorkdir
}

const fallbackWorkdir = this.getFallbackWorkdir()
const conversationId = this.sessionConversations.get(sessionId)
console.warn(
`[ACP] Missing session workdir for terminal session ${sessionId}${conversationId ? ` (conversation ${conversationId})` : ''}, using fallback workdir: ${fallbackWorkdir}`
)
return fallbackWorkdir
}

/**
* Provide a fallback workspace for sessions that haven't registered a workdir.
* Keeps file access constrained to a temp directory rather than the entire filesystem.
Expand Down Expand Up @@ -954,7 +973,10 @@ export class AcpProcessManager implements AgentProcessManager<AcpProcessHandle,
},
// Terminal operations
createTerminal: async (params) => {
return this.terminalManager.createTerminal(params)
return this.terminalManager.createTerminal({
...params,
cwd: this.resolveTerminalCwd(params.sessionId, params.cwd)
})
},
terminalOutput: async (params) => {
return this.terminalManager.terminalOutput(params)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import fs from 'fs'
import path from 'path'
import { spawn } from 'node-pty'
import type { IPty } from 'node-pty'
import { nanoid } from 'nanoid'
import { app } from 'electron'
import { RequestError } from '@agentclientprotocol/sdk'
import type * as schema from '@agentclientprotocol/sdk/dist/schema/index.js'

Expand Down Expand Up @@ -34,6 +37,27 @@ export class AcpTerminalManager {
private readonly terminals = new Map<string, TerminalState>()
private readonly defaultMaxOutputBytes = 1024 * 1024 // 1MB default

private resolveTerminalCwd(cwd?: string | null): string {
const normalized = cwd?.trim()
if (normalized) {
return path.resolve(normalized)
}

const fallbackDir = path.join(app.getPath('temp'), 'deepchat-acp', 'terminals')
try {
fs.mkdirSync(fallbackDir, { recursive: true })
console.warn(`[ACP Terminal] Missing cwd, using fallback directory: ${fallbackDir}`)
return fallbackDir
} catch (error) {
const tempDir = app.getPath('temp')
console.warn(
`[ACP Terminal] Failed to create fallback directory, using temp path instead: ${tempDir}`,
error
)
return tempDir
}
}

/**
* Create a new terminal to execute a command.
*/
Expand All @@ -42,6 +66,7 @@ export class AcpTerminalManager {
): Promise<schema.CreateTerminalResponse> {
const id = `term_${nanoid(12)}`
const maxOutputBytes = params.outputByteLimit ?? this.defaultMaxOutputBytes
const cwd = this.resolveTerminalCwd(params.cwd)

let exitResolve!: (status: { exitCode?: number | null; signal?: string | null }) => void
const exitPromise = new Promise<{ exitCode?: number | null; signal?: string | null }>(
Expand Down Expand Up @@ -75,7 +100,7 @@ export class AcpTerminalManager {
name: 'xterm-256color',
cols: 120,
rows: 30,
cwd: params.cwd ?? process.cwd(),
cwd,
env
})

Expand Down
118 changes: 108 additions & 10 deletions src/main/presenter/newAgentPresenter/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,12 @@ export class NewAgentPresenter {
if (input.generationSettings) {
initConfig.generationSettings = input.generationSettings
}
await agent.initSession(sessionId, initConfig)
try {
await this.initializeSessionRuntime(agent, sessionId, initConfig)
} catch (error) {
await this.cleanupFailedSessionInitialization(agent, sessionId)
throw error
}
console.log(`[NewAgentPresenter] agent.initSession done`)

// Bind to window and emit activated
Expand Down Expand Up @@ -224,13 +229,18 @@ export class NewAgentPresenter {
const sessionId = this.sessionManager.create(agentId, 'New Chat', projectDir, {
isDraft: true
})
await this.ensureSessionRuntimeInitialized(agent, sessionId, {
agentId,
providerId: 'acp',
modelId: agentId,
projectDir,
permissionMode
})
try {
await this.ensureSessionRuntimeInitialized(agent, sessionId, {
agentId,
providerId: 'acp',
modelId: agentId,
projectDir,
permissionMode
})
} catch (error) {
await this.cleanupFailedSessionInitialization(agent, sessionId)
throw error
}
record = this.sessionManager.get(sessionId)
if (!record) {
throw new Error(`Failed to read created ACP draft session: ${sessionId}`)
Expand Down Expand Up @@ -280,6 +290,12 @@ export class NewAgentPresenter {
}
}
this.assertAcpSessionHasWorkdir(providerId, session.projectDir ?? null)
await this.syncAcpSessionWorkdir(
providerId,
sessionId,
session.agentId,
session.projectDir ?? null
)
if (agent.queuePendingInput) {
await agent.queuePendingInput(sessionId, normalizedInput)
return
Expand Down Expand Up @@ -329,6 +345,12 @@ export class NewAgentPresenter {
}
}
this.assertAcpSessionHasWorkdir(providerId, currentSession.projectDir ?? null)
await this.syncAcpSessionWorkdir(
providerId,
sessionId,
currentSession.agentId,
currentSession.projectDir ?? null
)
return await agent.queuePendingInput(sessionId, normalizedInput)
}

Expand Down Expand Up @@ -465,7 +487,7 @@ export class NewAgentPresenter {
)

try {
await agent.initSession(targetSessionId, {
await this.initializeSessionRuntime(agent, targetSessionId, {
agentId: sourceSession.agentId,
providerId: sourceState.providerId,
modelId: sourceState.modelId,
Expand Down Expand Up @@ -1296,7 +1318,7 @@ export class NewAgentPresenter {
): Promise<void> {
const state = await agent.getSessionState(sessionId)
if (!state) {
await agent.initSession(sessionId, config)
await this.initializeSessionRuntime(agent, sessionId, config)
return
}

Expand All @@ -1307,6 +1329,82 @@ export class NewAgentPresenter {
) {
await agent.setPermissionMode(sessionId, config.permissionMode)
}

await this.syncAcpSessionWorkdir(
config.providerId,
sessionId,
config.agentId ?? config.modelId,
config.projectDir
)
}

private async initializeSessionRuntime(
agent: IAgentImplementation,
sessionId: string,
config: {
agentId?: string
providerId: string
modelId: string
projectDir?: string | null
permissionMode: PermissionMode
generationSettings?: Partial<SessionGenerationSettings>
}
): Promise<void> {
await agent.initSession(sessionId, config)
await this.syncAcpSessionWorkdir(
config.providerId,
sessionId,
config.agentId ?? config.modelId,
config.projectDir ?? null
)
}

private async syncAcpSessionWorkdir(
providerId: string,
conversationId: string,
agentId: string,
projectDir?: string | null
): Promise<void> {
if (providerId !== 'acp') {
return
}

const normalizedProjectDir = projectDir?.trim()
if (!normalizedProjectDir) {
return
}

try {
await this.llmProviderPresenter.setAcpWorkdir(
conversationId,
resolveAcpAgentAlias(agentId),
normalizedProjectDir
)
} catch (error) {
console.warn('[NewAgentPresenter] Failed to sync ACP workdir for session:', {
conversationId,
agentId,
projectDir: normalizedProjectDir,
Comment on lines +1383 to +1387

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fail the send when ACP workdir sync does not persist

This helper is awaited from createSession(), sendMessage(), and queuePendingInput() specifically to make the ACP workdir authoritative before the first turn runs, but the new catch converts every persistence/update failure into a warning. If setAcpWorkdir() fails (for example on a transient SQLite write error), acpProvider.coreStream() later reads the old record and AcpSessionPersistence.getWorkdir() falls back to the default home directory, so the agent's first command executes in the wrong workspace with no visible error. That makes the regression very hard to diagnose for affected users.

Useful? React with 👍 / 👎.

error
})
throw error
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

private async cleanupFailedSessionInitialization(
agent: IAgentImplementation,
sessionId: string
): Promise<void> {
try {
await agent.destroySession(sessionId)
} catch (cleanupError) {
console.warn(
`[NewAgentPresenter] Failed to cleanup session runtime after initialization error ${sessionId}:`,
cleanupError
)
}

this.sessionManager.delete(sessionId)
}

private buildExportConversation(
Expand Down
3 changes: 2 additions & 1 deletion src/main/presenter/skillPresenter/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,8 @@ export class SkillPresenter implements ISkillPresenter {
const scripts = (await this.listSkillScripts(metadata.name)).filter((script) => script.enabled)
const lines = [
'## DeepChat Runtime Context',
'- Skill root: resolved server-side by `skill_run`.',
'- Skill root: available via `SKILL_ROOT` and `DEEPCHAT_SKILL_ROOT`.',
'- `skill_run` executes from the current session workdir when available.',
'- Recommended base_directory: `<skill_root>`'
]

Expand Down
67 changes: 64 additions & 3 deletions src/main/presenter/skillPresenter/skillExecutionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ export interface SkillRunOptions {
conversationId: string
}

interface SkillExecutionServiceOptions {
resolveConversationWorkdir?: (conversationId: string) => Promise<string | null>
}

interface SkillExecutionResult {
output: string | { status: 'running'; sessionId: string }
rtkApplied: boolean
Expand Down Expand Up @@ -66,12 +70,15 @@ function toStringEnv(input: NodeJS.ProcessEnv | Record<string, string>): Record<
export class SkillExecutionService {
private readonly runtimeHelper = RuntimeHelper.getInstance()
private readonly configPresenter?: Pick<IConfigPresenter, 'getSetting'>
private readonly resolveConversationWorkdir?: (conversationId: string) => Promise<string | null>

constructor(
private readonly skillPresenter: ISkillPresenter,
configPresenter: IConfigPresenter
configPresenter: IConfigPresenter,
options: SkillExecutionServiceOptions = {}
) {
this.configPresenter = configPresenter
this.resolveConversationWorkdir = options.resolveConversationWorkdir
this.runtimeHelper.initializeRuntimes()
}

Expand Down Expand Up @@ -136,10 +143,13 @@ export class SkillExecutionService {

const extension = await this.skillPresenter.getSkillExtension(input.skill)
const shellEnv = await getShellEnvironment()
const executionCwd = await this.resolveExecutionCwd(conversationId, metadata.skillRoot)
const mergedEnv = {
...toStringEnv(process.env),
...shellEnv,
...extension.env
...extension.env,
SKILL_ROOT: metadata.skillRoot,
DEEPCHAT_SKILL_ROOT: metadata.skillRoot
}

const runtime = await this.resolveRuntimeCommand(
Expand All @@ -153,14 +163,65 @@ export class SkillExecutionService {
return {
command: runtime.command,
args,
cwd: metadata.skillRoot,
cwd: executionCwd,
env: mergedEnv,
shellCommand: this.buildShellCommand(runtime.command, args),
outputPrefix: `skillrun_${input.skill.replace(/[^a-zA-Z0-9_-]/g, '_')}`,
spawnMode: 'direct'
}
}

private async resolveExecutionCwd(conversationId: string, skillRoot: string): Promise<string> {
const normalizedSkillRoot = path.resolve(skillRoot)
if (!this.resolveConversationWorkdir) {
return normalizedSkillRoot
}

try {
const resolvedWorkdir = await this.resolveConversationWorkdir(conversationId)
const normalizedWorkdir = resolvedWorkdir?.trim()
if (normalizedWorkdir) {
const resolvedPath = path.resolve(normalizedWorkdir)
try {
const stat = await fs.promises.stat(resolvedPath)
if (stat.isDirectory()) {
return resolvedPath
}
logger.warn(
'[SkillExecutionService] Conversation workdir is not a directory, falling back to skill root',
{
conversationId,
invalidWorkdir: resolvedPath
}
)
} catch (error) {
logger.warn(
'[SkillExecutionService] Conversation workdir is invalid, falling back to skill root',
{
conversationId,
invalidWorkdir: resolvedPath,
error
}
)
}
}
} catch (error) {
logger.warn('[SkillExecutionService] Failed to resolve conversation workdir', {
conversationId,
error
})
}

logger.warn(
'[SkillExecutionService] Missing conversation workdir, falling back to skill root',
{
conversationId,
skillRoot: normalizedSkillRoot
}
)
return normalizedSkillRoot
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

private resolveRequestedScript(
requestedScript: string,
scripts: SkillScriptDescriptor[]
Expand Down
Loading
Loading