feat(dsh): integrate DeepSeek Harness through ACP - #1632
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Blocker] Add DSH to the native flavor catalogs before publishing the generated catalog —
shared/fixtures/catalogs/modes.json:128now containsdsh, but iOS still treats it as an unknown flavor and falls back to Claude permission modes. The currentpackage-testscheck fails atios/Packages/HapiKit/Tests/HapiProtocolTests/CatalogTests.swift:48and:55; Android likewise renders DSH asOtherwith Claude permission controls.
Suggested fix:case dsh // Add .dsh to knownFlavors/raw-value/label switches. case .dsh, .pi: return [] // permissionModes
data object Dsh : AgentFlavor { override val id = "dsh" } // Add Dsh to KNOWN/LABELS; do not grant model-change capability. "dsh" -> emptyList() // PermissionModes.forFlavor
-
[Major] Managed DSH launches still inherit the hidden global YOLO preference — the managed descriptor at
shared/src/agentConfig.ts:69hides the toggle, butweb/src/components/NewSession/index.tsx:1486still sendsyoloMode; the runner appends--yolo, andcli/src/commands/dsh.ts:10rejects it. Any user who previously enabled YOLO for another flavor cannot start DSH.
Suggested fix:yolo: agent === 'dsh' || agent === 'grok' || usesCodexFamilyPermissions ? undefined : yoloMode,
-
[Major] A single prompt failure permanently ends a fresh-only DSH conversation —
cli/src/dsh/dshRemoteLauncher.ts:86rethrows after reporting a turn error, sorunDshmarks the process crashed and disconnects. Because DSH sessions with messages cannot resume, a transient model/API error prevents any retry in the same context.
Suggested fix:} catch (error) { const message = error instanceof Error ? error.message : String(error) logger.warn('[dsh-acp] prompt failed', { message }) this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` }) // Keep the ACP session alive so the next queued prompt can retry. }
Summary
Review mode: initial
Three findings: one blocking native protocol/catalog mismatch and two DSH runtime regressions.
Testing
- Not run locally (automation; PR content treated as untrusted). Current-head checks observed: main test/integration pass; iOS
package-testsfails on the addeddshcatalog entry.
HAPI Bot
| "tone": "danger" | ||
| } | ||
| ], | ||
| "dsh": [], |
There was a problem hiding this comment.
[BLOCKER] Keep the native flavor catalogs in sync with this generated key.
iOS CatalogTests compares these keys exactly with AgentFlavor.knownFlavors; dsh is absent there, so the current package-tests job already fails and permissionModes(forFlavor: "dsh") incorrectly falls back to Claude modes instead of []. Android also maps dsh to Other, showing an unknown label and Claude permission controls.
Suggested fix:
case dsh
// Add .dsh to knownFlavors, raw-value parsing, and displayLabel.
case .dsh, .pi: return [] // permissionModesdata object Dsh : AgentFlavor { override val id = "dsh" }
// Add Dsh to KNOWN/LABELS; omit model-change capability.
"dsh" -> emptyList() // PermissionModes.forFlavor| { id: 'serviceTier', section: 'settings', kind: 'select', optionSource: 'model', availability: 'both' }, | ||
| { id: 'collaborationMode', section: 'settings', kind: 'select', optionSource: 'static', availability: 'both' } | ||
| ), | ||
| dsh: fields(MANAGED_PERMISSION), |
There was a problem hiding this comment.
[MAJOR] The managed DSH permission state still submits the hidden global YOLO preference.
NewSession keeps yoloMode across flavors and line 1486 sends it for every non-Grok/non-Codex-family agent. The runner then appends --yolo, while parseDshCommandOptions rejects that flag. A user who enabled YOLO for Claude/Pi and switches to DSH gets a failed child process even though this UI says permissions are server-managed.
Suggested fix:
yolo: agent === 'dsh' || agent === 'grok' || usesCodexFamilyPermissions
? undefined
: yoloMode,Add a form test that starts with persisted YOLO enabled, selects DSH, and asserts spawnSession receives no yolo field.
| logger.warn('[dsh-acp] prompt failed', { message }) | ||
| this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` }) | ||
| this.messageBuffer.addMessage(`DSH prompt failed: ${message}`, 'status') | ||
| throw error |
There was a problem hiding this comment.
[MAJOR] Do not crash a fresh-only session for a single prompt error.
This rethrow exits the loop, runDsh marks the session crashed, and cleanup disconnects the ACP process. Since DSH deliberately stores no resumable agent id, any transient API/model error leaves a conversation with user messages permanently non-resumable. The other ACP launchers report the turn error and keep accepting prompts.
Suggested fix:
} catch (error) {
const message = error instanceof Error ? error.message : String(error)
logger.warn('[dsh-acp] prompt failed', { message })
this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` })
// Keep the ACP session alive for the next queued prompt.
}Cover this with a backend mock that rejects the first prompt and successfully processes the second.
There was a problem hiding this comment.
Findings
-
[Major] DSH spawn construction still emits permission flags that its parser rejects — the new runner test expects
--permission-mode read-only, butparseDshCommandOptionsallows zero permission modes; persisted YOLO from Web/iOS/Android similarly becomes the explicitly rejected--yolo. Evidence:cli/src/runner/buildCliArgs.test.ts:370,cli/src/commands/dsh.ts:9.
Suggested fix:if (agent !== 'pi' && agent !== 'dsh') { // append --permission-mode / --yolo }
Also render DSH as managed and omit
yoloin all three create forms. -
[Major] A single prompt failure still destroys a fresh-only DSH conversation — rethrowing exits the launcher, marks the process crashed, and disconnects the ACP server; DSH stores no resumable agent id. Evidence:
cli/src/dsh/dshRemoteLauncher.ts:86.
Suggested fix:} catch (error) { const message = error instanceof Error ? error.message : String(error) logger.warn('[dsh-acp] prompt failed', { message }) this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` }) // Keep the ACP session alive for the next prompt. }
Summary
Review mode: follow-up after new commits
The native catalog mismatch from the prior review is addressed. Two Major runtime regressions remain: invalid managed-permission flags can prevent DSH startup, and turn errors terminate a non-resumable conversation.
Testing
- Not run locally (automation; PR content treated as untrusted).
- Current-head checks observed: Android build, fixture drift gate, iOS package/app, and integration pass; main test still in progress.
- Add coverage for persisted YOLO across Web/iOS/Android, DSH runner arguments with permission inputs, and a rejected first prompt followed by a successful second prompt.
HAPI Bot
| '--hapi-starting-mode', 'remote', | ||
| '--started-by', 'runner', | ||
| '--existing-session-id', 'existing-hub-id', | ||
| '--permission-mode', 'read-only' |
There was a problem hiding this comment.
[MAJOR] This expectation builds a DSH command that the DSH parser rejects.
parseDshCommandOptions() calls parseRemoteAgentCommandOptions(commandArgs, [], ['remote']), so --permission-mode read-only exits before ACP initialization. The same generic runner branch appends --yolo when a persisted Web/iOS/Android toggle is true, and DSH explicitly rejects that flag too. The newly added mobile catalog entries make DSH selectable, while both native forms still route non-Pi agents through the YOLO control.
Suggested fix:
if (agent !== 'pi' && agent !== 'dsh') {
if (options.permissionMode && (PERMISSION_MODES as readonly string[]).includes(options.permissionMode)) {
args.push('--permission-mode', options.permissionMode)
} else if (yolo) {
args.push('--yolo')
}
}Then assert DSH args contain neither flag, change Web to omit yolo for dsh, and render DSH as .managed / PermissionUi.Managed in the native forms.
| logger.warn('[dsh-acp] prompt failed', { message }) | ||
| this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` }) | ||
| this.messageBuffer.addMessage(`DSH prompt failed: ${message}`, 'status') | ||
| throw error |
There was a problem hiding this comment.
[MAJOR] Do not tear down a fresh-only session for one failed or cancelled turn.
This rethrow exits runMainLoop(), runDsh() marks the process crashed, and cleanup disconnects the ACP server. Because DSH deliberately persists no resumable agent id, a transient API error or cancelPrompt() rejection permanently ends the conversation. The existing Kimi ACP launcher reports the turn error and continues accepting prompts.
Suggested fix:
} catch (error) {
const message = error instanceof Error ? error.message : String(error)
logger.warn('[dsh-acp] prompt failed', { message })
this.session.sendSessionEvent({ type: 'message', message: `DSH prompt failed: ${message}` })
this.messageBuffer.addMessage(`DSH prompt failed: ${message}`, 'status')
// No rethrow: allow the next queued prompt to retry.
}Add a launcher test whose first prompt() rejects and whose second prompt succeeds.
There was a problem hiding this comment.
Findings
-
[Major] Emit DSH
readyafter each completed turn, not at ACP startup —readyis the completion signal used for “Ready for input” notifications. The current event fires before any prompt, while the promptfinallyblock only clearsthinking; successful, failed, and cancelled turns therefore produce no completion notification. Evidence:cli/src/dsh/dshRemoteLauncher.ts:67, related handlinghub/src/notifications/notificationHub.ts:70.
Suggested fix:// Remove the startup ready event. } finally { this.session.onThinkingChange(false) await this.permissionHandler?.cancelAll('Prompt finished') if (this.session.queue.size() === 0 && !this.shouldExit) { this.session.sendSessionEvent({ type: 'ready' }) } }
-
[Major] Native DSH create forms still expose an ignored YOLO control — adding DSH as a no-mode flavor makes both native UIs fall through to
YoloTogglebecause only Pi is rendered as managed. Their spawn builders also serializeyolo; the runner now discards it, so the permission control appears security-relevant but cannot change DSH policy. Evidence:ios/Packages/HapiKit/Sources/HapiClient/NewSession/NewSessionForm.swift:137; related contextsios/Hapi/Features/NewSession/NewSessionModel.swift:281,ios/Packages/HapiKit/Sources/HapiClient/NewSession/NewSessionForm.swift:177,android/app/src/main/kotlin/app/hapi/companion/feature/newsession/NewSessionViewModel.kt:605,android/app/src/main/kotlin/app/hapi/companion/feature/newsession/NewSessionForm.kt:98.
Suggested fix:if agent == .pi || agent == .dsh { return .managed } // ... yolo: (agent == .dsh || isGrok || codexFamily) ? nil : form.yolo
agent == "pi" || agent == "dsh" -> PermissionUi.Managed // ... yolo = if (agent == "dsh" || isGrok || codexFamily) null else form.yolo
Summary
Review mode: follow-up after new commits
The follow-up removes unsupported runner flags and keeps the ACP process alive after prompt failures. Two Major issues remain: turn completion signaling is misplaced, and native permission UI still advertises a DSH policy control that has no effect.
Testing
- Not run locally (automation; PR content treated as untrusted).
- Current-head CI observed passing: test, integration, build, Android app-build, iOS package-tests, and fixture drift-gate.
- Add tests asserting no startup
ready, onereadyafter each completed/failed prompt, and managed/no-yoloDSH creation on iOS and Android.
HAPI Bot
| onAbort: () => this.handleAbort(), | ||
| onSwitch: () => this.handleExitFromUi() | ||
| }) | ||
| this.session.sendSessionEvent({ type: 'ready' }) |
There was a problem hiding this comment.
[MAJOR] Emit ready after each prompt, not during startup.
This event is consumed as the “agent finishes working” signal and triggers Ready-for-input notifications. Here it fires immediately after ACP initialization, before any work. The prompt finally block never sends it, so every actual DSH turn completes without the remote completion notification.
Suggested fix:
// Remove this startup event.
} finally {
this.session.onThinkingChange(false)
await this.permissionHandler?.cancelAll('Prompt finished')
if (this.session.queue.size() === 0 && !this.shouldExit) {
this.session.sendSessionEvent({ type: 'ready' })
}
}Add a test that verifies ready is absent before prompt() settles and emitted after both a successful and a rejected turn.
| case .agy: return .alwaysProceed | ||
| case .codex, .copilot, .cursor, .gemini, .kimi, .opencode: return .yolo | ||
| case .pi, .other: return nil | ||
| case .dsh, .pi, .other: return nil |
There was a problem hiding this comment.
[MAJOR] DSH still falls through to the native YOLO toggle even though this mapping says no HAPI policy applies.
Both native UI selectors special-case only Pi as Managed, so DSH renders YoloToggle(...nil). Both spawn builders also include yolo for DSH. The runner now silently drops that field, leaving users with a permission/security control that cannot affect the ACP composition.
Suggested fix:
if agent == .pi || agent == .dsh { return .managed }
// buildSpawnRequest:
yolo: (agent == .dsh || isGrok || codexFamily) ? nil : form.yoloagent == "pi" || agent == "dsh" -> PermissionUi.Managed
// buildSpawnRequest:
yolo = if (agent == "dsh" || isGrok || codexFamily) null else form.yoloAdd exact spawn-body and permission-UI tests for DSH on both platforms.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
Review mode: follow-up after new commits
No reportable issues found in the latest full diff. The prior Major findings are resolved at this head. Residual risk: cli/src/dsh/dshRemoteLauncher.test.ts covers successful prompt completion, but does not directly exercise rejected prompts, abort/cancellation, or permission cleanup.
Testing
- Not run (automation; PR content treated as untrusted).
- Current-head CI observed passing: integration, build, Android app-build, iOS package-tests, and fixture drift-gate. The main test job was still pending at review time.
- Suggested coverage: rejected
backend.prompt(), abort during an active prompt, and pending ACP permission cancellation.
HAPI Bot
2 participants
Summary
ACP setup
Default command:
dsh-acp-demoThe official
@deepseek-ai/dsh-acp-demo@0.1.0-rc.7composition requires its own Cordis config andDEEPSEEK_API_KEY. HAPI passes an empty MCP list because the official server rejects injected MCP servers.Known ACP limitations
Verification
bun typecheckbun run testCloses #1538