feat(dsh): DeepSeek Harness integration (standalone review copy) - #9
feat(dsh): DeepSeek Harness integration (standalone review copy)#9swear01 wants to merge 59 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates DeepSeek Harness (DSH) as a first-class agent runtime in HAPI, spawning a local DSH host process and bridging its native events (such as queue, jobs, goals, subagents, and approvals) into HAPI's agent state and message streams. Feedback on the changes highlights a critical bug in runDsh.ts where a redeclared childCursorByChild map shadows the outer scope and prevents child cursors from being persisted. Additionally, a memory leak was identified in DshEventBridge.ts due to unremoved abort event listeners on a long-lived signal, and an improvement was suggested to use optional chaining when accessing metadata.dshChildCursors to avoid potential runtime crashes.
| // Latest child seq per child id, merged into metadata on cursor flush | ||
| // so subagent journals survive CLI restarts without replay. | ||
| const childCursorByChild = new Map<string, number>() |
There was a problem hiding this comment.
The block-scoped variable childCursorByChild is redeclared here, shadowing the outer childCursorByChild map declared at line 126. Because of this shadowing, the onChildCursor callback (which is defined in this inner scope) writes to this inner map, while the flushCursor function (defined in the outer scope) reads from the outer map. As a result, the outer map remains empty, and the child cursors are never persisted to metadata, breaking subagent journal durability across restarts.
| // Latest child seq per child id, merged into metadata on cursor flush | |
| // so subagent journals survive CLI restarts without replay. | |
| const childCursorByChild = new Map<string, number>() | |
| // Using the outer childCursorByChild map declared at line 126 |
| function waitForAbortableDelay(ms: number, signal: AbortSignal): Promise<void> { | ||
| return new Promise((resolve) => { | ||
| const timer = setTimeout(resolve, ms) | ||
| signal.addEventListener('abort', () => { | ||
| clearTimeout(timer) | ||
| resolve() | ||
| }, { once: true }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This function registers an abort event listener on the long-lived signal passed to start(). If the timeout completes normally, resolve is called, but the event listener is never removed from signal. Since signal lives for the entire duration of the session, every successful delay leaks an event listener, eventually leading to a memory leak and MaxListenersExceededWarning warnings.
| function waitForAbortableDelay(ms: number, signal: AbortSignal): Promise<void> { | |
| return new Promise((resolve) => { | |
| const timer = setTimeout(resolve, ms) | |
| signal.addEventListener('abort', () => { | |
| clearTimeout(timer) | |
| resolve() | |
| }, { once: true }) | |
| }) | |
| } | |
| function waitForAbortableDelay(ms: number, signal: AbortSignal): Promise<void> { | |
| return new Promise((resolve) => { | |
| let timer: any | |
| const onAbort = () => { | |
| clearTimeout(timer) | |
| resolve() | |
| } | |
| timer = setTimeout(() => { | |
| signal.removeEventListener('abort', onAbort) | |
| resolve() | |
| }, ms) | |
| signal.addEventListener('abort', onAbort, { once: true }) | |
| }) | |
| } |
| ...(childCursors ? { | ||
| dshChildCursors: { | ||
| ...((metadata as { dshChildCursors?: Record<string, number> }).dshChildCursors ?? {}), | ||
| ...childCursors | ||
| } | ||
| } : {}) |
There was a problem hiding this comment.
If metadata is null or undefined, accessing .dshChildCursors will throw a TypeError. Using optional chaining (metadata?.dshChildCursors) prevents potential runtime crashes. Additionally, since metadata is typed as Metadata (where dshChildCursors is already optional), the type cast is redundant and can be safely removed.
| ...(childCursors ? { | |
| dshChildCursors: { | |
| ...((metadata as { dshChildCursors?: Record<string, number> }).dshChildCursors ?? {}), | |
| ...childCursors | |
| } | |
| } : {}) | |
| ...(childCursors ? { | |
| dshChildCursors: { | |
| ...(metadata?.dshChildCursors ?? {}), | |
| ...childCursors | |
| } | |
| } : {}) |
![swear-review[bot]](https://avatars.githubusercontent.com/u/49574926?s=60&v=4){kind=small}
| export function isDshMessage(message: AgentMessage): boolean { | ||
| return message.type === 'dsh_native' || message.type === 'dsh_state' | ||
| }; |
There was a problem hiding this comment.
Trailing semicolon after a function declaration is unnecessary and is flagged by the no-extra-semi rule. Remove the ; after the closing brace.
| export function isDshMessage(message: AgentMessage): boolean { | |
| return message.type === 'dsh_native' || message.type === 'dsh_state' | |
| }; | |
| export function isDshMessage(message: AgentMessage): boolean { | |
| return message.type === 'dsh_native' || message.type === 'dsh_state' | |
| } |
low · style
— Swear Review
| export function isDshMessage(message: AgentMessage): boolean { | ||
| return message.type === 'dsh_native' || message.type === 'dsh_state' | ||
| }; |
There was a problem hiding this comment.
isDshMessage is exported but never referenced anywhere in the codebase (no call sites in cli, hub, or web). If it isn't part of a planned public API yet, consider removing it to avoid dead code; otherwise wire up its usage.
low · maintainability
— Swear Review
| } | ||
| const value = response.result.value as { accepted?: boolean; command?: { kind: 'success'; text?: string } } | ||
| return { | ||
| accepted: value.accepted === true ? (true as const) : (true as const), |
There was a problem hiding this comment.
The ternary value.accepted === true ? (true as const) : (true as const) always evaluates to true regardless of the host response. The condition is dead code: when the host returns accepted: false, the client still reports the prompt as accepted, so callers will treat a rejected/steered prompt as successfully queued. This silently loses the rejection signal and produces inconsistent session state. Fix by propagating the actual value and widening the DshPromptResult.accepted type from the literal true to boolean (the wire value is typed accepted?: boolean).
| accepted: value.accepted === true ? (true as const) : (true as const), | |
| accepted: value.accepted === true, |
high · bug
— Swear Review
| private readonly api: IApiClient, | ||
| private readonly baseUrl: string | ||
| ) { | ||
| this.transport = api as unknown as DshNodeTransport |
There was a problem hiding this comment.
The constructor blindly casts any IApiClient to DshNodeTransport, yet promptDirect exists only on the Node transport. Today every construction site goes through connect(), so it works, but the public constructor accepts any IApiClient — passing a non-Node implementation would throw TypeError: this.transport.promptDirect is not a function at runtime. The subsequent as never casts on updateQueueAction/goalCall/subagentCall/respond payloads also bypass compile-time verification of the wire shapes, hiding contract mismatches. Consider typing the constructor parameter as DshNodeTransport (since only connect() is used) and replacing the as never payload casts with properly typed request types from the official API package.
medium · maintainability
— Swear Review
| * registered ahead of the HTTP round-trip (the host may emit the | ||
| * user/message event before the prompt response returns). */ | ||
| reservePromptRpcId(): string { | ||
| return crypto.randomUUID() |
There was a problem hiding this comment.
crypto.randomUUID() is used (here, in prompt() and gatewayCall()) without importing it, relying on the global Web Crypto API which is only available in Node >= 19. Every other CLI file imports randomUUID from node:crypto (e.g. cli/src/dsh/runDsh.ts, cli/src/agent/messageConverter.ts). Import from node:crypto for consistency and to keep the DSH feature working on older Node runtimes that lack the global.
| return crypto.randomUUID() | |
| return randomUUID() |
low · maintainability
— Swear Review
| if (queued > 0) { | ||
| parts.push(`queue: ${queued}`) | ||
| } | ||
| const runningJobs = snapshot.jobs?.jobs.filter((job) => job.status === 'running' || job.status === 'stopping').length ?? 0 |
There was a problem hiding this comment.
Same incomplete optional chaining as above: snapshot.jobs?.jobs guards only the outer jobs field, not the nested jobs array. If a partial/older snapshot omits the inner array, .filter() throws and crashes the component during render.
| const runningJobs = snapshot.jobs?.jobs.filter((job) => job.status === 'running' || job.status === 'stopping').length ?? 0 | |
| const runningJobs = (snapshot.jobs?.jobs ?? []).filter((job) => job.status === 'running' || job.status === 'stopping').length |
medium · bug
— Swear Review
| </div> | ||
| ) : null} | ||
| {questions ? ( | ||
| <DshQuestionsDialog questions={questions} dispatch={dispatch} onClose={() => {}} /> |
There was a problem hiding this comment.
onClose={() => {}} is a no-op stub. DshQuestionsDialog renders as a fixed full-screen modal and provides no dismiss affordance (no close button/backdrop/ESC), so passing a no-op onClose means users with pending questions are trapped until they answer all of them. Either implement a real dismiss path (backdrop click / X / ESC) wired through this callback, or remove the prop so the blocking behavior is intentional and documented.
medium · bug
— Swear Review
| console.error('[dsh] action failed', action.type, error) | ||
| throw error |
There was a problem hiding this comment.
The dispatch wrapper logs the error and rethrows, but its callers in DshGoalBar and DshQueueDock fire void dispatch(...) without attaching a .catch. Every failed action therefore becomes an unhandled promise rejection, and no user-facing error is ever surfaced (only the questions dialog handles its own rejection). Consider not rethrowing here and letting the mutation's onError handle feedback, or returning a structured result that children can surface.
medium · bug
— Swear Review
| function statusSummary(snapshot: DshStateSnapshot): string[] | null { | ||
| const parts: string[] = [] | ||
| if (snapshot.goal?.objective) { | ||
| parts.push(`goal: ${snapshot.goal.status ?? 'active'}`) |
There was a problem hiding this comment.
These summary pills are hardcoded English labels (goal:, queue:, jobs:) while the rest of the component uses t() keys (e.g. dsh.sessionLabel), and the locale files already define dsh.queue, dsh.jobs, and dsh.goalStatus.*. For consistency with the en/zh-CN i18n setup, these user-facing labels should be localized rather than hardcoded.
low · maintainability
— Swear Review
| return run | ||
| }, [dshAction]) |
There was a problem hiding this comment.
useMemo is used to memoize a plain function; useCallback is the idiomatic hook for this scenario and makes the intent (stable callback identity) clearer. Minor readability improvement.
| return run | |
| }, [dshAction]) | |
| }, [dshAction]) |
low · style
— Swear Review
Critical: childCursorByChild was shadowed by a nested redeclaration — onChildCursor wrote the inner map while flushCursor read the outer one, so subagent journal cursors were never persisted. Removed the shadow. High/medium fixes: - Bridge: abort listener leak in waitForAbortableDelay; child-journal seal starts before pumps dispatch (queued frames before subscription); retryMs resets after a healthy generation; session-removed cleans childLastSeq/childBuffers; CURSOR_FLUSH_MS/lastCursorFlush dead code removed; asString evaluated once - Transport: promptDirect checks HTTP status; readWebSocket handles socket 'error' (enqueues stream end); SESSION_PROMPT_PATH constant - Runtime: install child stdout ignored (pipe-block risk); overlayDir cleaned on every failure path; readiness race timer cleared; stop() clears SIGKILL timer + exit listener; isDshRuntimeInstalled removed - Projector: tool-result content optional-chained; end-seed persisted as dsh_native; stateSnapshot seq no longer overridable by folded seq; dead step state (currentTurn/streamed/finished/finalized*) removed - Hub: rewind finalize moved outside the archive CAS try/catch - Web: queue/jobs arrays optional-chained (unvalidated payloads); dispatch swallows failures (callers are fire-and-forget); status summary labels localized; dsh.goal i18n keys added - Shared: dshChildCursors validated int().nonnegative() - Doctor: imports cleaned, unreachable catch removed, ternary flattened
|
|
||
| export function isDshMessage(message: AgentMessage): boolean { | ||
| return message.type === 'dsh_native' || message.type === 'dsh_state' | ||
| }; |
There was a problem hiding this comment.
Stray extra semicolon after the function declaration (an empty statement). Remove it to avoid lint warnings (e.g. no-extra-semi) and to match the surrounding code style.
| }; | |
| } |
low · style
— Swear Review
| dshMessageId?: string | ||
| } | ||
|
|
||
| export function isDshMessage(message: AgentMessage): boolean { |
There was a problem hiding this comment.
isDshMessage is newly exported but has no callers anywhere in the codebase (all other DSH modules import DshProjectedMessage instead). If it isn't consumed yet, consider removing it or adding the intended usage to avoid dead code.
low · maintainability
— Swear Review
| await runDsh({ | ||
| existingSessionId: base.existingSessionId, | ||
| workingDirectory: base.workingDirectory, | ||
| resumeSessionId: base.resumeSessionId, | ||
| startedBy: base.startedBy, | ||
| startingMode: 'remote', | ||
| }) |
There was a problem hiding this comment.
runDsh declares resumeSessionId in its options type, but the implementation never reads it (verified in cli/src/dsh/runDsh.ts — the only occurrence is the type declaration). DSH resume actually restores the native session via existingSessionId plus the persisted dshSessionId metadata. Passing resumeSessionId here is dead and misleads maintainers into thinking the native DSH session id is consumed. Consider dropping it from the call (and from runDsh's options) or wiring it into the resume path.
low · maintainability
— Swear Review
| if (target.flavor === 'dsh') { | ||
| const { runDsh } = await import('@/dsh/runDsh') |
There was a problem hiding this comment.
The new dsh resume branch has no corresponding test in cli/src/commands/resume.test.ts, while every other flavor (codex, agy, claude, grok, pi, gemini) has a dedicated case. Add a test asserting runDsh is invoked with the expected options (e.g., existingSessionId, workingDirectory, startingMode: 'remote') to guard this branch against regressions.
low · test
— Swear Review
| } | ||
| const value = response.result.value as { accepted?: boolean; command?: { kind: 'success'; text?: string } } | ||
| return { | ||
| accepted: value.accepted === true ? (true as const) : (true as const), |
There was a problem hiding this comment.
Both branches of this ternary are identical, so accepted is always coerced to true even when the server returns accepted: false (e.g., a steer prompt rejected while a generation is already running). This is dead code and a latent correctness bug: runDsh.ts marks the HAPI message as consumed as soon as prompt() resolves, so a rejected prompt would be silently treated as accepted with no way for the caller to distinguish. Fix by reflecting the server value — accepted: value.accepted ?? true — and change DshPromptResult.accepted from the literal true type to boolean.
| accepted: value.accepted === true ? (true as const) : (true as const), | |
| accepted: value.accepted ?? true, |
high · bug
— Swear Review
| await bridgeRun; | ||
|
|
||
| lifecycle.setSessionEndReason('completed'); |
There was a problem hiding this comment.
Unexpected host exit is handled outside this try/catch: the host-exit listener calls lifecycle.markCrash(...) (which sets sessionEndReason='error' and exitCode) then cleanupAndExit(1). When that listener aborts bridgeAbort, await bridgeRun resolves normally and crashed is still false, so lifecycle.setSessionEndReason('completed') overwrites the crash state and the session is recorded as a normal completion in the death/audit record. Gate the 'completed' end reason on the host having shut down gracefully (e.g. check stoppingHost or a dedicated crash flag) rather than only on !crashed.
medium · bug
— Swear Review
| logger.debug(`[dsh] image attachment read failed: ${error instanceof Error ? error.message : String(error)}`) | ||
| } | ||
| } else { | ||
| body += `\n[Attached file: ${attachment.path}]` |
There was a problem hiding this comment.
Non-image attachments interpolate attachment.path verbatim into the prompt body without sanitization and, unlike image attachments, never get an isPathWithinUploadDir check. A crafted filename/path (newlines or instruction-like text) is fed directly to the model, and raw local filesystem paths leak into the conversation. At minimum sanitize/truncate the interpolation, and validate non-image paths against the upload dir for consistency with the image path.
medium · security
— Swear Review
| }).catch((error) => { | ||
| logger.debug(`[dsh] prompt failed: ${error instanceof Error ? error.message : String(error)}`); |
There was a problem hiding this comment.
In the prompt-failure path the localId→rpcId binding is deleted and an error message is sent, but session.emitMessagesConsumed is never called for the HAPI user message (runPi.ts consumes the row on preparation/prompt failure). The HAPI message therefore stays visually pending/queued in the web UI forever alongside the error. Consume the message on failure too, so the transcript doesn't leave a stuck pending row.
medium · bug
— Swear Review
| const match = catalog.groups | ||
| .flatMap((group) => group.models.map((m) => ({ ...m, provider: group.id }))) | ||
| .find((m) => m.id === modelId); | ||
| return match?.provider ?? null; |
There was a problem hiding this comment.
Model resolution is inconsistent between the two paths: launch-time opts.model throws on ambiguity (matches.length !== 1), but resolveModelProvider used by SetSessionConfig for bare model ids silently picks the first provider with .find(). A model id that exists under multiple providers is therefore rejected at launch yet silently (and possibly wrongly) resolved in-session. Align both paths on the same ambiguity policy.
medium · bug
— Swear Review
| const startingMode: 'local' | 'remote' | 'pty' = opts.startingMode | ||
| ?? (startedBy === 'runner' ? 'remote' : 'remote'); |
There was a problem hiding this comment.
(startedBy === 'runner' ? 'remote' : 'remote') has identical branches, so the ternary is dead logic and startedBy has no effect on the derived starting mode. Sibling launchers differentiate terminal vs runner launches; if DSH is intentionally always 'remote', simplify to avoid misleading maintainers.
| const startingMode: 'local' | 'remote' | 'pty' = opts.startingMode | |
| ?? (startedBy === 'runner' ? 'remote' : 'remote'); | |
| const startingMode: 'local' | 'remote' | 'pty' = opts.startingMode | |
| ?? 'remote'; |
low · maintainability
— Swear Review
| export type DshProjectedMessage = AgentMessage & { | ||
| dshSeq?: number | ||
| /** DSH native assistant-message id (message feedback addressing). */ | ||
| dshMessageId?: string | ||
| } |
There was a problem hiding this comment.
Minor style: the rest of this file terminates every statement with a semicolon (e.g. stopReason?: string;), but the new DshProjectedMessage block omits them, and there is a double blank line after the closing brace. For consistency and maintainability, add semicolons after dshSeq?: number and dshMessageId?: string and collapse the extra blank line.
low · style
— Swear Review
| reservePromptRpcId(): string { | ||
| return crypto.randomUUID() | ||
| } |
There was a problem hiding this comment.
This uses the bare global crypto.randomUUID() without importing it, while the rest of the CLI consistently does import { randomUUID } from 'node:crypto' (see acpSessionTitle, sessionFactory, messageConverter, runDsh, etc.). The global Web Crypto object is only enabled by default on Node 19+ (on Node 18 it requires --experimental-global-webcrypto and in some sandboxed runtimes it's undefined), so on supported-but-older runtimes this throws on the prompt hot path. Import randomUUID from node:crypto and use it here and in prompt()/gatewayCall() for consistency and portability.
| reservePromptRpcId(): string { | |
| return crypto.randomUUID() | |
| } | |
| import { randomUUID } from 'node:crypto' | |
| reservePromptRpcId(): string { | |
| return randomUUID() | |
| } |
medium · bug
— Swear Review
| await this.api.respond({ | ||
| type: 'client-response', | ||
| rpcId: rpcId as never, | ||
| result: { ok: true, value } | ||
| }) |
There was a problem hiding this comment.
Unlike every other unary method in this class, respond() never inspects response.result.ok before returning. If the host rejects the client-response (e.g. stale/unknown rpcId, session already closed, invalid approval/question payload), the failure is silently swallowed and the pending approval/question stays unresolved — the caller (DshRpcBridge / Permission handler in runDsh) then reports { accepted: true } back to the user as if it succeeded. Match the other methods and throw a DshRpcError when result.ok is false.
| await this.api.respond({ | |
| type: 'client-response', | |
| rpcId: rpcId as never, | |
| result: { ok: true, value } | |
| }) | |
| const response = await this.api.respond({ | |
| type: 'client-response', | |
| rpcId: rpcId as never, | |
| result: { ok: true, value } | |
| }) | |
| if (!response.result.ok) { | |
| throw new DshRpcError(response.result.error.code, response.result.error.message, response.result.error.details) | |
| } |
medium · bug
— Swear Review
| if (!response.ok) { | ||
| throw new DshRpcError('transport', `gateway ${endpoint}: HTTP ${response.status}`) | ||
| } | ||
| const parsed = await response.json() as { type: string; result?: { ok: boolean; value?: unknown; error?: { code: string; message: string; details?: unknown } } } |
There was a problem hiding this comment.
response.json() is awaited unconditionally. If the gateway returns a non-JSON error body (e.g. a 404/500 HTML or plain-text page), response.json() throws a raw SyntaxError that bypasses the class's uniform DshRpcError contract — the same gap DshNodeTransport.promptDirect already defends against with a try/catch. Wrap the parse so a non-JSON failure is normalized into a DshRpcError with the HTTP status instead.
| const parsed = await response.json() as { type: string; result?: { ok: boolean; value?: unknown; error?: { code: string; message: string; details?: unknown } } } | |
| let parsed: { type: string; result?: { ok: boolean; value?: unknown; error?: { code: string; message: string; details?: unknown } } } | |
| try { | |
| parsed = await response.json() as typeof parsed | |
| } catch { | |
| throw new DshRpcError('transport', `gateway ${endpoint}: non-JSON body (HTTP ${response.status})`) | |
| } |
medium · bug
— Swear Review
| ) { | ||
| this.transport = api as unknown as DshNodeTransport | ||
| } |
There was a problem hiding this comment.
prompt() calls this.transport.promptDirect(...), but promptDirect only exists on DshNodeTransport — the public constructor accepts any IApiClient and hides the mismatch behind as unknown as DshNodeTransport. Any caller passing a plain IApiClient (tests, future reuse, or a transport that doesn't extend AbstractApiClient) would hit a silent TypeError: this.transport.promptDirect is not a function at runtime instead of a compile-time error. Make the constructor require DshNodeTransport (or a narrow interface declaring promptDirect/doFetch) so the dependency is explicit and type-checked.
| ) { | |
| this.transport = api as unknown as DshNodeTransport | |
| } | |
| ) { | |
| this.transport = api | |
| } |
medium · maintainability
— Swear Review
| const running = entries?.filter((entry) => entry.activity === 'running').length ?? 0 | ||
|
|
||
| return ( | ||
| <div className="fixed inset-0 z-40 flex items-center justify-center bg-black/70 p-4 backdrop-blur-sm" onClick={onClose}> |
There was a problem hiding this comment.
The modal is rendered as plain divs without role="dialog", aria-modal, aria-label, focus trapping, or Escape-key handling, and the close button uses a bare '✕' without an aria-label. This is both an accessibility regression and a functional gap: SessionChat's global hotkey blocker (isScratchlistHotkeyBlockedTarget) only suppresses shortcuts for targets inside [role="dialog"], so scratchlist/mode-toggle hotkeys can fire behind this open modal. Other dialogs in this codebase (ScheduleTimePicker, Fue, ImagePreview) all set role="dialog" + aria-label; align this one with them.
| <div className="fixed inset-0 z-40 flex items-center justify-center bg-black/70 p-4 backdrop-blur-sm" onClick={onClose}> | |
| <div | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={t('dsh.subagents')} | |
| className="fixed inset-0 z-40 flex items-center justify-center bg-black/70 p-4 backdrop-blur-sm" | |
| onClick={onClose} | |
| > |
medium · bug
— Swear Review
| {entries ? ( | ||
| <span className="text-xs text-[var(--app-hint)]"> | ||
| {running > 0 | ||
| ? t('dsh.subagentRunningCount', { count: running }) | ||
| : t('dsh.noSubagents')} | ||
| </span> | ||
| ) : null} |
There was a problem hiding this comment.
When entries is an empty array, running is 0 so the header renders t('dsh.noSubagents') at the same time the body's empty-state branch renders t('dsh.noSubagents') again — the same 'no subagents' message appears twice in the modal. Consider showing a count (e.g. '{0} running') or suppressing the header text when the list is empty.
| {entries ? ( | |
| <span className="text-xs text-[var(--app-hint)]"> | |
| {running > 0 | |
| ? t('dsh.subagentRunningCount', { count: running }) | |
| : t('dsh.noSubagents')} | |
| </span> | |
| ) : null} | |
| {entries && entries.length > 0 ? ( | |
| <span className="text-xs text-[var(--app-hint)]"> | |
| {running > 0 | |
| ? t('dsh.subagentRunningCount', { count: running }) | |
| : t('dsh.subagentIdle')} | |
| </span> | |
| ) : null} |
low · style
— Swear Review
| ).then((response) => { | ||
| setEntries(response.result.entries.filter((entry) => entry.kind === 'child')) | ||
| }).catch((e: unknown) => { |
There was a problem hiding this comment.
response.result.entries is dereferenced without a runtime shape check. Although the API type contract and the server-side zod validation (DshSubagentCatalogSchema) guarantee entries is present, a network/middleware error that still resolves would throw a TypeError here; the adjacent .catch() would then surface a confusing generic message instead of falling back to an empty list. Optional chaining with a default is a cheap defensive guard.
| ).then((response) => { | |
| setEntries(response.result.entries.filter((entry) => entry.kind === 'child')) | |
| }).catch((e: unknown) => { | |
| ).then((response) => { | |
| setEntries((response.result.entries ?? []).filter((entry) => entry.kind === 'child')) | |
| }).catch((e: unknown) => { |
low · maintainability
— Swear Review
| title={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} | ||
| aria-label={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} |
There was a problem hiding this comment.
These title/aria-label strings are hardcoded in English, unlike every other header toggle button in this file (files/outline/terminal) which uses t(...) via useTranslation. This leaves the header button unlocalized for non-English users. Please use t('dsh.subagents') for the label and add a corresponding close-state key (e.g. dsh.subagentsClose) to both en.ts and zh-CN.ts locale files.
| title={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} | |
| aria-label={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} | |
| title={props.dshSubagentsActive ? t('dsh.subagentsClose') : t('dsh.subagents')} | |
| aria-label={props.dshSubagentsActive ? t('dsh.subagentsClose') : t('dsh.subagents')} |
medium · maintainability
— Swear Review
| 'dsh.sessionLabel': 'DeepSeek Harness', | ||
| 'dsh.running': 'running', | ||
| 'dsh.idle': 'idle', | ||
| 'dsh.pendingApprovals': '{count} pending approval{s}', |
There was a problem hiding this comment.
This string relies on a hand-rolled {s} pluralization placeholder, but the project's i18n interpolate only substitutes provided params and returns the literal {key} when a param is missing. There are currently no call sites for this key, yet the established precedent (misc.newMessage) is called as t('misc.newMessage', { n: count }) without passing s, which would render a literal {s} in English. To avoid this footgun, prefer a placeholder-free phrasing such as '{count} pending approval(s)', or ensure callers always pass s: count === 1 ? '' : 's'. Note the zh-CN value '{count} 个待批准请求' does not use {s} at all, so the placeholder is English-specific and would display literally in English if omitted.
| 'dsh.pendingApprovals': '{count} pending approval{s}', | |
| 'dsh.pendingApprovals': '{count} pending approval(s)', |
low · maintainability
— Swear Review
| try { | ||
| // DSH permission presets are runtime-discovered from the host; pass | ||
| // an empty allow-list so --permission-mode is rejected. | ||
| const options = parseRemoteAgentCommandOptions(commandArgs, []) |
There was a problem hiding this comment.
Passing an empty allow-list rejects --permission-mode (the parser throws), but --yolo is only silently swallowed: since the allow-list is empty, yoloEquivalent resolves to undefined and the flag is dropped with no error. A user running hapi dsh --yolo would believe auto-approval is enabled when nothing happens. To match the stated intent of rejecting permission-related flags, explicitly reject --yolo here.
| const options = parseRemoteAgentCommandOptions(commandArgs, []) | |
| const options = parseRemoteAgentCommandOptions(commandArgs, []) | |
| if (commandArgs.includes('--yolo')) { | |
| throw new Error('--yolo is not supported for dsh; permission presets are runtime-discovered from the host') | |
| } |
low · other
— Swear Review
| private readonly api: IApiClient, | ||
| private readonly baseUrl: string | ||
| ) { | ||
| this.transport = api as unknown as DshNodeTransport |
There was a problem hiding this comment.
DshClient is typed to accept any IApiClient, but prompt() calls this.transport.promptDirect(...) and gatewayCall() calls (this.api as unknown as { doFetch }).doFetch(...) — both are DshNodeTransport-specific members that do not exist on the IApiClient interface. The api as unknown as DshNodeTransport cast hides this mismatch, so any caller passing a different IApiClient (a mock, the official browser client, etc.) will get a hard TypeError at runtime in these methods, which is hard to diagnose. All current call sites use connect() and are safe, but the public constructor makes this a latent hazard. Consider typing the constructor parameter as DshNodeTransport directly, or add a runtime capability check so the concrete dependency is explicit and misuse fails at compile time.
medium · bug
— Swear Review
| itemId: options.itemId as never, | ||
| action: options.action as never |
There was a problem hiding this comment.
These as never casts (here on itemId/action, and likewise on rpcId in respond() and payload in goalCall()/subagentCall()) fully suppress the compiler's contract checking against the official apiproxy types. Any drift between the locally defined payload shapes and the actual wire contract (field renames, enum changes, new required fields) will compile cleanly and only surface later as a confusing runtime failure. Prefer a small typed mapping layer or a targeted as unknown as <official type> on just the mismatching field instead of a blanket as never.
low · maintainability
— Swear Review
| */ | ||
| async gatewayCall<T = unknown>(endpoint: string, payload: unknown): Promise<T> { | ||
| const transport = this.api as unknown as { doFetch(input: URL, init?: RequestInit): Promise<Response> } | ||
| const response = await transport.doFetch(new URL(`/api/${endpoint}`, this.baseUrl), { |
There was a problem hiding this comment.
gatewayCall issues a raw doFetch without any timeout or abort signal, so a hung/missing host leaves this promise pending forever. This is inconsistent with promptDirect, which composes a 30s AbortSignal.timeout (plus the caller's signal) on the same fetch leg. Add a timeout here (and optionally accept a caller AbortSignal) so gateway calls can't block teardown/cancellation indefinitely.
low · bug
— Swear Review
| return response.result.value | ||
| } | ||
|
|
||
| async listSkills(sessionId: ReturnType<typeof SessionId>): Promise<ResponseValue<'skill.list'>> { |
There was a problem hiding this comment.
Session-id typing is inconsistent across the client: most methods take a plain string and validate via SessionId() internally, but listSkills, selectAgentPreset, goals.* and subagents.* require the branded ReturnType<typeof SessionId> directly. This forces callers to track which form each method expects and increases the chance of type friction at call sites. Consider standardizing on plain string + internal SessionId() conversion for all public methods for a uniform API.
low · maintainability
— Swear Review
| {/* DSH questions are blocking by design (official semantics): no | ||
| dismiss affordance until the agent's question is answered. */} | ||
| {questions ? ( | ||
| <DshQuestionsDialog questions={questions} dispatch={dispatch} onClose={() => {}} /> |
There was a problem hiding this comment.
DshQuestionsDialog accepts onClose?: () => void but never invokes it internally (verified in DshPanels.tsx). Passing a no-op here is inert and misleading — it suggests a close affordance exists when the dialog is intentionally blocking by design. Either drop the prop (it's optional) or remove it from the component's interface to avoid a dead contract.
low · maintainability
— Swear Review
| <button | ||
| type="button" | ||
| onClick={() => setOpen((v) => !v)} |
There was a problem hiding this comment.
This toggle button controls the collapse/expand of the DSH panel but lacks aria-expanded and aria-controls, so assistive technology cannot announce the panel's state. Add aria-expanded={open} and reference the collapsible region with aria-controls/id for an accessible disclosure pattern.
low · other
— Swear Review
| }).then(() => { | ||
| setSubmitting(false) | ||
| }).catch((e: unknown) => { | ||
| setSubmitting(false) | ||
| setError(e instanceof Error ? e.message : String(e)) | ||
| }) |
There was a problem hiding this comment.
The .catch/setError branch here is effectively unreachable: the Dispatch wired by the caller (DshSessionPanels) swallows rejections — dshAction.mutateAsync(action).catch(... return undefined) — so this promise always resolves. A failed question submission therefore gives the user no error feedback and the dialog silently stays open (its own comment even says the dialog 'observes errors through its own submit catch', which contradicts the actual wiring). Prefer async/await + try/catch, and ensure failures are propagated to the dialog (let dispatch reject, or pass an onError callback) instead of relying on this unreachable catch.
medium · bug
— Swear Review
| const action = (kind: 'pause' | 'resume' | 'complete' | 'clear') => { | ||
| if (!ref) return | ||
| void dispatch({ type: 'goal', action: kind, refId: ref.refId, revision: ref.revision }) | ||
| } |
There was a problem hiding this comment.
id/revision are optional in DshGoalState, so ref can be null even when goal.objective exists. In that case the Pause/Resume/Complete/Clear buttons still render as enabled but every click silently no-ops because of the if (!ref) return guard. Disable (or hide) the action buttons when ref is null so the UI reflects the actual capability.
low · bug
— Swear Review
| export function DshQuestionsDialog({ questions, dispatch }: { | ||
| questions: DshPendingQuestions | ||
| dispatch: Dispatch | ||
| onClose?: () => void |
There was a problem hiding this comment.
onClose is declared in the props type but never used inside the component, and the caller passes a no-op (onClose={() => {}}). It's a misleading dead prop — either wire it up (e.g., invoke it after a successful submit, or add a close affordance) or remove it from both the props type and the caller.
low · maintainability
— Swear Review
| try { | ||
| // DSH permission presets are runtime-discovered from the host; pass | ||
| // an empty allow-list so --permission-mode is rejected. | ||
| const options = parseRemoteAgentCommandOptions(commandArgs, []) |
There was a problem hiding this comment.
parseRemoteAgentCommandOptions will accept --effort and --model-reasoning-effort, but runDsh does not consume them (its opts type only covers startedBy/startingMode/model/resumeSessionId/existingSessionId/workingDirectory/agentPreset). Unlike --permission-mode (rejected via the empty allow-list), these flags would be silently parsed and ignored, so hapi dsh --effort high produces a confusing no-op. Consider stripping the unused fields before passing to runDsh, or explicitly rejecting these flags so the mismatch is surfaced to the user.
low · bug
— Swear Review
| async respond(rpcId: string, value: unknown): Promise<void> { | ||
| // client-response receipts carry no business result on the success | ||
| // path; transport errors (non-2xx, malformed envelope) reject via the | ||
| // official client's fetch leg. | ||
| await this.api.respond({ | ||
| type: 'client-response', | ||
| rpcId: rpcId as never, | ||
| result: { ok: true, value } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The response from api.respond() is discarded entirely. DshRpcBridge answers { accepted: true } to the web right after this call (approval.respond / question.respond), so if the host returns a server-response envelope with result.ok === false (e.g. stale/unknown rpcId because the approval already timed out), that business failure is silently swallowed and the client wrongly reports the response as accepted — a state divergence where the web believes the approval/question was answered while the host never processed it. The comment only guarantees transport-level errors surface; a 2xx envelope with a non-ok result will not. Please capture the returned response and at least validate result.ok, throwing DshRpcError (or logging) on business failure.
medium · bug
— Swear Review
| private readonly api: IApiClient, | ||
| private readonly baseUrl: string | ||
| ) { | ||
| this.transport = api as unknown as DshNodeTransport |
There was a problem hiding this comment.
The public constructor accepts api: IApiClient but immediately casts it to DshNodeTransport, and prompt() later calls this.transport.promptDirect(...) which only exists on the concrete transport. Today every call site goes through the static connect(), so it happens to work — but the constructor's declared type lies: any caller passing a plain IApiClient will hit a TypeError at runtime on the prompt path. Type the constructor parameter as DshNodeTransport (the real requirement) so the misuse is impossible at compile time instead of being hidden behind the double cast.
low · maintainability
— Swear Review
| itemId: options.itemId as never, | ||
| action: options.action as never |
There was a problem hiding this comment.
as never is used to bypass the type checker at several RPC boundaries (itemId/action here, plus payload as never in goalCall/subagentCall and rpcId as never in respond). This hides mismatches between the local payload types and the official HAPI signatures: if the wire contract changes (queue item id format, goal payload shape, …) it won't be caught at compile time and will only surface as an obscure runtime failure. Prefer aligning the local parameter types with the official API types (or defining explicit mapped types) instead of casting through never.
low · maintainability
— Swear Review
| case 'step/start': | ||
| case 'step/end': { | ||
| this.stepState(event.data.turn, event.data.step) |
There was a problem hiding this comment.
event.data is dereferenced without any null/shape guard in step/start, step/end, assistant/chunk, assistant/message, tool/call, tool/result and request/context. These events are persisted as dsh_native and re-fed to this projector during hub replay/backfill — if a session is resumed with a different DSH runtime version or a legacy event whose payload shape differs (note the turn/end branch already defensively guards event.data), a single event.data.turn access throws, the exception propagates out of onEvent, and the whole event's projections (including its dsh_native record) are dropped. Guard event.data consistently across all branches, as turn/end does.
medium · bug
— Swear Review
| {/* DSH questions are blocking by design (official semantics): no | ||
| dismiss affordance until the agent's question is answered. */} | ||
| {questions ? ( | ||
| <DshQuestionsDialog questions={questions} dispatch={dispatch} onClose={() => {}} /> |
There was a problem hiding this comment.
DshQuestionsDialog never invokes onClose (no close button or backdrop dismissal in its implementation), and the design comment states questions are blocking with no dismiss affordance. Passing a no-op onClose={() => {}} is therefore misleading dead prop plumbing; either remove the onClose prop from the dialog contract or drop the no-op here.
low · maintainability
— Swear Review
| <button | ||
| type="button" | ||
| onClick={() => setOpen((v) => !v)} |
There was a problem hiding this comment.
The collapse toggle button exposes no expanded state to assistive technology. Add aria-expanded={open} and aria-controls pointing at the collapsible panel's id so screen readers can announce the state and associate the button with the controlled content.
low · other
— Swear Review
| const load = () => { | ||
| setLoading(true) | ||
| setError(null) | ||
| void api.dshAction<{ entries: SubagentEntry[]; parentAvailable: boolean }>( | ||
| sessionId, | ||
| { type: 'subagent', action: 'list' } | ||
| ).then((response) => { |
There was a problem hiding this comment.
load() has no guard against out-of-order resolution or component unmount. If the user clicks Refresh multiple times (or sessionId changes while a request is pending), an earlier response can resolve last and overwrite newer entries; if the modal closes while a request is in flight, setEntries/setError/setLoading run on an unmounted component. Since api.dshAction offers no cancellation, guard with a request sequence counter (only apply the result if it matches the latest request) or a mounted flag reset in the useEffect cleanup. Prefer async/await here for clearer control flow.
medium · bug
— Swear Review
| useEffect(() => { | ||
| load() | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [sessionId]) |
There was a problem hiding this comment.
The effect depends only on [sessionId] but invokes load, which closes over api and sessionId; the eslint-disable hides the missing-dependency warning. If the api prop identity changes while the modal stays open, the subagent list won't refresh. Define load with useCallback (with explicit deps) or inline the fetch into the effect so dependencies are accurate.
low · maintainability
— Swear Review
| const running = entries?.filter((entry) => entry.activity === 'running').length ?? 0 | ||
|
|
||
| return ( | ||
| <div className="fixed inset-0 z-40 flex items-center justify-center bg-black/70 p-4 backdrop-blur-sm" onClick={onClose}> |
There was a problem hiding this comment.
This modal overlay lacks role="dialog"/aria-modal="true"/aria-labelledby, an Escape-to-close handler, and focus management. role="dialog" is load-bearing here: isScratchlistHotkeyBlockedTarget in SessionChat uses target.closest('[role="dialog"]') to stop the global scratchlist hotkey from firing behind open dialogs (PR tiann#798). Without it, pressing that hotkey while this modal is open toggles scratchlist mode behind the overlay. Add the dialog ARIA attributes, a useEffect keydown listener for Escape, and focus the panel on open.
medium · bug
— Swear Review
| {entries === null && !error ? ( | ||
| <div className="py-6 text-center text-xs text-[var(--app-hint)]">{t('dsh.loading')}</div> | ||
| ) : entries && entries.length === 0 ? ( |
There was a problem hiding this comment.
The body uses a nested ternary chain (entries === null && !error ? … : entries && entries.length === 0 ? … : …), which the project guidelines forbid and which hurts readability. Extract the loading / empty / list states into variables or early returns.
low · style
— Swear Review
| onClose?: () => void | ||
| }) { |
There was a problem hiding this comment.
The onClose prop is declared in the props type but never destructured or invoked anywhere in the component, and the parent passes a no-op (onClose={() => {}}). It is dead API surface that misleads callers into thinking the dialog supports a close/dismiss path. Either implement it (e.g., call after successful submission or add a dismiss affordance) or remove it from the props type.
low · maintainability
— Swear Review
| const action = (kind: 'pause' | 'resume' | 'complete' | 'clear') => { | ||
| if (!ref) return | ||
| void dispatch({ type: 'goal', action: kind, refId: ref.refId, revision: ref.revision }) | ||
| } |
There was a problem hiding this comment.
When goal.id or goal.revision is missing, ref becomes null and action() returns early, but all four buttons (Pause/Resume/Complete/Clear) are still rendered and look interactive while doing nothing on click. Hide or disable the action buttons when ref is null so users aren't presented with inert controls.
low · bug
— Swear Review
| title={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} | ||
| aria-label={props.dshSubagentsActive ? 'DeepSeek Harness subagents: close' : 'DeepSeek Harness subagents'} |
There was a problem hiding this comment.
These user-facing strings are hardcoded, while all sibling toggle buttons in this header use the translation hook (e.g. t('session.outline.open')). Since locale keys for this feature already exist (dsh.subagents in both en.ts and zh-CN.ts), these tooltips will not be localized and will always render in English for Chinese UI users. Use t() (e.g. t('dsh.subagents')) for both title and aria-label so the toggle follows the existing i18n convention.
low · maintainability
— Swear Review
| 'dsh.send': 'Send', | ||
| 'dsh.sending': 'Sending…', | ||
| 'dsh.composerPlaceholder': 'Message the agent… (Enter to send)', | ||
| 'dsh.goal': 'Goal', |
There was a problem hiding this comment.
Missing two-space indentation — this line is the only key in the block without the leading indent that all surrounding entries use, which can trip formatter/lint checks (e.g., Prettier) and reduces readability.
| 'dsh.goal': 'Goal', | |
| 'dsh.goal': 'Goal', |
low · style
— Swear Review
| return undefined | ||
| } | ||
|
|
||
| export { asString } |
There was a problem hiding this comment.
asString is imported from @hapi/protocol but never used anywhere in this module — it is only re-exported at the bottom. No other file imports asString from this module (only useDshSessionState is consumed), so this is an accidental/dead re-export. Remove the unused import and the export { asString } line.
low · maintainability
— Swear Review
| } | ||
|
|
||
| /** Extract the first text block of a raw user/agent message. */ | ||
| export function messageText(message: DecryptedMessage): string { |
There was a problem hiding this comment.
messageText only extracts text when message.content is an object with a string content.text or data.text/data.message. It returns '' for other common content shapes used in this codebase: plain-string content (e.g. a role-wrapped user message { role: 'user', content: 'hello' }) and array-style content blocks ([{ type: 'text', text }, ...]). The hub's extractUserMessageText (hub/src/sync/syncEngine.ts) demonstrates these shapes are real. Since this helper is exported as a general-purpose text extractor for raw user/agent messages, it should also handle plain strings and text-block arrays, or its scope should be narrowed/documented accordingly.
medium · bug
— Swear Review
| sessionCursorChatStore: (sessionId: string) => ['session-cursor-chat-store', sessionId] as const, | ||
| sessionPiModels: (sessionId: string) => ['session-pi-models', sessionId] as const, | ||
| sessionDshModels: (sessionId: string) => ['session-dsh-models', sessionId] as const, | ||
| sessionDshSkills: (sessionId: string) => ['session-dsh-skills', sessionId] as const, |
There was a problem hiding this comment.
This query key is declared but never referenced anywhere in the codebase (searched both queryKeys.sessionDshSkills and the literal 'session-dsh-skills'). Per the dead-code checklist, unused declarations should be removed until the consumer is added, to avoid accumulating stale registry entries.
low · maintainability
— Swear Review
| /** DSH state rows are retained for the panels but are invisible; only the | ||
| * latest snapshot is needed (higher-seq wins on the client fold), so they | ||
| * neither consume the visible window budget nor grow unboundedly. */ | ||
| function isInvisibleJournalRow(message: DecryptedMessage): boolean { | ||
| return isDshNativePayloadMessage(message) | ||
| } |
There was a problem hiding this comment.
isInvisibleJournalRow is dead code: it is defined but never referenced anywhere in the codebase (the only match is its own definition). The actual trim/retention logic uses isDshNativePayloadMessage and isDshStateRow directly. Please remove it to avoid confusion, or use it if it was intended for shouldRetainWindowMessage/trimPreservingQueued.
low · maintainability
— Swear Review
| /** Only the newest dsh_state snapshot matters to the client fold. */ | ||
| function isDshStateRow(message: DecryptedMessage): boolean { | ||
| const record = message.content && typeof message.content === 'object' | ||
| ? (message.content as { content?: unknown }).content | ||
| : null | ||
| const data = record && typeof record === 'object' | ||
| ? (record as { data?: unknown }).data | ||
| : null | ||
| return typeof data === 'object' && data !== null && (data as { type?: unknown }).type === 'dsh_state' | ||
| } |
There was a problem hiding this comment.
isDshStateRow duplicates the exact same record/data extraction logic already written in isDshNativePayloadMessage. If the two checks ever need to adapt to a new message envelope, they must be kept in sync. Consider extracting a shared helper (e.g. getDshPayloadData(message)) or deriving isDshStateRow from isDshNativePayloadMessage plus an extra type === 'dsh_state' check to eliminate the duplication.
low · maintainability
— Swear Review
Swear ReviewMode: Full PR Findings:
0 findings total Status: ✅ Completed |
| const value = response.result.value as { accepted?: boolean; command?: { kind: 'success'; text?: string } } | ||
| return { | ||
| accepted: value.accepted === true, | ||
| ...(value.command ? { command: value.command } : {}), |
There was a problem hiding this comment.
response.result.value is typed unknown on the ok: true branch of promptDirect, so at runtime it could be null/undefined (e.g., a soft-rejected prompt). Dereferencing value.accepted/value.command here would throw a TypeError inside the prompt dispatch path — the caller in runDsh.ts would then surface a misleading "Failed to send prompt" error even though the host may have processed the prompt. Add a null guard before reading the fields.
| const value = response.result.value as { accepted?: boolean; command?: { kind: 'success'; text?: string } } | |
| return { | |
| accepted: value.accepted === true, | |
| ...(value.command ? { command: value.command } : {}), | |
| const value = response.result.value as { accepted?: boolean; command?: { kind: 'success'; text?: string } } | null | undefined | |
| return { | |
| accepted: value?.accepted === true, | |
| ...(value?.command ? { command: value.command } : {}), |
medium · bug
— Swear Review
| method: M, | ||
| payload: unknown | ||
| ): Promise<unknown> { | ||
| const response = await this.api.goals[method](payload as never) |
There was a problem hiding this comment.
payload as never (and the equivalent casts in subagentCall and updateQueueAction's itemId/action) completely disables structural type-checking against the official apiproxy method signatures. The locally declared wrapper payload types can silently drift from the real wire contract (misspelled fields, wrong shapes) and only fail at runtime. Where possible, type the payloads against the official request types instead of forcing as never.
low · maintainability
— Swear Review
| signal.addEventListener('abort', handleAbort, { once: true }) | ||
| if (signal.aborted) { | ||
| handleAbort() | ||
| } |
There was a problem hiding this comment.
When the caller's AbortSignal is already aborted before the generator body first runs, the only thing that ends the stream is handleAbort() relying on socket.close() on a CONNECTING socket to either throw or eventually fire a close/error event. The code itself acknowledges runtime variance here (the catch comment notes "CONNECTING close() throws in some runtimes"). If close() silently no-ops without a terminal event, no end item is ever enqueued: the generator parks forever at await new Promise(...), the finally cleanup never runs, and the socket + listeners leak — which can also stall the bridge reconnect path that awaits the stream's termination. End the stream deterministically in the pre-aborted case instead of depending on close-event semantics.
| signal.addEventListener('abort', handleAbort, { once: true }) | |
| if (signal.aborted) { | |
| handleAbort() | |
| } | |
| signal.addEventListener('abort', handleAbort, { once: true }) | |
| if (signal.aborted) { | |
| handleAbort() | |
| enqueue({ kind: 'end' }) | |
| } |
medium · bug
— Swear Review
| const signal = externalSignal | ||
| ? AbortSignal.any([AbortSignal.timeout(30_000), externalSignal]) | ||
| : AbortSignal.timeout(30_000) |
There was a problem hiding this comment.
The 30s transport timeout is a hardcoded magic number. The rest of the DSH runtime code centralizes such values as named constants (e.g. STOP_TIMEOUT_MS, INSTALL_TIMEOUT_MS, READY_POLL_INITIAL_MS in DshRuntime.ts). Extract it to a module-level constant so it is documented, reusable, and consistent with the existing convention — and so a long-running prompt that needs a different budget can be tuned in one place.
| const signal = externalSignal | |
| ? AbortSignal.any([AbortSignal.timeout(30_000), externalSignal]) | |
| : AbortSignal.timeout(30_000) | |
| const PROMPT_TIMEOUT_MS = 30_000 | |
| const signal = externalSignal | |
| ? AbortSignal.any([AbortSignal.timeout(PROMPT_TIMEOUT_MS), externalSignal]) | |
| : AbortSignal.timeout(PROMPT_TIMEOUT_MS) |
low · maintainability
— Swear Review
| this.childRecoveryReleased.add(childSessionId) | ||
| buffered | ||
| .sort((a, b) => a.seq - b.seq) | ||
| .forEach((event) => this.handleSessionEvent(childSessionId, event, 'live', true)) |
There was a problem hiding this comment.
Backfilled child events never reach onMessage with source='backfill'. In backfillChildJournals() the replay passes source 'backfill', but while the child's buffer is sealed (journalRecoveryInFlight is true and the buffer was created at the top of the loop) handleSessionEvent re-buffers the event and returns. The drain below then forwards everything with source 'live' + forceForward, so the 'backfill' label is silently lost and any consumer distinguishing recovered vs live history (suppressing notifications/scroll for recovered subagent journals) would treat all replayed child events as live. Either drain with the original source or forward the replayed events directly (forceForward=true, source='backfill') before draining only the truly live frames.
medium · bug
— Swear Review
| : agentFlavor === 'dsh' | ||
| ? dshModelOptions | ||
| // Pi uses its own provider-qualified picker (piModels prop). |
There was a problem hiding this comment.
Deeply nested ternary chains for availableModelOptions / availableEffortOptions (and availableModelReasoningEffortOptions): after threading the dsh branch in, the model-options expression is now 4–6 levels deep, and the reasoning-effort expression similarly. This violates the no-nested-ternary guideline and makes later flavor additions error-prone. Suggest extracting a small computeModelOptionsForFlavor(...) / flavor-keyed lookup so each agent flavor is a flat case.
low · maintainability
— Swear Review
| return { | ||
| models: query.data ?? null, | ||
| isLoading: query.isLoading, | ||
| error: query.error instanceof Error ? query.error.message : (query.error ?? null), |
There was a problem hiding this comment.
The error mapping here returns the raw error message (e.g. HTTP 502 Bad Request: {"error":"..."} from the hub route), which is user-facing in the model picker. Sibling hooks (useCursorModels/usePiModels/useCodexModels) map errors to a friendly localized fallback like 'Failed to load X models'. Consider applying the same pattern (e.g. Failed to load DSH models) for consistency and better UX.
low · maintainability
— Swear Review
| 'newSession.type.worktree.placeholder': 'feature-x (默认 1228-xxxx)', | ||
| 'newSession.agent': '代理', | ||
| 'newSession.model': '模型', | ||
| 'newSession.dshModelHint': 'DeepSeek Harness 的模型會在 session 建立後動態載入 —— 建立後在 session 設定中選擇。', |
There was a problem hiding this comment.
This Simplified Chinese (zh-CN) locale string is written in Traditional Chinese (會/在/動態/載入/設定 instead of 会/在/动态/加载/设置), and it's the only Traditional Chinese line in the file. Since this is a zh-CN locale file, it should be converted to Simplified Chinese for consistency, e.g. 'DeepSeek Harness 的模型会在 session 建立后动态加载 —— 建立后在 session 设置中选择。'
| 'newSession.dshModelHint': 'DeepSeek Harness 的模型會在 session 建立後動態載入 —— 建立後在 session 設定中選擇。', | |
| 'newSession.dshModelHint': 'DeepSeek Harness 的模型会在 session 建立后动态加载 —— 建立后在 session 设置中选择。', |
medium · bug
— Swear Review
| 'dsh.model': '模型', | ||
| 'dsh.mode': '模式', | ||
| 'dsh.mode.standard': '标准模式', | ||
| 'dsh.mode.code': 'PTC 模式', |
There was a problem hiding this comment.
'dsh.mode.code' (English source: 'Code mode') is translated as 'PTC 模式'. 'PTC' appears nowhere else in the codebase and doesn't correspond to the key value 'code'; it looks like a copy-paste/typo error. Users will see an unexplained 'PTC 模式' in the mode selector. It should be '代码模式' to match the surrounding pattern (标准模式/极简模式/创造模式).
| 'dsh.mode.code': 'PTC 模式', | |
| 'dsh.mode.code': '代码模式', |
medium · bug
— Swear Review
| 'dsh.loadPresets': '加载预设', | ||
| 'dsh.skills': 'Skills', | ||
| 'dsh.subagents': '子代理', | ||
| 'dsh.loadSubagents': '列出子代理', |
There was a problem hiding this comment.
'dsh.loadSubagents' is translated as '列出子代理' (list subagents), while the sibling key 'dsh.loadPresets' is '加载预设' (load presets). Both keys describe the same 'load' action, so the translation is inconsistent. It should be '加载子代理' for consistency.
| 'dsh.loadSubagents': '列出子代理', | |
| 'dsh.loadSubagents': '加载子代理', |
low · style
— Swear Review
Critical: childCursorByChild was shadowed by a nested redeclaration — onChildCursor wrote the inner map while flushCursor read the outer one, so subagent journal cursors were never persisted. Removed the shadow. High/medium fixes: - Bridge: abort listener leak in waitForAbortableDelay; child-journal seal starts before pumps dispatch (queued frames before subscription); retryMs resets after a healthy generation; session-removed cleans childLastSeq/childBuffers; CURSOR_FLUSH_MS/lastCursorFlush dead code removed; asString evaluated once - Transport: promptDirect checks HTTP status; readWebSocket handles socket 'error' (enqueues stream end); SESSION_PROMPT_PATH constant - Runtime: install child stdout ignored (pipe-block risk); overlayDir cleaned on every failure path; readiness race timer cleared; stop() clears SIGKILL timer + exit listener; isDshRuntimeInstalled removed - Projector: tool-result content optional-chained; end-seed persisted as dsh_native; stateSnapshot seq no longer overridable by folded seq; dead step state (currentTurn/streamed/finished/finalized*) removed - Hub: rewind finalize moved outside the archive CAS try/catch - Web: queue/jobs arrays optional-chained (unvalidated payloads); dispatch swallows failures (callers are fire-and-forget); status summary labels localized; dsh.goal i18n keys added - Shared: dshChildCursors validated int().nonnegative() - Doctor: imports cleaned, unreachable catch removed, ternary flattened
6bc1fe0 to
0bdebd1
Compare
Shared:
- Add 'dsh' to AGENT_FLAVORS / AgentFlavorSchema / CREATABLE_AGENT_FLAVORS
- DSH permission presets are runtime-discovered: getPermissionModesForFlavor('dsh') returns []
- Metadata gains dshSessionId / dshRuntimeVersion / dshEventCursor (Zod-validated)
- sessionSummary maps flavor dsh -> dshSessionId
- Web: NewSession MODEL_OPTIONS.dsh = [] (runtime-discovered, no static presets);
AgentFlavorIcon renders the official DeepSeek Harness wave mark (currentColor)
CLI (cli/src/dsh/):
- DshRuntime: spawns the official pinned dsh runtime (--profile web) with a
host-only overlay (no web UI: GET / -> 404, verified), loopback port probe,
readiness handshake via host.describe, graceful SIGTERM stop, crash capture,
auto-install under HAPI_HOME/dsh-runtime (bun, npm fallback)
- DshNodeTransport: AbstractApiClient subclass speaking the exact official
apiproxy wire (HTTP POST /api/<endpoint> + WebSocket event streams) the
official web UI uses, without any browser bundle
- DshClient: typed unwrap of unary calls (DshRpcError on business failure),
create-as-resume with preallocated session ids, mux/host streams
- Tests: fixture host speaking official schemas (6), fake-runtime spawn tests
(4), real-host integration suite gated by HAPI_DSH_INTEGRATION=1 (4, verified
locally against dsh 0.1.0-rc.6: no frontend served, idempotent resume,
session-conflict on cwd mismatch, mux subscribed frames)
…idge Shared protocol (shared/src/dsh.ts): HAPI's own DSH-shaped, DSH-package-free allowlisted vocabulary — DshActionSchema (prompt/interrupt/approval/question/ queue/model/goal/subagent/presets/history/fork/feedback), durable native event + state snapshot views (queue/jobs/goal/questions/approvals), and web-facing model/subagent/preset/skill/history response schemas. New RPC methods dsh-action / dsh-models / dsh-skills. CLI session runner: - DshProjector: native SessionEvent → HAPI messages. assistant/chunk deltas stream as live snapshots with stable turn/step/block ids (reconnect-safe), assistant/message/tool events emit final forms; every non-chunk event is persisted as dsh_native (tool trees, subagent, workflow, plan, goal semantics survive replay); dshSeq + dshMessageId anchors for fork/feedback. - DshEventBridge: pumps official mux+host streams; approval frames → HAPI agentState permission lifecycle; question/queue/jobs/projection frames → dsh_state snapshots; subagent child events persisted natively; cursor callback for at-most-once resume. - DshRpcBridge: Zod-validated allowlisted dispatch, no arbitrary method proxy; legacy Permission RPC maps to the official two-outcome approval response; host-global surfaces (settings/credentials/preset authoring) deliberately absent. - runDsh: bootstrap → host-only DSH runtime spawn → create-as-resume (HAPI id = DSH id) → bridge → queue-mode prompts; kill handler + lifecycle stop the host gracefully. - hapi dsh command + runner buildCliArgs dispatch. Tests: projector 10, event bridge 5 (fixture host), full cli suite green.
The DSH host loads NAPI modules (node-pty) that crash under Bun's libuv shim (oven-sh/bun#18546: unsupported uv function uv_version_string). The HAPI CLI itself often runs under Bun, so process.execPath is wrong. - Spawn .js runtime bins with a resolved node binary (HAPI_DSH_NODE_PATH override, else 'node' from PATH). - installDshRuntime now requires node to be present (execution needs it even when bun performs the install) with a clear error message. Verified end-to-end against a real host: spawn -> create-as-resume -> prompt (queue accepted) -> mux frames (queue/turn/step/user-message/ projection) -> cancel -> graceful stop.
- rpcGateway: dshAction (allowlisted DshActionSchema payload), dshModels,
dshSkills session-scoped RPC wrappers
- SyncEngine public passthroughs (private rpcGateway stays encapsulated)
- REST: POST /api/sessions/:id/dsh/{action,models,skills} with namespace/
ownership gating via requireSessionFromParam, flavor gate (dsh only),
Zod validation before any RPC, 502 on CLI failures
- Tests: routing, flavor gate, malformed payload rejection, namespace
denial, model/skill catalog passthrough, RPC failure surfacing (7 tests)
- DshSessionView (flavor-gated in the session route): conversation with streaming text/reasoning/tool cards + fork-at-message, session header with model/effort/running/pending-approval state, goal bar (pause/resume/ complete/clear), queue dock (steer/remove), background jobs dock, pending user-question dialog (single/multi/free-form), approval panel driven by agentState (allow-once/deny through the legacy permission RPC), runtime- discovered model picker (provider groups + reasoning efforts), agent preset picker, skills palette (leading-/ insertion), subagent list - DshComposer: queue-mode sends through the standard message pipeline - normalizeAgent: DSH projection branches (text/reasoning/tool_call/ tool_result/usage/token-count); dsh_native/dsh_state are folded from raw messages by useDshSessionState (higher-seq-wins), never rendered as blocks - API client + hooks: dshAction (allowlisted), dshModels, dshSkills - i18n en + zh-CN Web suite green (2451 tests); web build passes.
- docs/guide/deepseek-harness.md: architecture, security boundary, feature parity matrix, troubleshooting, version pin - agents.md support matrix row; root + cli README mentions - NewSession: YoloToggle hidden for flavors without permission modes (dsh)
Spawns a fixture DSH host in a child process (real loopback HTTP + WS wire, official envelope schemas) and drives the production paths: create-as-resume, queue prompt, chunk streaming (live snapshots -> final settle), tool call, approval frame with rpcId mapping, tool result, turn_complete, usage, native journal persistence, dshSeq fork anchors, interrupt (cancel), graceful stop.
- runDsh keeps the hub session alive (session-alive every 2s, thinking flag follows host running status) — without it the hub marks the session inactive and drops the RPC target - DshRuntime kills the spawned host on readiness timeout so failed starts never leak child processes Verified end-to-end against a real host + hub: session active, dshSessionId = HAPI id, runtime version + conversationHistory capabilities persisted, queue-mode prompt streams dsh_native (27) / dsh_state (5) / turn_complete / error messages into the hub DB, interrupt + runtime model catalog (deepseek- official groups) served through the allowlisted routes.
- Fork: CLI registers ForkConversation handler — user-message localIds are mapped to native event seqs (conversationHistoryPoints/Indexes, flushed to metadata) so fork-at-message anchors the exact native log position; session.fork returns the new native id. Hub forkConversation creates the child row with dshSessionId + conversationHistory locators, hydrates the transcript prefix, spawns the child CLI (create-as-resume), and waits for exact native bind. - Rewind: no native DSH rewind exists — official semantics are fork. CLI acknowledges; hub forks a child at the anchor, archives the source with supersededBySessionId (web followSupersedingSession navigates), and stops the old CLI. - DshActionSchema hardened: conditional requirements via superRefine (goal.create needs objective; subagent prompts need child+text; feedback put/delete field requirements) with allowlist tests. - buildCliArgs: dsh never receives --permission-mode/--yolo (tested); hapi doctor reports DSH runtime install/version/node status. - Docs: DSH upgrade procedure (pin + overlay validation).
…ests - Web queue dock gains inline edit (official queue.action edit) - runDsh cursor throttle now persists the LATEST forwarded seq - hapi doctor: DSH runtime installed/version/node availability - Hub tests: DSH fork child metadata + exact-native bind; rewind = fork + archive with supersededBySessionId
The CLI now records each user message's localId when a native user/message
event claims it (FIFO), persisting conversationHistoryPoints +
conversationHistoryIndexes so the web shows fork-at-message affordances and
the ForkConversation handler can anchor the exact native event seq. Verified
live: points {localId: true} + indexes {localId: 108} after a queue prompt.
Also verified live: runner-spawned dsh session (spawn → prompt → native
events → fork current → child active with dshSessionId + 20 hydrated
transcript messages); DSH runtime auto-install under HAPI_HOME/dsh-runtime.
Full suites green (cli 2490 / hub 1089 / web 2452 / shared 269).
…ess pump The E2E pump is long-running by design; awaiting it after abort left the test hanging on runners where the host-stream socket close races. The fixture host now accepts /api/events.host so both pumps settle cleanly.
DSH sessions now render through the standard SessionChat / HappyThread / HappyComposer interface like every other agent — the custom DshSessionView is removed. DSH keeps its native semantics only as side panels: - Router gate removed: dsh -> standard SessionChat (conversation streaming, tool cards, permission cards, composer, queue bar, fork/rewind buttons all standard) - DshSessionPanels embedded above the composer: goal bar, queue dock (steer/remove/edit), jobs dock, runtime-discovered model picker, agent presets, skills (click to invoke /name), subagents, pending user-question dialog — folded from dsh_state snapshots (higher-seq-wins) - CLI: registerSessionConfigRpc (model endpoint -> DSH selectModel with provider resolved from the live catalog; permission/effort rejected — DSH presets are runtime-discovered); prompts emit messages-consumed so the standard queued bar stays empty (DSH queue owns its queue) - Skill palette: onInvoke sends the leading-/ reference directly Verified live on the 43006 test env: slash-commands/skills endpoints serve DSH catalogs, hub model endpoint switches to deepseek-v4-pro via SetSessionConfig -> selectModel. Full suites green (cli 2490 / hub 1089 / web 2455 / shared 269).
…ckers, subagents in header Per parity with Codex/Claude sessions, DSH now uses the standard HAPI surfaces end to end: - Model + reasoning effort: runtime-discovered DSH catalog feeds the standard composer picker (availableModelOptions / availableModelReasoningEffortOptions like Codex); changes go through the standard model / model-reasoning-effort endpoints -> SetSessionConfig -> DSH selectModel (provider resolved from the live catalog; hub effort route now allows dsh). The custom model panel is gone. - Skills: standard leading-/ composer invocation (HAPI slash commands); the custom skills palette is gone. - Agent presets panel removed (out of scope for now). - Subagents: session-header button with activity badge opens a status modal (catalog + running count); the side panel is gone. - DSH-native state (goal/queue/jobs) collapses into a slim status strip above the composer so the thread stays clean; pending user questions still pop a modal. Verified live on the 43006 test env: standard /model -> deepseek-v4-pro and /model-reasoning-effort -> high both apply through the CLI into the DSH host. Suites green (cli 2521 / hub 1102 / web 2481 / shared 269).
…der icon - HappyComposer generated reasoning-effort options only for codex/opencode; dsh now flows through the same dynamic-options path (catalog efforts: off/high/max with default high) and SessionChat passes modelReasoningEffort through so the current value renders - Header subagent toggle now uses the Lucide 'users' glyph at 18x18, matching Files/Outline/Terminal toggle style and size Verified live: dsh/models catalog exposes efforts for both deepseek models; environment restarted with the new bundle.
- Startup attach race includes hostDone: if the host stream closes before the root subscription, the generation aborts and reconnects instead of hanging - session-ready and user-message dispatch are gated on bridge readiness (root subscribed + initial backfill released): a prompt committed before the mux subscription is no longer replayed as backfill, which could consume its pending localId and corrupt fork/rewind anchors - DshActionSchema drops prompt / model.select / fork: those mutations are owned by HAPI orchestration (message persistence, localId mapping, config metadata, child-session creation) and would corrupt state if called directly; web already routes them through the standard HAPI paths
- Prompt identity via the host-echoed rpcId: DshClient.prompt returns the dispatch rpcId; user/message events' MessageSource.rpcId binds the HAPI localId to the exact native seq. Acceptance-order FIFO removed — it could not represent commands emitting no user/message, removed queue items, steered ordering, or edited prompt text (wrong fork/rewind anchors, diverging fork-child transcripts) - Subagent journals gap-fill on reconnect: per-child lastSeq tracked with seq guard, subagent.history paged (continuable with one-shot fallback) after every successful root backfill — child activity during a mux outage is no longer permanently missing from dsh_native - Hub dsh route test uses the remaining allowlisted actions (prompt/ model.select/fork are orchestration-owned and now rejected)
… (round 16) - Prompt rpcId is reserved and the localId binding registered BEFORE the HTTP round-trip (DshNodeTransport.mintRpcId drains a reserved queue): a user/message event that beats the prompt response back still correlates to its HAPI row; failed prompts drop the stale binding - Subagent recovery: children created entirely during an outage are discovered via subagent.list (carries each child's mode); live child frames are buffered while that child's history replay is in flight so the cursor can never advance past an unfetched gap
- Prompt dispatch bypasses the shared mint queue: DshClient.prompt posts session.prompt directly under a caller-owned rpcId (unrelated unary calls can no longer consume a queued prompt's reservation) - Child journal replay failure keeps the live buffer sealed and the cursor untouched so the next reconnect retries the gap; per-child cursors persist in metadata (dshChildCursors) and restore on restart, so subagent journals never replay into a fresh HAPI row - dsh_state snapshots never regress their overall seq (emitState clamps to the highest emitted seq); late bootstrap projections with older asOfSeq can no longer make the web discard valid fields - Fork cursor probe falls back to the fork point (atSeq) or the latest persisted root cursor, so a transient probe failure cannot replay the already-copied native prefix into the child row
- sessionFactory preserves dshChildCursors across reopens — child replay cursors are durable, not process-local - Failed child-history replay releases the live buffer (frames forward as live, advancing the durable cursor); the next reconnect's history fetch closes whatever gap the outage left — a child can never stay permanently buffered - Fork-current cursor fallback uses the bridge's live-forwarded seq (not just the throttle window's persisted metadata), so fork-current probe failure inside the flush window still seeds a correct cursor - DSH rewind re-reads the source row after KillSession and retries the metadata CAS once (the source CLI's final cursor flush can bump metadataVersion mid-kill) before reporting archive failure
- onCursor's throttle timer now routes through flushCursor, which writes BOTH the root cursor and per-child journal cursors in one metadata update — child cursors are no longer process-local - Failed child backfill keeps the buffer sealed and the cursor untouched (next reconnect retries the full gap) with a bounded 5k-frame overflow fallback; releasing the buffer would advance the durable cursor past the missing gap - Every KNOWN child journal is sealed before the root backfill starts — child events forwarded during root history fetch can no longer advance their cursors past the outage gap - latestForwardedSeq seeds from the persisted dshEventCursor, so a resumed idle session's fork-current fallback can never use the 0 sentinel - Successful rewind archival finalizes cache refresh + session-end broadcast BEFORE returning success (the previous return made the finalization dead code)
- Child-only activity arms the shared cursor-flush throttle (onChildCursor schedules the same timer), so per-child cursors persist in metadata even when the root stream is idle — restart replay of subagent journals is gone - Failed child-history replay aborts the generation and reconnects with backoff (buffers stay sealed, cursors untouched); the bounded overflow release that could advance a cursor over a missing range is removed - rootBackfillInFlight seals EVERY child journal during root history fetch — children first seen mid-backfill buffer their events too, so no child cursor can advance past outage events
- flushCursor now actually writes dshChildCursors (merged with stored values) — the round-18 patch had silently missed the replace target; child-only activity persists its cursors via the shared throttle - Failed child replay resets initialBackfillDone so the next generation re-runs the WHOLE initial recovery (root re-seals journals); root events stay buffered until recovery succeeds - journalRecoveryInFlight covers root history fetch AND child replays, so children first seen at any point during recovery are sealed; child replay buffers merge instead of overwriting previous generations' sealed buffers
11 fixture-host tests exercising the paths the review bots kept flagging: - A1-A3: first-generation backfill, live-events-during-backfill buffering, failed-backfill abort/retry - A4-A6: host-close-before-subscription, reconnect gap-fill (no dupes), live-events-before-reconnect-backfill - A7-A10: per-child cursors, unknown-child sealing + discovery replay, failed child backfill retry, live-child-during-replay buffering - A13: question/resolved emits null (no empty dialog) Also fixes a real bug the tests exposed: releasing a child's sealed buffer re-buffered the frames because journalRecoveryInFlight was still active — forceForward now bypasses the seal so released frames always forward. Fixture gains disconnectMux/disconnectHost/muxSocketCount + subagent handlers + closeAllConnections.
- C1-C3 (DshClient): promptDirect posts under the caller-owned rpcId, response rpcId mismatch throws, and an intervening unary call cannot consume a prompt reservation - E1-E4 (fixture): rpcId binds the HAPI localId to the native user/message seq; the mux event beating the prompt HTTP response still binds (pre-registered); rejected prompts leave no stale binding; events without a rpcId never consume a pending binding - E5: readiness contract (history probed before prompt dispatch)
- F1/F2 (hub): fork child seeds dshEventCursor from nativeCursor; absent cursor leaves it unset (child bridge defaults to its own fallback) - F3: rewind archive CAS failure cleans up the live fork child - G1: resolveAgentResumeId resolves dshSessionId - H1/H3 (cli): pickExistingSessionMetadata (now exported) preserves the full DSH resume identity — dshSessionId/runtimeVersion/eventCursor/ childCursors/selectedModel — and invents nothing for undefined fields
The setup() history wrapper compared the raw return of an async handler against undefined — a Promise<undefined> passed the check and reached the server as an undefined result, producing unhandled ZodErrors in CI. Await the custom handler before the fallback; recovery + prompt-identity suites now run clean.
- readDshRuntimeVersion resolves the dsh package manifest two parents above bin.js (the bot-flagged wrong-manifest bug is pinned by a test) - Unreadable manifests return null (upgrade path triggers) - Explicit overrides with a missing runtime fail with kind spawn and never auto-install
Critical: childCursorByChild was shadowed by a nested redeclaration — onChildCursor wrote the inner map while flushCursor read the outer one, so subagent journal cursors were never persisted. Removed the shadow. High/medium fixes: - Bridge: abort listener leak in waitForAbortableDelay; child-journal seal starts before pumps dispatch (queued frames before subscription); retryMs resets after a healthy generation; session-removed cleans childLastSeq/childBuffers; CURSOR_FLUSH_MS/lastCursorFlush dead code removed; asString evaluated once - Transport: promptDirect checks HTTP status; readWebSocket handles socket 'error' (enqueues stream end); SESSION_PROMPT_PATH constant - Runtime: install child stdout ignored (pipe-block risk); overlayDir cleaned on every failure path; readiness race timer cleared; stop() clears SIGKILL timer + exit listener; isDshRuntimeInstalled removed - Projector: tool-result content optional-chained; end-seed persisted as dsh_native; stateSnapshot seq no longer overridable by folded seq; dead step state (currentTurn/streamed/finished/finalized*) removed - Hub: rewind finalize moved outside the archive CAS try/catch - Web: queue/jobs arrays optional-chained (unvalidated payloads); dispatch swallows failures (callers are fire-and-forget); status summary labels localized; dsh.goal i18n keys added - Shared: dshChildCursors validated int().nonnegative() - Doctor: imports cleaned, unreachable catch removed, ternary flattened
Real bugs fixed: - DshClient.prompt accepted was coerced to true even when the host rejected (latent correctness bug) — now boolean - Projector: turn/end without reason guarded; tool-call-delta without argumentsDelta no longer appends 'undefined'; tool/result finds the result block instead of assuming content[0] - Bridge: children whose replay finished stay released (their live events forward while siblings replay — previously re-sealed forever); bootstrap retries after later re-subscription when all attempts fail; consumer throws contained; handler errors logged at warn - Transport: promptDirect composes the caller's abort signal, checks HTTP status + envelope shape, guards json() parse; WebSocket error listener registered/removed; CONNECTING close() guarded - Runtime: exit-before-readiness cleans overlayDir; install watchdog (5min SIGKILL); JSDoc placement fixed - Fixture: catch-all HTTP responses (never hang the client), OPEN-only sends, deterministic rpcIds, pendingServerRequests populated - resume.test.ts gains a DSH branch test; isDshMessage + unused import + dead code removed
- Bridge: empty-page hasMore=true bails (no infinite pagination); sealed children missing from childLastSeq are still replayed (their buffers release); session-added keeps an existing projector fold; child onMessage contained like root - Transport: abort during CONNECTING ends the stream; console.error replaced with structured logger (TUI-safe) - Runtime: readiness-timeout SIGTERM gets a 5s SIGKILL fallback; install failures wrap in DshRuntimeStartErrorImpl; stdout captured in the failure tail - DshClient: respond receipt documented; gatewayCall guards HTTP status, json parse, envelope shape; crypto.randomUUID -> node:crypto - runDsh: one catalog fetch per model switch (provider resolution + default effort share it) - hub: dshModels uses MODEL_LIST_RPC_TIMEOUT_MS; models/skills routes requireActive (consistent 409 instead of opaque 502); sessionCache redundant cast removed - DshRpcBridge workingDirectory documented as reserved
- Bridge: stream-pump rejections cannot escape the reconnect loop (race guarded); child-journal seal now starts BEFORE the pumps dispatch (covers frames queued before the subscription); subagent discovery failure retries the whole recovery instead of silently skipping outage-created children - Projector: turn/end tolerates missing reason/turn payloads - Runtime: install node probe honors HAPI_DSH_NODE_PATH; stop() guards kill races - DshClient: subagentList tolerates a missing entries array - runDsh: dead resolveModelProvider removed - DshRpcBridge: feedback.put/delete return the recorded row (not an empty list shape) - Integration fixture serves subagent.list/history
- Projector: assistant/message final ids now map through the recorded block-start chunk order — content array position can diverge when tool blocks interleave, which made the reducer duplicate instead of merge - Bridge: stream-attach race guarded like the pump race; children sealed dynamically during recovery but absent from the discovery snapshot get their buffers released; goal status omits undefined instead of spreading it - Runtime: sync spawn throw cleans the overlay dir and fails loud; install watchdog timeout constant; install verifies the installed version matches the pin - Transport: wss:// never downgraded; promptDirect error surfaces the response body; unused import removed - runDsh: attachment paths escaped in the marker line; effort options documented as intentionally not applied at launch - doctor: node availability checked against the actual binary, not the always-present install dir
- Bridge: question/resolved validates the pending question rpcId (stale or duplicated resolves ignored); subagentIds re-aligned against the host's durable catalog after reconnect (missed session-removed frames no longer keep subagentCount stale); waitForAbortableDelay resolves immediately on an already-aborted signal; onReady doc corrected - runDsh: startingMode ternary simplified (DSH is always remote); bridgeReady gains a 60s stall timeout that fails the session loudly instead of hanging in 'connecting' - DshRpcBridge: reasoning efforts array guarded before map - DshRuntime: options.port 0 now probes an ephemeral port - DshNodeTransport: prompt timeout constant - doctor: node probe honors HAPI_DSH_NODE_PATH - Test: A13 uses the real pending question rpcId
0bdebd1 to
39bb9f9
Compare
|
/swear-review incremental |
|
/swear-review full |
|
Swear Review: full review queued (job tiann#331). |
|
/swear-review full |
|
Swear Review: full review queued (job tiann#339). |
1 participant
Standalone review copy of tiann#1574 for Swear Review (app is installed on swear01 only).
This PR is for independent bot review only — the canonical PR remains tiann#1574.