diff --git a/src-node/claude-code-agent.js b/src-node/claude-code-agent.js index a25800ab88..c81b7ae484 100644 --- a/src-node/claude-code-agent.js +++ b/src-node/claude-code-agent.js @@ -39,6 +39,64 @@ const CLARIFICATION_HINT = " IMPORTANT: The user has typed a follow-up clarification while you were working." + " Call the getUserClarification tool to read it before proceeding."; +// Nudge the model when it has edited files that render in the user's live +// preview without ever looking at the result. Deliberately phrased as an FYI +// the model may act on or ignore — whether a change is worth verifying, and +// with which tool, is its call. +// +// Stated as a count rather than "you just edited…" because the PostToolUse +// fallback path can deliver this a tool call after the edit, and because +// naming the number makes it read as a summary rather than a per-edit echo. +function _livePreviewHintText(count) { + return "FYI: " + count + " file(s) you edited are rendered in the user's live preview," + + " and you have not inspected it since. Decide for yourself whether looking is worth" + + " a tool call here — a trivial or self-evident change usually is not. If it is, you" + + " pick the tool: execJsInLivePreview to read the DOM / computed styles / console," + + " takeScreenshot with selector='#panel-live-preview-frame' for a visual check, or" + + " resizeLivePreview for responsive behavior."; +} + +// Reason returned when a file-rewriting shell command is stopped on its first +// attempt. A speed bump, not a wall: re-running the identical command goes +// through (see _shellEditNeedsConfirm). That protects the first edit — which a +// note after the fact cannot, since by then undo is already gone — while +// leaving the final call with the model, at the cost of one extra round trip. +// +// Preferring Edit/Write is a default, not a rule. Shell rewrites genuinely win +// on mechanical bulk changes and on large files where Edit would burn tokens +// re-reading to change a little, so the text asks the model to weigh that +// against the lost undo rather than treating the shell as forbidden. One round +// trip is negligible next to the bulk operation it is gating. +function _shellEditDenyText(what) { + return "Phoenix did not run that. It rewrites a file from the shell (" + what + "), which" + + " bypasses the editor: the user's open buffer is not refreshed, no reviewable diff is" + + " rendered, and the change cannot be undone from the AI panel's Undo button. For an" + + " ordinary content change, use Edit for existing files or Write for new ones — those" + + " keep all three. But this is a default, not a rule: if the shell is genuinely the" + + " better tool here — the user named this command, the change is mechanical across many" + + " files or matches, doing it with Edit would mean dozens of calls or reading a very" + + " large file to change a little of it, or the target is generated / build output / a" + + " log — then run it again unchanged and it will go through. Weigh the token cost" + + " against the user losing undo for that file, and tell them which way you went."; +} + +// Nudge on the first unverified live preview edit, then stay quiet until this +// many more pile up without the model ever looking at the preview. +const LP_NUDGE_REPEAT_AFTER = 5; + +// Hard ceiling per user request. Without it a long unverified run (30 edits) +// would emit ~6 nudges, and every one persists in the transcript. If two +// haven't changed the model's behavior, a third won't either. +const LP_MAX_NUDGES_PER_REQUEST = 2; + +// Calling any of these means the model is already looking at the preview, so +// there is nothing to nag about — seeing one resets the pending count. +const LP_INSPECT_TOOLS = [ + "mcp__phoenix-editor__takeScreenshot", + "mcp__phoenix-editor__execJsInLivePreview", + "mcp__phoenix-editor__resizeLivePreview" +]; + // Lazy-loaded ESM module reference let queryModule = null; @@ -177,6 +235,119 @@ const _SAFE_BASH_PATTERNS = [ /^pnpm\s+--version$/ ]; +// Shell constructs whose purpose is rewriting a file in place. Bash is not +// interchangeable with Edit/Write here: the Edit/Write PostToolUse hooks +// refresh the open buffer, paint the diff card that backs the panel's Undo +// button, and carry the live preview signal. A shell rewrite skips all +// three, so the user silently loses undo for that change. +// +// A match stops the command once and offers a retry (see _shellEditDenyText), +// so the cost of a false positive is one wasted round trip rather than a +// refusal. Still worth keeping narrow: only constructs that exist to rewrite +// files belong here. +const _INPLACE_EDIT_PATTERNS = [ + // sed -i / -i.bak / -ri / --in-place. The lookahead stops at a pipe or + // separator so `grep -i x | sed 's/a/b/'` isn't caught by the grep flag. + { rx: /\bsed\b(?=[^|;&]*\s-(?:-in-place|[a-zA-Z]*i))/, what: "sed -i" }, + // perl -pi -e / perl -i.bak + { rx: /\bperl\b(?=[^|;&]*\s-[a-zA-Z]*i)/, what: "perl -i" }, + { rx: /\bawk\b(?=[^|;&]*\s-i\s+inplace)/, what: "awk -i inplace" }, + { rx: /\bed\s+-s\b/, what: "ed -s" }, + { rx: /\bex\s+-s(c|\s)/, what: "ex -s" }, + // PowerShell equivalents — on Windows the model may reach for these + // instead of sed. Set-Content/Add-Content/Out-File all rewrite a file. + { rx: /\b(?:Set-Content|Add-Content|Out-File)\b/i, what: "PowerShell Set-Content / Out-File" } +]; + +// Redirection / tee targets that aren't the user's files: device sinks and +// scratch dirs. `> /dev/null` and `> $TMPDIR/x` are ubiquitous and carry no +// undo cost, so hinting about them would be pure noise. +// +// Covers all three platforms, since the model may be driving bash, PowerShell +// or cmd depending on where Phoenix is running: macOS puts TMPDIR under +// /var/folders, Windows under %TEMP% / AppData\Local\Temp, and the null sink +// is /dev/null, NUL or $null respectively. +const _EXEMPT_WRITE_TARGETS = [ + /^\/dev\//, + /^\/proc\//, + /^\/(?:private\/)?tmp\//, + /^\/var\/(?:tmp|folders)\//, + /^(?:nul|\$null)$/i, + /^\$\{?TMPDIR\}?[\\/]/i, + /^%(?:TEMP|TMP)%[\\/]/i, + /^[a-zA-Z]:[\\/](?:temp|tmp)[\\/]/i, + // Git Bash rewrites C:\Temp to MSYS form (/c/temp), and it is the shell + // the Bash tool actually uses on Windows. + /^\/[a-zA-Z]\/(?:temp|tmp)\//i, + /[\\/]AppData[\\/]Local[\\/]Temp[\\/]/i, + /^[a-zA-Z]:[\\/]Windows[\\/]Temp[\\/]/i +]; + +function _isExemptWriteTarget(target) { + if (target === "-") { return true; } + return _EXEMPT_WRITE_TARGETS.some(function (rx) { return rx.test(target); }); +} + +// Walk the command tracking quote state so a `>` inside a string literal +// (`echo "a > b"`, `python -c "print(1 > 0)"`) is not mistaken for a +// redirection. Returns the write destinations found outside quotes. +// A heuristic guard, not a shell parser. +function _shellWriteTargets(rawCmd) { + // Drop file-descriptor duplications (2>&1, >&2, 1>&2) up front. + const cmd = (rawCmd || "").replace(/\d*>&\d*/g, " "); + const targets = []; + let quote = null; + for (let i = 0; i < cmd.length; i++) { + const ch = cmd[i]; + if (quote) { + if (ch === quote && cmd[i - 1] !== "\\") { quote = null; } + continue; + } + if (ch === "\"" || ch === "'") { quote = ch; continue; } + if (ch !== ">") { continue; } + // Skip the rest of a `>>` pair, then the whitespace before the target. + let j = i + 1; + while (cmd[j] === ">") { j++; } + while (cmd[j] === " " || cmd[j] === "\t") { j++; } + // Read the target, honouring quotes around a path with spaces. + let target = ""; + if (cmd[j] === "\"" || cmd[j] === "'") { + const closer = cmd[j]; + j++; + while (j < cmd.length && cmd[j] !== closer) { target += cmd[j++]; } + } else { + while (j < cmd.length && !/[\s;|&()]/.test(cmd[j])) { target += cmd[j++]; } + } + if (target) { targets.push(target); } + i = j - 1; + } + // tee writes to its path arguments rather than via redirection. + const tee = /\btee\s+(?:-a\s+)?("[^"]*"|'[^']*'|[^\s;|&()-][^\s;|&()]*)/g; + let m; + while ((m = tee.exec(cmd)) !== null) { + targets.push(m[1].replace(/^["']|["']$/g, "")); + } + return targets.filter(function (t) { return t && !_isExemptWriteTarget(t); }); +} + +/** + * Classify a Bash command that would rewrite file content instead of going + * through Edit/Write. Returns a short description of what was matched, or + * null when the command is fine to run. + */ +function _describeInPlaceFileEdit(rawCmd) { + const cmd = (rawCmd || "").trim(); + if (!cmd) { return null; } + for (const entry of _INPLACE_EDIT_PATTERNS) { + if (entry.rx.test(cmd)) { return entry.what; } + } + const targets = _shellWriteTargets(cmd); + if (targets.length) { + return "shell redirection to " + targets[0]; + } + return null; +} + function _isSafeReadOnlyBash(rawCmd) { const cmd = (rawCmd || "").trim(); if (!cmd) { return false; } @@ -772,6 +943,39 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // turn skip the prompt and use the cached "allow" decision so a multi-edit // turn doesn't pop a dialog before every edit. let _planExitApprovedThisTurn = false; + // Live preview nudge bookkeeping, per request so each new user prompt + // re-arms it. _lpPendingEdits counts live-preview-related edits since the + // model last inspected the preview; _lpNudgeCount enforces the hard cap. + let _lpPendingEdits = 0; + let _lpNudgeCount = 0; + // Shell-rewrite confirmation, scoped per request rather than per + // conversation: a new user prompt is a new intent, so the next request's + // first rewrite gets its own speed bump instead of riding on a + // confirmation given for something else. + let _shellEditAwaitingRetry = null; + let _shellEditConfirmed = false; + + // True when this command should be stopped and offered a retry. The + // identical command coming back means it was meant, so it goes through — + // and having confirmed once, the rest of the request goes through too. + // + // That last part matters: the model often has to fix its own command after + // the first attempt (BSD `sed -i ''` failing on GNU sed, say). Keying only + // on the exact string charged a second bump for what is one operation, so + // one confirmation now covers the request. The first edit is still + // protected, which is the whole point of the bump. + function _shellEditNeedsConfirm(command) { + if (_shellEditConfirmed) { + return false; + } + if (_shellEditAwaitingRetry === command) { + _shellEditAwaitingRetry = null; + _shellEditConfirmed = true; + return false; + } + _shellEditAwaitingRetry = command; + return true; + } let queryFn; let connectionTimer = null; @@ -912,6 +1116,22 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, "multiple Edit calls to make targeted changes rather than rewriting the entire " + "file with Write. This is critical because Write replaces the entire file content " + "which is slow and loses undo history." + + "\n\nThe user's project root is " + (projectPath || process.cwd()) + ". For files " + + "under it, default to Edit and Write over shell rewrites (sed -i, perl -i, tee, " + + "Set-Content/Out-File, `>` / `>>` redirection). Phoenix routes Edit and Write " + + "through the editor, so they refresh the user's open buffer, render a reviewable " + + "diff, and stay undoable from the AI panel; a shell rewrite skips all three, and " + + "the user cannot undo it. Outside the project root — scratch files, temp output, " + + "logs — the shell is fine and needs no thought. " + + "\nThis is a default, not a prohibition. The shell is the better call when the " + + "change is mechanical across many files or matches, when Edit would mean dozens of " + + "calls or reading a large file to alter a little of it, or when the target is " + + "generated output. Phoenix stops the first shell rewrite of each command and " + + "explains why; re-run it unchanged and it goes through. Judge it on the merits — " + + "tokens saved against undo lost — and tell the user when you take the shell route. " + + "When the saving would be marginal, take Edit: one shell call and one Edit call " + + "cost about the same, so a handful of files is not a reason to give up undo. The " + + "shell has to earn it." + "\n\nALWAYS call getEditorState as your FIRST tool call on any question that " + "references the user's current work — not just \"what file am I on\". This includes " + "implicit-context questions like \"the page\", \"this layout\", \"the nav bar\", " + @@ -948,7 +1168,8 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, "phoenix-editor.resizeLivePreview, and phoenix-editor.controlEditor cover virtually " + "every \"look at / poke at the page\" need. Only fall back to chrome-devtools or " + "another browser MCP if the user explicitly asks for a non-Phoenix browser context. " + - "These tools are for active iteration, not just final verification:" + + "These tools are for active iteration AND for checking your own work — " + + "use them as you go, not only when the user asks:" + "\n- takeScreenshot: see the rendered HTML preview, the rendered Markdown preview, " + "the editor, or any panel. Use it to confirm visual output, diagnose layout/styling " + "bugs, or check that HTML or Markdown rendered as expected. Simple selector rule: " + @@ -960,7 +1181,7 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, "JS edits) — saves a tool call vs. reloading separately." + "\n- execJsInLivePreview: run JS inside the HTML preview iframe to read the DOM, " + "query computed styles, click elements, or capture console output. Use it to debug " + - "behavior, not just to verify." + + "behavior and to confirm an edit actually took effect." + "\n- resizeLivePreview: change the preview viewport width to test responsive " + "breakpoints." + "\n- controlEditor: open files, move the cursor, change selection, toggle the live " + @@ -985,6 +1206,15 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, "feature-docs URL and the GitHub source repo URL. Call once near the start of any " + "non-trivial editor-control task; then Read / Grep the apiDocsPath and WebFetch the " + "featureDocsURL as needed. Do NOT search the codebase blindly when this exists." + + "\n\nEDITS THAT LAND IN THE LIVE PREVIEW: when you edit the file getEditorState " + + "reported as livePreviewFile — or a CSS / JS / SVG file it links to — the user is " + + "watching the result render. Whether that is worth checking is your judgement call, " + + "and so is how: execJsInLivePreview to read the DOM / computed styles / console, " + + "takeScreenshot with selector='#panel-live-preview-frame' for a visual check, " + + "resizeLivePreview for responsive behavior, or nothing at all when the change is " + + "trivial or self-evident. Weigh it at meaningful checkpoints (after a section lands, " + + "before you report done) rather than after every small edit. Files outside the live " + + "preview do not raise the question at all." + "\n\nName-collision rule: \"Phoenix Code\" (the editor the user is sitting inside) " + "and \"Claude Code\" (the SDK / CLI you happen to run on) BOTH have settings, " + "configs, auto-update toggles, themes, etc. When the user says \"set / change / " + @@ -1289,6 +1519,30 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, matcher: "Bash", hooks: [ async (input) => { + // Stop a file rewrite the first time it is tried, + // in every permission mode — "auto" hands the call + // to the SDK classifier, which happily approves + // sed -i. Denying here is what actually protects + // the edit: a note after the fact arrives once undo + // is already gone. Re-running the same command + // confirms intent and goes through. + const command = (input.tool_input && input.tool_input.command) || ""; + const inPlaceEdit = _describeInPlaceFileEdit(command); + if (inPlaceEdit && _shellEditNeedsConfirm(command)) { + console.log("[Phoenix AI] Stopped shell file rewrite (" + + inPlaceEdit + "), offering retry: " + command.slice(0, 70)); + return { + hookSpecificOutput: { + hookEventName: "PreToolUse", + permissionDecision: "deny", + permissionDecisionReason: _shellEditDenyText(inPlaceEdit) + } + }; + } + if (inPlaceEdit) { + console.log("[Phoenix AI] Shell file rewrite confirmed by retry: " + + command.slice(0, 70)); + } // Read from the runtime mutable so mid-stream // permission-mode flips (e.g. user switches Edit // Mode → Allow Everything while bash is in flight) @@ -1302,8 +1556,8 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // only for Edit Mode's manual approval flow. return {}; } - // Edit Mode: ask user confirmation before running bash - const command = input.tool_input.command || ""; + // Edit Mode: ask user confirmation before running bash. + // `command` is read above, for the rewrite check. // Skip prompting for well-known read-only commands // that mirror the Claude Code CLI's default safe // patterns. Cuts down on prompt fatigue during @@ -1438,7 +1692,14 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, console.warn("[Phoenix AI] Edit refresh fallback failed:", filePath, err.message); } } - // 2. Trigger aiToolEdit so the AI panel renders the + // 2. Count it toward the live preview nudge. Only + // incrementing here — the read-and-clear happens + // in one owner, since PostToolUse hooks can run + // concurrently for parallel tool calls. + if (result.isLivePreviewRelated) { + _lpPendingEdits++; + } + // 3. Trigger aiToolEdit so the AI panel renders the // diff card and the snapshot store records it. const counterId = _toolUseIdToCounter[toolUseID]; if (counterId !== undefined) { @@ -1473,6 +1734,9 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, } catch (err) { console.warn("[Phoenix AI] Write refresh failed:", filePath, err.message); } + if (refreshResult.isLivePreviewRelated) { + _lpPendingEdits++; + } const counterId = _toolUseIdToCounter[toolUseID]; if (counterId !== undefined) { nodeConnector.triggerPeer("aiToolEdit", { @@ -1496,13 +1760,45 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, // tool. Edit/Write/Read have their own hooks above, but // any tool can be a meaningful checkpoint (Bash, Grep, // Glob, WebFetch, Task, the Phoenix MCP tools, etc.) so - // we register one matcher-less hook that just returns - // the clarification context if any is queued. Once + // we register one matcher-less hook that returns the + // clarification context if any is queued. Once // getUserClarification runs and clears _queuedClarification, - // _maybeClarifyContext returns {} and this becomes a no-op. + // that part becomes a no-op. + // + // It also carries the live preview nudge as a fallback for + // Claude CLI versions that predate PostToolBatch: the batch + // hook below is the primary path, but we run the user's + // global CLI (findGlobalClaudeCli) so we can't assume it. + // Whichever fires first takes the hint; the other sees a + // cleared counter. + hooks: [ + async (input) => { + return _buildPostToolUseHint(input); + } + ] + } + ], + PostToolBatch: [ + { + // Primary emit point for the live preview nudge. Fires once + // after every tool call in a batch resolves, so unlike + // PostToolUse (which may run concurrently for parallel tool + // calls) it can safely read-and-clear shared state, and it + // sees the whole batch — including whether the model already + // inspected the preview itself. hooks: [ - async () => { - return _maybeClarifyContext(); + async (input) => { + const names = (input.tool_calls || []).map(function (call) { + return call.tool_name; + }); + const hint = _takeLivePreviewHint(names); + if (!hint) { return {}; } + return { + hookSpecificOutput: { + hookEventName: "PostToolBatch", + additionalContext: hint + } + }; } ] } @@ -1510,17 +1806,59 @@ async function _runQuery(requestId, prompt, projectPath, model, signal, locale, } }; - // Returns a PostToolUse SyncHookJSONOutput that injects the clarification - // hint as additionalContext when the user has typed a follow-up while the - // AI is streaming. With our PreToolUse hooks now returning {} (allow), the - // old practice of appending CLARIFICATION_HINT to permissionDecisionReason - // no longer reaches Claude — PostToolUse additionalContext is the new path. - function _maybeClarifyContext() { - if (!_queuedClarification) { return {}; } + // Read-and-clear for the live preview nudge. Returns the hint text when the + // model has piled up unverified live-preview edits, else null. Called from + // the PostToolBatch hook (primary) and the PostToolUse catch-all (fallback + // for older CLIs) — the body is synchronous, so whichever gets here first + // takes the hint and the other finds the counter already cleared. + // + // toolNames is what the model just called: seeing it inspect the preview + // itself means there is nothing to nag about. + function _takeLivePreviewHint(toolNames) { + if (toolNames && toolNames.some(function (name) { + return LP_INSPECT_TOOLS.indexOf(name) !== -1; + })) { + _lpPendingEdits = 0; + return null; + } + if (_lpNudgeCount >= LP_MAX_NUDGES_PER_REQUEST) { + return null; + } + const threshold = _lpNudgeCount === 0 ? 1 : LP_NUDGE_REPEAT_AFTER; + if (_lpPendingEdits < threshold) { + return null; + } + const text = _livePreviewHintText(_lpPendingEdits); + console.log("[Phoenix AI] live preview nudge:", _lpPendingEdits, "edit(s) unverified"); + _lpPendingEdits = 0; + _lpNudgeCount++; + return text; + } + + // Returns a PostToolUse SyncHookJSONOutput carrying whatever the model + // should see after a tool call: the clarification hint when the user has + // typed a follow-up while the AI is streaming, and/or the live preview + // nudge. With our PreToolUse hooks now returning {} (allow), the old + // practice of appending CLARIFICATION_HINT to permissionDecisionReason no + // longer reaches Claude — PostToolUse additionalContext is the new path. + // + // _queuedClarification is deliberately not cleared here; it clears only + // when the model calls getUserClarification. The live preview counter is + // cleared by _takeLivePreviewHint, so that half cannot repeat. + function _buildPostToolUseHint(input) { + const parts = []; + if (_queuedClarification) { + parts.push(CLARIFICATION_HINT); + } + const lpHint = _takeLivePreviewHint(input && input.tool_name ? [input.tool_name] : null); + if (lpHint) { + parts.push(lpHint); + } + if (!parts.length) { return {}; } return { hookSpecificOutput: { hookEventName: "PostToolUse", - additionalContext: CLARIFICATION_HINT + additionalContext: parts.join("\n\n") } }; }