diff --git a/app/src/main/kotlin/com/youcoded/app/runtime/SessionService.kt b/app/src/main/kotlin/com/youcoded/app/runtime/SessionService.kt index 97d46f981..aa34b255e 100644 --- a/app/src/main/kotlin/com/youcoded/app/runtime/SessionService.kt +++ b/app/src/main/kotlin/com/youcoded/app/runtime/SessionService.kt @@ -4234,6 +4234,9 @@ class SessionService : Service() { // native:set-binding below — has a msg.id, so this replies // not-implemented-on-mobile rather than no-op'ing. "native:queue-remove", + // "Send now" on a waiting message — request/response like + // native:queue-remove, so it replies not-implemented-on-mobile. + "native:queue-send-now", "native:interrupt", // Stalled-turn Retry. Fire-and-forget (no msg.id) exactly like // native:send / native:interrupt, so this is a correct no-op here: diff --git a/desktop/line-budgets.json b/desktop/line-budgets.json index 024dbcd6a..324a1fee6 100644 --- a/desktop/line-budgets.json +++ b/desktop/line-budgets.json @@ -7,22 +7,22 @@ "renderer/dev/workbench/fixtures/marketplace/registry.ts": "generated sample of the marketplace registries (regenerated by a script, not hand-edited)" }, "budgets": { - "main/ipc-handlers.ts": 5728, - "main/harness/native-session-host.ts": 5296, - "renderer/App.tsx": 4966, - "main/remote-server.ts": 4217, + "main/ipc-handlers.ts": 5731, + "main/harness/native-session-host.ts": 5311, + "renderer/App.tsx": 4981, + "main/remote-server.ts": 4223, "main/harness/harness-session.ts": 4229, "renderer/components/SettingsPanel.tsx": 3256, - "renderer/remote-shim.ts": 3176, + "renderer/remote-shim.ts": 3177, "renderer/state/chat-reducer.ts": 3397, "renderer/styles/globals.css": 2960, "renderer/components/SessionStrip.tsx": 2784, "main/main.ts": 2604, - "shared/types.ts": 2391, + "shared/types.ts": 2394, "renderer/components/SyncPanel.tsx": 2060, "main/engine/engine-manager.ts": 2035, "renderer/components/ResumeBrowser.tsx": 2069, - "main/preload.ts": 1961, + "main/preload.ts": 1963, "renderer/components/StatusBar.tsx": 1809, "renderer/components/ToolCard.tsx": 1636 } diff --git a/desktop/package.json b/desktop/package.json index 096bb30a8..07e79b5ff 100644 --- a/desktop/package.json +++ b/desktop/package.json @@ -17,7 +17,7 @@ "lint": "oxlint --type-aware", "postinstall": "node scripts/patch-node-pty.js && node scripts/patch-xterm-webgl-mipmap.js", "typecheck": "tsgo --noEmit -p tsconfig.json && tsgo --noEmit -p tsconfig.tests.json", - "lint:design": "oxlint -c .oxlintrc.design.json --format default --max-warnings 542" + "lint:design": "oxlint -c .oxlintrc.design.json --format default --max-warnings 537" }, "keywords": [], "author": "itsdestin", diff --git a/desktop/src/main/harness/busy-message-boundary.ts b/desktop/src/main/harness/busy-message-boundary.ts new file mode 100644 index 000000000..614d62bca --- /dev/null +++ b/desktop/src/main/harness/busy-message-boundary.ts @@ -0,0 +1,66 @@ +import type { ModelMessage } from 'ai'; +import { finalizeRemainingCalls } from './tool-group-finalization'; + +export interface BusyMessage { + id: string; + text: string; + attachments: string[]; + restore?: () => void; +} + +/** WHY: the claim and acceptance are one synchronous boundary. An exception + * returns the unaccepted head to its host, rather than losing a queued send. */ +export function claimBusyMessage( + take: (() => BusyMessage | undefined) | undefined, + accept: (item: BusyMessage) => void, + beforeAccept?: () => void, +): boolean { + const item = take?.(); + if (!item) return false; + try { + beforeAccept?.(); + accept(item); + } catch (err) { + item.restore?.(); + throw err; + } + return true; +} + +/** WHY: the emitted event is the authority for both live and rebuilt history; + * no synthetic steer or app-generated marker is used for human input. */ +export function appendUserHistory( + text: string, attachments: string[], emit: () => string, appGenerated: boolean, + imageParts: (paths: string[]) => Array<{ type: 'file'; mediaType: string; data: Buffer }>, + markAppGenerated: (message: ModelMessage) => ModelMessage, + history: ModelMessage[], origins: Array, record: (uuid: string) => void, +): void { + const images = imageParts(attachments); + const uuid = emit(); + const message = (images.length + ? { role: 'user', content: [{ type: 'text', text }, ...images] } as ModelMessage + : { role: 'user', content: text } as ModelMessage); + history.push(appGenerated ? markAppGenerated(message) : message); + origins.push([uuid]); + record(uuid); +} + +/** Pair every unstarted call before adding new human input. Already completed + * results stay at the front of the same tool message. */ +export function supersedeToolGroup( + calls: readonly TCall[], from: number, completed: TPart[], completedOrigins: string[], + emit: (data: { toolUseId: string; toolName: string; toolResult: string; isError: true }) => string, + part: (call: TCall, text: string) => TPart, + record: (uuid: string) => void, commit: (parts: TPart[], origins: string[]) => void, + injectCompleted: (calls: TCall[]) => void, + reason = 'Not run: new user input arrived before this action started.', + afterCompleted?: () => void, +): void { + const remaining = finalizeRemainingCalls(calls, from, + () => reason, + (call, text) => emit({ toolUseId: call.toolCallId, toolName: call.toolName, toolResult: text, isError: true }), part); + remaining.origins.forEach(record); + commit([...completed, ...remaining.parts], [...completedOrigins, ...remaining.origins]); + injectCompleted(calls.slice(0, from)); + afterCompleted?.(); +} diff --git a/desktop/src/main/harness/eval/run-case.ts b/desktop/src/main/harness/eval/run-case.ts index fea8c4f68..085709833 100644 --- a/desktop/src/main/harness/eval/run-case.ts +++ b/desktop/src/main/harness/eval/run-case.ts @@ -9,6 +9,7 @@ import { ASSISTANT_PRESET } from '../../../shared/harness-manifest'; import { CORE_TOOLS } from '../tools'; import { seedFixtureWorkspace } from './fixture-workspace'; import { assembleSystemPrompt } from '../prompt-assembly'; +import { prepareProjectInstructions } from '../injection/project-instructions'; import { resolvePreset } from '../preset-registry'; import { BATTERY_PROMPT } from './battery'; import type { TranscriptEvent } from '../../../shared/types'; @@ -411,6 +412,9 @@ export async function runCase(opts: RunCaseOpts): Promise { const wrapUpPrompt = opts.wrapUpPrompt ?? WRAP_UP_PROMPT; const fixtureRoot = seedFixtureWorkspace(opts.instructions); + // WHY: evaluator uses the production async inventory, but fixture experiments + // cannot silently inherit real instructions above their disposable root. + const projectInstructionFiles = await prepareProjectInstructions(fixtureRoot, 20_000, fixtureRoot); const events: TranscriptEvent[] = []; let toolCalls = 0; let asks = 0; @@ -486,6 +490,7 @@ export async function runCase(opts: RunCaseOpts): Promise { presetBody: resolvePreset('assistant').body, cwd: fixtureRoot, appVersion: 'eval', + projectInstructionFiles, }), // Load-bearing (2026-08-11): omitted, HarnessSession takes fitToContext's // own 32_768 default, which against this harness's output ceiling left a diff --git a/desktop/src/main/harness/harness-session.ts b/desktop/src/main/harness/harness-session.ts index d19f01fe7..a3bcd6a54 100644 --- a/desktop/src/main/harness/harness-session.ts +++ b/desktop/src/main/harness/harness-session.ts @@ -172,6 +172,9 @@ export function rememberedRuleFor( } import { formatAnswers } from './tools/ask-user-question'; import { formatArgErrors } from './tools/arg-errors'; +import { parseToolArgs } from './tool-args'; +import { PermissionCallbackFailure, permissionCallback, finalizeRemainingCalls } from './tool-group-finalization'; +import { appendUserHistory, claimBusyMessage, supersedeToolGroup } from './busy-message-boundary'; import type { AskRequest, AskDecision } from './permission-broker'; import { CLOUD_DEFAULT, type CapabilityProfile } from './capability-profile'; import { adaptForWire } from './wire-adapter'; @@ -187,7 +190,10 @@ import type { RunningCalls } from './askpass/running-calls'; import type { AdminPasswordServiceLike } from './admin-password-service'; import { createSkillCatalog, type SkillCatalog } from './skills/skill-catalog'; import { fitInjection } from './injection/injection-budget'; +import { pendingRules, deliverRules } from './injection/rule-delivery'; +import type { PathTrigger } from './injection/path-triggers'; import type { TriggerIndex } from './injection/path-triggers'; +import { retainedRuleMessages } from './injection/retained-rule-visibility'; import { mcpToolsFor, estimateToolSchemaTokens } from './mcp/mcp-tools'; import type { ReadyServer } from './mcp/mcp-manager'; import { @@ -220,6 +226,9 @@ export interface HarnessSessionOpts { /** The tool set this session may call. Absent/[] = v0 chat behavior (the * Chat-preset path: plain text, no tool plumbing invoked). */ tools?: NativeTool[]; + /** Root-only synchronous FIFO claim. restore returns an unaccepted claim to the host + * if an event append fails; no second turn or await occurs during a claim. */ + takeReadyBusyMessage?: () => ({ id: string; text: string; attachments: string[]; restore?: () => void } | undefined); /** Pure permission decision for (tool, subject) — the configured layers * (preset/mode/deny-list/remembered). Absent → every gated tool asks. */ decide?: (tool: string, subject: string | undefined) => Promise; @@ -812,8 +821,8 @@ export class HarnessSession extends EventEmitter { private toolByName: Map; private readRegistry = new Map(); // canonical path → content fingerprint at last Read (tools/file-fingerprint.ts) /** G-11 (2026-08-26 tools investigation) — what Read has already served this - * session (`path|offset|limit` → mtime + which call). Read answers a repeat - * of an unchanged slice with "the content you already have is current" + * session (`path|offset|limit` → fingerprint + which call). Only after + * verifying current bytes does Read say "the content you already have is current" * instead of the content. That sentence is only true while the earlier * result is still in the model's view, so this is cleared at EVERY site that * discards or shrinks history: seedHistory (resume), clearHistory (/clear), @@ -847,9 +856,14 @@ export class HarnessSession extends EventEmitter { // Resolved capability profile (Task 5). Drives the doom-loop window + tool // attachment; re-assigned by setBinding on a mid-session model swap. private profile: CapabilityProfile; - /** Trigger ids already injected in this session. Survives across turns on - * purpose — a rule is a standing instruction, not a per-turn reminder. */ + /** Rule dedupe survives turns, but is reconciled after history rewrites. */ private readonly injectedTriggerIds = new Set(); + private retainedTriggerMessages = new Set(); + + private reconcileTriggerVisibility(): void { + this.injectedTriggerIds.clear(); + this.retainedTriggerMessages = retainedRuleMessages(this.history); + } /** Delivered-image dedupe: canonical path → { mtimeMs at delivery, the tool * call whose result carried the image }. A model that * re-Reads the SAME unchanged file gets "already visible" text, not a second @@ -997,9 +1011,8 @@ export class HarnessSession extends EventEmitter { continuationBinding?: string; messageOrigins?: Array; }): void { this.history = messages; - // WHY: a flat accepted seed cannot locate a tail cut. Only the event-by- - // event rebuild supplies aligned origins; reject a mismatched or invented - // map instead of guessing a portable resume point. + this.reconcileTriggerVisibility(); + // WHY: only event-by-event origins prove a portable resume cut. const known = new Set(seed?.eventUuids ?? []); this.historyOrigins = seed?.messageOrigins?.length === messages.length && seed.messageOrigins.every(part => part === null || part.every(uuid => known.has(uuid))) @@ -1371,49 +1384,29 @@ export class HarnessSession extends EventEmitter { } } - /** Append any project rules / nested project instructions that the paths this - * step touched activate (M3 item 3). - * - * As a MESSAGE, never a system-prompt edit. prompt-assembly.ts is byte-stable - * by construction and a mid-session change would discard the KV cache prefix - * every local model reuses, turning a cheap follow-up turn into a full - * re-prefill of the entire conversation. - * - * ONCE per trigger per session. A rule re-sent after every Read of a matching - * file would dominate the conversation and blow the window it was sized - * against — and repetition does not make a model follow a rule harder. - * - * Bash is skipped: its permission subject is a command string, not a path, - * so feeding it to a path matcher would be a category error. - */ - private injectPathTriggers(calls: ToolCall[]): void { - const index = this.opts.triggers; - if (!index) return; - for (const call of calls) { - // Same reasoning as the path guard: matching a rule glob against a bash - // command or a skill id is a category error, not a near-miss. - if (NON_PATH_SUBJECT_TOOLS.has(call.toolName)) continue; - const tool = this.toolByName.get(call.toolName); - const subject = tool?.permissionSubject(call.input as any); - if (!subject) continue; - for (const t of index.match(subject)) { - if (this.injectedTriggerIds.has(t.id)) continue; - this.injectedTriggerIds.add(t.id); - // t.source is the rule's path relative to cwd — inside the project, so the - // model can actually read it when the notice tells it to. - const fitted = fitInjection(t.body, this.profile.injectionBudgetTokens, t.source); + /** Project guidance is history, never a system-prompt edit (KV-cache prefix). + * Selection, fit and retained-context dedupe are shared with the pre-write + * barrier in rule-delivery.ts; Bash/Task/Skill/web have no file subjects. */ + private pendingPathTriggers(calls: ToolCall[]): PathTrigger[] { + return pendingRules(calls, this.opts.triggers, this.toolByName, + this.profile.injectionBudgetTokens, NON_PATH_SUBJECT_TOOLS, + this.injectedTriggerIds, this.retainedTriggerMessages); + } + + private appendPathTriggers(triggers: readonly PathTrigger[], bounded = false): void { + deliverRules(triggers, this.profile.injectionBudgetTokens, bounded, + this.injectedTriggerIds, this.retainedTriggerMessages, content => { this.historyOrigins.push(null); - this.history.push(markAppGenerated({ - role: 'user', - content: `\n${fitted.text}\n`, - })); - this.capture.mutated(); // injected rule text, backed by no event - } - } + this.history.push(markAppGenerated({ role: 'user', content })); + this.capture.mutated(); + }); + } + + private injectPathTriggers(calls: ToolCall[]): void { + this.appendPathTriggers(this.pendingPathTriggers(calls)); } /** Add or remove the Skill tool to match the CURRENT profile (M3 item 1). - * * Skill is the one conditional tool: its description lists every offered * skill's id and one-liner, and that rides the schema on every turn, so a * small window cannot afford it. Those sessions still reach skills through @@ -1950,6 +1943,7 @@ export class HarnessSession extends EventEmitter { // automatically at the trigger instead of only on an explicit /clear. this.shownImages.clear(); this.history = replacement; + this.reconcileTriggerVisibility(); this.historyOrigins = [null, ...this.historyOrigins.slice(cut)]; this.reprojectContextUsed(estimateBefore); // Existing frozen event (no new type). `summary` is the canonical field the @@ -1977,9 +1971,7 @@ export class HarnessSession extends EventEmitter { return true; } - /** Make a pruned copy of the history the live history, with every - * bookkeeping consequence a prune has. Shared by the 'prune' decision and - * the summary commit above. */ + /** Adopt a pruned history and its bookkeeping consequences. */ private commitPrune(pruned: ModelMessage[]): void { const before = this.history; // Prune can collapse an image 'content' output to text (compaction.ts) @@ -1998,7 +1990,10 @@ export class HarnessSession extends EventEmitter { // tagging those as a transformation would bump the revision and invalidate a // published checkpoint for a history that never moved. Only a changed // message means "tool output no longer equals its event's text". - if (pruned.some((m, i) => m !== before[i])) { this.capture.markPruned(); this.prefixMoved = true; } + if (pruned.some((m, i) => m !== before[i]) || pruned.length !== before.length) { + this.reconcileTriggerVisibility(); + this.capture.markPruned(); this.prefixMoved = true; + } if (countImageOutputs(pruned) < imagesBefore) this.shownImages.clear(); // G-11: prune may have sliced an old Read result down to 2,000 chars (and // a summary discards it outright) — forget what was served so Read never @@ -2180,6 +2175,7 @@ export class HarnessSession extends EventEmitter { this.shownImages.clear(); this.servedReads.clear(); this.servedSkills.clear(); // same reason as maybeCompact this.history = replacement; + this.reconcileTriggerVisibility(); this.historyOrigins = [null, ...this.historyOrigins.slice(cut)]; this.reprojectContextUsed(estimateBeforeSummary); this.emit('transcript-event', committed); @@ -2211,6 +2207,7 @@ export class HarnessSession extends EventEmitter { if (this.abort) return { ok: false, reason: 'turn-in-flight' }; const estimateBefore = this.rewriteBaseline(); this.history = []; + this.reconcileTriggerVisibility(); this.historyOrigins = []; // What the model still holds after the barrier: the system prompt and the // tool schemas, not the conversation. Without this the status bar kept @@ -2407,22 +2404,10 @@ export class HarnessSession extends EventEmitter { }), [], true); } - /** `attachments` are absolute paths to files the user attached in the composer. - * Image ones become image parts on the user message when the model can see - * them; everything else is ignored here and reaches the model as the path - * text the composer already put in `text`. - * - * WHY images ride ALONGSIDE the text rather than replacing it: `text` is the - * dedup key. The renderer's optimistic bubble is confirmed by an EXACT match - * against the `user-message` event's text (see native-send.ts), so the string - * must stay byte-identical to what the composer built. The paths therefore - * remain in the text AND the pixels are attached — the model gets both, and - * the bubble still resolves. */ + /** WHY: keep composer text byte-identical for optimistic bubble dedup; + * image parts ride alongside it, and paths persist for replay. */ async send(text: string, attachments: string[] = []): Promise { - // Attachments ride the persisted event (paths only — events carry no binary) - // so rebuildHistory can restore the pixels on resume. Emitted only when - // present to keep the no-attachment event byte-identical to before (#290 - // follow-up fix 2). + // Persist paths for image replay; keep no-attachment event shape unchanged. return this.beginTurn(text, () => this.emitEvent('user-message', attachments.length ? { text, attachments } : { text }), attachments); } @@ -2566,34 +2551,29 @@ export class HarnessSession extends EventEmitter { return { text, images }; } - /** The turn driver. `emit` names how this turn ENTERED the conversation — a - * typed message, or a skill invocation — which is the only thing that differs - * between send() and runSkill(). Everything downstream is identical. */ + /** WHY: busy input shares the opening send's durable event/image/capture path. */ + private acceptUserMessage(text: string, attachments: string[], emit: () => string, appGenerated = false): void { + appendUserHistory(text, attachments, emit, appGenerated, paths => this.imagePartsFor(paths), + markAppGenerated, this.history, this.historyOrigins, uuid => this.capture.recordEvent(uuid)); + } + + /** Restore an unaccepted ready head before the host's next drain. */ + private acceptReadyBusyMessage(): boolean { + return claimBusyMessage(this.opts.takeReadyBusyMessage, item => this.acceptUserMessage( + item.text, item.attachments, () => this.emitEvent('user-message', + item.attachments.length ? { text: item.text, attachments: item.attachments } : { text: item.text }))); + } + + /** `emit` distinguishes user sends from skill invocation; the driver is shared. */ private async beginTurn(text: string, emit: () => string, attachments: string[] = [], appGenerated = false): Promise { - // Re-entrancy guard: a non-null abort means a turn is already streaming. - // Throw loudly rather than corrupt the single-slot turn state (see the - // class-level CONCURRENCY PRECONDITION note). + // Never overwrite the active turn's single-slot state. if (this.abort) { throw new Error('HarnessSession: a turn is already in flight — callers must serialize send()/runSkill() per session.'); } this.interrupted = false; this._currentUsageProgress = null; this.bashOutputReadsThisTurn = 0; // G-1 (D7): the cap is per TURN, notice turns included - // The uuid of the user-message / skill-invoked event this turn entered on — - // recorded at the history push below, which is the mutation it accounts for. - const enteringEventUuid = emit(); - // A plain string when there are no image parts — that is the byte-identical - // shape every existing test and rebuildHistory() already assert on, so the - // no-attachment path must not become a one-element parts array. - const imageParts = this.imagePartsFor(attachments); - const enteringMessage = (imageParts.length - ? { role: 'user', content: [{ type: 'text', text }, ...imageParts] } as ModelMessage - : { role: 'user', content: text } as ModelMessage); - this.history.push(appGenerated ? markAppGenerated(enteringMessage) : enteringMessage); - this.historyOrigins.push([enteringEventUuid]); - // Both shapes descend from the SAME event — its text, and (for the image - // parts) the attachment paths it carries. - this.capture.recordEvent(enteringEventUuid); + this.acceptUserMessage(text, attachments, emit, appGenerated); this.abort = new AbortController(); this.turnEverParked = false; // cleared at the start of every turn — see field WHY this.lastStepPromptTokens = 0; // a new turn always begins with a full prefill @@ -2682,6 +2662,14 @@ export class HarnessSession extends EventEmitter { ? undefined : this.opts.harness.limits?.maxSteps; let stepsSinceApproval = 0; + // WHY (Destin, 2026-09-28 PR-review deck Q-2): a message you send mid-task + // is you taking part, so the "keep going?" count starts over whenever one + // joins the running turn — as it did when every message began a new turn. + const absorbReadyMessage = (): boolean => { + if (!this.acceptReadyBusyMessage()) return false; + stepsSinceApproval = 0; + return true; + }; // Consecutive contentless steps (empty-step recovery, spec 2026-08-21). // The single silent retry is allowed only at count 1; any real step resets // it, so an all-empty turn costs exactly two provider calls. @@ -2774,8 +2762,8 @@ export class HarnessSession extends EventEmitter { this.overflowOutputStarted = false; while (true) { try { - step = await withChatGptRequest(this.opts.sessionId, this.opts.isSpecialistChild ? 'specialist' : 'chat', () => this.withRetry(() => - this.consumeStep(model, aiTools, (t) => { partialAssistantText = t; }), + step = await withChatGptRequest(this.opts.sessionId, this.opts.isSpecialistChild ? 'specialist' : 'chat', () => this.withRetry((registerRetraction) => + this.consumeStep(model, aiTools, (t) => { partialAssistantText = t; }, registerRetraction), )); break; } catch (err) { @@ -2969,18 +2957,20 @@ export class HarnessSession extends EventEmitter { }); continue turnLoop; } - // Second consecutive empty step: an orderly completion with an honest - // reason. Set HERE, not in mapStopReason — 'empty_response' is a - // loop-level judgment about two steps, not a mapping of one - // provider finishReason. + // WHY: second empty is a final boundary; new input re-arms recovery. + if (absorbReadyMessage()) { + consecutiveEmptySteps = 0; + continue turnLoop; + } stopReason = 'empty_response'; break; } consecutiveEmptySteps = 0; // any real step re-arms the single retry if (step.toolCalls.length === 0) { - // Natural stop. finishReason 'length' (truncated output, including a - // truncated tool-call) collapses to 'max_tokens' via mapStopReason. + // WHY: no await before completion; late heads use the host drain. + if (absorbReadyMessage()) continue turnLoop; + // Natural stop; mapStopReason handles truncated output as max_tokens. stopReason = mapStopReason(step.finishReason); break; } @@ -3022,50 +3012,78 @@ export class HarnessSession extends EventEmitter { const recordResult = (uuid: string): void => { this.capture.recordEvent(uuid); resultOrigins.push(uuid); }; for (let i = 0; i < step.toolCalls.length; i++) { const call = step.toolCalls[i]; - const payload = await this.runOneTool(call, recentCalls); // NEVER throws - if (payload === 'interrupted') { - // Interrupt during a permission ask. Back-fill canceled tool-results - // for THIS call AND every remaining un-executed call in the step - // (earlier calls already have real results in resultParts + emitted - // events). Every call's tool-use event was already emitted up front, - // so each still gets a matching tool-result event here. Without this, - // the assistant(tool-call) message has no matching tool message — a - // dangling tool_call that provider APIs hard-reject (HTTP 400) on the - // NEXT send, bricking the session (the bad message persists in history - // across sends). CC does the same canceled back-fill. The synthesized - // tool-result events keep the persisted transcript in agreement with - // the model-facing history. - for (let j = i; j < step.toolCalls.length; j++) { - const rem = step.toolCalls[j]; - recordResult( - this.emitEvent('tool-result', { toolUseId: rem.toolCallId, toolName: rem.toolName, toolResult: CANCELED_TOOL_TEXT, isError: true })); - resultParts.push(this.toolResultPart(rem, CANCELED_TOOL_TEXT)); + if ((call.toolName === 'Write' || call.toolName === 'Edit') && + !this.interrupted && !this.abort.signal.aborted) { + const newlyRelevant = this.pendingPathTriggers([call]); + if (newlyRelevant.length) { + // WHY (Destin, 2026-09-28 PR-review deck Q-5): show the new rules + // ONCE, then let the model decide again — even on a small model + // whose window only fits a shortened version. A shortened rule + // ends with "Read for the rest", so the model can fetch the + // remainder itself. The earlier design refused the change outright + // when rules could not fit, which silently ended the turn and left + // small models unable to edit files in rule-heavy projects. + supersedeToolGroup(step.toolCalls, i, resultParts, resultOrigins, + data => this.emitEvent('tool-result', data), (rem, text) => this.toolResultPart(rem, text), + uuid => this.capture.recordEvent(uuid), (parts, origins) => { + this.history.push({ role: 'tool', content: parts }); + this.historyOrigins.push(origins); + }, calls => this.injectPathTriggers(calls), + 'Not run: newly applicable project instructions need review before changing this file.', + () => { + if (!this.interrupted && !this.abort?.signal.aborted) + this.appendPathTriggers(newlyRelevant.filter(t => !this.injectedTriggerIds.has(t.id)), true); + }); + if (this.interrupted || this.abort.signal.aborted) { + this.emitEvent('user-interrupt', abandonedTurnUsage()); + return; + } + absorbReadyMessage(); // ready human correction precedes replan + continue turnLoop; } + } + let payload: ToolResultPayload | 'interrupted' | EndTurnResult; + try { + payload = await this.runOneTool(call, recentCalls); + } catch (err) { + if (!(err instanceof PermissionCallbackFailure)) throw err; + // WHY: the assistant already announced every call. A failed permission + // lookup is neither approval nor a human refusal; complete the accepted + // group before the outer error path settles the turn. + const failure = `Permission check failed: ${describeProviderError(err.cause)}`; + const remaining = finalizeRemainingCalls(step.toolCalls, i, + j => j === i ? failure : 'Not run: an earlier permission check failed.', + (rem, text) => this.emitEvent('tool-result', { + toolUseId: rem.toolCallId, toolName: rem.toolName, toolResult: text, isError: true, + }), (rem, text) => this.toolResultPart(rem, text)); + for (const origin of remaining.origins) recordResult(origin); + resultParts.push(...remaining.parts); + this.history.push({ role: 'tool', content: resultParts }); + this.historyOrigins.push([...resultOrigins]); + throw err; + } + if (payload === 'interrupted') { + // WHY: uses were emitted up front; back-fill canceled results for + // this and all unstarted siblings or the next send gets a provider 400. + const canceled = finalizeRemainingCalls(step.toolCalls, i, () => CANCELED_TOOL_TEXT, + (rem, text) => this.emitEvent('tool-result', { toolUseId: rem.toolCallId, toolName: rem.toolName, toolResult: text, isError: true }), + (rem, text) => this.toolResultPart(rem, text)); + canceled.origins.forEach(recordResult); + resultParts.push(...canceled.parts); this.history.push({ role: 'tool', content: resultParts }); this.historyOrigins.push([...resultOrigins]); this.emitEvent('user-interrupt', abandonedTurnUsage()); return; } - // The user dismissed a question → end the turn ORDERLY. Record THIS - // call's real result, then mark every remaining un-executed call in the - // step as not-run (same dangling-tool_call hazard the interrupt branch - // above guards against). `turn-complete` rather than `user-interrupt` - // on purpose: usage should be reported, and anything the user queued - // while the turn ran should drain — typing during the turn IS taking - // over. The max_steps gate below is the existing precedent for a - // driver-decided orderly stop. + // WHY: a dismissed question ends the turn orderly with its real result + // and not-run siblings; turn-complete reports usage and drains the queue. if ('kind' in payload) { - recordResult(this.emitEvent('tool-result', { - toolUseId: call.toolCallId, toolName: call.toolName, - toolResult: payload.payload.text, isError: true, - })); - resultParts.push(this.toolResultPart(call, payload.payload.text)); - for (let j = i + 1; j < step.toolCalls.length; j++) { - const rem = step.toolCalls[j]; - recordResult( - this.emitEvent('tool-result', { toolUseId: rem.toolCallId, toolName: rem.toolName, toolResult: NOT_RUN_TOOL_TEXT, isError: true })); - resultParts.push(this.toolResultPart(rem, NOT_RUN_TOOL_TEXT)); - } + const dismissed = finalizeRemainingCalls(step.toolCalls, i, + j => j === i ? payload.payload.text : NOT_RUN_TOOL_TEXT, + (rem, text) => this.emitEvent('tool-result', { toolUseId: rem.toolCallId, toolName: rem.toolName, toolResult: text, isError: true }), + (rem, text) => this.toolResultPart(rem, text)); + dismissed.origins.forEach(recordResult); + resultParts.push(...dismissed.parts); this.history.push({ role: 'tool', content: resultParts }); this.historyOrigins.push([...resultOrigins]); stopReason = DISMISSED_STOP_REASON; @@ -3095,7 +3113,10 @@ export class HarnessSession extends EventEmitter { // just learned, and before it decides the next step. this.injectPathTriggers(step.toolCalls); - stepsSinceApproval++; + // WHY (Destin, 2026-09-28 PR-review deck Q-1): a message sent mid-task is + // read once the whole batch of actions the assistant already chose has + // run — never by discarding planned actions — then it replans with it. + if (this.interrupted || this.abort.signal.aborted || !absorbReadyMessage()) stepsSinceApproval++; // Budget gate (spec §2.4) — surfaces as a permission ASK, not a new // event. Allow resets the counter and continues; anything else ends the // turn with stopReason 'max_steps'; canceled is an interrupt. @@ -3237,9 +3258,7 @@ export class HarnessSession extends EventEmitter { }, }); } catch (err: any) { - // v0's catch, unchanged: push any in-flight partial, then split - // interrupt vs error. withRetry has already exhausted retries for a - // transient provider error before it lands here. + // Push in-flight partials; provider retries have already exhausted. // The attempt that produced this partial threw, so the loop never saw its // StepResult — this is the one acceptance decision made outside it. // trim(): the same emptiness rule as every other assistant push (and as @@ -3252,12 +3271,14 @@ export class HarnessSession extends EventEmitter { } else if (this.lastAttempt !== undefined) { this.capture.abandonAttempt(this.lastAttempt); } - if (this.interrupted || err?.name === 'AbortError' || this.abort?.signal.aborted) { + // WHY: a permission callback's own AbortError is not proof the user stopped + // the turn. Keep the tag until here while reporting its original detail. + const failure = err instanceof PermissionCallbackFailure ? err.cause : err; + if (this.interrupted || this.abort?.signal.aborted || (!(err instanceof PermissionCallbackFailure) && err?.name === 'AbortError')) { this.emitEvent('user-interrupt', abandonedTurnUsage()); } else { - // An errored turn spent the same real tokens an interrupted one did. - const errorCode = classifyProviderError(err); - this.emitEvent('session-error', { text: describeProviderError(err), ...(errorCode ? { errorCode } : {}), ...abandonedTurnUsage() }); + const errorCode = classifyProviderError(failure); + this.emitEvent('session-error', { text: describeProviderError(failure), ...(errorCode ? { errorCode } : {}), ...abandonedTurnUsage() }); } } finally { this._currentUsageProgress = null; @@ -3274,6 +3295,7 @@ export class HarnessSession extends EventEmitter { model: LanguageModel, aiTools: Record, reportPartial: (text: string) => void, + registerRetraction: (retract: () => void) => void, ): Promise { // Attempt 0 stalls with nothing streamed → runStreamOnce returns // STALL_RETRY → we re-run. That AUTOMATIC retry is available once per step: @@ -3290,7 +3312,7 @@ export class HarnessSession extends EventEmitter { // The loop is no longer bounded at two iterations — a MANUAL Retry also // returns STALL_RETRY, and the user may press it as often as they like. for (let attempt = 0; ; attempt++) { - const outcome = await this.runStreamOnce(model, aiTools, reportPartial, attempt === 0); + const outcome = await this.runStreamOnce(model, aiTools, reportPartial, attempt === 0, registerRetraction); if (outcome !== STALL_RETRY) return outcome; // Re-running after EITHER a silent stall (nothing streamed) or a manual // Retry (content streamed, then erased via dropPart): clear the on-screen @@ -3309,6 +3331,7 @@ export class HarnessSession extends EventEmitter { aiTools: Record, reportPartial: (text: string) => void, isFirstAttempt: boolean, + registerRetraction: (retract: () => void) => void, ): Promise { // One capture attempt per stream attempt. Its delta uuids stay provisional // until the turn loop (or send()'s catch) says what became of the step. @@ -3545,6 +3568,22 @@ export class HarnessSession extends EventEmitter { // rather than appended to. const emittedPartIds = new Set(); const textPartIds = new Set(); + // WHY: only the retry decision may erase an attempt. Register its LOCAL + // output before consuming; an exhausted/non-transient error keeps its honest + // partial, while a replay withdraws all four surfaces before the next request. + const retractAttempt = () => { + reportPartial(''); + for (const [prepId, entry] of preparing) { + this.emitEvent('assistant-thinking', { + toolPreparing: { toolCallId: prepId, toolName: entry.toolName, chars: entry.chars, cleared: true }, + }); + } + if (emittedPartIds.size > 0) { + this.emitEvent('assistant-thinking', { dropPart: { partIds: [...emittedPartIds] } }); + } + this.capture.abandonAttempt(attempt); + }; + registerRetraction(retractAttempt); try { while (true) { @@ -3568,28 +3607,7 @@ export class HarnessSession extends EventEmitter { void Promise.resolve(result.usage).catch(() => {}); void Promise.resolve(result.finishReason).catch(() => {}); this.resolveRetry = null; - // Retract the erased text from the model's own memory, not just the - // screen: partialAssistantText is reset per STEP (not per attempt), so - // without this, a re-run that throws before emitting anything would - // leave send()'s catch pushing the ABANDONED half-sentence — text the - // user just watched get erased via dropPart — silently back into - // this.history as an assistant message. - reportPartial(''); - // Withdraw any preparing card: the step re-runs INSIDE the same turn, - // so endTurn's reaping never fires and the card would spin forever - // beside the one the re-run mints. (Same reason as the auto-retry path.) - for (const [prepId, entry] of preparing) { - this.emitEvent('assistant-thinking', { - toolPreparing: { toolCallId: prepId, toolName: entry.toolName, chars: entry.chars, cleared: true }, - }); - } - // Erase what the abandoned attempt put on screen BEFORE re-running. - if (emittedPartIds.size > 0) { - this.emitEvent('assistant-thinking', { dropPart: { partIds: [...emittedPartIds] } }); - } - // The fourth place Retry erases: the model's memory, the screen and - // the store already forget this text, so its provenance must too. - this.capture.abandonAttempt(attempt); + retractAttempt(); return STALL_RETRY; } if (chunk === 'stall') { @@ -3602,15 +3620,7 @@ export class HarnessSession extends EventEmitter { void Promise.resolve(result.usage).catch(() => {}); void Promise.resolve(result.finishReason).catch(() => {}); if (!emittedAny && isFirstAttempt) { - // The step re-runs INSIDE the same turn, so endTurn's reaping never - // fires. Withdraw any preparing card explicitly or it spins for the - // rest of the turn while the retry mints a second card beside it. - for (const [prepId, entry] of preparing) { - this.emitEvent('assistant-thinking', { - toolPreparing: { toolCallId: prepId, toolName: entry.toolName, chars: entry.chars, cleared: true }, - }); - } - this.capture.abandonAttempt(attempt); // the re-run starts this step over + retractAttempt(); // the re-run starts this step over return STALL_RETRY; } // Name the phase honestly: a model that never STARTED (prefill) has not @@ -3871,8 +3881,9 @@ export class HarnessSession extends EventEmitter { } /** Run one tool call through the EXACT permission sequence (spec §2.1/§2.4): - * validate → doom-loop → guards → decide → (ask) → execute. NEVER throws — - * every failure mode is a tool RESULT the model can repair from, except a + * validate → doom-loop → guards → decide → (ask) → execute. Permission + * callbacks throw a tagged failure for the driver to finalize the group; + * ordinary tool failures are RESULTS the model can repair from, except a * user cancel which returns the 'interrupted' sentinel, and a dismissed * question which returns EndTurnResult — both let the loop unwind. */ private async runOneTool(call: ToolCall, recentCalls: string[]): Promise { @@ -3881,24 +3892,7 @@ export class HarnessSession extends EventEmitter { // 1. Validate (zod) — invalid args are a RESULT the model repairs from, not // a crash, and precede permissions (never ask about garbage). - let parsed = tool.inputSchema.safeParse(call.input); - if (!parsed.success && typeof call.input === 'string') { - // Weak-model hardening (Task 12, spec §3): the ai@7 SDK already parses a - // provider tool-call's stringified args into an object for us (see - // harness-sdk-toolcall-contract.test.ts), but a weak local model - // sometimes puts its WHOLE args object as a STRING one level further in - // — e.g. it emits `"{\"prompt\": ...}"` where a real object belongs. If - // the raw string itself JSON.parses to an object, give it ONE recovery - // attempt before falling back to the normal arg error — never a general - // coercion layer (YAGNI: one attempt, then the ordinary failure path). - try { - const recovered: unknown = JSON.parse(call.input); - if (recovered && typeof recovered === 'object') { - const reparsed = tool.inputSchema.safeParse(recovered); - if (reparsed.success) parsed = reparsed; - } - } catch { /* not JSON — fall through to the normal arg error below */ } - } + const parsed = parseToolArgs(tool, call.input); if (!parsed.success) { // Worded in arg-errors.ts (ledger D-2): names the unknown / missing / // mistyped parameter and, for an unknown one, the parameters that exist — @@ -3930,7 +3924,7 @@ export class HarnessSession extends EventEmitter { recentCalls.push(sig); if (recentCalls.length > threshold) recentCalls.shift(); if (recentCalls.length === threshold && recentCalls.every((s) => s === sig)) { - const d = await this.opts.askUser?.({ sessionId: this.opts.sessionId, toolName: 'doom_loop', toolInput: { repeated: call.toolName }, denyListed: false }); + const d = this.opts.askUser ? await permissionCallback(() => this.opts.askUser!({ sessionId: this.opts.sessionId, toolName: 'doom_loop', toolInput: { repeated: call.toolName }, denyListed: false })) : undefined; if (d?.behavior === 'canceled') return 'interrupted'; // Threshold-accurate: the doom-loop window length varies by profile (2 for // small local models, 3 for cloud), so quote the ACTUAL threshold, not a @@ -3948,7 +3942,7 @@ export class HarnessSession extends EventEmitter { // three times IS a doom loop and should still trip. if (tool.interactive) { if (!this.opts.askUser) return { text: `No user-interaction handler is wired for this session; ${call.toolName} cannot run. This is a configuration error.`, isError: true }; - const d = await this.opts.askUser({ sessionId: this.opts.sessionId, toolName: call.toolName, toolInput: call.input as any, denyListed: false }); + const d = await permissionCallback(() => this.opts.askUser!({ sessionId: this.opts.sessionId, toolName: call.toolName, toolInput: call.input as any, denyListed: false })); if (d.behavior === 'canceled') return 'interrupted'; // A HUMAN dismissal ENDS THE TURN: closing the card is the user taking the // turn back, not permission to guess. Still a real tool result so @@ -4044,7 +4038,7 @@ export class HarnessSession extends EventEmitter { // of rules; otherwise consult decide() (default: ask — never silent-allow). const configured: PermissionDecision = externalAsk ? { action: 'ask', denyListed: false } - : await (this.opts.decide?.(call.toolName, subject) ?? Promise.resolve({ action: 'ask', denyListed: false })); + : await permissionCallback(() => this.opts.decide?.(call.toolName, subject) ?? Promise.resolve({ action: 'ask', denyListed: false })); // denyListed: true so Full auto shows its stop band (worded per floorStop — // deny-list-copy.ts) like any deny-list stop, instead of silently running. const decision: PermissionDecision = floorStop && configured.action !== 'deny' @@ -4071,7 +4065,7 @@ export class HarnessSession extends EventEmitter { // as `pattern` — threaded through so a routed CHILD ask (child-ask- // router.ts, which has no other way to reach it) can persist the exact // same rule a root session's own remember-rule listener would. - const d = await this.opts.askUser({ sessionId: this.opts.sessionId, toolName: call.toolName, toolInput: call.input as any, denyListed: decision.denyListed, external: externalAsk, ...(floorStop ? { floorStop } : {}), subject }); + const d = await permissionCallback(() => this.opts.askUser!({ sessionId: this.opts.sessionId, toolName: call.toolName, toolInput: call.input as any, denyListed: decision.denyListed, external: externalAsk, ...(floorStop ? { floorStop } : {}), subject })); if (d.behavior === 'canceled') return 'interrupted'; // Task 8: d.message carries specific copy for a deny that ISN'T a real // user decline — e.g. child-ask-router's outside-the-folder refusal for @@ -4166,15 +4160,19 @@ export class HarnessSession extends EventEmitter { /** Exponential backoff for transient provider errors (429/5xx/network), * honoring retry-after. Layers ON TOP of the SDK's internal retry (this is * step-level resilience). Exhaustion rethrows → the session-error path. */ - private async withRetry(fn: () => Promise): Promise { + private async withRetry(fn: (registerRetraction: (retract: () => void) => void) => Promise): Promise { const delays = this.retryDelays; for (let attempt = 0; ; attempt++) { + let retractAttempt: (() => void) | undefined; try { - return await fn(); + return await fn((retract) => { retractAttempt = retract; }); } catch (err: any) { const status = err?.statusCode ?? err?.status; const retryable = status === 429 || (status >= 500 && status < 600) || err?.code === 'ECONNRESET'; if (!retryable || attempt >= delays.length || this.abort?.signal.aborted) throw err; + // WHY: retrying the whole step must first erase ONLY its failed attempt; + // previously a late provider error left streamed text on disk and screen. + retractAttempt?.(); const ra = Number(err?.responseHeaders?.['retry-after']) * 1000; await new Promise((r) => setTimeout(r, Number.isFinite(ra) && ra > 0 ? ra : delays[attempt])); } diff --git a/desktop/src/main/harness/injection/fit-rule-group.ts b/desktop/src/main/harness/injection/fit-rule-group.ts new file mode 100644 index 000000000..d06bfffbd --- /dev/null +++ b/desktop/src/main/harness/injection/fit-rule-group.ts @@ -0,0 +1,38 @@ +import { fitInjection } from './injection-budget'; +import type { PathTrigger } from './path-triggers'; + +/** WHY: one pre-write boundary can activate many rules; granting each the + * entire injection budget would silently multiply the model's context cost. */ +export function fitRuleGroupDelivery(rules: readonly PathTrigger[], budgetTokens: number): { contents: string[]; omitted: string[] } { + const budget = Math.max(0, Math.floor(budgetTokens * 4)); + const wrap = (source: string, body: string) => `\n${body}\n`; + const floor = rules.map(r => `[Read ${r.source}: rule shortened or omitted.]`); + const overhead = rules.reduce((n, r) => n + wrap(r.source, '').length, 0); + const separators = Math.max(0, rules.length - 1) * 2; + if (overhead + floor.reduce((n, text) => n + text.length, 0) + separators > budget) { + // Every source must remain identifiable when space permits. Never claim a + // body was supplied if the budget cannot even hold its labelled wrapper. + const sources = rules.map(r => r.source).join(', '); + const notice = `[Project rules omitted to fit context. Read: ${sources}]`; + return { contents: [notice.length <= budget ? notice : '[Project rules omitted.]'.slice(0, budget)], omitted: rules.map(r => r.id) }; + } + let remaining = budget - overhead - separators; + const omitted: string[] = []; + const contents = rules.map((rule, i) => { + const reserve = floor.slice(i + 1).reduce((n, s) => n + s.length, 0); + const room = Math.max(floor[i].length, Math.floor((remaining - reserve) / (rules.length - i))); + const fitted = fitInjection(rule.body, Math.floor(room / 4), rule.source); + const body = fitted.text.length <= room ? fitted.text : floor[i]; + // fitInjection can itself return only a truncation notice. A label plus a + // notice is NOT delivered guidance, even if its wrapper technically fits. + if (body === floor[i] || (fitted.truncated && body.trimStart().startsWith('[...truncated'))) omitted.push(rule.id); + remaining -= body.length; + return wrap(rule.source, body); + }); + return { contents, omitted }; +} + +/** The fitted text alone, for callers that only display it. */ +export function fitRuleGroup(rules: readonly PathTrigger[], budgetTokens: number): string[] { + return fitRuleGroupDelivery(rules, budgetTokens).contents; +} diff --git a/desktop/src/main/harness/injection/path-triggers.ts b/desktop/src/main/harness/injection/path-triggers.ts index 60bd74f2d..6c3732672 100644 Binary files a/desktop/src/main/harness/injection/path-triggers.ts and b/desktop/src/main/harness/injection/path-triggers.ts differ diff --git a/desktop/src/main/harness/injection/project-instructions.ts b/desktop/src/main/harness/injection/project-instructions.ts new file mode 100644 index 000000000..5f4e58267 --- /dev/null +++ b/desktop/src/main/harness/injection/project-instructions.ts @@ -0,0 +1,116 @@ +import { promises as fs } from 'fs'; +import * as path from 'path'; +import { fitProjectInstructions } from './injection-budget'; + +export interface ProjectInstructionFile { + path: string; + name: string; + full: string; + text: string; + truncated: boolean; + note?: string; + /** A chain-wide omission notice is plain text, not a pretend file body. */ + unwrapped?: boolean; + /** A second instructions file in the SAME folder that was not used (AGENTS.md + * wins over CLAUDE.md). Shown in the panel so a dropped file is never silent. */ + notUsed?: string; +} + +// WHY plain words here: `note` is shown to the PERSON in the "What the assistant +// was given" panel. The bracketed notices the fitter writes are addressed to the +// model ("Read X for the rest"), so they are summarized, never copied. +const LEFT_OUT = 'Left out — too long for this model'; +const SHORTENED = 'Shortened to fit this model'; +function shortenedNote(fittedText: string): string { + const outline = /(\d+) of (\d+) sections above are shown as/.exec(fittedText); + return outline ? `${SHORTENED} · ${outline[1]} of ${outline[2]} sections shortened` : SHORTENED; +} + +export function renderProjectInstructionFiles(files: readonly ProjectInstructionFile[]): string | null { + const parts = files.filter(f => f.text).map(f => f.unwrapped + ? f.text + : `\n${f.text}\n`); + return parts.length ? parts.join('\n\n') : null; +} + +/** WHY: discovery belongs to session startup, not synchronous prompt assembly or + * context-panel retrieval. Git boundaries do not stop instruction-file ancestry. */ +export async function prepareProjectInstructions(cwd: string, budgetTokens: number, fixtureBoundary?: string): Promise { + const dirs: string[] = []; + let dir = path.resolve(cwd); + for (;;) { + dirs.unshift(dir); + // Evaluator-only boundary: disposable A/B fixtures must not inherit real + // machine instructions; production always leaves this unset. + if (fixtureBoundary && dir === path.resolve(fixtureBoundary)) break; + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + } + const sources: Array<{ path: string; name: string; full: string; notUsed?: string }> = []; + const seen = new Set(); + for (const folder of dirs) { + for (const name of ['AGENTS.md', 'CLAUDE.md']) { + const file = path.join(folder, name); + let full: string; + try { full = await fs.readFile(file, 'utf8'); } + catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; + // An unreadable preferred file is not permission to silently switch sources. + break; + } + // WHY: aliases through a symlink can expose the same instruction twice. + // Canonicalize only identity, never the displayed path or the session cwd. + let identity: string; + try { identity = await fs.realpath(file); } + catch { identity = file; } // If it disappeared after reading, preserve the captured text. + if (!seen.has(identity)) { + // WHY record it: only one file per folder is used, so a CLAUDE.md next + // to an AGENTS.md is silently ignored unless we say so. + const other = name === 'AGENTS.md' ? path.join(folder, 'CLAUDE.md') : null; + const otherExists = other ? await fs.access(other).then(() => true, () => false) : false; + sources.push({ path: file, name, full, ...(otherExists ? { notUsed: 'CLAUDE.md' } : {}) }); + seen.add(identity); + } + break; + } + } + // WHY: account for every byte in the final part (including labels, wrappers, + // separators and omission notices). Never print empty per-file wrappers when + // there is no room to tell the model which sources it missed. + const budget = Math.max(0, Math.floor(budgetTokens * 4)); + const wrappers = sources.reduce((n, s) => n + `\n\n`.length, 0) + + Math.max(0, sources.length - 1) * 2; + const notices = sources.map(s => `[Read ${s.path}: shortened.]`); + const omitted = (text: string | null): ProjectInstructionFile[] => sources.map((source, i) => ({ + ...source, text: i === 0 ? (text ?? '') : '', truncated: true, + note: LEFT_OUT, + ...(i === 0 && text ? { unwrapped: true } : {}), + })); + if (wrappers + notices.reduce((n, s) => n + s.length, 0) > budget) { + const detailed = `[Project instructions omitted. Read ${sources.map(s => s.path).join(', ')}.]`; + const brief = '[Project instructions omitted.]'; + return omitted(detailed.length <= budget ? detailed : brief.length <= budget ? brief : null); + } + // WHY nearest first (Destin, 2026-09-28 PR-review deck Q-4): the file in the + // session's own folder is the most specific and usually the most relevant, so + // it takes the room it needs before any broader parent file. Each broader file + // then gets what is left, outward toward the root. Only the one-line "Read + // " notices of the files still waiting are held back, so a parent that + // is squeezed out is still named to the model, never silently lost. The + // result keeps broad-to-narrow ORDER; only who gets space first changes. + let remaining = budget - wrappers; + const fittedFiles: ProjectInstructionFile[] = new Array(sources.length); + for (let index = sources.length - 1; index >= 0; index--) { + const source = sources[index]; + const reserved = notices.slice(0, index).reduce((n, s) => n + s.length, 0); + const share = Math.max(notices[index].length, remaining - reserved); + const fitted = fitProjectInstructions(source.full, Math.floor(share / 4), source.path); + const text = fitted.text.length <= share ? fitted.text : notices[index]; + const truncated = fitted.truncated || text !== source.full; + remaining -= text.length; + fittedFiles[index] = { ...source, text, truncated, + ...(truncated ? { note: text === notices[index] ? LEFT_OUT : shortenedNote(text) } : {}) }; + } + return fittedFiles; +} diff --git a/desktop/src/main/harness/injection/retained-rule-visibility.ts b/desktop/src/main/harness/injection/retained-rule-visibility.ts new file mode 100644 index 000000000..3683a669e --- /dev/null +++ b/desktop/src/main/harness/injection/retained-rule-visibility.ts @@ -0,0 +1,16 @@ +import type { ModelMessage } from 'ai'; +import { isAppGenerated } from '../compaction'; + +/** WHY: a session-lifetime ID cannot vouch for guidance discarded by clear or + * summary. Inspect only at history replacement boundaries, never per token. + * Literal source labels are accepted only on app-authored, complete rule blocks; + * a human quotation or a summary mentioning a rule does not mean it is loaded. */ +export function retainedRuleMessages(history: readonly ModelMessage[]): Set { + const messages = new Set(); + for (const message of history) { + if (!isAppGenerated(message) || typeof message.content !== 'string') continue; + const match = /^\n[\s\S]*\n<\/project-rule>$/.exec(message.content); + if (match) messages.add(message.content); + } + return messages; +} diff --git a/desktop/src/main/harness/injection/rule-delivery.ts b/desktop/src/main/harness/injection/rule-delivery.ts new file mode 100644 index 000000000..3f658c4a7 --- /dev/null +++ b/desktop/src/main/harness/injection/rule-delivery.ts @@ -0,0 +1,72 @@ +import type { NativeTool } from '../tools/types'; +import { parseToolArgs } from '../tool-args'; +import { fitInjection } from './injection-budget'; +import { fitRuleGroupDelivery } from './fit-rule-group'; +import type { PathTrigger, TriggerIndex } from './path-triggers'; + +export interface PathCall { toolName: string; input: unknown } +const fullContent = (t: PathTrigger, budget: number) => + `\n${fitInjection(t.body, budget, t.source).text}\n`; +// fitInjection may fit *only* a notice; that is not guidance for a later write. +const noticeOnly = (content: string) => /\n\s*\[\.\.\.truncated/.test(content); + +/** WHY: both post-read and pre-write selection use the same validated subject + * and exact retained-body dedupe. Non-path tool subjects are never file paths. */ +export function pendingRules(calls: readonly PathCall[], index: TriggerIndex | undefined, + byName: ReadonlyMap, budget: number, nonPath: ReadonlySet, + injected: Set, retained: Set): PathTrigger[] { + if (!index) return []; + const pending: PathTrigger[] = []; + const seen = new Set(); + for (const call of calls) { + if (nonPath.has(call.toolName)) continue; + const tool = byName.get(call.toolName); + if (!tool) continue; + const parsed = parseToolArgs(tool, call.input); + if (!parsed.success) continue; + const subject = tool.permissionSubject(parsed.data); + if (!subject) continue; + for (const rule of index.match(subject)) { + if (seen.has(rule.id) || injected.has(rule.id)) continue; + seen.add(rule.id); + const full = fullContent(rule, budget); + if (retained.has(full) && !noticeOnly(full)) { injected.add(rule.id); continue; } + pending.push(rule); + } + } + // A resumed pre-write group may retain its *bounded* body rather than the + // per-file full-budget fit. Compare exact app-generated content, not source + // alone; a changed body must still be delivered again. + const bounded = fitRuleGroupDelivery(pending, budget); + return pending.filter((rule, i) => { + const content = bounded.contents[pending.length > 1 && bounded.contents.length === 1 ? 0 : i]; + if (!bounded.omitted.includes(rule.id) && content?.startsWith(' { + const eq = arg.match(/^-{1,2}([^=]+)=(.+)$/); + if (eq) { if (SECRET_NAME.test(eq[1])) out.push(eq[2]); return; } + if (/^-{1,2}\S/.test(arg) && SECRET_NAME.test(arg) && i + 1 < args.length && !args[i + 1].startsWith('-')) out.push(args[i + 1]); + }); + return out; +} + +/** The error text with every env/header value this server was given — plus + * credentials in its launch arguments or address (see transportSecrets) — + * replaced by [redacted]. Values under 4 characters are left alone: blanking + * "1" or "true" everywhere would mangle the message while protecting nothing. */ +export function redactSecrets(text: string | null, server: Pick & { transport?: ResolvedMcpServer['transport'] }): string | null { + if (!text) return text; + const values = [...Object.values(server.env ?? {}), ...Object.values(server.headers ?? {}), ...transportSecrets(server.transport)] + .filter(v => typeof v === 'string' && v.length >= 4) + // "Bearer abc…" headers: an error may quote only the token part. + .flatMap(v => [v, ...v.split(/\s+/).filter(part => part.length >= 8 && part !== v)]) + // Longest first, so a secret that contains a shorter one is removed whole. + .sort((a, b) => b.length - a.length); + let out = text; + for (const v of values) out = out.split(v).join('[redacted]'); + return out; +} + export class McpManager { private readonly registry: McpRegistryLike; private readonly connectionFactory: McpConnectionFactory; - // serverId -> pooled entry. Absence means "never touched yet" — NOT the - // same as a connected-but-errored server, which stays present (see acquire). + // serverId -> CURRENT pooled entry. Retired generations remain in entries + // until their holders release; an errored current entry stays reportable. private pool = new Map(); + /** Retired entries stay alive until THEIR holders release, even if ID reused. */ + private entries = new Set(); + /** Exact entries held by a lease, including retired generations. */ + private leaseEntries = new Map(); // Monotonic, process-lifetime counter behind every lease id. Its ONLY job is // uniqueness — no ordering is read off it — so wraparound/overflow concerns // don't apply at any realistic session count. @@ -156,20 +224,18 @@ export class McpManager { if (entry.conn.state === 'ready') { ready.push({ id: entry.server.id, - label: entry.server.label, + label: servers.find(s => s.id === entry.server.id)?.label ?? entry.server.label, tools: entry.conn.listTools(), call: (tool, args, signal) => entry.conn.callTool(tool, args, signal), }); } else { - // Fix (Finding 5): status()/lastError have always been correct — - // nothing has ever LOGGED them. A developer with a typo'd command - // got no tool, no error dialog, and (until now) no log line either: - // the exact silent failure this whole design exists to prevent, and - // the reason the dogfood checklist's "confirm the server drops and - // is reported" step could never actually pass. One line per - // excluded server, naming it and its real (never reworded) error. + // WHY: a server that fails to start must leave its REAL reason in + // the log (a typo'd command was otherwise invisible), but a provider + // error can echo a resolved credential. Log the error with every + // secret value this server was given blanked out. log('WARN', 'McpManager', 'MCP server excluded from this session — not ready', { - sessionId, serverId: entry.server.id, state: entry.conn.state, error: entry.conn.lastError, + sessionId, serverId: entry.server.id, state: entry.conn.state, + error: redactSecrets(entry.conn.lastError, entry.server), }); } } @@ -209,22 +275,17 @@ export class McpManager { // awaits), so only one connect() ever runs no matter how many sessions ask // at once. // - // Except for a credential-error placeholder whose secrets now resolve (below), - // WHY no retry: if `entry` already exists — even in `error` or - // `needs-setup` state — it is returned as-is; connect() is never retried - // while the entry stays pooled (i.e. while any holder remains). This is - // deliberate, not an oversight: retry/backoff was out of scope for this - // manager. The user-visible effect is that fixing a broken server's config - // has no effect until every current holder releases (or the app - // restarts) — see status(), which surfaces the real lastError so this - // isn't silent. + // WHY no retry for unchanged settings: an error/needs-setup entry stays + // pooled until its holders release. A changed effective configuration gets + // a fresh generation immediately, without disturbing those older holders; + // retrying an unchanged broken connection on each acquire remains out of scope. private ensureConnected(server: ResolvedMcpServer, leaseId: string): PooledEntry { + const key = effectiveConfig(server); let entry = this.pool.get(server.id); - let recoveredHolders: Set | undefined; - if (entry?.server.credentialError && !server.credentialError) { - // WHY: this entry was a resource-free error placeholder, never a live - // connection. Retry after wallet recovery without discarding older leases. - recoveredHolders = entry.holders; + if (entry && entry.configKey !== key) { + // WHY: old leases retain their exact connection. Never move holders into + // the replacement (even an error placeholder): release of OLD must not + // close NEW, and A→B→A must not resurrect the retired A connection. this.pool.delete(server.id); entry = undefined; } @@ -244,6 +305,13 @@ export class McpManager { // case in mcp-client.ts); this reuses that same state value rather than // inventing a parallel one, so acquire()'s ready-check and status() // both treat it exactly like any other not-ready server. + // Snapshot before connectionFactory: a mutable registry object must not + // change the settings that an already-acquired connection actually uses. + const snapshot: ResolvedMcpServer = { ...server, transport: server.transport.type === 'stdio' + ? { ...server.transport, args: server.transport.args?.slice() } : { ...server.transport }, + env: server.env ? { ...server.env } : undefined, + headers: server.headers ? { ...server.headers } : undefined, + missingSecrets: [...server.missingSecrets] }; const credentialFailure = server.credentialError || (server.missingSecrets.length > 0 ? `${server.label} needs setup — missing secret(s): ${server.missingSecrets.join(', ')}.` : null); const conn: McpConnectionLike = credentialFailure @@ -261,9 +329,10 @@ export class McpManager { }), close: async () => {}, } - : this.connectionFactory(server); - entry = { server, conn, holders: recoveredHolders ?? new Set() }; + : this.connectionFactory(snapshot); + entry = { server: snapshot, configKey: key, conn, holders: new Set() }; this.pool.set(server.id, entry); + this.entries.add(entry); if (!credentialFailure) { entry.connecting = conn.connect().finally(() => { entry!.connecting = undefined; @@ -271,6 +340,9 @@ export class McpManager { } } entry.holders.add(leaseId); + const held = this.leaseEntries.get(leaseId) ?? []; + held.push(entry); + this.leaseEntries.set(leaseId, held); return entry; } @@ -292,42 +364,54 @@ export class McpManager { * server" check; no separate `has()` is needed. */ private async releaseLease(leaseId: string): Promise { - for (const [id, entry] of [...this.pool.entries()]) { - if (!entry.holders.delete(leaseId)) continue; - if (entry.holders.size > 0) continue; - // Remove from the pool BEFORE awaiting close(): an acquire() that lands - // during the await must build a fresh entry rather than hand out this - // one, which is already on its way down. - this.pool.delete(id); - // Unguarded close() would throw out of this loop and strand every - // remaining entry this lease still holds. The connection is being - // discarded either way, so a close() failure isn't actionable for the - // caller — log the REAL error rather than swallow it - // (error-message-standards.md), matching McpConnection.close()'s own - // best-effort teardown policy. + const held = this.leaseEntries.get(leaseId) ?? []; + this.leaseEntries.delete(leaseId); + for (const entry of held) { + if (!entry.holders.delete(leaseId) || entry.holders.size > 0) continue; + // WHY: only delete an ID when this EXACT generation is still current. + // Remove before awaiting close so an acquire during teardown connects anew. + if (this.pool.get(entry.server.id) === entry) this.pool.delete(entry.server.id); try { - await entry.conn.close(); + // WHY: keep the generation discoverable to destroyAll until close + // settles; removing it before this await let app teardown return while + // an older released transport was still closing. + await this.closeEntry(entry); } catch (err) { log('ERROR', 'McpManager', 'closing a released MCP connection failed', { - serverId: id, error: String(err), + serverId: entry.server.id, errorType: err instanceof Error ? err.name : 'unknown', }); + } finally { + this.entries.delete(entry); } } } - /** App-quit teardown: close every pooled connection regardless of - * refcount. A leaked MCP subprocess would otherwise outlive the app. */ - async destroyAll(): Promise { - const entries = [...this.pool.values()]; - this.pool.clear(); - for (const entry of entries) { + private closeEntry(entry: PooledEntry): Promise { + // WHY: release and destroyAll may overlap; close exactly once and wait for + // a pending connect before closing its transport. Never reuse while closing. + if (!entry.closing) entry.closing = (async () => { + if (entry.connecting) { try { await entry.connecting; } catch { /* close failed connect too */ } } await entry.conn.close(); - } + })(); + return entry.closing; + } + + /** App-quit teardown: close current AND retired/closing generations. */ + async destroyAll(): Promise { + const entries = [...this.entries]; + this.pool.clear(); // no acquisition may reuse a closing connection + this.leaseEntries.clear(); + // WHY: a second destroyAll must also await pending closes. Retain each + // generation in entries until its close settles, even across concurrent + // destroy/release calls. closeEntry ensures the transport closes only once. + await Promise.all(entries.map(async entry => { + try { await this.closeEntry(entry); } + finally { this.entries.delete(entry); } + })); } - /** Every server this manager has ever touched, including ones that failed - * to connect — their REAL error (McpConnection.lastError, never reworded) - * so a session picker can show why a server is unavailable. */ + /** One current entry per server ID, including failed connections. Retired + * generations remain usable through leases but never duplicate status rows. */ status(): Array<{ id: string; state: string; error: string | null }> { return [...this.pool.values()].map((entry) => ({ id: entry.server.id, diff --git a/desktop/src/main/harness/native-session-host.ts b/desktop/src/main/harness/native-session-host.ts index 0ec5dd2c3..587999065 100644 --- a/desktop/src/main/harness/native-session-host.ts +++ b/desktop/src/main/harness/native-session-host.ts @@ -34,7 +34,8 @@ import { resolvePreset, type ResolvedPreset } from './preset-registry'; import { decidePermission } from './permission-engine'; import { getShell } from './tools/bash'; import { rulesForMode, sameRule, isCrossProjectRule, CROSS_PROJECT_SLUG, DESTRUCTIVE_DENY_LIST, type NativePermissionMode, type PermissionRule } from '../../shared/permission-types'; -import { assembleSystemPrompt, assembleSystemPromptParts, findProjectInstructions, gitSnapshotAsync } from './prompt-assembly'; +import { assembleSystemPrompt, assembleSystemPromptParts, gitSnapshotAsync } from './prompt-assembly'; +import { prepareProjectInstructions, type ProjectInstructionFile } from './injection/project-instructions'; import { resolveProfile, effectiveContextForModel, type CapabilityProfile, type ProfileProviderType } from './capability-profile'; import { CORE_TOOLS } from './tools'; import type { ToolServices, SpecialistReservation, SpecialistSpawnOpts, SpecialistManageOutcome, SpecialistResumeOutcome } from './tools/types'; @@ -54,7 +55,7 @@ import { PermissionModeStore } from './permission-mode-store'; import { computeReportBudget } from './specialists/report-budget'; import { truncateOutput, composeNotice } from './tools/truncate'; import { APPROX_CHARS_PER_TOKEN } from './message-size'; -import { fitInjection, fitProjectInstructions } from './injection/injection-budget'; +import { fitInjection } from './injection/injection-budget'; import { frameSkillInvocation } from './skills/skill-invocation'; import { buildTriggerIndex, type TriggerIndex } from './injection/path-triggers'; import { costForUsage, isFreePricing, type ModelPricing } from './pricing'; @@ -305,6 +306,7 @@ interface LiveEntry { // The "What the assistant was given" record last pushed for this session, so a // model swap can re-push it with the new model's window (republishWindow). sessionContext?: SessionContext; + projectInstructionFiles?: readonly ProjectInstructionFile[]; // Per-session append serialization: each transcript event extends this chain // (append(prev).then(next)) so the SessionStore contract (serialized appends) // holds. Starts resolved; a failed append is logged but never breaks the @@ -329,7 +331,7 @@ interface LiveEntry { // `attachments` are absolute composer file paths; image ones become image // parts on the user message. Carried through the QUEUE too, or a message sent // while a turn was in flight would silently lose its pictures. - queue: { id: string; text: string; attachments: string[] }[]; + queue: { id: string; text: string; attachments: string[]; ready?: boolean }[]; // True from dispatch until runTurns finishes the last queued turn. Host-owned // (HarnessSession's in-flight state is private); safe because Node is single-threaded. inFlight: boolean; @@ -383,16 +385,6 @@ function persistedUuidsAfterLastClear(events: TranscriptEvent[]): string[] { // pass" (a typed message, a queued message, a skill) from "the host did". const IDLE_PASS = async (): Promise => {}; -/** The bracketed sentence a fitter appends to say what it cut. Read back rather - * than re-described here: a second wording would eventually disagree with the - * one the model was actually given, and this panel exists to be trusted. - * Null when the text carries no such notice. */ -function truncationNote(fittedText: string): string | null { - const at = fittedText.lastIndexOf('\n\n['); - if (at === -1 || !fittedText.trimEnd().endsWith(']')) return null; - return fittedText.slice(at + 3).trimEnd().slice(0, -1); -} - /** The name a person recognises out of a skill id. Ids are `plugin:skill` or a * bare name; the part after the colon is what the user typed to install it. */ function skillLabel(id: string): string { @@ -999,9 +991,9 @@ export class NativeSessionHost extends EventEmitter { const { contextLength, profile, pricing, free } = await this.resolveContextAndProfile(binding); const title = header.title ?? record.title; - const [gitSnapshot, triggers] = await Promise.all([gitSnapshotAsync(workDir), buildTriggerIndex(workDir)]); + const [gitSnapshot, triggers, projectInstructionFiles] = await Promise.all([gitSnapshotAsync(workDir), buildTriggerIndex(workDir), prepareProjectInstructions(workDir, profile.injectionBudgetTokens)]); const session = this.buildSpecialistSession( - parentId, opts.childId, workDir, title, specialist, binding, contextLength, profile, pricing, free, opts.parentToolCallId, preset, parent, gitSnapshot, triggers, + parentId, opts.childId, workDir, title, specialist, binding, contextLength, profile, pricing, free, opts.parentToolCallId, preset, parent, gitSnapshot, triggers, projectInstructionFiles, ); // Cold state rebuilt from the child's OWN transcript — seedHistory resets // readRegistry + todos too (the same reset-on-resume contract root @@ -2734,7 +2726,7 @@ export class NativeSessionHost extends EventEmitter { * `profile` is accepted here so Task 6 can add a prompt variant without another * signature change; this task doesn't use it yet (the session itself carries it * via opts.profile). */ - private toolWiring(sessionId: string, cwd: string, preset: ResolvedPreset, profile: CapabilityProfile, gitSnapshot: string, triggers: TriggerIndex): Pick { + private toolWiring(sessionId: string, cwd: string, preset: ResolvedPreset, profile: CapabilityProfile, gitSnapshot: string, triggers: TriggerIndex, projectInstructionFiles: readonly ProjectInstructionFile[]): Pick { return { // G-1: this session's background-command registry, host-owned. shells: this.shellsFor(sessionId), @@ -2873,7 +2865,7 @@ export class NativeSessionHost extends EventEmitter { // timeouts) and could land on a different date or branch than the prompt the // model actually got. presetName is label-only and reaches no model. ...(() => { - const promptParts = assembleSystemPromptParts({ presetBody: preset.body, cwd, appVersion: this.appVersion, promptVariant: profile.promptVariant, hasTools: profile.supportsTools, instructionBudgetTokens: profile.injectionBudgetTokens, supportsParallelToolCalls: profile.supportsParallelToolCalls, audience: 'user', presetName: preset.manifest.name, gitSnapshot }); + const promptParts = assembleSystemPromptParts({ presetBody: preset.body, cwd, appVersion: this.appVersion, promptVariant: profile.promptVariant, hasTools: profile.supportsTools, instructionBudgetTokens: profile.injectionBudgetTokens, supportsParallelToolCalls: profile.supportsParallelToolCalls, audience: 'user', presetName: preset.manifest.name, gitSnapshot, projectInstructionFiles }); return { promptParts, systemPrompt: promptParts.map((p) => p.text).join('\n\n') }; })(), }; @@ -3285,8 +3277,8 @@ export class NativeSessionHost extends EventEmitter { /** Subscribe a freshly-built HarnessSession: forward its events to the * renderer immediately, and enqueue each on the session's append chain. */ - private wire(sessionId: string, cwd: string, session: HarnessSession, mcpLease?: McpLease): void { - const entry: LiveEntry = { session, cwd, appendChain: Promise.resolve(), compactionGeneration: this.restoredCompactionGeneration.get(session) ?? 0, queue: [], inFlight: false, mcpLease }; + private wire(sessionId: string, cwd: string, session: HarnessSession, mcpLease?: McpLease, projectInstructionFiles: readonly ProjectInstructionFile[] = []): void { + const entry: LiveEntry = { session, cwd, appendChain: Promise.resolve(), compactionGeneration: this.restoredCompactionGeneration.get(session) ?? 0, queue: [], inFlight: false, mcpLease, projectInstructionFiles }; this.live.set(sessionId, entry); this.retainModel(sessionId, session.binding.modelId); // ref-count this model // Persist "Always allow" decisions for THIS session's project. The session @@ -3322,11 +3314,10 @@ export class NativeSessionHost extends EventEmitter { // AFTER the listeners above and BEFORE the held-message drain, so the line is // on screen before the first turn's output starts arriving. // - // Never fatal: this reads the instruction file and syncs the tool set, and a - // session must still open if either fails. A missing line is a missing - // explanation; a thrown one is a chat that never starts. + // Never fatal: this describes the already-captured instructions and tool + // set; a missing explanation must not stop a chat from opening. try { - entry.sessionContext = this.buildSessionContext(cwd, session); + entry.sessionContext = this.buildSessionContext(entry); this.emit('session-context', { sessionId, context: entry.sessionContext }); } catch (err) { log('ERROR', 'NativeSessionHost', 'could not describe the session context', { sessionId, error: String(err) }); @@ -3354,8 +3345,8 @@ export class NativeSessionHost extends EventEmitter { /** What this session was given, for the line above the conversation and the * "What the assistant was given" panel. * - * Built ONCE per session, at wire() — root sessions only, so a specialist - * child (which is never wired) never grows a line of its own. + * Built ONCE per session from its prompt's captured inventory, at wire() — + * root sessions only; children have no context-panel record. * * NO FILE BODIES. The only text here is the system prompt, which is already * assembled and in memory; every file the panel can show is fetched by @@ -3367,10 +3358,10 @@ export class NativeSessionHost extends EventEmitter { * a small model — the skill catalog itself, which the model is simply never * told about. A skill being too long to fit is a thing that has not happened * yet, so it is reported on the skill's own row and never in this summary. */ - private buildSessionContext(cwd: string, session: HarnessSession): SessionContext { + private buildSessionContext(entry: LiveEntry): SessionContext { + const session = entry.session; const inv = session.contextInventory(); - const found = findProjectInstructions(cwd); - const fitted = found ? fitProjectInstructions(found.text, inv.injectionBudgetTokens, found.name) : null; + const files = entry.projectInstructionFiles ?? []; return { modelLabel: session.binding.modelId, contextWindowTokens: session.contextWindowTokens, @@ -3379,9 +3370,10 @@ export class NativeSessionHost extends EventEmitter { // The project-instructions part is filtered out: it has its own tab, and // showing it under System too would say the same thing twice. systemPromptSections: inv.promptParts.filter((p) => p.id !== 'project'), - projectInstructions: found && fitted - ? { path: found.path, truncated: fitted.truncated, note: truncationNote(fitted.text) } + projectInstructions: files.length + ? { path: files[files.length - 1].path, truncated: files[files.length - 1].truncated, note: files[files.length - 1].note ?? null } : null, + projectInstructionFiles: files.map(f => ({ path: f.path, truncated: f.truncated, note: f.note ?? null, ...(f.notUsed ? { notUsed: f.notUsed } : {}) })), skills: inv.skills.map((s) => ({ id: s.id, label: skillLabel(s.id), description: s.description })), skillsOffered: inv.skillsOffered, tools: inv.toolNames, @@ -3422,10 +3414,11 @@ export class NativeSessionHost extends EventEmitter { if (!entry) return { error: 'not-live' }; const budget = entry.session.profileSnapshot.injectionBudgetTokens; if (kind === 'project') { - const found = findProjectInstructions(entry.cwd); + const files = entry.projectInstructionFiles ?? []; + // WHY: id is only a selector into the captured inventory, never a path read. + const found = id ? files.find(f => f.path === id) : files[files.length - 1]; if (!found) return { error: 'not-found' }; - const fitted = fitProjectInstructions(found.text, budget, found.name); - return { path: found.path, text: fitted.text, full: found.text, truncated: fitted.truncated }; + return { path: found.path, text: found.text, full: found.full, truncated: found.truncated }; } if (!id) return { error: 'not-found' }; try { @@ -3490,7 +3483,7 @@ export class NativeSessionHost extends EventEmitter { await this.specialistCatalog.ensureFresh(opts.cwd); // The git line, read off the main thread before anything is built // (2026-09-16 C3). Never throws (a non-repo answers a fixed string). - const [gitSnapshot, triggers] = await Promise.all([gitSnapshotAsync(opts.cwd), buildTriggerIndex(opts.cwd)]); + const [gitSnapshot, triggers, projectInstructionFiles] = await Promise.all([gitSnapshotAsync(opts.cwd), buildTriggerIndex(opts.cwd), prepareProjectInstructions(opts.cwd, profile.injectionBudgetTokens)]); // Acquire this session's MCP servers (Task 6) BEFORE constructing the // session, so mcpServers is available for the very first buildAiTools(). const mcpLease = await this.acquireMcp(opts.sessionId); @@ -3508,8 +3501,9 @@ export class NativeSessionHost extends EventEmitter { session = new HarnessSession( { sessionId: opts.sessionId, cwd: opts.cwd, harness, binding: opts.binding, contextLength, profile, pricing, free, commitCompaction: proposal => this.commitCompaction(opts.sessionId, session, proposal), + takeReadyBusyMessage: () => this.takeReadyBusyMessage(opts.sessionId, session), ...(mcpServers ? { mcpServers } : {}), - ...this.toolWiring(opts.sessionId, opts.cwd, preset, profile, gitSnapshot, triggers) }, + ...this.toolWiring(opts.sessionId, opts.cwd, preset, profile, gitSnapshot, triggers, projectInstructionFiles) }, this.modelFactory, ); } catch (err) { @@ -3517,7 +3511,7 @@ export class NativeSessionHost extends EventEmitter { throw err; } this.presetIdFor.set(opts.sessionId, preset.manifest.id); - this.wire(opts.sessionId, opts.cwd, session, mcpLease); + this.wire(opts.sessionId, opts.cwd, session, mcpLease, projectInstructionFiles); if (slotsUnknown) this.live.get(opts.sessionId)!.refreshSlotsAfterTurn = true; } @@ -3590,9 +3584,9 @@ export class NativeSessionHost extends EventEmitter { // buildSpecialistSession is fallible synchronous work, and a throw after // the header write would leave a session file on disk for a child that // never existed. The git line (C3) and trigger walk (B8) are awaited first. - const [gitSnapshot, triggers] = await Promise.all([gitSnapshotAsync(workDir), buildTriggerIndex(workDir)]); + const [gitSnapshot, triggers, projectInstructionFiles] = await Promise.all([gitSnapshotAsync(workDir), buildTriggerIndex(workDir), prepareProjectInstructions(workDir, profile.injectionBudgetTokens)]); const session = this.buildSpecialistSession( - parentId, childId, workDir, title, opts.specialist, binding, contextLength, profile, pricing, free, opts.parentToolCallId, preset, parent, gitSnapshot, triggers, + parentId, childId, workDir, title, opts.specialist, binding, contextLength, profile, pricing, free, opts.parentToolCallId, preset, parent, gitSnapshot, triggers, projectInstructionFiles, ); // `title` was drawn earlier (before this session was built — see that @@ -3642,9 +3636,15 @@ export class NativeSessionHost extends EventEmitter { pricing: ModelPricing | null, free: boolean, parentToolCallId: string, preset: ResolvedPreset, parent: LiveEntry, // The child's git line and trigger index, awaited by the caller off the main thread (C3, B8). - gitSnapshot: string, triggers: TriggerIndex, + gitSnapshot: string, triggers: TriggerIndex, projectInstructionFiles: readonly ProjectInstructionFile[], ): HarnessSession { - const allowed = new Set(specialist.allowedTools); + // WHY: the spawn-time Bash capability already advertises these helpers; + // hand the SAME derived list to tool exposure and the permission cap so a + // child can manage only its own background runs without editing the definition. + const effectiveAllowedTools = specialist.allowedTools.includes('Bash') + ? [...new Set([...specialist.allowedTools, 'BashOutput', 'KillShell'])] + : [...specialist.allowedTools]; + const allowed = new Set(effectiveAllowedTools); let session: HarnessSession; session = new HarnessSession( { @@ -3654,12 +3654,10 @@ export class NativeSessionHost extends EventEmitter { // lifecycle controls, and the delegation spawn backstop—not an arbitrary // per-child action count, so root limits never flow into a child. harness: preset.manifest, - // TOOLS: the definition's allowlist, filtered out of the same CORE_TOOLS - // set every session is built from. The Task tool is structurally absent - // because no definition lists it — that omission IS the depth-1 rule. - // G-1: a helper allowed Bash gets the companions too — its own - // background command would otherwise be unreadable and unstoppable. - tools: CORE_TOOLS.filter((t) => allowed.has(t.name) || (allowed.has('Bash') && (t.name === 'BashOutput' || t.name === 'KillShell'))), + // TOOLS: the spawn-time effective list, filtered from CORE_TOOLS. + // Task stays absent by definition (depth 1); Bash companions are usable + // only when Bash is in that definition, never through an unrelated grant. + tools: CORE_TOOLS.filter((t) => allowed.has(t.name)), // G-1: children get their OWN registry; their runs die with the child // under 'conversation-closed' when destroyChildrenOf tears them down. shells: this.shellsFor(childId), @@ -3676,7 +3674,7 @@ export class NativeSessionHost extends EventEmitter { // parent's conversation crosses over — the brief in the first user turn // is the entire context the child gets. systemPrompt: assembleSystemPrompt({ - presetBody: specialist.systemPrompt, cwd: workDir, appVersion: this.appVersion, gitSnapshot, + presetBody: specialist.systemPrompt, cwd: workDir, appVersion: this.appVersion, gitSnapshot, projectInstructionFiles, promptVariant: profile.promptVariant, hasTools: profile.supportsTools, instructionBudgetTokens: profile.injectionBudgetTokens, // audience 'parent': the shared doctrine's writing-for-the-user block is @@ -3709,7 +3707,7 @@ export class NativeSessionHost extends EventEmitter { decide: buildChildDecide({ parentDecide: this.buildDecide(parentId, parent.cwd, preset.presetRules, { specialistScope: specialist.id }), charter: specialist.charter, - allowedTools: specialist.allowedTools, + allowedTools: effectiveAllowedTools, // envelopeGranted: true means the hire was PERMITTED, not that the user was necessarily // asked. Two ways that happens: (1) a Task-tool ask card (new spawn) or the original // spawn's ask card (resume) was answered — real consent; or (2) the active permission @@ -3995,7 +3993,7 @@ export class NativeSessionHost extends EventEmitter { // destroy() can only release the lease on the LiveEntry it captured. See // McpLease in mcp-manager.ts. // The git line + trigger index, off the main thread, before the session is built (C3, B8). - const [gitSnapshot, triggers] = await Promise.all([gitSnapshotAsync(cwd), buildTriggerIndex(cwd)]); + const [gitSnapshot, triggers, projectInstructionFiles] = await Promise.all([gitSnapshotAsync(cwd), buildTriggerIndex(cwd), prepareProjectInstructions(cwd, profile.injectionBudgetTokens)]); const mcpLease = await this.acquireMcp(sessionId); const mcpServers = mcpLease?.servers; const harness = header.stepGuard === undefined @@ -4014,8 +4012,9 @@ export class NativeSessionHost extends EventEmitter { // `binding` (not header.binding) — same override reason as above. { sessionId, cwd, harness, binding, contextLength, profile, pricing, free, commitCompaction: proposal => this.commitCompaction(sessionId, session, proposal), + takeReadyBusyMessage: () => this.takeReadyBusyMessage(sessionId, session), ...(mcpServers ? { mcpServers } : {}), - ...this.toolWiring(sessionId, cwd, preset, profile, gitSnapshot, triggers) }, + ...this.toolWiring(sessionId, cwd, preset, profile, gitSnapshot, triggers, projectInstructionFiles) }, this.modelFactory, ); // Full history rebuild (spec §2.5): rebuildHistory reconstructs the assistant @@ -4030,7 +4029,7 @@ export class NativeSessionHost extends EventEmitter { throw err; } this.presetIdFor.set(sessionId, preset.manifest.id); - this.wire(sessionId, cwd, session, mcpLease); + this.wire(sessionId, cwd, session, mcpLease, projectInstructionFiles); if (slotsUnknown) this.live.get(sessionId)!.refreshSlotsAfterTurn = true; // Task 9 — AFTER wire(), not before: reconcileDelegations's own // queueDelivery() call needs this.live.get(sessionId) to already resolve @@ -4095,7 +4094,16 @@ export class NativeSessionHost extends EventEmitter { // Task 11: mint a stable id per queued entry so the renderer can target // this exact message later with removeQueued() (Cancel/Edit before send). const queueId = randomUUID(); - entry.queue.push({ id: queueId, text, attachments }); + // WHY: both the host drainer and a future in-turn claimant must leave this + // head untouched until the IPC acknowledgement has had a macrotask to flush. + const queued = { id: queueId, text, attachments, ready: false }; + entry.queue.push(queued); + setImmediate(() => { + queued.ready = true; + // The turn may have settled while this head was unready. Readiness is + // itself a progress signal, not something the next send must discover. + if (this.live.get(sessionId) === entry) this.kickReadyQueue(sessionId, entry); + }); return { status: 'queued', queueId }; } entry.inFlight = true; @@ -4146,97 +4154,104 @@ export class NativeSessionHost extends EventEmitter { return true; } - // Runs the dispatched turn, then drains the queue turn-by-turn. send() settling - // is the ONLY drain trigger — it settles strictly after turn-complete / - // session-error / user-interrupt, and stays unsettled across a permission ask - // (an ask pauses the turn; draining on it would hard-throw re-entrancy). + /** "Send now" (Destin, 2026-09-28): Stop, then send THIS waiting message + * next, ahead of the others. WHY: ordinary mid-task messages wait for the + * current batch; this is for not waiting at all. Move + stop are sync, so + * the drain after the stop finds it first. Contract as removeQueued. */ + sendQueuedNow(sessionId: string, queueId: string): boolean { + const held = this.startingSends.get(sessionId); // still starting: reorder only + if (held) { + const i = held.findIndex((q) => q.id === queueId); + if (i !== -1) { held.unshift(...held.splice(i, 1)); return true; } + } + const entry = this.live.get(sessionId); + if (!entry) return false; + const idx = entry.queue.findIndex((q) => q.id === queueId); + if (idx === -1) return false; + const [item] = entry.queue.splice(idx, 1); + item.ready = true; // its IPC ack flushed long before a button could be pressed + entry.queue.unshift(item); + this.interrupt(sessionId); + return true; + } + + /** WHY: only the live root driver may remove an acknowledged FIFO head. + * No awaits between checking generation/readiness/quiesce and the shift. */ + private takeReadyBusyMessage(sessionId: string, session: HarnessSession): + { id: string; text: string; attachments: string[]; restore: () => void } | undefined { + const entry = this.live.get(sessionId); + if (!entry || entry.session !== session || entry.parentSessionId || !entry.inFlight || + entry.quiescing || entry.compacting || entry.queue[0]?.ready === false) return; + const item = entry.queue.shift(); + if (!item) return; + return { ...item, restore: () => { + if (this.live.get(sessionId) === entry && !entry.quiescing) entry.queue.unshift(item); + else log('ERROR', 'NativeSessionHost', 'claimed message could not be restored after delivery failure', { sessionId, queueId: item.id }); + } }; + } + + /** Claim an idle slot for a head whose acknowledgement fence just opened. + * Never bypass an unready FIFO head; runTurns does the actual shift. */ + private kickReadyQueue(sessionId: string, entry: LiveEntry): void { + if (entry.inFlight || entry.quiescing || entry.compacting || !entry.queue[0] || entry.queue[0].ready === false) return; + entry.inFlight = true; + entry.running = new Promise((resolve) => { + setImmediate(() => { void this.runTurns(sessionId, entry, IDLE_PASS).then(resolve, resolve); }); + }); + } + + // Run turns serially. A settled send, the notice tail and a queued head's + // readiness transition are drain triggers. An ask pauses the current turn; + // no other send runs until it settles (the driver rejects re-entrancy). /** `first` is a plain string for an ordinary send, or a THUNK when the turn * starts some other way — today only /skill-name, whose opener is * `session.runSkill` (same turn machinery, different transcript event). * Queued follow-ups are always plain sends, so queue semantics are unchanged. */ private async runTurns(sessionId: string, entry: LiveEntry, first: SendUnit | (() => Promise)): Promise { - // Fix (Task 4 fix pass 3): the WHOLE body is now wrapped in a single - // try/finally so "every runTurns exit clears entry.inFlight" is true by - // CONSTRUCTION — one statement, not a comment asserting a property the - // code has to remember to uphold at every return/break/throw site. Before - // this, `entry.inFlight = false` was a bare statement at the tail: a - // throw from ANY of the unguarded `await this.ledger.*` calls inside the - // delivery loop below (claimUndelivered at the top of the loop, - // releaseClaim on either liveness-mismatch branch) propagated straight - // out of this function, so that tail statement was never reached and the - // session was permanently stuck "in flight" — the exact bug the loop - // unification was meant to prevent, arriving by exception instead of by - // `return`. The early `return` in the queue-drain loop below (destroy() - // raced this turn) still runs this finally too, which is harmless: by - // definition `this.live.get(sessionId) !== entry` there, so `entry` is - // already a discarded object and setting its `inFlight` flag touches - // nothing live. + // WHY: even a failed ledger claim or a replaced generation must release + // this pass's slot; the finally covers every exit. try { - // Task 4 (plan 1c) — re-read this project folder's specialist catalog - // before dispatching this pass's turn(s): a file dropped into a - // specialists folder since the last turn is offered starting on THIS - // turn, without a session restart. ONE call per runTurns invocation - // (not per queued follow-up) — every turn drained in this same pass - // shares the roster this one read resolved. ensureFresh()'s own - // fingerprint check makes an unchanged folder cheap (a handful of - // stat() calls, never a re-parse); it never throws (every fallible fs - // call inside it is already individually guarded — see catalog.ts's - // own WHY comments), so this is not wrapped in its own try/catch. - // Fix (Task 4 review): gated on `!entry.parentSessionId` — ROOT sessions - // ONLY. A specialist child DOES reach this function: runSpecialist's - // runTurn() closure calls this.send(childId, ...), and send() dispatches - // unconditionally into this.runTurns() for whatever id it's given, so - // every child turn (the opening turn AND the empty-report nudge) used to - // run this call too — an earlier comment here claimed otherwise, which - // was simply wrong. The real reason to skip it for a child: its roster - // is fixed at spawn (R12) and never read again — createChild builds a - // child's tools by hand and never calls toolWiring(), so no child ever - // consults this.specialistCatalog.roster(cwd) for ITS OWN cwd. Without - // this gate, every child turn wrote a fresh entry into the catalog's - // per-cwd cache (a Map with no eviction) for a cwd nothing will ever - // read a roster for — often a work_dir narrowed to a subfolder nothing - // else touches — so the cache grew forever across the process's life. + // WHY: refresh the root's roster once per pass, not per queued turn. + // Children use their fixed spawn roster; refreshing each child's cwd + // would grow the catalog cache without a consumer. if (!entry.parentSessionId) await this.specialistCatalog.ensureFresh(entry.cwd); let next: SendUnit | (() => Promise) | undefined = first; - while (next !== undefined) { - // A user-started turn lifts the post-Stop hold: everything parked since - // the Stop is spliced into history NOW, silently, so the model reads it - // as context for this message rather than getting a turn of its own. - // Checked per turn, not once per pass, because a message queued before - // the Stop still runs after it (pinned: "the queue still drains"). - if (entry.holdDeliveries && next !== IDLE_PASS) { - entry.holdDeliveries = false; - await this.drainDeliveries(sessionId, entry, 'splice'); + // WHY: notices may await the model after the last queued shift. Alternate + // delivery and FIFO dispatch until BOTH lanes are quiet at the same boundary. + for (;;) { + while (next !== undefined) { if (this.live.get(sessionId) !== entry) return; - } - try { - if (typeof next === 'function') await next(); - else await entry.session.send(next.text, next.attachments); - } catch (err) { - log('ERROR', 'NativeSessionHost', 'send failed', { sessionId, error: String(err) }); - } - // Destroy() may have removed/replaced the entry mid-turn — stop draining then. - if (this.live.get(sessionId) !== entry) return; - // A local model is loaded by now (a real turn just ran): re-read the - // helper cap the engine could not answer at create/resume/swap. INSIDE - // the drain loop, before the shift below, on purpose — a message - // queued while this await is in the air is picked up by that shift, - // whereas an await placed after the loop (first cut, 2026-09-16) - // stranded such a message until the next send, since inFlight was - // still true and nothing re-read the queue. Root sessions only (a - // child's roster is fixed at spawn); never after a delivery-only pass, - // which loads nothing. - if (entry.refreshSlotsAfterTurn && !entry.parentSessionId && typeof next !== 'function') { - await this.refreshLocalSlots(sessionId, entry); + // A user-started turn lifts the post-Stop hold so the model reads + // parked reports as context, not as a new turn of their own. + if (entry.holdDeliveries && next !== IDLE_PASS) { + entry.holdDeliveries = false; + await this.drainDeliveries(sessionId, entry, 'splice'); + if (this.live.get(sessionId) !== entry) return; + } + try { + if (typeof next === 'function') await next(); + else await entry.session.send(next.text, next.attachments); + } catch (err) { + log('ERROR', 'NativeSessionHost', 'send failed', { sessionId, error: String(err) }); + } if (this.live.get(sessionId) !== entry) return; + // A local model can answer its slot count only after a real turn. + // Refresh inside the loop so sends accepted during the await are seen. + if (entry.refreshSlotsAfterTurn && !entry.parentSessionId && typeof next !== 'function') { + await this.refreshLocalSlots(sessionId, entry); + if (this.live.get(sessionId) !== entry) return; + } + // The atomic shift owns the queue ID; an unready FIFO head blocks + // dispatch, and its scheduled readiness transition restarts the pass. + next = entry.quiescing || entry.queue[0]?.ready === false ? undefined : entry.queue.shift(); } - // .text: queue entries are {id, text} (Task 11) — the id only matters to - // removeQueued(); shift() here is what makes a removed entry unreachable. - next = entry.queue.shift(); + // Stop pressed during this pass: leave reports parked, but never park + // submitted user messages. A notice pass can receive sends while awaiting. + if (!entry.holdDeliveries && !entry.quiescing) await this.drainDeliveries(sessionId, entry, 'turn'); + if (this.live.get(sessionId) !== entry || entry.quiescing) return; + next = entry.queue[0]?.ready === false ? undefined : entry.queue.shift(); + if (next === undefined) break; } - // Stop pressed during this pass: leave everything parked (holdDeliveries' - // own WHY). The next user message drains it via the splice above. - if (!entry.holdDeliveries) await this.drainDeliveries(sessionId, entry, 'turn'); } finally { entry.inFlight = false; } diff --git a/desktop/src/main/harness/prompt-assembly.ts b/desktop/src/main/harness/prompt-assembly.ts index 45f73bdc1..8200d1a67 100644 --- a/desktop/src/main/harness/prompt-assembly.ts +++ b/desktop/src/main/harness/prompt-assembly.ts @@ -4,8 +4,9 @@ // // WHY not reuse project-context.ts / context-discovery.ts: the former is a pure // mapper over pre-computed basenames and the latter only scans the exact project -// dir + .claude (async, for the context UI). Neither does the session-start -// walk-up-to-git-root that the assembled prompt needs, so this owns its own IO. +// dir + .claude (async, for the context UI). Production now supplies the +// asynchronously captured ancestor inventory; the sync reader remains for +// legacy single-file callers and Claude Code context compatibility. import * as fs from 'fs'; import * as path from 'path'; import { execFile, execFileSync } from 'child_process'; @@ -14,6 +15,7 @@ import type { PromptVariant } from './capability-profile'; import { variantOverlay } from './prompts/variants'; import { fitProjectInstructions } from './injection/injection-budget'; import { sharedDoctrine } from './prompts/shared-doctrine'; +import { renderProjectInstructionFiles, type ProjectInstructionFile } from './injection/project-instructions'; // promptVariant is the capability-profile steering overlay (see prompts/variants.ts). // Optional so pre-variant callers assemble byte-identically; only local-small adds text. @@ -38,7 +40,7 @@ const DEFAULT_INSTRUCTION_BUDGET_TOKENS = 20_000; // gitSnapshot: the git line, computed AHEAD by the caller with // gitSnapshotAsync (2026-09-16 C3). When absent the sync shell-out below runs, // which only the evaluator and tests should reach — see the WHY on gitSnapshotAsync. -export interface PromptInputs { presetBody: string; cwd: string; appVersion: string; promptVariant?: PromptVariant; hasTools?: boolean; instructionBudgetTokens?: number; supportsParallelToolCalls?: boolean; audience?: 'user' | 'parent'; presetName?: string; gitSnapshot?: string } +export interface PromptInputs { presetBody: string; cwd: string; appVersion: string; promptVariant?: PromptVariant; hasTools?: boolean; instructionBudgetTokens?: number; supportsParallelToolCalls?: boolean; audience?: 'user' | 'parent'; presetName?: string; gitSnapshot?: string; projectInstructionFiles?: readonly ProjectInstructionFile[] } const GIT_TIMEOUT_MS = 3000; @@ -107,7 +109,10 @@ export function findProjectInstructions(cwd: string): { path: string; name: stri return null; } -function projectInstructions(cwd: string, budgetTokens: number): string | null { +function projectInstructions(cwd: string, budgetTokens: number, files?: readonly ProjectInstructionFile[]): string | null { + // WHY: the host supplies one captured startup snapshot; neither the prompt nor + // the panel may repeat discovery after disk contents have changed. + if (files) return renderProjectInstructionFiles(files); const found = findProjectInstructions(cwd); if (!found) return null; // The budget bounds the FILE BODY only — the wrapping tag is added after, @@ -159,7 +164,7 @@ export function assembleSystemPromptParts(i: PromptInputs): PromptPart[] { text: 'You are the YouCoded assistant, an agentic AI running inside the YouCoded app. You may be running on any model the user chose, cloud or local — Claude, GPT, Grok, Gemini, Qwen, Gemma and others.', }, { id: 'preset', label: i.presetName ? `Its preset — ${i.presetName}` : 'Its preset', text: i.presetBody }, - partOrNull('project', 'Your project instructions', projectInstructions(i.cwd, i.instructionBudgetTokens ?? DEFAULT_INSTRUCTION_BUDGET_TOKENS)), + partOrNull('project', 'Your project instructions', projectInstructions(i.cwd, i.instructionBudgetTokens ?? DEFAULT_INSTRUCTION_BUDGET_TOKENS, i.projectInstructionFiles)), { id: 'doctrine', label: 'How it works', diff --git a/desktop/src/main/harness/shell-registry.ts b/desktop/src/main/harness/shell-registry.ts index 855e62208..517254e9a 100644 --- a/desktop/src/main/harness/shell-registry.ts +++ b/desktop/src/main/harness/shell-registry.ts @@ -305,11 +305,19 @@ export class ShellRegistry extends EventEmitter { } catch (e: any) { return { ok: false, reason: 'spawn-failed', detail: e?.message ?? String(e) }; } - const run = this.register({ - toolUseId: spec.toolUseId, command: spec.command, cwd: spec.cwd, child, - startedAt: Date.now(), seedLog: null, recent: '', logPath: null, logStream: null, captureEnv: false, - }, { detached: false, explicit: true }); - return { ok: true, run, runningExplicit: running.length + 1 }; + try { + const run = this.register({ + toolUseId: spec.toolUseId, command: spec.command, cwd: spec.cwd, child, + startedAt: Date.now(), seedLog: null, recent: '', logPath: null, logStream: null, captureEnv: false, + }, { detached: false, explicit: true }); + return { ok: true, run, runningExplicit: running.length + 1 }; + } catch (e: any) { + // WHY: a spawned but unregistered child is still ours to stop; give the + // caller a real setup failure, never an id for a run no registry owns. + child.on('error', () => {}); // late spawn errors cannot crash main + if (child.exitCode === null && child.signalCode === null) killTree(child, { graceMs: 0 }); + return { ok: false, reason: 'spawn-failed', detail: e?.message ?? String(e) }; + } } /** Hand-off (spec §5.5): the same process bash.ts already spawned, adopted @@ -331,6 +339,9 @@ export class ShellRegistry extends EventEmitter { fs.mkdirSync(dir, { recursive: true }); logPath = path.join(dir, `bash-${Date.now()}-${shellId}.txt`); logStream = fs.createWriteStream(logPath); + // WHY: opening is asynchronous; an EACCES can arrive before the sweep or + // the registry's run listeners. Keep the tail as the fallback, not a crash. + logStream.on('error', () => {}); // The 7-day sweep used to fire only from bash.ts's foreground spill; a // user whose long commands all run in the background would never have // triggered it, and these logs would pile up forever (2026-08-28 review). diff --git a/desktop/src/main/harness/tool-args.ts b/desktop/src/main/harness/tool-args.ts new file mode 100644 index 000000000..0ecbde166 --- /dev/null +++ b/desktop/src/main/harness/tool-args.ts @@ -0,0 +1,17 @@ +import type { NativeTool } from './tools/types'; + +/** WHY: the pre-write guidance gate must see exactly the same validated, + * normalized path as execution; invalid args remain ordinary tool errors. */ +export function parseToolArgs(tool: NativeTool, input: unknown): ReturnType { + let parsed = tool.inputSchema.safeParse(input); + if (!parsed.success && typeof input === 'string') { + try { + const recovered: unknown = JSON.parse(input); + if (recovered && typeof recovered === 'object') { + const reparsed = tool.inputSchema.safeParse(recovered); + if (reparsed.success) parsed = reparsed; + } + } catch { /* Let the normal argument-error result explain the invalid input. */ } + } + return parsed; +} diff --git a/desktop/src/main/harness/tool-group-finalization.ts b/desktop/src/main/harness/tool-group-finalization.ts new file mode 100644 index 000000000..ebe5dc414 --- /dev/null +++ b/desktop/src/main/harness/tool-group-finalization.ts @@ -0,0 +1,27 @@ +// WHY: a permission callback may fail after the assistant has announced an entire +// group. Tag only pre-execution callback failures; a started action is not retried +// or silently reclassified as an approval/denial. +export class PermissionCallbackFailure { + constructor(readonly cause: unknown) {} +} + +export async function permissionCallback(call: () => Promise): Promise { + try { return await call(); } + catch (err) { throw new PermissionCallbackFailure(err); } +} + +/** Emit exactly one result for each not-yet-recorded call; the caller retains + * completed results and commits the complete tool message and event origins. */ +export function finalizeRemainingCalls( + calls: readonly TCall[], start: number, textFor: (index: number) => string, + emit: (call: TCall, text: string) => string, part: (call: TCall, text: string) => TPart, +): { parts: TPart[]; origins: string[] } { + const parts: TPart[] = []; + const origins: string[] = []; + for (let i = start; i < calls.length; i++) { + const text = textFor(i); + origins.push(emit(calls[i], text)); + parts.push(part(calls[i], text)); + } + return { parts, origins }; +} diff --git a/desktop/src/main/harness/tools/ask-user-question.ts b/desktop/src/main/harness/tools/ask-user-question.ts index 06ef4ad60..7e1bc38a0 100644 --- a/desktop/src/main/harness/tools/ask-user-question.ts +++ b/desktop/src/main/harness/tools/ask-user-question.ts @@ -48,15 +48,25 @@ export function formatAnswers(args: AskUserQuestionInput, updatedInput: Record; - const lines = args.questions.map((qq) => { - const answer = renderAnswer(answers[qq.question]); + // WHY: the legacy text-keyed map cannot distinguish identical wording. Only + // accept a COMPLETE, well-typed ordered response; malformed remote clients + // fall back to the old map without ever creating a dangling tool result. + const rawOrdered = updatedInput?.orderedAnswers; + const ordered = Array.isArray(rawOrdered) && rawOrdered.length === args.questions.length + && rawOrdered.every((item) => item && typeof item === 'object' && !Array.isArray(item) + && (typeof item.answer === 'string' || (Array.isArray(item.answer) && item.answer.every((v: unknown) => typeof v === 'string'))) + && (item.note === undefined || typeof item.note === 'string')) + ? rawOrdered as Array<{ answer: string | string[]; note?: string }> : null; + const lines = args.questions.map((qq, index) => { + const answer = renderAnswer(ordered ? ordered[index].answer : answers[qq.question]); // A typed "Other" answer arrives as plain text in the answer slot. Tell the // model when what came back was NOT one of its options, so it reads the // text as the user's own answer instead of hunting for a matching label. const labels = new Set(qq.options.map((o) => o.label)); const parts = answer === NO_SELECTION ? [] : answer.split(', '); const ownAnswer = parts.length > 0 && parts.some((p) => !labels.has(p)); - const note = typeof notes[qq.question] === 'string' ? (notes[qq.question] as string).trim() : ''; + const note = ordered ? (ordered[index].note ?? '').trim() + : typeof notes[qq.question] === 'string' ? (notes[qq.question] as string).trim() : ''; let out = `Q: ${qq.question}\nA${ownAnswer ? ' (the user typed their own answer)' : ''}: ${answer}`; if (note) out += `\nNote from the user: ${note}`; return out; diff --git a/desktop/src/main/harness/tools/bash.ts b/desktop/src/main/harness/tools/bash.ts index 8ee0fabc3..2161c49ac 100644 --- a/desktop/src/main/harness/tools/bash.ts +++ b/desktop/src/main/harness/tools/bash.ts @@ -654,10 +654,11 @@ export const BashTool = defineTool({ return; } // admin-password design §2.3/§11 task 5: registered for the foreground - // path here — a hand-off (the timeout branch below) removes the - // close/error listeners BEFORE ctx.shells.adopt() re-registers the - // SAME pid into the SAME RunningCalls, so registration correctly - // survives the hand-off rather than needing a separate carry-over. + // path here — a hand-off (the timeout branch below) lets + // ctx.shells.adopt() re-register the SAME pid into the SAME RunningCalls, + // then removes this call's close/error listeners without firing them, so + // registration survives the hand-off. A FAILED adopt keeps them attached: + // the kill that follows reaches onCallExit and unregisters normally. if (ctx.runningCalls && child.pid) { void ctx.runningCalls.registerPid(child.pid, { sessionId: ctx.sessionId, toolCallId: ctx.toolCallId ?? 'unknown' }); } @@ -753,8 +754,10 @@ export const BashTool = defineTool({ tailBuf = (tailBuf + s).slice(-TAIL_RETAIN_CHARS); if (probe) probeTail = (probeTail + s).slice(-4096); }; - child.stdout.on('data', (d) => cap(String(d))); - child.stderr.on('data', (d) => cap(String(d))); + const onStdout = (d: Buffer) => cap(String(d)); + const onStderr = (d: Buffer) => cap(String(d)); + child.stdout.on('data', onStdout); + child.stderr.on('data', onStderr); let done = false; /** G-1: set by the timeout timer once the registry adopted this process. */ let handedOffTo: string | null = null; @@ -1046,6 +1049,7 @@ export const BashTool = defineTool({ // restarted — and this call resolves with the output so far. Two // cases keep the old kill: no registry in this context (tests, // one-off tools), and a leading `sleep` (backgrounding a sleep is churn). + if (done) return; // A close/abort that won the deadline retains its result. if (!ctx.shells || LEADING_SLEEP.test(args.command)) { killTree(child, { graceMs: 0 }); finish( @@ -1056,24 +1060,37 @@ export const BashTool = defineTool({ ); return; } - // From here the registry reads the pipes; this call must stop - // listening or every byte would be counted twice. - child.stdout.removeAllListeners('data'); - child.stderr.removeAllListeners('data'); - child.removeAllListeners('close'); - child.removeAllListeners('error'); + // WHY: registration can throw before ownership transfers. Keep foreground + // listeners until it succeeds, then remove only OUR listeners (not the + // registry's) so no output or close/error is lost during a failed setup. + let run; + try { + run = ctx.shells.adopt({ + toolUseId: ctx.toolCallId ?? 'unknown', command: args.command, cwd: startCwd, child, startedAt, + seedLog: spillStream ? null : headBuf, + recent: tailBuf, logPath: spillPath, logStream: spillStream, captureEnv, + }); + } catch (e: any) { + // A failed handoff must neither strand the Bash promise nor leave an + // untracked command running. C2's deferred descendant guarantees do + // not follow from this best-effort stop of our freshly spawned child. + if (child.exitCode === null && child.signalCode === null) killTree(child, { graceMs: 0 }); + finish(`Failed to hand off background command: ${e?.message ?? e}. Cleanup was attempted; process exit was not confirmed.\n`, true); + return; + } + child.stdout.off('data', onStdout); + child.stderr.off('data', onStderr); + child.off('close', onClose); + child.off('error', onError); + if (done) { + // An abort/close can settle during a re-entrant adoption callback. + // The registry now owns this run; stop it there, never report success. + void ctx.shells.kill(run.shellId, 'user').catch(() => {}); + return; + } // D4: a handed-off command can no longer be answered — close stdin so // a prompt fails fast with its own error instead of hanging forever. try { child.stdin?.end(); } catch { /* already closed */ } - const run = ctx.shells.adopt({ - toolUseId: ctx.toolCallId ?? 'unknown', command: args.command, cwd: startCwd, child, startedAt, - // headBuf is complete when no spill has started (nothing dropped yet); - // otherwise the spill stream already holds everything. - seedLog: spillStream ? null : headBuf, - recent: tailBuf, - logPath: spillPath, logStream: spillStream, - captureEnv, - }); handedOffTo = run.shellId; spillPath = run.logPath; finish( @@ -1102,9 +1119,9 @@ export const BashTool = defineTool({ if (ctx.signal.aborted) onAbort(); // admin-password design §2.3/§5/§6/§11 task 5: the ONLY two listeners // that mean "this foreground call's own root pid is truly done" — - // removed wholesale by the hand-off branch above BEFORE adopt(), so - // this unregister/forget/wipe never double-fires once ownership moves - // to ShellRegistry (its own onExit does the equivalent there). + // removed by the hand-off branch above once adopt() succeeds, so this + // unregister/forget/wipe never double-fires once ownership moves to + // ShellRegistry (its own onExit does the equivalent there). const onCallExit = () => { if (ctx.runningCalls && child.pid) { ctx.runningCalls.unregister(child.pid); @@ -1120,18 +1137,20 @@ export const BashTool = defineTool({ // Async spawn failure (the path Windows takes for a bad cwd): name the // shell + cwd actually used, not just Node's bare `spawn ` — // same diagnosability contract as the sync catch above. - child.on('error', (err) => { + const onError = (err: Error) => { onCallExit(); finish(`Failed to start shell: ${err.message} (shell=${shell.cmd}; cwd=${startCwd})\n`, true); - }); + }; + child.on('error', onError); // WHY no exit-code prefix here anymore: the metadata line above now states // `exit N` for every result, so a leading "(exit code N)" duplicated the // same fact in two places. The timeout/abort handlers keep their prefixes — // those are messages, not exit codes. - child.on('close', (code) => { + const onClose = (code: number | null) => { onCallExit(); finish('', code !== 0, code); - }); + }; + child.on('close', onClose); }); }, }); diff --git a/desktop/src/main/harness/tools/file-fingerprint.ts b/desktop/src/main/harness/tools/file-fingerprint.ts index 369875be3..26b324af7 100644 --- a/desktop/src/main/harness/tools/file-fingerprint.ts +++ b/desktop/src/main/harness/tools/file-fingerprint.ts @@ -31,3 +31,25 @@ export function fingerprintOf(buf: Buffer | string): string { export async function fingerprintFile(absPath: string): Promise { return fingerprintOf(await fs.promises.readFile(absPath)); } + +/** The same fingerprint as fingerprintOf, computed in small pieces as the file + * streams in. WHY (Destin, 2026-09-28: no new stutters): the app's main + * process serves every window, and hashing a large file in one go holds it for + * tens of milliseconds (~36 ms at 50 MB). Between pieces the process is free to + * handle other work, so even the largest readable file causes no visible hitch. + * Used by Read's repeat check, where most answers are "unchanged" and the + * whole-file decode and line split can then be skipped entirely. */ +export function fingerprintFileInPieces(absPath: string): Promise { + return new Promise((resolve, reject) => { + const hash = createHash('sha256'); + let length = 0; + fs.createReadStream(absPath, { highWaterMark: 256 * 1024 }) + .on('data', (chunk: Buffer | string) => { + const b = typeof chunk === 'string' ? Buffer.from(chunk, 'utf8') : chunk; + length += b.length; + hash.update(b); + }) + .on('error', reject) + .on('end', () => resolve(`${length}:${hash.digest('hex')}`)); + }); +} diff --git a/desktop/src/main/harness/tools/net-guard.ts b/desktop/src/main/harness/tools/net-guard.ts index f27ba56d3..6947a908b 100644 --- a/desktop/src/main/harness/tools/net-guard.ts +++ b/desktop/src/main/harness/tools/net-guard.ts @@ -53,7 +53,8 @@ export function isPrivateIp(ip: string): boolean { /** Scheme + address validation for ONE URL. Throws NetGuardError with an honest, * specific message (docs/error-message-standards.md). Returns the parsed URL. */ -export async function assertPublicHttpUrl(raw: string, lookup: LookupFn = defaultLookup): Promise { +export async function assertPublicHttpUrl(raw: string, lookup: LookupFn = defaultLookup, signal?: AbortSignal): Promise { + signal?.throwIfAborted(); let url: URL; try { url = new URL(raw); } catch { throw new NetGuardError(`"${raw}" is not a valid URL.`); } if (url.protocol !== 'http:' && url.protocol !== 'https:') { @@ -69,10 +70,32 @@ export async function assertPublicHttpUrl(raw: string, lookup: LookupFn = defaul if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local')) { throw new NetGuardError(`${host} is a local address — fetching it is blocked.`); } + // WHY: DNS promises do not accept a portable cancellation signal. Settle our + // wait on abort while consuming any late lookup result/rejection. The listener + // is scoped to this hop; the same signal/deadline is shared by every hop. + const resolveHost = () => Promise.resolve().then(() => { + signal?.throwIfAborted(); + try { + return Promise.resolve(lookup(host)).catch(() => { + throw new NetGuardError(`Could not resolve ${host} — check the URL or the network connection.`); + }); + } catch { + throw new NetGuardError(`Could not resolve ${host} — check the URL or the network connection.`); + } + }); let addrs: Array<{ address: string }>; - try { addrs = await lookup(host); } catch { - throw new NetGuardError(`Could not resolve ${host} — check the URL or the network connection.`); - } + if (signal) { + let onAbort!: () => void; + const aborted = new Promise((_resolve, reject) => { + onAbort = () => reject(signal.reason); + signal.addEventListener('abort', onAbort, { once: true }); + }); + try { + signal.throwIfAborted(); + addrs = await Promise.race([resolveHost(), aborted]); + } finally { signal.removeEventListener('abort', onAbort); } + } else addrs = await resolveHost(); + signal?.throwIfAborted(); if (addrs.length === 0) throw new NetGuardError(`Could not resolve ${host}.`); const bad = addrs.find((a) => isPrivateIp(a.address)); if (bad) throw new NetGuardError(`${host} resolves to the private/internal address ${bad.address} — fetching it is blocked.`); @@ -122,8 +145,8 @@ export interface GuardedFetchOpts { } /** Fetch with MANUAL redirect following: every hop re-runs assertPublicHttpUrl. - * Timeout/abort surfaces as a DOMException (AbortError) from fetch — NOT a - * NetGuardError — which the tool layer translates for the user (spec §3.1). */ + * Caller abort / deadline (AbortError / TimeoutError) also ends DNS waiting; + * neither is a NetGuardError or proof that OS resolution was canceled. */ export async function guardedFetch(rawUrl: string, opts: GuardedFetchOpts): Promise<{ res: Response; finalUrl: string }> { const fetchImpl = opts.fetchImpl ?? fetch; const lookup = opts.lookup ?? defaultLookup; @@ -136,7 +159,7 @@ export async function guardedFetch(rawUrl: string, opts: GuardedFetchOpts): Prom let method = (opts.method ?? 'GET').toUpperCase(); let body = opts.body; for (let hop = 0; ; hop++) { - const url = await assertPublicHttpUrl(current, lookup); + const url = await assertPublicHttpUrl(current, lookup, deadline); const host = url.hostname.toLowerCase(); if (originHost === null) originHost = host; const decision = opts.allowHost?.(url.hostname, hop); @@ -147,6 +170,7 @@ export async function guardedFetch(rawUrl: string, opts: GuardedFetchOpts): Prom const onOrigin = host === originHost; if (!onOrigin) for (const p of opts.credentialQueryParams ?? []) url.searchParams.delete(p); const target = url.toString(); + deadline.throwIfAborted(); // no dispatch after DNS or allowHost used the remaining budget const res = await fetchImpl(target, { redirect: 'manual', method, diff --git a/desktop/src/main/harness/tools/read.ts b/desktop/src/main/harness/tools/read.ts index 604ef7a30..34d88aa75 100644 --- a/desktop/src/main/harness/tools/read.ts +++ b/desktop/src/main/harness/tools/read.ts @@ -5,7 +5,7 @@ import { defineTool } from './registry'; import { canonicalize, resolveP, shellCwdMissHint, lunaPathRefused } from './guards'; import { deliverableImageMediaType, UNDELIVERABLE_IMAGE_EXTENSIONS, MAX_ATTACHMENT_BYTES } from '../image-support'; import { readPdfAsToolResult } from '../pdf-text'; -import { fingerprintFile, fingerprintOf } from './file-fingerprint'; +import { fingerprintFile, fingerprintFileInPieces, fingerprintOf } from './file-fingerprint'; const BINARY_SNIFF_BYTES = 8000; @@ -203,30 +203,29 @@ export const ReadTool = defineTool({ const offset = args.offset ?? 1; const limit = Math.min(args.limit ?? 2000, 2000); const canonical = canonicalize(args.file_path, ctx.cwd); - // G-11 (2026-08-26 tools investigation): the same slice of an UNCHANGED file - // was already served this session — the model still has it (the session - // forgets these marks whenever history is discarded or shrunk, so this is - // never claimed across a compaction or resume). A short notice beats a - // second copy: Claude Code and Hermes both do this. Checked BEFORE the file - // is read so the repeat costs a stat and nothing else. Still counts as a - // Read for the edit gate — the registry stamp happens here as well as on the - // full path below (a binary file is only stamped AFTER it passes the binary - // refusal, so a refused Read never satisfies the gate; a dedupe hit can - // only be for a file that already passed it). const servedKey = `${canonical}|${offset}|${limit}`; const prior = ctx.servedReads?.get(servedKey); - if (prior && prior.mtimeMs === st.mtimeMs) { - ctx.readRegistry.set(canonical, prior.fingerprint); - const ago = ctx.toolCallIndex !== undefined ? ctx.toolCallIndex - prior.callIndex : undefined; - const when = ago !== undefined ? `(${ago} call${ago === 1 ? '' : 's'} ago)` : '(earlier this session)'; - return { - text: `Read ${args.file_path}: lines ${prior.from}–${prior.to} — ` - + `Unchanged since your earlier Read this session ${when} — the content you already have is current. ` - + 'Use a different offset/limit to see another part of the file.', - }; - } // fs.promises (2026-09-16 C4): up to MAX_READ_BYTES used to be read // synchronously on the main thread, several times per turn. + // WHY check a repeat BEFORE loading the file: an unchanged repeat is the + // common case, and answering it from a piecewise fingerprint skips the + // whole-file decode + line split (~100 ms of main-process time at 50 MB) + // that would stall every window. A size mismatch skips even the hash. The + // bytes, not the mtime, decide: a replacement can keep mtime and size. + if (prior && prior.fingerprint.startsWith(`${st.size}:`)) { + let current: string | null = null; + try { current = await fingerprintFileInPieces(abs); } catch { /* fall through to the normal read and its error */ } + if (current === prior.fingerprint) { + ctx.readRegistry.set(canonical, current); + const ago = ctx.toolCallIndex !== undefined ? ctx.toolCallIndex - prior.callIndex : undefined; + const when = ago !== undefined ? `(${ago} call${ago === 1 ? '' : 's'} ago)` : '(earlier this session)'; + return { + text: `Read ${args.file_path}: lines ${prior.from}–${prior.to} — ` + + `Unchanged since your earlier Read this session ${when} — the content you already have is current. ` + + 'Use a different offset/limit to see another part of the file.', + }; + } + } const buf = await fs.promises.readFile(abs); if (looksBinary(buf)) return { text: `Read rejected: ${args.file_path}: it is a binary file.`, isError: true }; const raw = buf.toString('utf8'); @@ -235,15 +234,13 @@ export const ReadTool = defineTool({ // 4) — drop it so line counts and the paging trailer are honest. if (raw.endsWith('\n')) all.pop(); const totalLines = all.length; - // Record for the read-before-edit gate (a content fingerprint, so a later - // external change invalidates it and a mere touch does not) — the file - // exists and was readable, so it counts as read even if the requested page - // is past EOF. - const fingerprint = fingerprintOf(buf); - ctx.readRegistry.set(canonical, fingerprint); if (offset > totalLines) { return { text: `Read failed: ${args.file_path}: offset ${offset} is past the end of the file (${totalLines} lines).`, isError: true }; } + // The file changed (or was never served): stamp the read-before-edit gate + // from the SAME buffer that passed the binary/offset checks above. + const fingerprint = fingerprintOf(buf); + ctx.readRegistry.set(canonical, fingerprint); const slice = all.slice(offset - 1, offset - 1 + limit); const MAX_LINE = 2000; // G-5: a second, per-call cap in CHARS on top of the line cap — 2,000 lines diff --git a/desktop/src/main/harness/tools/types.ts b/desktop/src/main/harness/tools/types.ts index b1aaa876d..0e49f2ad0 100644 --- a/desktop/src/main/harness/tools/types.ts +++ b/desktop/src/main/harness/tools/types.ts @@ -214,13 +214,10 @@ export interface ToolServices { }; } -/** What an earlier Read served (G-11): the file's mtime at that moment, which - * tool call did it, and the line range it showed. */ +/** What an earlier text Read served: verified byte identity, call and slice. */ export interface ServedRead { mtimeMs: number; - /** The content fingerprint recorded for the read gate at that Read, so a - * dedupe hit (mtime unchanged) can re-stamp the registry without reading - * the file again (2026-09-16). */ + /** Compare against freshly read, validated bytes before claiming unchanged. */ fingerprint: string; callIndex: number; from: number; diff --git a/desktop/src/main/ipc-handlers.ts b/desktop/src/main/ipc-handlers.ts index 0f466e498..334619925 100644 --- a/desktop/src/main/ipc-handlers.ts +++ b/desktop/src/main/ipc-handlers.ts @@ -3622,6 +3622,9 @@ export function registerIpcHandlers( // too late / unknown), so this is a thin pass-through like NATIVE_SEND above. ipcMain.handle(IPC.NATIVE_QUEUE_REMOVE, (_e, { sessionId, queueId }: { sessionId: string; queueId: string }) => nativeHost.removeQueued(sessionId, queueId)); + // "Send now" on a waiting message — same sync, never-throws boolean contract. + ipcMain.handle(IPC.NATIVE_QUEUE_SEND_NOW, (_e, { sessionId, queueId }: { sessionId: string; queueId: string }) => + nativeHost.sendQueuedNow(sessionId, queueId)); // Fire-and-forget I/O (no response): interrupt only. The host never throws for unknown ids. ipcMain.on(IPC.NATIVE_INTERRUPT, (_e, { sessionId }: { sessionId: string }) => { nativeHost.interrupt(sessionId); diff --git a/desktop/src/main/preload.ts b/desktop/src/main/preload.ts index 9155285ee..6e19dc8d4 100644 --- a/desktop/src/main/preload.ts +++ b/desktop/src/main/preload.ts @@ -392,6 +392,7 @@ const IPC = { NATIVE_SEND: 'native:send', // Task 11: cancel/edit a queued-but-not-yet-sent message. NATIVE_QUEUE_REMOVE: 'native:queue-remove', + NATIVE_QUEUE_SEND_NOW: 'native:queue-send-now', NATIVE_INTERRUPT: 'native:interrupt', // Stalled-turn Retry — fire-and-forget, same shape as interrupt above. NATIVE_RETRY: 'native:retry', @@ -1522,6 +1523,7 @@ contextBridge.exposeInMainWorld('claude', { // (unlike interrupt below) — the renderer needs the true/false result to // decide between "removed, proceed" and a "too late" toast. queueRemove: (sessionId: string, queueId: string) => ipcRenderer.invoke(IPC.NATIVE_QUEUE_REMOVE, { sessionId, queueId }), + queueSendNow: (sessionId: string, queueId: string) => ipcRenderer.invoke(IPC.NATIVE_QUEUE_SEND_NOW, { sessionId, queueId }), // Fire-and-forget: match ipcMain.on handler that destructures { sessionId }. interrupt: (sessionId: string) => ipcRenderer.send(IPC.NATIVE_INTERRUPT, { sessionId }), // Fire-and-forget like interrupt: the stalled card needs no answer — either diff --git a/desktop/src/main/remote-server.ts b/desktop/src/main/remote-server.ts index 672f0451a..c882db713 100644 --- a/desktop/src/main/remote-server.ts +++ b/desktop/src/main/remote-server.ts @@ -1994,6 +1994,12 @@ export class RemoteServer { this.respond(client.ws, type, id, removed); break; } + // "Send now" — same sync boolean contract as queue-remove above. + case 'native:queue-send-now': { + const sent = this.nativeRuntime ? this.nativeRuntime.nativeHost.sendQueuedNow(payload.sessionId, payload.queueId) : false; + this.respond(client.ws, type, id, sent); + break; + } case 'native:sessions-list': { this.respond(client.ws, type, id, this.nativeRuntime ? await this.nativeRuntime.nativeHost.listAsync() : []); break; diff --git a/desktop/src/renderer/App.tsx b/desktop/src/renderer/App.tsx index 6a139871b..50799c2a6 100644 --- a/desktop/src/renderer/App.tsx +++ b/desktop/src/renderer/App.tsx @@ -851,6 +851,20 @@ function AppInner() { inputBarRef.current?.fillDraft(text); }, [dispatch]); + // "Send now": the row stays until the drain sends it (TRANSCRIPT_USER_MESSAGE + // removes it); false = already on its way, handled like Cancel's too-late. + const handleSendQueuedNow = useCallback(async (sid: string, queueId: string) => { + // WHY catch (PR #585 review): over remote access the request can fail (e.g. the + // connection drops) and a click that silently does nothing reads as broken. + // The cause is unknown here, so the words stay general and the row stays put. + const moved = await window.claude.native.queueSendNow(sid, queueId).catch(() => null); + if (moved === null) { setToast("Send now didn't go through — try again."); return; } + if (!moved) { + setToast('Already sending.'); + dispatch({ type: 'QUEUED_MESSAGE_REMOVED', sessionId: sid, queueId }); + } + }, [dispatch]); + // Compaction watchdog: activity-aware — resets on any reducer update for a // session with compactionPending set. Any transcript event bumps the timer // forward, so long compactions (large sessions) don't trigger a false "may @@ -3887,6 +3901,7 @@ function AppInner() { onAddCredit={chatViewHandlers.addCredit} onCancelQueued={handleCancelQueued} onEditQueued={handleEditQueued} + onSendQueuedNow={handleSendQueuedNow} conversationStatus={conversationStatus} onRefreshConversation={handleRefreshConversation} modelLoadingDemo={s.id === sessionId && new URLSearchParams(location.search).get('mode') === 'workbench' && new URLSearchParams(location.search).get('modelLoading') === '1'} diff --git a/desktop/src/renderer/components/ChatView.tsx b/desktop/src/renderer/components/ChatView.tsx index 970280fb2..7d60075fa 100644 --- a/desktop/src/renderer/components/ChatView.tsx +++ b/desktop/src/renderer/components/ChatView.tsx @@ -89,6 +89,7 @@ interface Props { // session's ChatView instance. onCancelQueued?: (sessionId: string, queueId: string) => void; onEditQueued?: (sessionId: string, queueId: string, text: string) => void; + onSendQueuedNow?: (sessionId: string, queueId: string) => void; /** Remote access batch 2 (questions deck 2026-09-10, Q-3 "keep it, say so"): * where a PHONE's copy of this conversation stands. `reconnecting` and * `restoring` show a quiet busy strip; `incomplete` says the copy may be @@ -102,7 +103,7 @@ interface Props { } // Memoised at the bottom of the file — see the WHY there. -function ChatView({ sessionId, visible, sessionActive, cwd, gamePane, provider, onOpenProviderSettings, onSwitchProviders, onUpgradePlan, onAddCredit, onCancelQueued, onEditQueued, conversationStatus, onRefreshConversation, modelLoadingDemo }: Props) { +function ChatView({ sessionId, visible, sessionActive, cwd, gamePane, provider, onOpenProviderSettings, onSwitchProviders, onUpgradePlan, onAddCredit, onCancelQueued, onEditQueued, onSendQueuedNow, conversationStatus, onRefreshConversation, modelLoadingDemo }: Props) { const state = useChatState(sessionId, { paused: !visible }); // WHY paused: hidden, it redrew per streamed word; live again on show (see useChatState) const dispatch = useChatDispatch(); @@ -1360,6 +1361,7 @@ function ChatView({ sessionId, visible, sessionActive, cwd, gamePane, provider, queuedMessages={state.queuedMessages} onCancel={onCancelQueued ? (queueId) => onCancelQueued(sessionId, queueId) : undefined} onEdit={onEditQueued ? (queueId, text) => onEditQueued(sessionId, queueId, text) : undefined} + onSendNow={onSendQueuedNow ? (queueId) => onSendQueuedNow(sessionId, queueId) : undefined} /> {/* WHY mount the actual model floater in the chat column: when Files or Games opens, outer-root centering would span the drawer as well. */} diff --git a/desktop/src/renderer/components/EditPencilButton.tsx b/desktop/src/renderer/components/EditPencilButton.tsx new file mode 100644 index 000000000..259efb9ce --- /dev/null +++ b/desktop/src/renderer/components/EditPencilButton.tsx @@ -0,0 +1,39 @@ +import React from 'react'; +import { isAndroid } from '../platform'; +import { Tooltip } from './ui'; + +// Pencil SVG icon — matches the one used in StatusBar.tsx +export function PencilIcon({ size = 10 }: { size?: number }) { + return ( + + + + ); +} + +/** The small square pencil button the quick-chips row uses to open its editor. + * WHY shared (Destin, 2026-09-28 Send now review: "re-use the edit element + * used for the status bar, quick chips menu"): one component, so the waiting- + * message strip's Edit and the quick chips' edit cannot drift apart. + * `className` adds hooks only (QuickChips passes `quick-chip-edit`, which float + * chrome uses to give it the chips' own surface); the look lives here. */ +export function EditPencilButton({ label, onClick, className = '', plain = false }: { label: string; onClick: () => void; className?: string; plain?: boolean }) { + const android = isAndroid(); + // `plain` (Destin, 2026-09-28: "no background on the edit button" in the + // waiting-message strip): the same pencil and size with no box, so it sits + // like the plain trash icon beside it. The quick chips keep their box. + const surface = plain ? 'rounded-md text-fg-dim hover:text-fg' : 'rounded-md bg-well border border-edge-dim text-fg-muted hover:bg-inset hover:text-fg'; + return ( + + + + ); +} diff --git a/desktop/src/renderer/components/InputBar.test.tsx b/desktop/src/renderer/components/InputBar.test.tsx index 74b211230..031ed9574 100644 --- a/desktop/src/renderer/components/InputBar.test.tsx +++ b/desktop/src/renderer/components/InputBar.test.tsx @@ -3,9 +3,10 @@ import '@testing-library/jest-dom/vitest'; import React from 'react'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen, cleanup, fireEvent, waitFor, act } from '@testing-library/react'; -import { ChatProvider, useChatDispatch, useChatStore } from '../state/chat-context'; +import { ChatProvider, useChatDispatch, useChatStore, useChatState } from '../state/chat-context'; import { SkillProvider } from '../state/skill-context'; import InputBar, { InputBarHandle } from './InputBar'; +import QueuedMessagesStrip from './QueuedMessagesStrip'; import { buildContextMenu } from './context-menu/build-menu'; import type { VoiceEvent, VoiceReadiness } from '../../shared/voice-types'; @@ -472,6 +473,56 @@ describe('InputBar — stop button (Task 10 placement)', () => { expect(screen.queryByRole('button', { name: 'Stop generating' })).not.toBeInTheDocument(); }); + it('keeps Stop beside native busy queued input, with only Edit and Cancel on its queued row', () => { + const onEdit = vi.fn(); + const onCancel = vi.fn(); + function QueuedFromStore() { + const state = useChatState('sess-1'); + return ; + } + render( + + + + + + + , + ); + act(() => { + capturedDispatch!({ type: 'SESSION_INIT', sessionId: 'sess-1' }); + capturedDispatch!({ type: 'USER_PROMPT', sessionId: 'sess-1', content: 'first action', timestamp: 1 }); + capturedDispatch!({ type: 'QUEUED_MESSAGE_ADDED', sessionId: 'sess-1', queueId: 'q-1', content: 'follow up', timestamp: 2 }); + }); + const stop = screen.getByRole('button', { name: 'Stop generating' }); + expect(stop).toBeEnabled(); + expect(screen.getByRole('button', { name: 'Send message' })).toBeInTheDocument(); + const strip = screen.getByLabelText('Queued messages'); + const queuedActions = () => [...strip.querySelectorAll('button')].map(button => button.getAttribute('aria-label')); + // An exact DOM inventory detects any extra urgent-send control, regardless + // of its label. The separate forbidden-name check catches one elsewhere. + const expected = ['Edit queued message', 'Cancel queued message']; + expect(queuedActions()).toEqual(expected); + const injected = document.createElement('button'); + injected.setAttribute('aria-label', 'Send now'); + strip.append(injected); + expect(() => expect(queuedActions()).toEqual(expected)).toThrow(); + injected.remove(); + expect(screen.queryByRole('button', { name: /send now|after this finishes|stop and send|steer|urgent|priority/i })).not.toBeInTheDocument(); + fireEvent.click(stop); + expect((window as any).claude.native.interrupt).toHaveBeenCalledWith('sess-1'); + expect((window as any).claude.session.sendInput).not.toHaveBeenCalled(); + fireEvent.click(screen.getByRole('button', { name: 'Edit queued message' })); + fireEvent.click(screen.getByRole('button', { name: 'Cancel queued message' })); + expect(onEdit).toHaveBeenCalledWith('q-1', 'follow up'); + expect(onCancel).toHaveBeenCalledWith('q-1'); + act(() => { + capturedDispatch!({ type: 'PERMISSION_REQUEST', sessionId: 'sess-1', toolName: 'Bash', input: { command: 'build' }, requestId: 'req-1' }); + }); + expect(screen.getByRole('button', { name: 'Stop generating' })).toBeEnabled(); + expect(queuedActions()).toEqual(expected); + }); + it('provider="native": click calls native.interrupt, never session.sendInput', () => { renderInputBar('native'); act(() => { capturedDispatch!({ type: 'SESSION_INIT', sessionId: 'sess-1' }); }); diff --git a/desktop/src/renderer/components/MarkdownContent.test.tsx b/desktop/src/renderer/components/MarkdownContent.test.tsx index 52b8d5425..ee1b1072b 100644 --- a/desktop/src/renderer/components/MarkdownContent.test.tsx +++ b/desktop/src/renderer/components/MarkdownContent.test.tsx @@ -555,6 +555,12 @@ describe('MarkdownContent while a reply streams in', () => { live.unmount(); }); + // WHY a named budget (measured 2026-09-28): these fixed-count streaming + // checks take 1.5–7 s alone, but in verify.sh's full run — every suite and the + // screenshot checks at once — two of them passed 30 s and timed out while + // correct. They count work, never clock time, so more time tests nothing less. + const STREAMING_SWEEP_BUDGET_MS = 120_000; + // Review F4: seeded random replies, streamed word by word into a bubble that // sometimes mounts mid-reply, compared as DRAWN PAGES (not parse trees) with // the whole-message render after every word. Mixes in the constructs that act @@ -605,7 +611,7 @@ describe('MarkdownContent while a reply streams in', () => { live.unmount(); today.unmount(); } - }); + }, STREAMING_SWEEP_BUDGET_MS); // What each streamed update costs, in characters: everything the splitter // parsed plus everything handed to react-markdown. Today's whole-message @@ -643,7 +649,7 @@ describe('MarkdownContent while a reply streams in', () => { expectNoMoreThanToday(costs); // A definition only re-draws the blocks that use its label. for (const c of costs) expect(c.drawn.length, `after ${JSON.stringify(c.md.slice(-40))}`).toBeLessThanOrEqual(2); - }); + }, STREAMING_SWEEP_BUDGET_MS); // A reply that ends in one big block with no blank line in it: the splitter // must not parse that block again on top of drawing it. @@ -922,5 +928,5 @@ describe('MarkdownContent while a reply streams in', () => { live.unmount(); today.unmount(); } - }); + }, STREAMING_SWEEP_BUDGET_MS); }); diff --git a/desktop/src/renderer/components/QueuedMessagesStrip.tsx b/desktop/src/renderer/components/QueuedMessagesStrip.tsx index d46167801..7a6ca4a09 100644 --- a/desktop/src/renderer/components/QueuedMessagesStrip.tsx +++ b/desktop/src/renderer/components/QueuedMessagesStrip.tsx @@ -1,5 +1,6 @@ import React from 'react'; import { Button } from './ui'; +import { EditPencilButton } from './EditPencilButton'; // Task 12: renders messages the native host FIFO'd behind an in-flight turn // (SessionChatState.queuedMessages) docked at the bottom of the chat area — @@ -23,6 +24,62 @@ import { Button } from './ui'; // enqueue the message also never see it in their own strip — same // renderer-local scope. +/** Same trash glyph as the doc-comments delete action (comments/CommentActions + * .tsx on its branch, Destin 2026-09-28: "matching ... delete a comment") — + * 24×24 viewBox, stroke currentColor, the app's inline-icon convention. */ +function TrashGlyph() { + return ( + + + + + + ); +} + +/** "Send now" (Destin, 2026-09-28 review decks): rightmost in the row, styled + * like the message box's own send button — the same primary + ); +} + interface QueuedMessage { queueId: string; content: string; @@ -39,6 +96,8 @@ interface Props { // sessionId + queueId through, no IPC/dispatch happens here). onCancel?: (queueId: string) => void; onEdit?: (queueId: string, text: string) => void; + // "Send now" — App stops the current task and sends this message next. + onSendNow?: (queueId: string) => void; } // forwardRef (review fix, post-approval): ChatView needs the strip's OWN @@ -51,7 +110,7 @@ interface Props { // itself to the absolutely-positioned child's content, so measuring a // wrapper instead of this element would always read 0. const QueuedMessagesStrip = React.forwardRef(function QueuedMessagesStrip( - { queuedMessages, onCancel, onEdit }, + { queuedMessages, onCancel, onEdit, onSendNow }, ref, ) { if (queuedMessages.length === 0) return null; @@ -71,19 +130,10 @@ const QueuedMessagesStrip = React.forwardRef(function Que Queued
{q.content}
- {(onEdit || onCancel) && ( + {(onEdit || onCancel || onSendNow) && (
- {onEdit && ( - - )} + {/* The quick chips' own edit button (Destin, 2026-09-28). */} + {onEdit && onEdit(q.queueId, q.content)} />} {onCancel && ( )} + {onSendNow && onSendNow(q.queueId)} />}
)} diff --git a/desktop/src/renderer/components/QuickChips.tsx b/desktop/src/renderer/components/QuickChips.tsx index cf04c7d16..4a745b18a 100644 --- a/desktop/src/renderer/components/QuickChips.tsx +++ b/desktop/src/renderer/components/QuickChips.tsx @@ -7,15 +7,7 @@ import { Button, Dialog, TextInput, Textarea, Tooltip } from './ui'; import { useScrollFade } from '../hooks/useScrollFade'; import { useEscClose } from '../hooks/use-esc-close'; import { useScreenOpen } from '../shoot-mode'; - -// Pencil SVG icon — matches the one used in StatusBar.tsx -function PencilIcon({ size = 10 }: { size?: number }) { - return ( - - - - ); -} +import { EditPencilButton, PencilIcon } from './EditPencilButton'; // Drag grip (6-dot braille) — mirrors SessionStrip's DragGrip function DragGrip() { @@ -64,7 +56,6 @@ export default function QuickChips({ onChipTap }: Props) { const android = isAndroid(); const chipHeight = android ? 'h-8' : 'h-6'; - const pencilSize = android ? 'w-8 h-8' : 'w-6 h-6'; return ( // select-none: quick chips are chrome, not highlightable or copyable @@ -84,17 +75,10 @@ export default function QuickChips({ onChipTap }: Props) { ))} - {/* Pencil button — opens chip editor */} - - - + {/* Pencil button — opens chip editor. `quick-chip-edit`: float chrome + gives it the chips' own surface, as the status bar's edit button + shares its chips' surface. */} + setEditorOpen(!editorOpen)} className="quick-chip-edit" /> {/* Chip editor popup — centered L2 modal (Scrim + OverlayPanel) to match diff --git a/desktop/src/renderer/components/SessionContextPopup.tsx b/desktop/src/renderer/components/SessionContextPopup.tsx index 8ce3c5556..82e40c17f 100644 --- a/desktop/src/renderer/components/SessionContextPopup.tsx +++ b/desktop/src/renderer/components/SessionContextPopup.tsx @@ -67,7 +67,21 @@ function windowLabel(tokens?: number | null): string { return tokens >= 1000 ? `${Math.round(tokens / 1000)}k` : String(tokens); } -const basename = (p: string) => p.split('/').slice(-1)[0]; +// Both separators: a native chat on Windows reports `C:\\Users\\…` paths. +const basename = (p: string) => p.split(/[\\/]/).slice(-1)[0]; +/** WHY a folder name, not the full path: ancestor instruction files have long + * absolute paths that overflow the card on a phone. The nearest file is "This + * project"; each broader one is named by the folder it lives in. */ +function instructionLabel(filePath: string, isNearest: boolean): string { + if (isNearest) return 'This project'; + const folder = filePath.split(/[\\/]/).slice(-2, -1)[0]; + return folder && !/^[A-Za-z]:$/.test(folder) ? `From the “${folder}” folder` : 'From the top-level folder'; +} +/** "Shortened · CLAUDE.md" for one file; "2 files shortened · AGENTS.md, CLAUDE.md" for more. */ +function shortenedSummary(files: ReadonlyArray<{ path: string; truncated: boolean }>): string { + const cut = files.filter(f => f.truncated).map(f => basename(f.path)); + return cut.length === 1 ? `Shortened · ${cut[0]}` : `${cut.length} files shortened · ${cut.join(', ')}`; +} const countLines = (s: string) => s.split('\n').filter((l) => l.trim() !== '').length; /** "1 lines" is the kind of small wrongness that makes a panel look unfinished. */ const linesLabel = (n: number) => `${n} line${n === 1 ? '' : 's'}`; @@ -215,16 +229,17 @@ function FileText({ sessionId, kind, id, what }: { sessionId?: string; kind: 'pr * Claude Code chat it means only that WE did not cut it — Claude Code manages * its own window and we cannot see what it did. Saying "read in full" there * would be a claim about someone else's work. */ -function RulesCard({ file, kind, label, sessionId, openFile, assembledByClaudeCode }: { - file: { path: string; truncated: boolean }; +function RulesCard({ file, kind, label, sessionId, openFile, assembledByClaudeCode, id }: { + file: { path: string; truncated: boolean; note?: string | null; notUsed?: string | null }; kind: 'project' | 'user'; + id?: string; label: string; sessionId?: string; openFile: (p: string) => void | Promise; assembledByClaudeCode: boolean; }) { const description = file.truncated - ? 'Shortened to headings only' + ? (file.note ?? 'Shortened to fit') : assembledByClaudeCode ? 'YouCoded didn’t shorten it' : 'Read in full'; @@ -236,12 +251,13 @@ function RulesCard({ file, kind, label, sessionId, openFile, assembledByClaudeCo className="rounded-none bg-transparent" icon={} title={basename(file.path)} - description={`${label} · ${description}`} + // Only one file per folder is used; name the one that was not. + description={`${label} · ${description}${file.notUsed ? ` · ${file.notUsed} here not used` : ''}`} accessory={} /> )} > - + ); } @@ -352,7 +368,10 @@ function SessionContextPanel({ open, onClose, context, sessionId }: Props & { co const [tab, setTab] = useState('overview'); const openFile = useOpenFilepath(sessionId); - const rules = context.projectInstructions ?? null; + // WHY: older native/Claude Code records still have one file; captured native + // sessions list the actual broad-to-narrow chain without a second selection. + const projectFiles = context.projectInstructionFiles ?? (context.projectInstructions ? [context.projectInstructions] : []); + const rules = projectFiles[projectFiles.length - 1] ?? null; const skills = context.skills ?? []; const tools = context.tools ?? []; const dropped = context.droppedMcpServers ?? []; @@ -435,12 +454,12 @@ function SessionContextPanel({ open, onClose, context, sessionId }: Props & { co

What was left out

- {rules?.truncated && ( + {projectFiles.some(f => f.truncated) && ( } title="This project’s rules" - description={`Shortened to headings only · ${basename(rules.path)}`} + description={shortenedSummary(projectFiles)} onClick={() => setTab('project')} /> )} @@ -480,7 +499,7 @@ function SessionContextPanel({ open, onClose, context, sessionId }: Props & { co 0 ? toolsWord : null].filter(Boolean).join(' · ')} + value={[projectFiles.length ? `${projectFiles.length} rules file${projectFiles.length === 1 ? '' : 's'}` : null, skillsWord, tools.length > 0 ? toolsWord : null].filter(Boolean).join(' · ')} />
@@ -534,16 +553,18 @@ function SessionContextPanel({ open, onClose, context, sessionId }: Props & { co

There is no rules file for this project, and none of your own.

) : (
- {rules && ( + {projectFiles.map((file, i) => ( - )} + ))} {/* Your own rules, the ones that apply everywhere. Claude Code reads this file; the native harness does not, so this card appears on a Claude Code chat and not on a native one — the diff --git a/desktop/src/renderer/components/ToolCard.tsx b/desktop/src/renderer/components/ToolCard.tsx index 7a9e8fc78..c59746d1e 100644 --- a/desktop/src/renderer/components/ToolCard.tsx +++ b/desktop/src/renderer/components/ToolCard.tsx @@ -17,6 +17,7 @@ import { useExpandAllToggle, getInitialExpanded } from '../hooks/useExpandAllTog import { isTypingTarget } from '../utils/is-typing-target'; import { useCardKeysLive } from '../state/card-keys-context'; import { asString } from '../utils/tool-input'; +import { normalizeQuestions, isValidQuestions } from './ask-question-normalize'; // Full-auto safety stop (spec 2026-08-12, M5 2b): per-family copy + the // status-bar chip colors, so the footer band can never drift from the chip. import { FullAutoStops, BROAD_ALLOW_COLORS } from './permissions/FullAutoStops'; @@ -930,45 +931,6 @@ export function PermissionButtons({ requestId, suggestions, denyListed, command, // Unlike regular tools (allow/deny), we must collect the user's selections // and return them via updatedInput.answers so Claude gets the actual answer. -interface AskQuestion { - question: string; - header: string; - options: Array<{ label: string; description?: string }>; - multiSelect: boolean; -} - -// Fix: isValidQuestions only checked questions[0].question, so an object-valued -// header/label/description DEEPER in the array still reached React children in -// the expanded card ("Objects are not valid as a React child" crashes the whole -// Chat pane via its ErrorBoundary). Normalize the whole array up front with the -// same asString idiom as friendlyToolDisplay: coerce every rendered field, and -// drop members that can't render or be answered (no question text, no labeled -// options) so one malformed member degrades gracefully instead of crashing or -// invalidating the rest of the card. -function normalizeQuestions(input: Record): AskQuestion[] { - const raw = input.questions; - if (!Array.isArray(raw)) return []; - const out: AskQuestion[] = []; - for (const q of raw as Array | null | undefined>) { - const question = asString(q?.question); - if (!question) continue; // no question text — nothing to render or key answers by - const rawOptions = Array.isArray(q?.options) ? (q.options as Array | null | undefined>) : []; - const options: AskQuestion['options'] = []; - for (const o of rawOptions) { - const label = asString(o?.label); - if (!label) continue; // an unlabeled option can't be selected or echoed back - options.push({ label, description: asString(o?.description) || undefined }); - } - if (options.length === 0) continue; // nothing selectable - out.push({ question, header: asString(q?.header), multiSelect: q?.multiSelect === true, options }); - } - return out; -} - -function isValidQuestions(input: Record): boolean { - return normalizeQuestions(input).length > 0; -} - // Ledger G-2 (2026-08-26 harness comparison): every other harness lets the user // type their own answer; ours only offered the listed choices, and Dismiss ended // the turn. The card now adds an "Other" row per question plus one text box @@ -998,11 +960,12 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { // see normalizeQuestions above. Memoized so the array identity is stable for // the hook deps below. const questions = useMemo(() => normalizeQuestions(tool.input), [tool.input]); - // answers: map from question text → selected label(s) + const isNative = requestId.startsWith('native-'); + // WHY: native asks have independent request-local positions; Claude Code's + // legacy answer map and card state were keyed by question wording. Q22 keeps + // that CC behavior, including duplicates, without inventing CC transport IDs. + const keyFor = (question: string, index: number) => isNative ? String(index) : question; const [answers, setAnswers] = useState>>({}); - // text: question text → what the user typed in that question's box. One - // state serves both roles (Other's answer, or a note on a listed choice); - // which role it plays is decided at submit from the selection. const [text, setText] = useState>({}); const textRefs = useRef>({}); const [responding, setResponding] = useState(false); @@ -1012,18 +975,18 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { // Track which question is "active" for keyboard nav, and which option is focused const [focusedOption, setFocusedOption] = useState(0); - const allAnswered = questions.every(q => { - const sel = answers[q.question]; + const allAnswered = questions.every((q, qi) => { + const sel = answers[keyFor(q.question, qi)]; if (!sel || sel.size === 0) return false; // "Other" with nothing typed is not an answer yet — Submit stays off until // the user explains, so the model never receives an empty "own answer". - if (sel.has(OTHER) && !(text[q.question] ?? '').trim()) return false; + if (sel.has(OTHER) && !(text[keyFor(q.question, qi)] ?? '').trim()) return false; return true; }); - const handleSelect = useCallback((question: string, label: string, multiSelect: boolean) => { + const handleSelect = useCallback((key: string, label: string, multiSelect: boolean) => { setAnswers(prev => { - const current = prev[question] || new Set(); + const current = prev[key] || new Set(); const next = new Set(current); if (multiSelect) { // Toggle for multi-select @@ -1034,12 +997,12 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { next.clear(); next.add(label); } - return { ...prev, [question]: next }; + return { ...prev, [key]: next }; }); // Picking Other is an invitation to type — put the cursor in the box so the // user doesn't have to click twice. (Deselecting Other leaves focus alone.) if (label === OTHER) { - requestAnimationFrame(() => textRefs.current[question]?.focus()); + requestAnimationFrame(() => textRefs.current[key]?.focus()); } }, []); @@ -1058,12 +1021,15 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { // `annotations[question].notes` — send that shape too so a CC session // receives the note through the same contract its CLI uses. const annotationsObj: Record = {}; - for (const q of questions) { - const sel = answers[q.question] ?? new Set(); - const typed = (text[q.question] ?? '').trim(); + const orderedAnswers: Array<{ answer: string; note?: string }> = []; + for (const [qi, q] of questions.entries()) { + const key = keyFor(q.question, qi); + const sel = answers[key] ?? new Set(); + const typed = (text[key] ?? '').trim(); const labels = Array.from(sel).filter((l) => l !== OTHER); if (sel.has(OTHER) && typed) labels.push(typed); answersObj[q.question] = labels.join(', '); + orderedAnswers.push({ answer: labels.join(', '), ...(!sel.has(OTHER) && typed ? { note: typed } : {}) }); if (!sel.has(OTHER) && typed) { notesObj[q.question] = typed; annotationsObj[q.question] = { notes: typed }; @@ -1078,6 +1044,9 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { questions, // Echo back the (normalized) questions array answers: answersObj, ...(hasNotes ? { notes: notesObj, annotations: annotationsObj } : {}), + // Native broker alone understands this field. Never add it to CC's + // documented question-text-keyed response envelope. + ...(isNative ? { orderedAnswers } : {}), }, }, }); @@ -1095,7 +1064,7 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { // The card stays answerable and says the answer could not be confirmed. setUnconfirmed(true); } - }, [allAnswered, responding, questions, answers, text, requestId, onResponded, onFailed]); + }, [allAnswered, responding, questions, answers, text, requestId, isNative, onResponded, onFailed]); const handleDeny = useCallback(async () => { setResponding(true); @@ -1120,9 +1089,9 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { // Total flat list of all options across questions (for keyboard nav). The // synthetic Other row is a real stop in the list, after each question's own // options, so arrow keys reach it like any other choice. - const allOptions = questions.flatMap(q => [ - ...q.options.map(o => ({ q, o })), - { q, o: { label: OTHER, description: OTHER_DESCRIPTION } }, + const allOptions = questions.flatMap((q, qi) => [ + ...q.options.map(o => ({ q, qi, o })), + { q, qi, o: { label: OTHER, description: OTHER_DESCRIPTION } }, ]); const optionCount = allOptions.length; @@ -1145,8 +1114,8 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { e.preventDefault(); // Select the focused option if (optionCount > 0) { - const { q, o } = allOptions[focusedOption]; - handleSelect(q.question, o.label, q.multiSelect); + const { q, qi, o } = allOptions[focusedOption]; + handleSelect(keyFor(q.question, qi), o.label, q.multiSelect); } } else if (e.key === 'Enter' && (e.ctrlKey || e.metaKey)) { // Ctrl/Cmd+Enter to submit @@ -1172,7 +1141,7 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: {
{[...q.options, { label: OTHER, description: OTHER_DESCRIPTION }].map((opt, oi) => { const idx = flatIdx++; - const selected = answers[q.question]?.has(opt.label) ?? false; + const selected = answers[keyFor(q.question, qi)]?.has(opt.label) ?? false; const focused = idx === focusedOption; const isOther = opt.label === OTHER; return ( @@ -1180,7 +1149,7 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { key={oi} aria-label={isOther ? `${OTHER_LABEL} — ${OTHER_DESCRIPTION}` : undefined} disabled={responding} - onClick={() => handleSelect(q.question, opt.label, q.multiSelect)} + onClick={() => handleSelect(keyFor(q.question, qi), opt.label, q.multiSelect)} // P-18: one row shape for both states — a visible dim border // when unselected, the accent border when selected — so rows // don't jump between "flat" and "outlined" as the user picks. @@ -1212,16 +1181,16 @@ function AskUserQuestionCard({ tool, requestId, onResponded, onFailed }: { inside the box, matching the card-wide shortcut — the window handler skips typing targets, so the box handles it itself. */}