diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e9ba66..17cfecc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,184 @@ All notable changes to HirayaCoder are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.7.0] — unreleased + +Everything here follows from one observation: a small model that is stuck does not know +it is stuck, and everything it does next makes the run worse. Across the 0.5.3 and 0.6.0 +rounds a model that had run out of ideas ended a run in one of three ways — resend the +failing action until the step budget was gone, announce success it had not achieved, or +abandon a task it was one decision away from finishing. All three cost the user more +than a question would have. + +So 0.7.0 adds the three things that were missing between "the action failed" and "the +run ended badly": a diagnosis for the failures nobody had written a rule for, an +escalation ladder when the diagnosis does not take, and — last, not first — a way to ask +the user. + +### Added — undefined symbols are now a diagnosis, not a stack trace + +The commonest way code written by a small model fails at runtime had no rule at all. The +model got forty lines of stack trace, the generic "the error points at a file" fallback, +and no mention of the name that was actually missing; it then rewrote the file from +memory and produced the same error. + +Seven rules across JavaScript, Python and Java: `ReferenceError`, `NameError`, javac's +`cannot find symbol`, reading a property of `undefined` or `null`, `AttributeError`, +`x is not a function`, and `NullPointerException`. Each names the symbol the error named, +because *"something is undefined"* is not actionable and *"`addTodo` is used in that file +but never defined or imported there"* is the same information with the one detail that +makes it a next move. + +- `scriptDiagnosis` rules may now compute their sentence from the match, which is what + makes naming the symbol possible. The matcher moved from `test` to `exec`, with a test + pinning that repeated calls agree. +- The property-of-undefined rule says to fix *what produced the undefined*, not the line + that read it — a model told only "map failed" adds a `?.` and moves on. +- `module is not defined in ES module scope` is still claimed by `MODULE_SYSTEM`, which + is a `ReferenceError` with a completely different fix. + +### Added — an escalation ladder, and the user at the end of it + +`agent/errorRecovery` covers what no rule matches, and what a rule matched twice. +Failures are compared by signature — line numbers, addresses and absolute paths +normalised out — so the same problem is recognised across attempts and across machines. + +| Seen | What happens | +|---|---| +| 1st, diagnosed | Nothing extra; the diagnosis already said it | +| 1st, undiagnosed | Read the error literally; here is what to look at | +| 2nd | Ask the user | + +Asking on the *second* failure rather than a rounder number is set against +`reactLoop.REPEAT_LIMIT`, which is 2: the loop ends a run once the model has sent the +same action twice, so a ladder waiting for a third would never reach its top rung and +the run would end with the user never asked. The second failure is the last moment at +which asking can still change the outcome. + +A refused write or a declined delete is **not** treated as being stuck. That is the +permission model working, and escalating it would ask the user the same question twice, +the second time with less context than the confirmation dialog had. + +### Added — the agent asks, with options rather than an open question + +`agent/clarification` is the shape of a mid-run question: two to four options, exactly +one recommended, every option stating what it does to the queue, and free text always +available. A question with seven options is a second task handed to the user; four +unranked options move the decision without helping with it. When the same thing has +failed twice, the recommendation is **skip**, because recommending another attempt would +recommend the thing that has already not worked. + +The card renders in the chat panel and stays there once answered, as part of the record +of the run. + +The property that mattered most while building this is the negative one, and it is +tested from both sides: **a session with nothing to ask never blocks.** Closing the tab, +pressing Stop, and a turn that throws all settle an outstanding question, so a run +continuing in the background cannot hold its lane in the turn queue waiting on a card +nobody can see. + +### Added — the request is read before the model sees it + +"Update mian.js" in a project containing `main.js` is not ambiguous to a person for even +a moment. To a small model it is a path that does not resolve, and what happens next is +the part that costs: it creates `mian.js`, reports the update done, and the user now has +two files where they had one. + +`core/commonSense` decides from the evidence rather than from confidence: + +- **Exactly one near-match** — repaired, with both names stated in the summary and in + memory, so the user can see their request was altered and disagree with it. +- **Two to four near-matches** — asked, offering them. +- **No near-match** — nothing said. The file may simply not exist yet, and a check that + fired on those would be worse than no check. + +The verb governing a filename is read from the few words in front of it, so "update +mian.js **to add** a header" is a typo rather than a creation. A right name in a wrong +folder is left alone — nothing was misspelled, and the model has the workspace listing. +"Fix it" as the first message of a session resolves to the open editor file, or asks. + +Comparison is Damerau-Levenshtein, not `memoryStore.similarity`: that one is Jaccard over +whole words and scores `mian.js` against `main.js` at zero. + +### Changed — the checklist can change while it runs + +An answer that only reaches the model is half-applied. The item's text is what a retry is +briefed on, what `stepGuard` checks the changed files against, and what the summary reads +back — so a free-text answer now rewords the running item, and a skip closes it as the +user's decision rather than as a failure. That distinction matters twice: a failed item +puts a `[!]` against a row the user themselves closed, and it feeds "an earlier step +failed" to every item after it. + +`TodoList` keeps a change log, and the summary reports three things it previously hid: +how the request was read, what the user said when asked, and what kept failing anyway. +A run that ends "4 of 4 completed" after hitting the same error eleven times is +technically true and actively misleading. + +### Changed — memory is recalled by relevance on every turn + +`readRelevant` existed but only fired in experimental step sessions, so an ordinary +six-item run shared one memory block, selected by recency, against the whole request +before the list existed. That is the wrong block for every item after the first: on the +React benchmark the item assembling `App.jsx` ran sixth, by which point the notes naming +`useTodos.js` and `TodoInput.jsx` were the oldest in the file and the first to fall out +of a five-entry window. The note that survived was about the README. + +Recall is now by subject on every turn, and ranked by **how much** of the subject each +note shares rather than by whether any single token matched — a weak match displacing a +strong one is the failure the selector exists to prevent. It is never worse than recency: +a subject that matches nothing gets exactly the window it would have had. + +### Fixed — `_routeForStep` dropped `readOnlyTurn` + +Rebuilding a route per item exposed this. The step route replaced the session's for the +whole step but did not carry the read-only flag, so the mutating tools came back for a +turn the router had already decided was a look-only request. Latent while the path was +experimental; carried through now that every item rebuilds its route. + +### Fixed — the three findings the 0.6.1 SAST pass deferred + +- **`npx` reached the network and never asked.** It was on the default allow-list, + `NON_INTERACTIVE_ENV` sets `npm_config_yes` which suppresses npx's own *"Ok to + proceed?"*, and it was absent from `ALWAYS_CONFIRM` — so under auto-approve it fetched + and ran arbitrary remote code with no click, in an extension whose headline claim is + that it works fully offline. A live 0.6.0 run has it in the audit log, auto-approved + and recorded as routine. All six spellings now confirm: `npx`, `npm exec`/`x`/`create`, + `yarn dlx`/`create`, `pnpm dlx`/`create`. Bare `npm init` is left alone — it writes a + manifest and touches no network, and a click for nothing is a click trained away. +- **Two writes skipped `pathGuard`.** `ensureGitignore` and `environmentProfile.persist` + wrote through `path.join` directly, the only two writes in the extension that missed + the symlink check. Both now resolve through the guard, which gained a synchronous twin + for the activation path; the containment half is shared so the two cannot drift on the + part that decides. +- **Dev-only advisories in the `mocha` tree.** mocha 11.8.0 with `overrides` pinning + `diff@9` and `serialize-javascript@7.1.0`, which is what actually clears them — + upstream mocha still asks for `diff@^7` and `serialize-javascript@^6`, and + `npm audit fix --force` would have moved sideways to 11.3.0 without fixing either. + `npm audit` now reports zero across the full tree rather than only under `--omit=dev`. + Verified serially, in parallel mode, and against a failing assertion. + +### Fixed — two regexes that were genuinely super-linear + +Both flagged by `eslint-plugin-security` in `core/commonSense`, and worth recording +because the previous two passes reviewed and dismissed every warning of this class: + +- `DANGLING_REFERENCE` had `^\s*` and `\s*$` around an optional character — the classic + ambiguous shape. **68 ms at 10,000 trailing spaces, 1,660 ms at 50,000.** The caller + trims and the anchors are gone: 0 ms. +- `PATH_TOKEN` matches linearly but is scanned with `/g`, so a string with no match gets + one scan per start position — **3,089 ms on 50,000 characters** of `a/a/a/…`. Input is + now bounded to 4,000 characters. + +Neither is an attack — it is the user's own composer — but three seconds of frozen +extension host is a bug either way. Both are pinned by tests with a 250 ms budget. + +### Removed + +- `TodoList.insertAfterCurrent`, written for a user path that does not exist. An + unreachable method that mutates the one structure both the user and the model treat as + settled is worse than a slightly narrower feature. +- `ErrorRecovery.hasAsked`, which had no caller. + ## [0.6.1] — unreleased A macOS run of the same React TODO spec on the 0.6.0 build, `ornith:9b`, high thinking diff --git a/README.md b/README.md index 6f9cc61..ecdf32f 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@

- HirayaCoder v0.6.1 — your AI pair programmer, fully offline. A VS Code chat panel showing the agent reading two files, writing two files, and asking for approval before running npm run build. + HirayaCoder v0.7.0 — your AI pair programmer, fully offline. A VS Code chat panel showing the agent reading two files, writing two files, and asking for approval before running npm run build.

*A local Filipino-inspired AI coder that brings imagination and speed to your VS Code workflow.* diff --git a/app/agent/agentSession.js b/app/agent/agentSession.js index 52e7a49..903bcb5 100644 --- a/app/agent/agentSession.js +++ b/app/agent/agentSession.js @@ -41,6 +41,9 @@ const earnedHints = require('./earnedHints'); const stepBrief = require('./stepBrief'); const stepGuard = require('./stepGuard'); const answerCheck = require('./answerCheck'); +const commonSense = require('../core/commonSense'); +const clarification = require('./clarification'); +const { ErrorRecovery } = require('./errorRecovery'); const { TodoList } = require('./todoList'); /** @@ -499,11 +502,91 @@ class AgentSession { */ this.conversation = []; + /** + * How a question reaches the user, and their answer comes back. + * + * Null is the ordinary state for a benchmark, a test, or any caller that has no + * panel — and every path that would raise a question checks first and settles for + * guidance instead. A run must never block on an answer that nothing can give. + * + * @type {((request: import('./clarification').Clarification) => + * Promise) | null} + */ + this.onClarify = typeof options.onClarify === 'function' ? options.onClarify : null; + + /** + * What has failed this run, and what the user has already said about it. + * + * Rebuilt per `run` rather than per session: "you have tried this three times" is a + * claim about one turn, and a new message is a new chance. + * + * @type {ErrorRecovery} + */ + this._recovery = new ErrorRecovery({ canAsk: false }); + + /** + * Set when the user answers a mid-run question with something the loop cannot + * express — skip this item, or stop altogether. Read by `_runWithTodos` once the + * loop it was raised inside has returned. + * + * @type {'' | 'skip' | 'stop'} + */ + this._interrupt = ''; + + /** What the user said mid-run, for the summary. @type {string[]} */ + this._userGuidance = []; + + /** + * The checklist currently running, so an answer given inside a loop can adjust it. + * + * Null except between `_runWithTodos` building the list and reporting it. + * + * @type {TodoList | null} + */ + this._todos = null; + /** @type {AbortController | null} */ this._controller = null; this.running = false; } + /** + * Put a question to the user and wait for the answer. + * + * Returns the resolved effect, or null when there was nobody to ask — callers treat + * that as "carry on as you were", never as a refusal. + * + * @param {import('./clarification').Clarification} request + * @param {(event: object) => void} emit + * @returns {Promise<{effect: import('./clarification').ClarificationEffect, guidance: string, label: string} | null>} + * @private + */ + async _ask(request, emit) { + if (!this.onClarify) return null; + + emit({ type: 'clarification', request }); + logger.info(`Pausing to ask: ${request.question}`); + + /** @type {import('./clarification').ClarificationAnswer | null} */ + let answer = null; + try { + answer = await this.onClarify(request); + } catch (err) { + // A question that could not be shown must not take the run down with it. The + // model is mid-item with a step budget and a change set; failing closed here + // means carrying on without the answer, not discarding the work. + logger.warn(`Could not ask the user: ${/** @type {Error} */ (err).message}`); + return null; + } + + const resolved = clarification.resolve(request, answer); + emit({ type: 'clarification-answered', id: request.id, label: resolved.label }); + logger.info(`The user answered "${resolved.label}" (${resolved.effect}).`); + + if (resolved.guidance) this._userGuidance.push(resolved.guidance); + return resolved; + } + /** Stop the session after the action in flight completes. */ cancel() { if (this._controller) this._controller.abort(); @@ -524,10 +607,24 @@ class AgentSession { async run(task, options = {}) { const mode = options.mode || 'agent'; const emit = options.onEvent || (() => {}); + // Held on the session so `_execute` can raise a question from inside a loop it was + // not handed the emitter for. Cleared in the `finally` below with everything else + // that is per-turn. + this._emit = emit; + /** The TODO item in flight, so a question can say what it is about. @type {string} */ + this._currentItem = ''; this.conversation = Array.isArray(options.conversation) ? options.conversation : []; this._controller = new AbortController(); this.running = true; this._startedAt = Date.now(); + // Per turn, not per session: three failures ago was a different request, and a run + // that opened already out of patience would ask about a wall the user may have + // taken down in between. + this._recovery = new ErrorRecovery({ canAsk: Boolean(this.onClarify) }); + this._interrupt = ''; + this._userGuidance = []; + /** What was read differently from what was typed, for the summary. @type {string[]} */ + this._interpretations = []; // Snapshotted rather than measured per call: the client already totals every // request it makes, so the difference across a session is the time spent waiting on // Ollama — including the planning and TODO-splitting passes, which happen outside @@ -535,6 +632,26 @@ class AgentSession { this._modelMsAtStart = this._modelMsSoFar(); try { + // Read before routed. A request naming a file that is one letter away from a real + // one is not ambiguous to a person, and a small model handed it literally creates + // the misspelled file and reports success — so the reading happens here, before + // anything downstream treats the name as given. Agent mode only: Plan and Ask + // produce words rather than files, so a wrong name costs a sentence. + if (mode === 'agent') { + task = await this._interpretRequest(task, options, emit); + if (this._interrupt === 'stop') { + const stopped = { + summary: 'Stopped before starting, at your request.', + steps: [], + changeSet: new ChangeSet(), + stopReason: 'cancelled', + mode, + }; + emit({ type: 'done', summary: stopped.summary }); + return stopped; + } + } + // Only Agent mode is routed by intent — Plan and Ask are the user saying what they // want, and second-guessing an explicit choice is not this classifier's job. const intent = mode === 'agent' ? intentRouter.classify(task) : { intent: 'task', reason: 'mode was chosen' }; @@ -546,7 +663,12 @@ class AgentSession { mode, capability: this.capability, thinkingCapacity: this.thinkingCapacity, - memory: await this._renderMemory(), + // Selected against what was asked rather than by recency. A session's memory + // file outlives the subject that filled it, so on any turn but the first the + // most recent notes are about the *previous* request; the notes bearing on this + // one are further back. Recall is never worse for it — a message that matches + // nothing falls back to the same recency window this used to take. + memory: await this._renderMemory({ about: task }), earnedHints: await this._earnedHints(mode), environment: this.environment, intent: intent.intent, @@ -653,11 +775,12 @@ class AgentSession { /** @type {SessionResult} */ const result = { - summary: appendUnverifiedNote( - appendUnfinishedNote(checkedSummary, outcome.steps), - outcome, - changeSet - ), + summary: + appendUnverifiedNote( + appendUnfinishedNote(checkedSummary, outcome.steps), + outcome, + changeSet + ) + this._describeInterventions(), steps: outcome.steps, changeSet, stopReason: outcome.stopReason, @@ -672,9 +795,88 @@ class AgentSession { } finally { this.running = false; this._controller = null; + this._emit = null; + this._currentItem = ''; + // Also cleared on the paths that leave `_runWithTodos` early — a cancel, or a + // throw — where the line at the end of it is never reached. + this._todos = null; } } + /** + * Read the request the way a person would before handing it to the model. + * + * Returns the task to actually run, which is usually the one that came in. See + * `core/commonSense` for what is checked and why the line between fixing something + * and asking about it is drawn where it is. + * + * Never throws: an interpretation that fails leaves the request exactly as the user + * typed it, which is the behaviour every version before 0.7.0 had. + * + * @param {string} task + * @param {object} options + * @param {(event: object) => void} emit + * @returns {Promise} + * @private + */ + async _interpretRequest(task, options, emit) { + let files = []; + try { + files = await this._workspaceFiles(null); + } catch (err) { + logger.debug(`Could not list the workspace to read the request against: ${/** @type {Error} */ (err).message}`); + return task; + } + + /** @type {import('../core/commonSense').Interpretation} */ + let reading; + try { + reading = commonSense.interpret({ + task, + files, + conversation: this.conversation, + editorPath: options.editor ? options.editor.path : '', + canAsk: Boolean(this.onClarify), + }); + } catch (err) { + logger.warn(`Could not read the request: ${/** @type {Error} */ (err).message}`); + return task; + } + + if (reading.kind === 'repaired') { + this._interpretations.push(reading.note); + emit({ type: 'interpretation', note: reading.note }); + // Into memory as well as the summary. A user who names the same file the same + // wrong way twice in one session should not have it re-derived from scratch, and + // the note is composed rather than model-written so it is safe to keep. + if (this.memory) { + try { + await this.memory.append(reading.note); + } catch (err) { + logger.debug(`Could not record the reading: ${/** @type {Error} */ (err).message}`); + } + } + return reading.task; + } + + if (reading.kind === 'ask' && reading.clarification) { + const resolved = await this._ask(reading.clarification, emit); + if (!resolved) return task; + if (resolved.effect === 'stop') { + this._interrupt = 'stop'; + return task; + } + // Appended rather than substituted. The user answered a question about their own + // request, so what they said is an addition to it — replacing the text would + // discard the half the question was not about. + const note = `You asked about this and the user said: ${resolved.guidance}`; + this._interpretations.push(note); + return `${task}\n\n${note}`; + } + + return task; + } + /** * Work through a request as a TODO list, one item at a time. * @@ -727,6 +929,10 @@ class AgentSession { } const todos = new TodoList(items); + // Reachable from `_recover`, which runs inside a loop this method is awaiting and + // has no other way to reach the checklist. Cleared in the `finally` below so a + // later turn cannot write into a list that has already been reported. + this._todos = todos; emit({ type: 'todo', items: todos.items.map((item) => item.text) }); logger.info(`Running ${todos.items.length} TODO item(s) one at a time.`); @@ -754,6 +960,8 @@ class AgentSession { /** Set when a step failed twice and the run is stopping deliberately. */ let stopNotice = ''; let cancelled = false; + /** Set when the user, asked mid-run, chose to stop. Not the same as cancelling. */ + let stoppedByUser = false; while (todos.current() && remainingSteps > 0) { if (this._controller.signal.aborted) { @@ -763,6 +971,8 @@ class AgentSession { const item = todos.current(); const position = todos.position(); + // So a question raised from inside this item's loop can say what it is about. + this._currentItem = item.text; // The snapshot rides along with the event because the UI has no other way to // learn the list changed: it holds the items it was given at `todo` time and // nothing else, so an index alone would leave it guessing at the other rows. @@ -838,7 +1048,7 @@ class AgentSession { // the oldest one in the file by the time the step runs. See // `memoryStore.readRelevant`. const context = await this._buildContext(itemTask, itemRoute, options.editor, { - recallAbout: this.stepSessions ? item.text : '', + recallAbout: item.text, }); // Each item runs a fresh loop, and a loop numbers its steps from its own @@ -895,10 +1105,19 @@ class AgentSession { attemptResult = { outcome, verdict, attempts: attempt }; + // The user was asked mid-item and said stop. Distinct from a cancel: they made + // a decision about this run rather than abandoning it, and the summary says so. + if (this._interrupt === 'stop') { + stoppedByUser = true; + break; + } if (outcome.stopReason === 'cancelled') { cancelled = true; break; } + // Asked mid-item and told to skip. The retry below is exactly what they + // declined, so it does not happen. + if (this._interrupt === 'skip') break; if (verdict.ok || attempt === maxAttempts) break; // About to be written off, so it gets one more run with the diagnosis stated — @@ -932,9 +1151,32 @@ class AgentSession { } const changedPaths = produced.map((change) => change.path); + + // The user was asked about this item and chose to leave it. That is a decision, + // not a failure, and recording it as one would put a `[!]` against a row the user + // themselves closed — and would then feed "an earlier step failed" to every item + // after it. + if (this._interrupt === 'skip') { + this._interrupt = ''; + todos.skipCurrent('you asked me to skip this one'); + emit({ + type: 'todo-item-done', + index: position, + status: 'skipped', + text: item.text, + items: snapshotTodos(todos), + }); + continue; + } + todos.finishCurrent(status, outcomeText, itemStepCount, { changedPaths, attempts }); emit({ type: 'todo-item-done', index: position, status, text: item.text, items: snapshotTodos(todos) }); + if (stoppedByUser) { + todos.skipRemaining('you stopped the run here'); + break; + } + if (cancelled) { todos.skipRemaining('the session was cancelled'); break; @@ -968,8 +1210,15 @@ class AgentSession { const summary = `${progress.done} of ${progress.total} item(s) completed.${caveat}\n\n${todos.describe()}` + (stopNotice ? `\n\n${stopNotice}` : '') + + // What the user was asked and what they said, before the per-item detail. A run + // that only succeeded because they answered a question should say so — otherwise + // the checklist reads as though the model worked it out on its own. + this._describeInterventions(todos) + (summaries.length > 0 ? `\n\nDetail:\n${summaries.join('\n')}` : ''); + // The list is reported now, so nothing may write into it afterwards. + this._todos = null; + return { summary: appendUnfinishedNote(summary, allSteps), steps: allSteps, @@ -980,12 +1229,67 @@ class AgentSession { }; } + /** + * What the run had to be told, and by whom, for the end of the summary. + * + * Three separate things end up here and they are not interchangeable: what was read + * differently from what the user typed, what the user said when asked mid-run, and + * what kept failing regardless. A summary that reports only the checklist hides all + * three — and the third is the one a user most needs, because a run that ends "4 of 4 + * completed" after hitting the same error eleven times is technically true and + * actively misleading. + * + * @param {TodoList} [todos] + * @returns {string} Empty when the run needed none of it. + * @private + */ + _describeInterventions(todos) { + const blocks = []; + + if (this._interpretations && this._interpretations.length > 0) { + blocks.push(`How I read the request:\n${this._interpretations.map((note) => `- ${note}`).join('\n')}`); + } + + if (this._userGuidance && this._userGuidance.length > 0) { + blocks.push(`What you told me when I asked:\n${this._userGuidance.map((note) => `- ${note}`).join('\n')}`); + } + + if (todos) { + const changed = todos.describeChanges(); + if (changed) blocks.push(changed); + } + + const stuck = this._recovery ? this._recovery.persistent() : []; + if (stuck.length > 0) { + const lines = stuck + .slice(0, 3) + .map((entry) => `- ${entry.action} failed ${entry.count} times: ${entry.headline}`); + blocks.push(`What kept going wrong:\n${lines.join('\n')}`); + } + + return blocks.length > 0 ? `\n\n${blocks.join('\n\n')}` : ''; + } + /** * The route for one step, with its memory block recalled against that step. * - * Returns the session's own route untouched unless step sessions are on, so the - * ordinary TODO path keeps the single system prompt it has always had and pays for - * nothing it does not use. + * ## Why every item gets this, not only a briefed step + * + * Until 0.7.0 this returned the session's route untouched unless step sessions were + * on, so a six-item run shared one memory block — selected by recency, before the + * list existed, against the whole request. That is the wrong block for every item + * after the first. The React benchmark shows the failure exactly: the item that had + * to assemble `App.jsx` ran sixth, and the notes naming `useTodos.js` and + * `TodoInput.jsx` — the two files it needed to import — were by then the oldest in + * the file and the first to fall out of a five-entry recency window. The note that + * survived was about the README. + * + * Recall by subject is never worse than recall by recency: an item whose text + * matches nothing gets exactly the recency window it would have had (see + * `memoryStore.readRelevant`). What it costs is one extra prompt assembly per item, + * which is string work against an already-loaded file — no inference, no disk. + * + * Returns the session's own route untouched when there is no memory to select from. * * @param {import('../core/promptRouter').Route} activeRoute * @param {string} about The step's text. @@ -993,7 +1297,7 @@ class AgentSession { * @private */ async _routeForStep(activeRoute, about) { - if (!this.stepSessions || !this.memory) return activeRoute; + if (!this.memory || !about) return activeRoute; try { return promptRouter.route({ @@ -1004,6 +1308,13 @@ class AgentSession { earnedHints: await this._earnedHints(activeRoute.mode), environment: this.environment, intent: 'task', + // Carried, not recomputed. This route replaces the session's for the whole + // step, so dropping the flag would hand the mutating tools back to a turn the + // router had already decided was a look-only request — and now that every item + // rebuilds its route, that would be the default path rather than an + // experimental one. The decision belongs to the session's route; this only + // re-renders the memory block underneath it. + readOnlyTurn: activeRoute.readOnlyTurn, }); } catch (err) { // A step must never fail to start because its recall could not be assembled. @@ -1421,12 +1732,15 @@ class AgentSession { * has always received without anyone calling it a tool. Looking up the handler under * a read-only mode is how that read is performed; it is not a grant. * - * @param {import('../core/promptRouter').Route} activeRoute + * @param {import('../core/promptRouter').Route | null} activeRoute + * Null when the read happens before a route exists — `_interpretRequest` needs the + * listing to decide whether a filename in the request resolves, and that decision + * comes before routing. Treated as read-only, which is what such a read is. * @returns {Promise} * @private */ async _workspaceFiles(activeRoute) { - const loopless = activeRoute.strategy === 'none' || activeRoute.strategy === 'chat'; + const loopless = !activeRoute || activeRoute.strategy === 'none' || activeRoute.strategy === 'chat'; // 'plan' is the read-only mode. Substituted only to resolve the handler; the gate // below still audits the read under the mode the user is actually in. const lookupMode = loopless ? 'plan' : activeRoute.mode; @@ -1702,6 +2016,11 @@ class AgentSession { // action passes through regardless of which tier produced it. this._recordStep(action, result, activeRoute, Boolean(tool.mutating), ms); + // Same reason, and the reason this is the right place for it: a failure that + // repeats has to be counted across the whole turn, and only this method sees every + // action from both loop tiers. + if (!result.ok) result = await this._recover(action, result); + // The one line that reconstructs a session afterwards: what was asked for, what // happened, and how long it took. Debug rather than info, because a run is dozens // of these — but when a user reports "it said it edited the file and it did not", @@ -1714,6 +2033,100 @@ class AgentSession { return result; } + /** + * Decide what a failed action gets told, and whether the user gets asked. + * + * Returns the result the loop will see — usually the same one with a sentence added. + * The observation is the only channel back to the model on a Tier B loop, so that is + * where the guidance has to go; there is no side band it would read. + * + * @param {object} action + * @param {import('./toolRegistry').ToolResult} result + * @returns {Promise} + * @private + */ + async _recover(action, result) { + /** @type {import('./errorRecovery').Failure} */ + const failure = { + action: String(action.action || ''), + path: action.path, + command: action.command, + error: result.error, + observation: String(result.observation || ''), + // `runScript` appends its own "What went wrong: …" sentence when a rule matched, + // and a second explanation saying the same thing differently is noise in a + // context window that has none spare. `detail.reason` is set exactly when that + // happened — see `tools/runScript`. + diagnosed: Boolean(result.detail && result.detail.reason), + item: this._currentItem || '', + }; + + /** @type {import('./errorRecovery').RecoveryDecision} */ + let decision; + try { + decision = this._recovery.observe(failure); + } catch (err) { + logger.warn(`Could not assess the failure: ${/** @type {Error} */ (err).message}`); + return result; + } + + if (decision.note && this.memory) { + try { + await this.memory.append(decision.note); + } catch (err) { + logger.debug(`Could not record the failure: ${/** @type {Error} */ (err).message}`); + } + } + + if (decision.kind === 'ask' && decision.clarification) { + const resolved = await this._ask(decision.clarification, this._emit || (() => {})); + if (resolved) { + const note = this._recovery.recordAnswer(failure, resolved.guidance || resolved.label); + if (this.memory) { + try { + await this.memory.append(note); + } catch (err) { + logger.debug(`Could not record the answer: ${/** @type {Error} */ (err).message}`); + } + } + + if (resolved.effect === 'stop') { + this._interrupt = 'stop'; + // Aborting is what actually stops a loop mid-item; the flag alone is only + // read once the loop has returned, which could be several steps later. + if (this._controller) this._controller.abort(); + return { ...result, observation: `${result.observation}\n\nThe user stopped the run here.` }; + } + if (resolved.effect === 'skip') { + this._interrupt = 'skip'; + } + + // The user has just said what this step is actually supposed to do, so the + // checklist should say it too. Not cosmetic: the item's text is what the retry + // is briefed on, what `stepGuard` checks the changed files against, and what + // the summary reads back — leaving it as the model's original wording means + // the one authoritative statement of the step is the only place their + // correction does not reach. + if (resolved.effect === 'instruct' && this._todos && resolved.guidance) { + this._todos.replaceCurrent(`${this._todos.current().text} — ${resolved.guidance}`); + } + + return { + ...result, + observation: `${result.observation}\n\n${resolved.guidance || resolved.label}`, + }; + } + // Nobody answered. Fall through to guidance rather than blocking. + return result; + } + + if (decision.kind === 'guidance' && decision.guidance) { + return { ...result, observation: `${result.observation}\n\n${decision.guidance}` }; + } + + return result; + } + /** * Condense the session into memory, at the configured frequency. * diff --git a/app/agent/clarification.js b/app/agent/clarification.js new file mode 100644 index 0000000..1115c03 --- /dev/null +++ b/app/agent/clarification.js @@ -0,0 +1,223 @@ +'use strict'; + +/** + * A question the agent puts to the user mid-run, with the options it can act on. + * + * ## Why an agent that runs offline still stops to ask + * + * The alternative to asking is guessing, and the guesses a small model makes when it + * is stuck are not neutral. Across the v0.5.3 and 0.6.0 rounds the three observed + * endings for a model that had run out of ideas were: repeat the failing action until + * the step budget ran out, announce success it had not achieved, or abandon a task it + * was one decision away from finishing. All three cost the user more than a question + * would have. + * + * So this is the escape hatch, and its rules come from what makes a question cheap to + * answer rather than from what is easy to generate: + * + * - **Two to four options, never more.** A question with seven options is a second + * task handed to the user. Two to four fits on a card and can be answered without + * reading twice. + * - **Exactly one is recommended.** "Here are your choices" moves the decision without + * helping with it. The agent has the trace, the diagnosis, and the change set; it + * knows more about the situation than the user has looked at, and it should say what + * it would do. `build` refuses a set that recommends none or several. + * - **Every option states its effect on the queue**, so answering is not a second + * guess about what happens next. + * - **Free text is always available.** The options are the agent's reading of the + * situation, and the whole reason it is asking is that its reading may be wrong. + * + * ## What this module is not + * + * It does not ask anything. It builds and validates the request; `agentSession` owns + * when to raise one and what to do with the answer, and `chatTab` owns showing it. + * The split matters because a malformed request must fail here, at construction, and + * not in the webview where the run is already blocked waiting on a card that cannot + * render. + * + * @module agent/clarification + */ + +const logger = require('../utils/logger'); + +/** Fewer than this is not a choice; more is a task. */ +const MIN_OPTIONS = 2; +const MAX_OPTIONS = 4; + +/** Labels and questions are shown on a card in a narrow panel. */ +const MAX_LABEL_CHARS = 80; +const MAX_QUESTION_CHARS = 300; +const MAX_DETAIL_CHARS = 200; + +/** + * What the run does with the answer. + * + * `instruct` is the interesting one: the run continues on the same item, with what the + * user said added to the model's next prompt as a correction. That is the case the + * whole mechanism is for — the user knows the one fact the model was missing. + * + * @typedef {'instruct' | 'retry' | 'skip' | 'stop'} ClarificationEffect + */ + +/** + * @typedef {object} ClarificationOption + * @property {string} id + * @property {string} label What will happen, in the imperative. + * @property {string} [detail] One sentence on the consequence. + * @property {boolean} [recommended] + * @property {ClarificationEffect} effect + * @property {string} [guidance] Given to the model when this option is chosen. + */ + +/** + * @typedef {object} Clarification + * @property {string} id + * @property {'error' | 'ambiguous' | 'choice'} kind Why it is being asked. + * @property {string} question + * @property {string} [context] What was already tried, so the user is not guessing. + * @property {ClarificationOption[]} options + * @property {boolean} allowFreeText + */ + +/** + * @typedef {object} ClarificationAnswer + * @property {string} id The clarification being answered. + * @property {string} [optionId] + * @property {string} [text] Free text, when the user typed instead of choosing. + * @property {boolean} [cancelled] The user closed the panel or stopped the run. + */ + +let counter = 0; + +/** @returns {string} A per-run unique id. Not security-bearing; it only pairs answers to questions. */ +function nextId() { + counter += 1; + return `clarify-${Date.now().toString(36)}-${counter}`; +} + +/** + * Trim a model- or code-supplied string to something a card can hold. + * + * @param {unknown} value + * @param {number} max + * @returns {string} + */ +function clamp(value, max) { + const text = String(value == null ? '' : value).replace(/\s+/g, ' ').trim(); + return text.length > max ? `${text.slice(0, max - 1).trimEnd()}…` : text; +} + +/** + * Build a validated clarification request. + * + * Throws rather than repairing a malformed set, because every caller here is code in + * this repository rather than model output — a bad set is a bug to fix, and quietly + * padding it to two options would hide it behind a question the user cannot answer + * usefully. + * + * The one thing that *is* repaired is a missing recommendation: the first option is + * promoted, with a warning. A run already blocked on a question should not also fail. + * + * @param {object} spec + * @param {'error' | 'ambiguous' | 'choice'} spec.kind + * @param {string} spec.question + * @param {string} [spec.context] + * @param {Array & {id?: string}>} spec.options + * @param {boolean} [spec.allowFreeText] + * @returns {Clarification} + */ +function build(spec) { + const question = clamp(spec.question, MAX_QUESTION_CHARS); + if (!question) throw new Error('A clarification needs a question.'); + + const given = Array.isArray(spec.options) ? spec.options : []; + if (given.length < MIN_OPTIONS || given.length > MAX_OPTIONS) { + throw new Error( + `A clarification needs between ${MIN_OPTIONS} and ${MAX_OPTIONS} options; got ${given.length}.` + ); + } + + /** @type {ClarificationOption[]} */ + const options = given.map((option, index) => { + const label = clamp(option.label, MAX_LABEL_CHARS); + if (!label) throw new Error(`Clarification option ${index + 1} has no label.`); + return { + id: option.id || `option-${index + 1}`, + label, + detail: clamp(option.detail, MAX_DETAIL_CHARS), + recommended: option.recommended === true, + effect: option.effect || 'instruct', + guidance: typeof option.guidance === 'string' ? option.guidance : '', + }; + }); + + const recommended = options.filter((option) => option.recommended); + if (recommended.length > 1) { + throw new Error('A clarification must recommend exactly one option, not several.'); + } + if (recommended.length === 0) { + logger.warn('A clarification was built with no recommended option; promoting the first.'); + options[0].recommended = true; + } + + return { + id: nextId(), + kind: spec.kind || 'choice', + question, + context: clamp(spec.context, MAX_QUESTION_CHARS), + options, + // Always, and deliberately not settable. The options are the agent's reading of a + // situation it has just demonstrated it does not fully understand. + allowFreeText: true, + }; +} + +/** + * Resolve an answer against the question it answers. + * + * Returns a decision even for input that matches nothing, because the caller is a run + * that is currently blocked: "the answer made no sense" has to resolve to *something*, + * and the safe something is to stop rather than to pick an option on the user's behalf. + * + * @param {Clarification} clarification + * @param {ClarificationAnswer | null} answer + * @returns {{effect: ClarificationEffect, guidance: string, label: string}} + */ +function resolve(clarification, answer) { + if (!answer || answer.cancelled) { + return { effect: 'stop', guidance: '', label: 'stopped' }; + } + + // Free text wins over a selected option: a user who typed something meant it, and it + // carries more than the option they may also have clicked. + const typed = clamp(answer.text, 500); + if (typed) { + return { + effect: 'instruct', + guidance: typed, + label: 'answered', + }; + } + + const chosen = clarification.options.find((option) => option.id === answer.optionId); + if (!chosen) { + logger.warn(`Clarification ${clarification.id} answered with an unknown option; stopping.`); + return { effect: 'stop', guidance: '', label: 'stopped' }; + } + + return { + effect: chosen.effect, + guidance: chosen.guidance || chosen.label, + label: chosen.label, + }; +} + +module.exports = { + build, + resolve, + clamp, + MIN_OPTIONS, + MAX_OPTIONS, + MAX_LABEL_CHARS, + MAX_QUESTION_CHARS, +}; diff --git a/app/agent/errorRecovery.js b/app/agent/errorRecovery.js new file mode 100644 index 0000000..72e29c4 --- /dev/null +++ b/app/agent/errorRecovery.js @@ -0,0 +1,430 @@ +'use strict'; + +/** + * What to do about a failure the model is not getting past on its own. + * + * ## The gap this fills + * + * `scriptDiagnosis` classifies a *command* failure into a named reason and one + * actionable sentence, and it is the right answer whenever a rule matches. This module + * covers the two cases it cannot: + * + * 1. **The failure nobody wrote a rule for.** `diagnose` returns null, and the model + * is handed a stack trace with no sentence saying what to do about it. + * 2. **The failure that has already been diagnosed, twice, and is still happening.** + * The diagnosis was correct and the model did not act on it — or acted on it and + * it did not help. Repeating the same sentence a third time is not a strategy. + * + * Both end the same way if nothing intervenes. From the v0.5.3 and 0.6.0 rounds, a + * small model that is stuck does one of three things: resends the identical action + * until the step budget is gone, reports success it did not achieve, or abandons a + * task it was one step from finishing. `gemma3:1b` sent the same failing `node + * src/main.js` four times; `ornith:9b` resent a refused `javac` three times. + * + * ## The escalation ladder + * + * Repetition is the signal, because a failure that happens once is information and the + * same failure twice is a model that is not learning from it: + * + * | Seen | What happens | + * |---|---| + * | 1st, diagnosed | Nothing extra — the diagnosis already said it | + * | 1st, undiagnosed | Read the error literally; here is what to look at | + * | 2nd | Ask the user | + * + * The user is asked **last**, not first. An agent that asks whenever it is unsure is an + * agent that has moved its work to the user, and this project's whole premise is a + * model small enough that "unsure" is its resting state. + * + * ## Why the second time and not the third + * + * Because there is no third. `reactLoop.REPEAT_LIMIT` is 2: the loop ends a run once + * the model has sent the same action twice, which is the right call and long predates + * this module. A ladder that waited for a third occurrence would therefore never reach + * its top rung in the commonest case — the model resending an identical failing + * command — and the run would end with the user never asked and the step budget spent. + * + * So the rungs are set against the loop's own limit rather than against a round number. + * The second failure is the last moment at which asking can still change the outcome, + * which makes it the right one and not merely an available one. A failure reached by + * two *different* actions does not trip the loop's guard, and this counts those the + * same way — same problem, same question. + * + * ## Why the error goes into memory + * + * A failure the run never resolved is exactly what the *next* run needs to know, and it + * is the thing session memory was least likely to contain: `contextTranslator` records + * what steps did, and a step that failed did nothing worth narrating. So the error is + * recorded here, in composed text rather than model-written, alongside whatever the + * user said about it — because "the user was asked about this and said X" is the + * highest-value note in the file and the one no amount of re-reading the workspace + * recovers. + * + * @module agent/errorRecovery + */ + +const logger = require('../utils/logger'); +const clarification = require('./clarification'); + +/** + * How many times one failure repeats before the user is asked about it. + * + * Two, and it is pinned to `reactLoop.REPEAT_LIMIT` rather than chosen — see the + * module header. Raising it without raising that one means never asking at all. + */ +const REPEATS_BEFORE_ASKING = 2; + +/** Signatures are compared, not read; this is long enough to separate real failures. */ +const SIGNATURE_CHARS = 200; + +/** A failure message is quoted back to the user and into memory, so it is bounded. */ +const MAX_QUOTED_CHARS = 300; + +/** + * Tool failures that are the permission model working, not the model being stuck. + * + * A refused write is a *decision*, and the correct response to it is to stop trying — + * which the model is already told at the point of refusal. Escalating a declined + * delete to "shall I ask the user?" would ask them the same question twice, and the + * second time with less context than the confirmation dialog had. + */ +const NOT_STUCK = new Set([ + 'PERMISSION_DENIED', + 'DECLINED', + 'CANCELLED', + 'TOOL_UNAVAILABLE', +]); + +/** + * Reduce a failure message to something that compares equal across repeats. + * + * Line numbers, addresses, and paths change between two runs of the same broken code; + * the sentence does not. Digits go last and wholesale, which merges "line 12" with + * "line 40" — correct here, because this asks "is this the same problem again", not + * "is this the same character sequence". + * + * @param {string} action + * @param {string} message + * @returns {string} + */ +function signatureOf(action, message) { + const text = String(message == null ? '' : message) + // Windows absolute paths, then POSIX ones. Both appear in stack traces and both + // differ between machines running the identical failure. + .replace(/[A-Za-z]:[\\/][^\s'"()]+/g, '') + .replace(/(^|[\s'"(])\/[^\s'"()]{2,}/g, '$1') + .replace(/\b0x[0-9a-f]+\b/gi, '') + .replace(/\b\d+\b/g, '') + .replace(/\s+/g, ' ') + .trim() + .toLowerCase() + .slice(0, SIGNATURE_CHARS); + + return `${action}::${text}`; +} + +/** + * The first line of a failure that says something, for quoting back. + * + * Stack frames are skipped: `at Object. (/app/x.js:4:11)` is where it + * happened, and the line above it is what happened. A user being asked about a failure + * needs the second one. + * + * @param {string} message + * @returns {string} + */ +function headline(message) { + const lines = String(message == null ? '' : message).split(/\r?\n/); + for (const line of lines) { + const trimmed = line.trim(); + if (!trimmed) continue; + if (/^at\s/.test(trimmed)) continue; + if (/^\s*(File|Traceback)\b/.test(trimmed)) continue; + return clarification.clamp(trimmed, MAX_QUOTED_CHARS); + } + return clarification.clamp(String(message || '').trim(), MAX_QUOTED_CHARS); +} + +/** + * @typedef {object} Failure + * @property {string} action The tool that failed. + * @property {string} [path] + * @property {string} [command] + * @property {string} [error] Machine-readable code, when the tool gave one. + * @property {string} observation What the model was shown. + * @property {boolean} diagnosed Whether `scriptDiagnosis` already explained it. + * @property {string} [item] The TODO item being worked on, for the question text. + */ + +/** + * @typedef {object} RecoveryDecision + * @property {'none' | 'guidance' | 'ask'} kind + * @property {string} [guidance] Appended to the observation the model reads next. + * @property {string} [note] Composed memory text. Present whenever there is one. + * @property {import('./clarification').Clarification} [clarification] + * @property {number} count How many times this failure has now been seen. + */ + +class ErrorRecovery { + /** + * @param {object} [options] + * @param {number} [options.repeatsBeforeAsking] + * @param {boolean} [options.canAsk] + * False when nothing is listening for a question — an Ask-mode turn, a benchmark, + * a session whose panel has gone away. The ladder then tops out at guidance, which + * is strictly better than blocking a run on a card nobody will ever see. + */ + constructor(options = {}) { + this.repeatsBeforeAsking = options.repeatsBeforeAsking || REPEATS_BEFORE_ASKING; + this.canAsk = options.canAsk !== false; + + /** + * Signature → what we know about that failure so far. + * + * Per-session rather than persisted: "you have tried this three times" is a claim + * about this run, and carrying it across runs would have a new session open already + * out of patience with a failure the user may have fixed in between. + * + * @type {Map} + */ + this._seen = new Map(); + + /** + * What the user said when they were asked, keyed by signature, so the same failure + * later in the run is answered from what they already told us instead of asking + * twice. + * + * @type {Map} + */ + this._answers = new Map(); + } + + /** + * Record a failure and decide what to do about it. + * + * @param {Failure} failure + * @returns {RecoveryDecision} + */ + observe(failure) { + const action = String(failure.action || 'action'); + const observation = String(failure.observation || ''); + + // A refusal is the permission model working. Nothing here applies. + if (failure.error && NOT_STUCK.has(failure.error)) { + return { kind: 'none', count: 0 }; + } + + const signature = signatureOf(action, observation); + const known = this._seen.get(signature) || { + count: 0, + headline: headline(observation), + action, + asked: false, + }; + known.count += 1; + this._seen.set(signature, known); + + const target = failure.path || failure.command || ''; + const note = this._note(known, target, failure.diagnosed); + + // Already asked about this one. The user's answer is the guidance from here on — + // asking again would be asking them to repeat themselves. + const answered = this._answers.get(signature); + if (answered) { + return { + kind: 'guidance', + guidance: + `You asked the user about this and they said: "${answered}" — do that, ` + + 'rather than trying the same thing again.', + note, + count: known.count, + }; + } + + if (known.count >= this.repeatsBeforeAsking && this.canAsk && !known.asked) { + known.asked = true; + logger.info( + `The same failure has happened ${known.count} times (${known.headline}); asking the user.` + ); + return { + kind: 'ask', + clarification: this._question(known, failure, target), + note, + count: known.count, + }; + } + + if (known.count === 1) { + // A diagnosed failure has already been given its sentence by `scriptDiagnosis`, + // and a second one saying the same thing in different words is noise in a context + // window that has none to spare. + if (failure.diagnosed) return { kind: 'none', note, count: known.count }; + return { kind: 'guidance', guidance: this._firstGuidance(failure), note, count: known.count }; + } + + return { + kind: 'guidance', + guidance: this._repeatGuidance(known, target), + note, + count: known.count, + }; + } + + /** + * Remember what the user said, so the rest of the run acts on it. + * + * @param {Failure} failure The failure they were asked about. + * @param {string} answer What they chose or typed. + * @returns {string} The memory note for the answer. + */ + recordAnswer(failure, answer) { + const signature = signatureOf(String(failure.action || 'action'), String(failure.observation || '')); + const text = clarification.clamp(answer, MAX_QUOTED_CHARS); + if (text) this._answers.set(signature, text); + + const known = this._seen.get(signature); + const about = known ? known.headline : headline(String(failure.observation || '')); + return `The user was asked about a repeated failure ("${about}") and said: ${text}`; + } + + /** + * Everything that failed more than once this run, worst first. + * + * Used by the session summary: a run that ended "3 of 4 items completed" is more + * honest when it can also say which wall it kept hitting. + * + * @returns {Array<{headline: string, action: string, count: number}>} + */ + persistent() { + return [...this._seen.values()] + .filter((known) => known.count > 1) + .sort((a, b) => b.count - a.count) + .map(({ headline: text, action, count }) => ({ headline: text, action, count })); + } + + /** + * What to say the first time an unexplained failure appears. + * + * @param {Failure} failure + * @returns {string} + * @private + */ + _firstGuidance(failure) { + const where = failure.path ? ` It was working on ${failure.path}.` : ''; + return ( + 'This failure does not match anything known, so read the error above literally rather than guessing: ' + + 'it names what went wrong and usually where.' + + where + + ' Change one thing that the error actually mentions, then try again. Do not resend the same ' + + 'action unchanged — it will fail the same way.' + ); + } + + /** + * What to say once it is clear the model is going round in a circle. + * + * Names the count, because "you have already tried this twice" is a fact the model + * cannot get from its own context on a Tier B loop — the trace is rebuilt each turn + * and the earlier attempt is no longer in it. + * + * @param {{count: number, headline: string}} known + * @param {string} target + * @returns {string} + * @private + */ + _repeatGuidance(known, target) { + const what = target ? ` on ${target}` : ''; + return ( + `This is the same failure as before — it has now happened ${known.count} times${what}: ` + + `"${known.headline}". Whatever you changed between attempts did not address it. Do not try that ` + + 'approach a third time. Either work on a different part of the task, or fix the thing the error ' + + 'names directly — and if you genuinely cannot, say so plainly rather than repeating the attempt.' + ); + } + + /** + * The question put to the user once the ladder runs out. + * + * Three options, one recommended, and the recommendation is *skip* rather than retry: + * the same thing has now failed twice with targeted guidance in between, and + * recommending another attempt would recommend the thing that has already not worked. + * Skipping keeps the rest of the list moving, which is usually what the user wants + * and always what they can undo. + * + * @param {{count: number, headline: string}} known + * @param {Failure} failure + * @param {string} target + * @returns {import('./clarification').Clarification} + * @private + */ + _question(known, failure, target) { + const what = target ? ` \`${target}\`` : ''; + const item = failure.item ? ` while working on "${clarification.clamp(failure.item, 120)}"` : ''; + + return clarification.build({ + kind: 'error', + question: `I am stuck on${what || ' this step'}. How should I carry on?`, + context: + `The same failure has happened ${known.count} times${item}: "${known.headline}". ` + + 'I have changed my approach between attempts and it keeps failing the same way.', + options: [ + { + id: 'skip', + label: 'Skip this and carry on', + detail: 'Leaves this step unfinished and moves to the rest of the list.', + effect: 'skip', + recommended: true, + guidance: + 'The user chose to skip this step. Leave it alone, do not try it again, and move on to the ' + + 'next item.', + }, + { + id: 'retry', + label: 'Try once more', + detail: 'Runs the same step again from a clean start.', + effect: 'retry', + guidance: + 'The user asked for one more attempt. Start from what the error says rather than from what ' + + 'you tried last time.', + }, + { + id: 'stop', + label: 'Stop the run', + detail: 'Ends here, keeping every change made so far.', + effect: 'stop', + }, + ], + }); + } + + /** + * The memory note for a failure. + * + * Composed here rather than written by the translator: a failed step has nothing to + * narrate, so the translator's grounding check rejects whatever it invents and the + * note ends up bare. This one is built from what is known for certain. + * + * @param {{count: number, headline: string, action: string}} known + * @param {string} target + * @param {boolean} diagnosed + * @returns {string} + * @private + */ + _note(known, target, diagnosed) { + // Only worth remembering once it is a pattern. A single failure that the next step + // fixes is noise, and the recall window is small enough that noise costs something. + if (known.count < 2) return ''; + + const what = target ? ` on ${target}` : ''; + const explained = diagnosed ? '' : ' It was not a recognised kind of failure.'; + return `${known.action}${what} failed ${known.count} times with: ${known.headline}.${explained}`; + } +} + +module.exports = { + ErrorRecovery, + signatureOf, + headline, + REPEATS_BEFORE_ASKING, + NOT_STUCK, +}; diff --git a/app/agent/scriptDiagnosis.js b/app/agent/scriptDiagnosis.js index d2722e4..a32dc7b 100644 --- a/app/agent/scriptDiagnosis.js +++ b/app/agent/scriptDiagnosis.js @@ -50,7 +50,12 @@ * once you squint. Each rule states the fix in the imperative, naming a concrete next * command wherever there is one. * - * @type {Array<{reason: string, pattern: RegExp, summary: string, fix: string, retryable?: boolean}>} + * `fix` may be a function of the match when the sentence has to name what the error + * named. "Something is undefined" is not actionable to a 1B model; "`addTodo` is used + * in that file but never defined or imported" is the same information with the one + * detail that makes it a next move. + * + * @type {Array<{reason: string, pattern: RegExp, summary: string | ((m: RegExpExecArray) => string), fix: string | ((m: RegExpExecArray) => string), retryable?: boolean, fixFirst?: boolean}>} */ const RULES = [ { @@ -138,6 +143,97 @@ const RULES = [ 'use `"@tailwindcss/postcss": {}` in place of `tailwindcss: {}`. Then run the command again.', fixFirst: true, }, + // ── Undefined symbols ──────────────────────────────────────────────────────── + // + // The single most common way code written by a small model fails at runtime, and + // until 0.7.0 none of it matched a rule: the model got a stack trace, the generic + // "the error points at a file" fallback, and no mention of the name that was + // actually missing. It then rewrote the file from memory and produced the same + // error, which is the loop these rules exist to break. + // + // Ordered after MODULE_SYSTEM, which claims the one `is not defined` that is really + // a module-system mismatch (`module is not defined in ES module scope`) and has a + // completely different fix. + { + reason: 'UNDEFINED_SYMBOL', + pattern: /ReferenceError:\s*(\w+) is not defined/, + summary: (m) => `${m[1]} is used but never defined`, + fix: (m) => + `\`${m[1]}\` is used in that file but is never defined or imported there. Open the file the error names and ` + + `either import \`${m[1]}\` from the file that exports it, or define it. If you meant a name that already ` + + 'exists, check the spelling matches exactly — it is case-sensitive. Then run the command again.', + fixFirst: true, + }, + { + reason: 'UNDEFINED_SYMBOL', + pattern: /NameError: name '([^']+)' is not defined/, + summary: (m) => `${m[1]} is used but never defined`, + fix: (m) => + `\`${m[1]}\` is used in that file but is never defined or imported there. Open the file the error names and ` + + `either add the import for \`${m[1]}\`, or define it above where it is used. Then run the command again.`, + fixFirst: true, + }, + { + reason: 'UNDEFINED_SYMBOL', + // javac prints the location, then `symbol: variable foo` on a later line. + pattern: /cannot find symbol[\s\S]{0,300}?symbol:\s*(?:variable|method|class)\s+(\w+)/, + summary: (m) => `${m[1]} does not exist where it is used`, + fix: (m) => + `javac cannot find \`${m[1]}\`. Either it is spelled differently where it is declared, or the class that ` + + 'declares it is not imported, or it was never written. Open the file and line the error names, check that ' + + 'name against where you defined it, and fix whichever side is wrong. Then compile again.', + fixFirst: true, + }, + { + reason: 'UNDEFINED_PROPERTY', + // Node ≥16 phrasing, then the older one. Both name the property being read. + pattern: /Cannot read properties of (undefined|null) \(reading '([^']+)'\)|Cannot read property '([^']+)' of (undefined|null)/, + summary: (m) => `something was ${m[1] || m[4]} when '${m[2] || m[3]}' was read from it`, + fix: (m) => { + const property = m[2] || m[3]; + const value = m[1] || m[4]; + return ( + `The value that \`${property}\` was read from is ${value} at that point — the property is fine, the thing ` + + `holding it does not exist yet. Open the file and line the error names and work out why it is ${value}: a ` + + 'function that returned nothing, a prop or argument never passed in, state read before it was set, or an ' + + `import that did not resolve. Fix the source of the ${value}, not the line that read it. Then run it again.` + ); + }, + fixFirst: true, + }, + { + reason: 'UNDEFINED_PROPERTY', + pattern: /AttributeError: '(\w+)' object has no attribute '([^']+)'/, + summary: (m) => `a ${m[1]} has no attribute '${m[2]}'`, + fix: (m) => + `\`${m[2]}\` was read from a \`${m[1]}\`, which does not have it.` + + (m[1] === 'NoneType' + ? ' NoneType means the value is None — something returned None where an object was expected, so fix whatever produced the None rather than the line that used it.' + : ` Either the attribute is spelled differently on that class, or the object is not the type you expected. Open the file the error names and check what \`${m[1]}\` actually defines.`) + + ' Then run it again.', + fixFirst: true, + }, + { + reason: 'NOT_A_FUNCTION', + pattern: /TypeError: ([\w.]+) is not a function/, + summary: (m) => `${m[1]} is not a function`, + fix: (m) => + `\`${m[1]}\` is being called, but what is there is not a function — commonly a wrong import shape ` + + '(default vs named), a typo, or a value that is undefined. Open the file the error names, check how ' + + `\`${m[1]}\` is imported against how the other file exports it, and fix whichever side is wrong. ` + + 'Then run the command again.', + fixFirst: true, + }, + { + reason: 'NULL_DEREFERENCE', + pattern: /NullPointerException/, + summary: 'something was null when it was used', + fix: + 'A value was null where an object was expected. Open the file and line named in the stack trace, find which ' + + 'reference is null there, and fix what should have set it — initialise the field, pass the argument, or ' + + 'check for null before using it. Then run it again.', + fixFirst: true, + }, { reason: 'SYNTAX_ERROR', pattern: /SyntaxError|Unexpected token|Parse failure|error TS\d+|\berror: expected\b|Unterminated|Transform failed/i, @@ -246,11 +342,15 @@ function matchRules(result) { if (!text.trim()) return null; for (const rule of RULES) { - if (rule.pattern.test(text)) { + // `exec` rather than `test` so a rule whose sentence names what the error named + // has the capture groups to name it with. None of these patterns are global, so + // there is no `lastIndex` to reset between calls. + const match = rule.pattern.exec(text); + if (match) { return { reason: rule.reason, - summary: rule.summary, - fix: rule.fix, + summary: typeof rule.summary === 'function' ? rule.summary(match) : rule.summary, + fix: typeof rule.fix === 'function' ? rule.fix(match) : rule.fix, retryable: Boolean(rule.retryable), fixFirst: Boolean(rule.fixFirst), }; diff --git a/app/agent/todoList.js b/app/agent/todoList.js index c9480a6..a9a15a9 100644 --- a/app/agent/todoList.js +++ b/app/agent/todoList.js @@ -65,6 +65,19 @@ class TodoList { /** @type {TodoItem[]} */ this.items = items.slice(0, MAX_ITEMS).map((text) => ({ text: String(text).trim(), status: 'pending' })); if (this.items.length > 0) this.items[0].status = 'active'; + + /** + * Every change made to the list after it was planned, in order. + * + * The list is the one thing in a run that both the user and the model treat as + * settled — it is shown once, ticked off as it goes, and read back in the summary. + * So when it changes mid-run because the user was asked something, that has to be + * visible: a checklist that quietly grew an item reads afterwards as a model that + * did work nobody asked for. + * + * @type {Array<{kind: 'reworded' | 'dropped', detail: string, at: number}>} + */ + this.changes = []; } /** @returns {boolean} */ @@ -133,6 +146,58 @@ class TodoList { return this.items.filter((item) => wanted.includes(item.status)).map((item) => item.text); } + /** + * Restate the active item. + * + * The original text is kept in the change log rather than overwritten in place: the + * summary has to be able to say what the item *was*, or a user reading it afterwards + * cannot tell that their answer is why it succeeded. + * + * @param {string} text + * @returns {boolean} + */ + replaceCurrent(text) { + const active = this.current(); + const wanted = String(text || '').trim(); + if (!active || !wanted || wanted === active.text) return false; + + this.changes.push({ kind: 'reworded', detail: `"${active.text}" → "${wanted}"`, at: this.position() }); + logger.info(`Checklist item ${this.position()} reworded: ${active.text} → ${wanted}`); + active.text = wanted; + return true; + } + + /** + * Close the active item as skipped and move on, leaving the rest of the list alone. + * + * Distinct from `skipRemaining`, which gives up on everything. This is the user + * saying "not that one" — the run continues. + * + * @param {string} reason + * @returns {TodoItem | null} The next item. + */ + skipCurrent(reason) { + const active = this.current(); + if (!active) return null; + this.changes.push({ kind: 'dropped', detail: active.text, at: this.position() }); + return this.finishCurrent('skipped', reason); + } + + /** + * What changed about the list after it was planned, in plain language. + * + * @returns {string} Empty when the list ran as planned. + */ + describeChanges() { + if (this.changes.length === 0) return ''; + const lines = this.changes.map((change) => + change.kind === 'reworded' + ? `- Item ${change.at} reworded: ${change.detail}` + : `- Item ${change.at} dropped: ${change.detail}` + ); + return `The checklist changed while it ran:\n${lines.join('\n')}`; + } + /** Mark every remaining item as skipped — used when the session is cut short. */ skipRemaining(reason) { for (const item of this.items) { diff --git a/app/core/commonSense.js b/app/core/commonSense.js new file mode 100644 index 0000000..bf4ff50 --- /dev/null +++ b/app/core/commonSense.js @@ -0,0 +1,475 @@ +'use strict'; + +/** + * Reading a request the way a person would, before handing it to a 1B model. + * + * ## The failure this is about + * + * A request does not have to be well-formed to be obvious. "Update mian.js" in a + * project containing `main.js` is not ambiguous to a human being for even a moment. + * To a small model it is a path that does not resolve, and what happens next is the + * part that costs: it creates `mian.js`, empty or near enough, reports the update + * done, and the user now has two files where they had one. Observed shape, repeatedly + * — the model does not question the premise of a request, it executes it literally, + * and a literal reading of a typo is a new file. + * + * The same applies to a reference with nothing behind it. "Fix it" as the first + * message of a session names nothing; a model handed that will pick a file, and the + * file it picks is essentially arbitrary. + * + * ## The rule this follows + * + * **Work around it when the workaround is obvious; ask when it is a real choice.** + * That line is drawn by the evidence, not by confidence: + * + * - Exactly one near-match → repair it, say so in the summary. A user who wrote + * `mian.js` next to a `main.js` meant `main.js`, and stopping to ask would be + * pedantry with a dialog on it. + * - Two to four near-matches → ask, offering them. This is the case where guessing is + * a coin toss with the user's files as the stake. + * - No near-match → say nothing. The file may simply not exist yet, which is what + * "create" requests look like, and a check that fires on those would be worse than + * no check. + * + * ## Why this is patterns and not a model call + * + * Same reason as `intentRouter` and `factStore`: asking a 1B model whether a request + * makes sense costs an inference and returns the confidence of a model that could not + * answer the request either. Everything here is decided from the workspace listing and + * the text, so it is deterministic, free, and testable — and it can be wrong in only + * one direction, since a check that finds nothing leaves the request exactly as it was. + * + * @module core/commonSense + */ + +const clarification = require('../agent/clarification'); +const logger = require('../utils/logger'); + +/** + * How alike two filenames must be before one is treated as a typo of the other. + * + * Paired with `MAX_TYPO_DISTANCE` below, which is the constraint that actually does the + * work on short names: a ratio alone would rate `app.js` against `api.js` at 0.83 for + * the same single edit that separates `mian.js` from `main.js`, and those are not the + * same kind of guess. + */ +const TYPO_THRESHOLD = 0.75; + +/** + * The most edits a typo is allowed to be. + * + * Two, because a doubled and a dropped character in one filename is an ordinary slip + * and three is a different word. `memoryStore.similarity` is not used here despite + * doing a similar-sounding job: it is Jaccard over *whole words*, so it scores + * `mian.js` against `main.js` at zero — correct for the note de-duplication it was + * written for, useless for a misspelling. + */ +const MAX_TYPO_DISTANCE = 2; + +/** Beyond this many candidates the question is worse than the guess. */ +const MAX_CANDIDATES = 3; + +/** + * Verbs that presuppose the thing already exists. + * + * The distinction is the whole safety of this module. "Update mian.js" is a mistake; + * "create mian.js" is a filename, and it is not this module's business to correct a + * name the user chose. Only the first list is checked. + */ +const EXPECTS_EXISTING = /\b(update|edit|fix|change|modify|refactor|rename|read|open|delete|remove|check|review|revert|document|test)\b/i; + +/** + * Verbs that create, so a name the user is inventing is left alone. + * + * Tested against the words immediately before the filename rather than the whole + * message, which is a distinction with teeth: "update mian.js **to add** a header" is a + * typo in a request that happens to contain the word "add", and a whole-message test + * reads it as a creation and silently declines to help. The window is short for the + * same reason — "create the parser, then update mian.js" must not have its second + * clause excused by its first. + */ +const CREATES = /\b(create|add|make|write|new|generate|scaffold|build|init|implement|call(?:ed)?|name[ds]?)\b/i; + +/** How many words before a filename are read as its verb. */ +const VERB_WINDOW = 6; + +/** + * A stem shorter than this cannot be judged. `a.js` and `b.js` are one edit apart and + * have nothing to do with each other. + */ +const MIN_STEM_CHARS = 3; + +/** + * A token that looks like a path or a filename. + * + * Requires an extension, so ordinary prose survives: "update the parser" names no file + * and must not be read as one. Backticks and quotes are stripped by the caller. + * + * The nested quantifier is unambiguous — each repetition of the outer group must + * consume a separator, and `[\w.@-]` cannot match one — so a single match is linear. + * What is *not* linear is scanning with `/g` for matches that are not there: every + * start position in the string gets its own scan, which is O(n²) overall. Measured at + * 111 ms on 10,000 characters of `a/a/a/…` and 3,089 ms at 50,000. That is a paste + * into the composer rather than an attack, but three seconds of frozen extension host + * is a bug either way, which is what `MAX_SCANNED_CHARS` is for. + */ +const PATH_TOKEN = /(?:[\w.@-]+[/\\])*[\w.@-]+\.[a-z0-9]{1,6}\b/gi; + +/** + * How much of a request is scanned for filenames. + * + * A message longer than this is not a request naming a file the user mistyped; it is a + * paste. The same bounding discipline the TODO list uses on its items. + */ +const MAX_SCANNED_CHARS = 4000; + +/** + * Extensions worth treating as a file reference. A version number like `1.2` and a + * sentence ending in `.js` are told apart here rather than by cleverness. + */ +const SOURCE_EXTENSION = + /\.(jsx?|tsx?|mjs|cjs|json|css|s[ac]ss|html?|md|ya?ml|py|java|go|rs|rb|php|vue|svelte|txt|toml|ini|xml|sql|sh|bat|ps1)$/i; + +/** + * A message that refers to something without naming it. + * + * Anchored, and the pronoun has to be the object of the verb — "fix it" matches, + * "fix items in the list" does not. + * + * The caller trims before testing, and that is load-bearing rather than tidiness. With + * `^\s*` and `\s*$` around the body this had the classic ambiguous shape — two + * unbounded whitespace runs either side of an optional character — and a message + * ending in a long run of spaces could be split between them in quadratically many + * ways. Measured on the version with the anchors: 68 ms at 10,000 trailing spaces and + * **1,660 ms at 50,000**, against 0 ms for the trimmed form here. The optional groups + * that remain each begin with a distinct literal (`please`, `can`, `could`), so there + * is one way to match any candidate. + */ +const DANGLING_REFERENCE = /^(?:please\s+)?(?:can you\s+|could you\s+)?(fix|update|change|finish|redo|revert|delete|remove|continue|do)\s+(it|this|that|those|these|them|the thing|the file|that file|this file)[.!?]?$/i; + +/** + * @typedef {object} Interpretation + * @property {'ok' | 'repaired' | 'ask'} kind + * @property {string} [task] The rewritten request, when repaired. + * @property {string} [note] What was changed and why, for the summary and memory. + * @property {import('../agent/clarification').Clarification} [clarification] + */ + +/** + * Every filename-looking token in a message. + * + * @param {string} task + * @returns {string[]} Unique, in the order they appear. + */ +function referencedPaths(task) { + const text = String(task || '').slice(0, MAX_SCANNED_CHARS).replace(/[`'"]/g, ' '); + const found = text.match(PATH_TOKEN) || []; + + /** @type {string[]} */ + const unique = []; + for (const raw of found) { + const token = raw.replace(/\\/g, '/'); + if (!SOURCE_EXTENSION.test(token)) continue; + if (!unique.includes(token)) unique.push(token); + } + return unique; +} + +/** + * Does this path exist in the workspace listing? + * + * Compared case-insensitively and on the tail, so "update useTodos.js" matches + * `src/hooks/useTodos.js` — a user naming a file rarely types its full path, and + * treating that as a miss would fire this module on nearly every well-formed request. + * + * @param {string} candidate + * @param {string[]} files Workspace-relative paths. + * @returns {boolean} + */ +function existsIn(candidate, files) { + const wanted = candidate.toLowerCase(); + // A reference whose folders are wrong but whose *name* is right counts as existing. + // That is a different mistake from a typo and it needs no help: the model is given + // the workspace listing, so `src/hooks/useTodos.js` is one read away from + // `app/useTodos.js`. Treating it as a miss would hand it to the typo matcher, which + // would find the same file and "correct" a name that was never misspelled. + const wantedBase = wanted.split('/').pop(); + return files.some((file) => { + const path = file.toLowerCase().replace(/\\/g, '/'); + return path === wanted || path.endsWith(`/${wanted}`) || path.split('/').pop() === wantedBase; + }); +} + +/** + * Edit distance counting a transposition as one edit, not two. + * + * The transposition case is the reason this is not plain Levenshtein: `mian` for `main` + * is the single commonest way a filename gets mistyped, and Levenshtein charges it two + * edits — the same as `main` to `mail` plus another change. On a seven-character name + * that is the difference between a confident match and a miss. + * + * Bounded by construction: both inputs are basenames, and the caller has already + * rejected anything without a file extension. + * + * @param {string} a + * @param {string} b + * @returns {number} + */ +function editDistance(a, b) { + if (a === b) return 0; + if (!a.length) return b.length; + if (!b.length) return a.length; + + // Full matrix rather than two rolling rows: a transposition needs the row before + // last, and these are filenames — the matrix is tens of cells, not thousands. + // + // Every index below is a loop counter bounded by the length of one of the two + // strings, and every read is from the matrix this function just built. There is no + // key here that came from anywhere, which is what the rule is looking for. + /* eslint-disable security/detect-object-injection -- numeric loop indices into a local matrix */ + /** @type {number[][]} */ + const d = []; + for (let i = 0; i <= a.length; i += 1) d.push(new Array(b.length + 1).fill(0)); + for (let i = 0; i <= a.length; i += 1) d[i][0] = i; + for (let j = 0; j <= b.length; j += 1) d[0][j] = j; + + for (let i = 1; i <= a.length; i += 1) { + for (let j = 1; j <= b.length; j += 1) { + const cost = a.charAt(i - 1) === b.charAt(j - 1) ? 0 : 1; + let best = Math.min(d[i - 1][j] + 1, d[i][j - 1] + 1, d[i - 1][j - 1] + cost); + if (i > 1 && j > 1 && a.charAt(i - 1) === b.charAt(j - 2) && a.charAt(i - 2) === b.charAt(j - 1)) { + best = Math.min(best, d[i - 2][j - 2] + 1); + } + d[i][j] = best; + } + } + + return d[a.length][b.length]; + /* eslint-enable security/detect-object-injection */ +} + +/** + * How alike two filenames are, 0 to 1. + * + * @param {string} a + * @param {string} b + * @returns {number} + */ +function nameSimilarity(a, b) { + const longest = Math.max(a.length, b.length); + if (longest === 0) return 0; + return 1 - editDistance(a, b) / longest; +} + +/** + * Workspace files whose name is close enough to be a typo of this one. + * + * Compared on the basename only. A path that is wrong in its folders but right in its + * filename is the common case — `src/mian.js` for `app/main.js` — and comparing whole + * paths would score that on the folders, which are not what was misspelled. + * + * @param {string} candidate + * @param {string[]} files + * @returns {string[]} Best first, at most `MAX_CANDIDATES`. + */ +function nearMatches(candidate, files) { + const wanted = String(candidate).split('/').pop().toLowerCase(); + if (wanted.replace(SOURCE_EXTENSION, '').length < MIN_STEM_CHARS) return []; + + /** @type {Array<{file: string, score: number}>} */ + const scored = []; + for (const file of files) { + const base = file.replace(/\\/g, '/').split('/').pop().toLowerCase(); + if (base === wanted) continue; + + // An extension mismatch is a different file, not a typo: `main.js` and `main.css` + // score high on characters and are never the same mistake. + const wantedExt = (wanted.match(SOURCE_EXTENSION) || [''])[0]; + const baseExt = (base.match(SOURCE_EXTENSION) || [''])[0]; + if (wantedExt && baseExt && wantedExt.toLowerCase() !== baseExt.toLowerCase()) continue; + + if (editDistance(base, wanted) > MAX_TYPO_DISTANCE) continue; + const score = nameSimilarity(base, wanted); + if (score >= TYPO_THRESHOLD) scored.push({ file, score }); + } + + scored.sort((a, b) => b.score - a.score); + return scored.slice(0, MAX_CANDIDATES).map((entry) => entry.file); +} + +/** + * Is this filename one the request is asking to bring into existence? + * + * Reads the few words in front of the name, which is where the verb governing it sits + * in every phrasing that came up: "create X", "add a file called X", "write X". + * + * @param {string} task + * @param {string} referenced + * @returns {boolean} + */ +function isBeingCreated(task, referenced) { + const text = String(task || '').replace(/[`'"]/g, ' '); + const at = text.toLowerCase().indexOf(referenced.toLowerCase()); + if (at <= 0) return false; + + const before = text.slice(0, at).trim().split(/\s+/).slice(-VERB_WINDOW).join(' '); + return CREATES.test(before); +} + +/** + * Read a request against the workspace it is about. + * + * @param {object} input + * @param {string} input.task + * @param {string[]} [input.files] Workspace-relative paths. + * @param {Array<{role: string, text: string}>} [input.conversation] + * @param {string} [input.editorPath] The file open in the editor, if any. + * @param {boolean} [input.canAsk] False when nothing can show a question. + * @returns {Interpretation} + */ +function interpret(input) { + const task = String(input.task || ''); + const files = Array.isArray(input.files) ? input.files : []; + const canAsk = input.canAsk !== false; + + const dangling = danglingReference(task, input); + if (dangling) return dangling; + + // Nothing to check a path against. A session with no workspace listing is not + // evidence that a file is missing. + if (files.length === 0) return { kind: 'ok' }; + + // Nothing in the message presupposes an existing file, so a name that does not + // resolve is not yet evidence of anything. + if (!EXPECTS_EXISTING.test(task)) return { kind: 'ok' }; + + for (const referenced of referencedPaths(task)) { + if (existsIn(referenced, files)) continue; + // A name the user is inventing is theirs. Judged per path, not per message: a + // request can read one file and create another. + if (isBeingCreated(task, referenced)) continue; + + const candidates = nearMatches(referenced, files); + if (candidates.length === 0) continue; + + if (candidates.length === 1) { + const chosen = candidates[0]; + logger.info(`"${referenced}" is not in this workspace; reading it as "${chosen}".`); + return { + kind: 'repaired', + // Both names, not a silent substitution. The user has to be able to see that + // their request was altered and disagree with it — and the model needs the + // real path, since it is the one that will resolve. + task: task.replace(referenced, chosen), + note: `The request named "${referenced}", which does not exist here. Read as "${chosen}".`, + }; + } + + if (!canAsk) { + logger.info(`"${referenced}" matches ${candidates.length} files and nothing can ask; leaving it alone.`); + return { kind: 'ok' }; + } + + return { + kind: 'ask', + clarification: clarification.build({ + kind: 'ambiguous', + question: `There is no "${referenced}" in this project. Which file did you mean?`, + context: `${candidates.length} files have a similar name.`, + options: candidates.map((file, index) => ({ + id: `file-${index + 1}`, + label: file, + effect: 'instruct', + // The first is recommended because `nearMatches` sorts by closeness, so it + // is the most similar name rather than an arbitrary one. + recommended: index === 0, + guidance: `The user means ${file}. Work on that file, and do not create "${referenced}".`, + })), + }), + }; + } + + return { kind: 'ok' }; +} + +/** + * "Fix it" with nothing to attach "it" to. + * + * The editor is checked first because it is nearly always the answer: a user typing + * "fix it" is looking at the thing they mean. Only a request with no conversation, no + * open file, and nothing recently changed is genuinely unanswerable. + * + * @param {string} task + * @param {object} input + * @returns {Interpretation | null} + */ +function danglingReference(task, input) { + // Trimmed here so the pattern does not have to carry `\s*` anchors — see the note on + // DANGLING_REFERENCE for what those cost. + const match = DANGLING_REFERENCE.exec(String(task || '').trim()); + if (!match) return null; + + const conversation = Array.isArray(input.conversation) ? input.conversation : []; + // An earlier turn is an antecedent, and the context builder already carries it. This + // module has nothing to add to a message that is only short. + if (conversation.length > 0) return null; + + const verb = match[1].toLowerCase(); + + if (input.editorPath) { + logger.info(`"${task.trim()}" names nothing; taking it as the open file, ${input.editorPath}.`); + return { + kind: 'repaired', + task: `${task.trim()} — the file open in the editor, ${input.editorPath}`, + note: `The request did not say what to ${verb}. Took it to mean the open file, ${input.editorPath}.`, + }; + } + + if (input.canAsk === false) return { kind: 'ok' }; + + const files = Array.isArray(input.files) ? input.files : []; + // Offered rather than assumed. With no open file and no conversation there is no + // evidence at all, and picking one of these would be a guess wearing a decision's + // clothes. + const offer = files.slice(0, 2); + if (offer.length < 1) return { kind: 'ok' }; + + return { + kind: 'ask', + clarification: clarification.build({ + kind: 'ambiguous', + question: `What would you like me to ${verb}?`, + context: 'This is the first message in the session, so there is nothing for "it" to refer to yet.', + options: [ + ...offer.map((file, index) => ({ + id: `file-${index + 1}`, + label: file, + effect: /** @type {const} */ ('instruct'), + recommended: index === 0, + guidance: `The user means ${file}.`, + })), + { + id: 'describe', + label: 'Something else — I will say', + detail: 'Type what you want changed.', + effect: /** @type {const} */ ('instruct'), + }, + ], + }), + }; +} + +module.exports = { + interpret, + referencedPaths, + nearMatches, + existsIn, + danglingReference, + editDistance, + nameSimilarity, + TYPO_THRESHOLD, + MAX_TYPO_DISTANCE, + MAX_CANDIDATES, + EXPECTS_EXISTING, + CREATES, +}; diff --git a/app/core/environmentProfile.js b/app/core/environmentProfile.js index 7010c51..22b14db 100644 --- a/app/core/environmentProfile.js +++ b/app/core/environmentProfile.js @@ -44,6 +44,7 @@ const os = require('os'); const path = require('path'); const logger = require('../utils/logger'); +const pathGuard = require('../security/pathGuard'); const { platformName } = require('../utils/platform'); /** Where the profile is persisted, relative to the workspace root. */ @@ -198,9 +199,21 @@ function render(profile, opts = {}) { function persist(workspaceRoot, profile = detect()) { if (!workspaceRoot) return profile; - const file = path.join(workspaceRoot, ...PROFILE_PATH.split('/')); + // Routed through pathGuard for the symlink check, like every other write in the + // extension. The directory is created first so the realpath probe resolves the + // `.hirayacoder` link itself rather than the workspace root it would fall back to. + // See SAST-0.6.1 §2. + let file; + try { + const resolved = pathGuard.resolvePath(workspaceRoot, PROFILE_PATH); + fs.mkdirSync(path.dirname(resolved.absolute), { recursive: true }); + file = pathGuard.assertRealPathSync(resolved).absolute; + } catch (err) { + logger.warn(`Refusing to write ${PROFILE_PATH}: ${/** @type {Error} */ (err).message}`); + return profile; + } + try { - fs.mkdirSync(path.dirname(file), { recursive: true }); fs.writeFileSync(file, `${JSON.stringify(profile, null, 2)}\n`, 'utf8'); } catch (err) { logger.warn(`Could not write ${PROFILE_PATH}: ${/** @type {Error} */ (err).message}`); diff --git a/app/core/memoryStore.js b/app/core/memoryStore.js index 7d9d60f..fe93a66 100644 --- a/app/core/memoryStore.js +++ b/app/core/memoryStore.js @@ -577,17 +577,30 @@ class MemoryStore { /** @type {Set} */ const picked = new Set(); - // Related first, newest of them first, so a long history does not fill the window - // with the earliest thing it ever matched. - for (let index = this.entries.length - 1; index >= 0 && picked.size < limit; index -= 1) { + // How many of the subject's own tokens each entry shares. Counted rather than + // treated as a yes/no, because "any one token matches" puts a note that mentions + // `src` level with the note that names `src/hooks/useTodos.js` and the identifier + // inside it — and on a recall window of five, the weak match displacing the strong + // one is the whole failure this selector exists to prevent. + /** @type {Array<{index: number, score: number}>} */ + const scored = []; + for (let index = this.entries.length - 1; index >= 0; index -= 1) { // eslint-disable-next-line security/detect-object-injection -- numeric loop index. const words = recallTokens(this.entries[index]); + let score = 0; for (const word of words) { - if (wanted.has(word)) { - picked.add(index); - break; - } + if (wanted.has(word)) score += 1; } + if (score > 0) scored.push({ index, score }); + } + + // Strongest first; newest breaks a tie, so a long history does not fill the window + // with the earliest thing it ever matched. `scored` is already newest-first, and + // `sort` is stable, so the tie-break needs no comparator of its own. + scored.sort((a, b) => b.score - a.score); + for (const { index } of scored) { + if (picked.size >= limit) break; + picked.add(index); } // Then recency, for the remainder. diff --git a/app/core/workspaceBootstrap.js b/app/core/workspaceBootstrap.js index 6b5fc47..bf41f8a 100644 --- a/app/core/workspaceBootstrap.js +++ b/app/core/workspaceBootstrap.js @@ -42,9 +42,9 @@ */ const fs = require('fs'); -const path = require('path'); const logger = require('../utils/logger'); +const pathGuard = require('../security/pathGuard'); const environmentProfile = require('./environmentProfile'); /** The entry written into `.gitignore`. Trailing slash: it is always a directory. */ @@ -89,7 +89,17 @@ function alreadyIgnored(contents) { */ function ensureGitignore(workspaceRoot) { if (!workspaceRoot) return 'failed'; - const file = path.join(workspaceRoot, '.gitignore'); + + // Every other write in the extension resolves through pathGuard, which has a + // realpath stage precisely because a lexically-clean path can still be a symlink + // pointing outside the workspace. This one used to skip it. See SAST-0.6.1 §2. + let file; + try { + file = pathGuard.assertRealPathSync(pathGuard.resolvePath(workspaceRoot, '.gitignore')).absolute; + } catch (err) { + logger.warn(`Refusing to touch .gitignore: ${/** @type {Error} */ (err).message}`); + return 'failed'; + } try { if (!fs.existsSync(file)) { diff --git a/app/features/chatTab.js b/app/features/chatTab.js index b3615bc..2738e67 100644 --- a/app/features/chatTab.js +++ b/app/features/chatTab.js @@ -77,6 +77,16 @@ class ChatTab { */ this._detachedAt = null; + /** + * The question this tab is currently blocked on, if any. + * + * The run holding the other end of `resolve` cannot proceed until it is called, so + * every path that removes the card has to settle it — see `_cancelClarification`. + * + * @type {{id: string, resolve: (answer: object | null) => void} | null} + */ + this._pendingClarification = null; + /** @type {vscode.WebviewPanel | null} */ this.panel = null; /** @type {AgentSession | null} */ @@ -246,6 +256,8 @@ class ChatTab { return this._attachImage(); case 'detach': return this._detach(message); + case 'clarify': + return this._answerClarification(message); default: logger.warn(`Ignored unknown webview message: ${String(type)}`); return undefined; @@ -257,6 +269,84 @@ class ChatTab { if (this.panel) this.panel.webview.postMessage(message); } + /** + * Put a question to the user and wait for the card to come back answered. + * + * ## The two ways this must not hang + * + * A run is blocked while this promise is pending, so every way out of the panel has + * to resolve it. Closing the tab disposes the panel, which fires `onDidDispose` and + * settles this as cancelled (see `_retire`); pressing Stop aborts the session, which + * lands in the same place. Without both, a user who closes a tab mid-question leaves + * an agent session pinned on a promise nothing will ever resolve — holding its lane + * in the turn queue against every other tab. + * + * Only one question can be outstanding at a time, which is a property of the caller: + * `agentSession` asks and awaits inside a single loop step. A second arriving while + * one is pending is a bug, so the first is cancelled rather than silently dropped. + * + * @param {import('../agent/clarification').Clarification} request + * @returns {Promise} + * @private + */ + _askUser(request) { + if (!this.panel) return Promise.resolve(null); + + if (this._pendingClarification) { + logger.warn('A second question arrived while one was still open; cancelling the first.'); + this._pendingClarification.resolve({ id: this._pendingClarification.id, cancelled: true }); + } + + return new Promise((resolve) => { + this._pendingClarification = { id: request.id, resolve }; + this._post({ type: 'clarify', request }); + }); + } + + /** + * The user answered — or the card was dismissed. + * + * @param {object} message + * @private + */ + _answerClarification(message) { + const pending = this._pendingClarification; + if (!pending) { + logger.warn('An answer arrived with no question outstanding; ignoring it.'); + return; + } + // Ids are matched so a stale card from a previous turn cannot answer the current + // question. The webview is trusted to render, never to decide what it is replying to. + if (message.id && message.id !== pending.id) { + logger.warn('An answer arrived for a question that is no longer open; ignoring it.'); + return; + } + + this._pendingClarification = null; + pending.resolve({ + id: pending.id, + optionId: typeof message.optionId === 'string' ? message.optionId : undefined, + text: typeof message.text === 'string' ? message.text : undefined, + cancelled: message.cancelled === true, + }); + } + + /** + * Settle any outstanding question as cancelled. + * + * Called from every path that takes the panel away from the user mid-run. Safe to + * call when nothing is pending. + * + * @private + */ + _cancelClarification() { + if (!this._pendingClarification) return; + logger.info('A question was still open when the session ended; treating it as cancelled.'); + const pending = this._pendingClarification; + this._pendingClarification = null; + pending.resolve({ id: pending.id, cancelled: true }); + } + /** @private */ async _sendInit() { // Restored before the first paint, so a resumed session shows its conversation @@ -605,6 +695,10 @@ class ChatTab { environment: this.app.environment, stepSessions: this.stepSessions, images: images.map((image) => image.base64), + // How the session reaches the user when it is stuck or the request was + // ambiguous. Bound to this tab, so the question appears in the conversation it + // came from rather than in whichever tab happens to be focused. + onClarify: (request) => this._askUser(request), }); try { @@ -633,6 +727,9 @@ class ChatTab { logger.error(`Chat turn failed: ${message}`); this._post({ type: 'error', message }); } finally { + // A turn that threw, or finished while a question was somehow still open, must + // not leave the card up with nothing behind it. + this._cancelClarification(); this.session = null; this._starting = false; this._queueAbort = null; @@ -682,6 +779,19 @@ class ChatTab { type: 'status', text: `Retrying step ${event.index}: ${String(event.reason || 'it did not land').slice(0, 120)}`, }); + // The card itself is posted by `_askUser`, which needs the promise. This only + // moves the status line, so a run that is waiting on someone does not look like a + // run that has hung. + case 'clarification': + return this._post({ type: 'status', text: 'Waiting for your answer…' }); + case 'clarification-answered': + return this._post({ type: 'status', text: `You chose: ${String(event.label || '').slice(0, 80)}` }); + // How the request was read, when that differed from what was typed. Shown as it + // happens rather than only in the summary — a user watching the agent open + // `main.js` after they typed `mian.js` should not have to wait until the end to + // find out why. + case 'interpretation': + return this._post({ type: 'status', text: String(event.note || '').slice(0, 160) }); default: return undefined; } @@ -712,6 +822,11 @@ class ChatTab { if (panel && this.panel && panel !== this.panel) return; this.panel = null; + // Before anything else. The run below is allowed to continue in the background, and + // a background run blocked on a card that no longer exists would hold its lane in + // the turn queue forever — the one way this feature could wedge the whole extension. + this._cancelClarification(); + if (!this.session) { // Nothing has started. A closed tab must not keep the lane: without this, a tab // closed while queued would hold its place and every other tab would sit behind a @@ -736,6 +851,9 @@ class ChatTab { /** Stop whatever this tab is doing, queued or running. */ cancel() { + // A session waiting on an answer is not watching its abort signal — it is parked on + // a promise. Settling the question is what lets it reach the signal at all. + this._cancelClarification(); if (this._queueAbort) this._queueAbort.abort(); if (this.session) this.session.cancel(); } diff --git a/app/security/pathGuard.js b/app/security/pathGuard.js index dc39691..70a562c 100644 --- a/app/security/pathGuard.js +++ b/app/security/pathGuard.js @@ -261,6 +261,23 @@ async function assertRealPath(resolved, opts = {}) { // eslint-disable-next-line security/detect-non-literal-fs-filename const realRoot = await fs.promises.realpath(resolved.root).catch(() => resolved.root); + return verifyRealContainment(resolved, real, realRoot, platform); +} + +/** + * The containment half of the real-path check, shared by both twins. + * + * Split out so the sync and async forms cannot drift on the part that decides — the + * only thing they are allowed to differ on is how they reach the filesystem. + * + * @param {ResolvedPath} resolved + * @param {string} real + * @param {string} realRoot + * @param {string} platform + * @returns {ResolvedPath} + * @throws {PathGuardError} + */ +function verifyRealContainment(resolved, real, realRoot, platform) { if (!isInside(realRoot, real, platform)) { throw new PathGuardError( 'SYMLINK_ESCAPE', @@ -272,11 +289,69 @@ async function assertRealPath(resolved, opts = {}) { return resolved; } +/** + * Synchronous twin of {@link assertRealPath}, with identical rules. + * + * It exists for `core/workspaceBootstrap`, which runs during activation on a call path + * that is synchronous all the way up to `this.environment` being read immediately after. + * Making that path async to reach the guard would have been a wider change than the + * check it was there to add. + * + * The 0.6.1 SAST pass found `ensureGitignore` and `environmentProfile.persist` writing + * to `path.join(workspaceRoot, …)` directly — the only two writes in the extension that + * skipped this module. Reaching it needs a workspace whose `.gitignore` is already a + * symlink pointing somewhere the user did not intend, which is a strange thing to + * arrange for yourself, but the inconsistency was real and this closes it. + * + * @param {ResolvedPath} resolved + * @param {object} [opts] + * @param {string} [opts.platform] + * @returns {ResolvedPath} + * @throws {PathGuardError} + */ +function assertRealPathSync(resolved, opts = {}) { + const platform = opts.platform || os.platform(); + let probe = resolved.absolute; + let real = null; + + for (;;) { + try { + // Resolving an attacker-influenced path is precisely this function's purpose; + // the result is containment-checked against the workspace root just below. + // eslint-disable-next-line security/detect-non-literal-fs-filename + real = fs.realpathSync(probe); + break; + } catch (err) { + const code = /** @type {NodeJS.ErrnoException} */ (err).code; + if (code !== 'ENOENT') { + throw new PathGuardError('REALPATH_FAILED', `Could not verify "${resolved.relative}": ${/** @type {Error} */ (err).message}`, resolved.relative); + } + const parent = path.dirname(probe); + if (parent === probe) { + throw new PathGuardError('REALPATH_FAILED', `Could not verify "${resolved.relative}".`, resolved.relative); + } + probe = parent; + } + } + + let realRoot = resolved.root; + try { + // The workspace root comes from VS Code, never from model output. + // eslint-disable-next-line security/detect-non-literal-fs-filename + realRoot = fs.realpathSync(resolved.root); + } catch { + // Fall back to the lexical root, matching the async twin's `.catch()`. + } + + return verifyRealContainment(resolved, real, realRoot, platform); +} + module.exports = { PathGuardError, resolvePath, resolveForMutation, assertRealPath, + assertRealPathSync, isInside, matchProtectedPrefix, DEFAULT_PROTECTED_PREFIXES, diff --git a/app/security/scriptRunner.js b/app/security/scriptRunner.js index 56fdecd..bf27de1 100644 --- a/app/security/scriptRunner.js +++ b/app/security/scriptRunner.js @@ -63,6 +63,34 @@ const DEFAULT_ALLOWED_BINARIES = [ 'java', 'javac', ]; +/** + * Reason attached to every rule that fetches a package and executes it. + * + * Kept as one constant because the six spellings below are the same act, and a user + * reading the prompt should not have to work out that `npm x` and `pnpm dlx` are `npx`. + */ +const REMOTE_PACKAGE_REASON = 'this downloads and runs a package from the internet'; + +/** + * Does this subcommand name a package to go and fetch? + * + * `exec`, `x`, `dlx`, and `create` always do. `init` is the exception that earns the + * argument check: bare `npm init` writes a `package.json` into the current directory + * and touches no network at all, while `npm init vite` downloads `create-vite` and runs + * it. Confirming the first would be a click for nothing, which is how confirmation + * prompts get trained away. + * + * @param {string[]} args + * @returns {boolean} + */ +function fetchesRemotePackage(args) { + const subcommand = String(args[0] || '').toLowerCase(); + if (['exec', 'x', 'dlx', 'create'].includes(subcommand)) return true; + // An initializer is the first argument that is not a flag. + if (subcommand === 'init') return args.slice(1).some((a) => !a.startsWith('-')); + return false; +} + /** * Allow-listed programs whose *subcommands* still demand an explicit click, even * when auto-approve-scripts is on. Each rule is [binary, subcommand-matcher, why]. @@ -70,6 +98,36 @@ const DEFAULT_ALLOWED_BINARIES = [ * @type {Array<{binary: string, match: (args: string[]) => boolean, reason: string}>} */ const ALWAYS_CONFIRM = [ + { + // Every argument form of `npx` ends in "fetch a package and execute it", including + // the ones that hit the local `node_modules/.bin` first — the fallback is still a + // registry download. There is no subcommand to discriminate on, so the rule is the + // binary. + // + // This is the finding the 0.6.1 SAST pass carried forward: `npx` was on the default + // allow-list, `NON_INTERACTIVE_ENV` sets `npm_config_yes` which suppresses npx's own + // "Ok to proceed?", and nothing here asked. Under auto-approve that ran arbitrary + // remote code with no click, in an extension whose headline claim is that it works + // fully offline. Observed on a live 0.6.0 run, auto-approved and logged as routine. + binary: 'npx', + match: () => true, + reason: REMOTE_PACKAGE_REASON, + }, + { + binary: 'npm', + match: fetchesRemotePackage, + reason: REMOTE_PACKAGE_REASON, + }, + { + binary: 'yarn', + match: fetchesRemotePackage, + reason: REMOTE_PACKAGE_REASON, + }, + { + binary: 'pnpm', + match: fetchesRemotePackage, + reason: REMOTE_PACKAGE_REASON, + }, { binary: 'git', match: (args) => ['push', 'remote', 'clone', 'fetch', 'pull', 'submodule'].includes(args[0]), @@ -521,6 +579,7 @@ module.exports = { ScriptRunnerError, DEFAULT_ALLOWED_BINARIES, ALWAYS_CONFIRM, + fetchesRemotePackage, SHELL_METACHARACTERS, DEFAULT_TIMEOUT_MS, MAX_OUTPUT_CHARS, diff --git a/app/webview/components/clarificationCard.js b/app/webview/components/clarificationCard.js new file mode 100644 index 0000000..eb37f5c --- /dev/null +++ b/app/webview/components/clarificationCard.js @@ -0,0 +1,126 @@ +/** + * The card the agent shows when it stops to ask the user something. + * + * A run is *blocked* while this is on screen, which is what shapes the design. Every + * other component here reports on work that is happening; this one is the work not + * happening, so it has to be answerable at a glance: + * + * - The recommended option is first and marked, because the agent has the trace and + * the user has a card. Presenting four equal choices moves the decision without + * helping with it. + * - Every option says what happens next, not just what it is called. + * - There is always a way to type something else. The options are the agent's reading + * of a situation it has just demonstrated it does not fully understand. + * - Once answered the card stays, showing what was chosen. It is part of the record of + * the run — a summary that says "you told me to skip that" should have something to + * point at. + * + * Nothing here is built from a string of HTML: the question, the context and every + * label originate in the extension host but describe model output and file paths, and + * the rule in this folder is that model-adjacent text is data. See the frontend design + * notes, §6. + */ + +/** + * @param {object} request The clarification, as built by `agent/clarification`. + * @param {(answer: object) => void} onAnswer + * @returns {HTMLElement} + */ +export function renderClarification(request, onAnswer) { + const wrapper = document.createElement('div'); + wrapper.className = 'clarify'; + + const question = document.createElement('p'); + question.className = 'clarify-question'; + question.textContent = request.question; + wrapper.appendChild(question); + + if (request.context) { + const context = document.createElement('p'); + context.className = 'clarify-context'; + context.textContent = request.context; + wrapper.appendChild(context); + } + + const options = document.createElement('div'); + options.className = 'clarify-options'; + + /** Answering twice would resolve a promise the host has already moved past. */ + let answered = false; + + /** + * @param {object} answer + * @param {string} chosenLabel + */ + function answer(answer_, chosenLabel) { + if (answered) return; + answered = true; + onAnswer({ ...answer_, id: request.id }); + + // Replaced rather than disabled: a row of dead buttons reads as a card that failed, + // and this one succeeded. + const chosen = document.createElement('p'); + chosen.className = 'clarify-chosen'; + chosen.textContent = `You chose: ${chosenLabel}`; + options.replaceWith(chosen); + if (freeText.isConnected) freeText.remove(); + } + + for (const option of request.options || []) { + const button = document.createElement('button'); + button.type = 'button'; + // The recommended option gets the send button's treatment — the one accent already + // used for "this is the action". A fifth accent would stop it meaning anything. + button.className = option.recommended ? 'send clarify-option' : 'control clarify-option'; + + const label = document.createElement('span'); + label.className = 'clarify-label'; + label.textContent = option.recommended ? `${option.label} — recommended` : option.label; + button.appendChild(label); + + if (option.detail) { + const detail = document.createElement('span'); + detail.className = 'clarify-detail'; + detail.textContent = option.detail; + button.appendChild(detail); + } + + button.addEventListener('click', () => answer({ optionId: option.id }, option.label)); + options.appendChild(button); + } + + wrapper.appendChild(options); + + const freeText = document.createElement('div'); + freeText.className = 'clarify-free'; + + const input = document.createElement('input'); + input.type = 'text'; + input.className = 'control clarify-input'; + input.placeholder = 'Or tell me what to do instead…'; + input.setAttribute('aria-label', 'Answer in your own words'); + + const submit = document.createElement('button'); + submit.type = 'button'; + submit.className = 'control'; + submit.textContent = 'Send'; + + function sendTyped() { + const text = input.value.trim(); + if (text) answer({ text }, text); + } + + submit.addEventListener('click', sendTyped); + input.addEventListener('keydown', (event) => { + if (event.key === 'Enter') { + event.preventDefault(); + sendTyped(); + } + }); + + freeText.appendChild(input); + freeText.appendChild(submit); + wrapper.appendChild(freeText); + + return wrapper; +} diff --git a/app/webview/main.js b/app/webview/main.js index 0d16ea1..5a08bcf 100644 --- a/app/webview/main.js +++ b/app/webview/main.js @@ -10,6 +10,7 @@ import { createMessage, appendImages, TraceView, renderTodos, renderChanges } from './components/messageBubble.js'; import { ThinkingIndicator } from './components/thinkingIndicator.js'; import { renderPlanChecklist } from './components/planChecklist.js'; +import { renderClarification } from './components/clarificationCard.js'; import { render } from './components/markdown.js'; const vscode = acquireVsCodeApi(); @@ -241,6 +242,19 @@ const handlers = { else state.body.appendChild(fresh); }, + // The agent has stopped and is waiting on an answer. The card goes into the running + // message rather than below it, so it sits with the work it is about — and it stays + // there once answered, as part of the record of the run. + clarify(msg) { + if (!state.body || !msg.request) return; + state.body.appendChild( + renderClarification(msg.request, (answer) => { + vscode.postMessage({ type: 'clarify', ...answer }); + }) + ); + scrollToEnd(); + }, + action(msg) { if (!state.trace) return; state.trace.addAction(msg.step, msg.action); diff --git a/app/webview/style.css b/app/webview/style.css index 5f2eb96..eec0b02 100644 --- a/app/webview/style.css +++ b/app/webview/style.css @@ -671,3 +671,84 @@ select.control { clip: rect(0 0 0 0); white-space: nowrap; } + +/* + The agent asking the user something. + + Bordered and inset rather than accented: the run is stopped while this is up, and + the thing that needs to catch the eye is the *recommended option* inside it, not the + container. Giving the card the sunrise as well would put two competing marks in one + small area and leave neither meaning anything. +*/ +.clarify { + margin: var(--sp-3) 0 0; + padding: var(--sp-3); + border: 1px solid var(--border); + border-radius: var(--radius-lg); + background: var(--surface); +} + +.clarify-question { + margin: 0; + font-weight: 600; +} + +.clarify-context { + margin: var(--sp-1) 0 0; + font-size: var(--fs-sm); + color: var(--muted); +} + +.clarify-options { + display: flex; + flex-direction: column; + gap: var(--sp-2); + margin-top: var(--sp-3); +} + +/* + Stacked, full width, and tall enough for two lines. A row of side-by-side buttons + truncates the labels — which are file paths often enough that the truncated part is + the part that distinguishes them. +*/ +.clarify-option { + display: flex; + flex-direction: column; + align-items: flex-start; + gap: 2px; + width: 100%; + height: auto; + padding: var(--sp-2) var(--sp-3); + text-align: left; + white-space: normal; +} + +.clarify-label { + font-weight: 600; +} + +/* + Inherits colour rather than using --muted, because on the recommended option this + detail sits on the sunrise gradient where the muted grey is unreadable. +*/ +.clarify-detail { + font-size: var(--fs-xs); + opacity: 0.85; +} + +.clarify-free { + display: flex; + gap: var(--sp-2); + margin-top: var(--sp-3); +} + +.clarify-input { + flex: 1; + min-width: 0; +} + +.clarify-chosen { + margin: var(--sp-3) 0 0; + font-size: var(--fs-sm); + color: var(--muted); +} diff --git a/doc/ARCHITECTURE.md b/doc/ARCHITECTURE.md index e41c415..98b8252 100644 --- a/doc/ARCHITECTURE.md +++ b/doc/ARCHITECTURE.md @@ -1,7 +1,7 @@ # HirayaCoder — Architecture -How the extension is put together and, more usefully, *why*. Roughly 13,000 lines of -JavaScript across 46 modules in `app/`, with **no production dependencies** — the +How the extension is put together and, more usefully, *why*. Roughly 22,000 lines of +JavaScript across 73 modules in `app/`, with **no production dependencies** — the `dependencies` field in `package.json` is empty and the packaged `.vsix` contains no `node_modules` at all. @@ -99,6 +99,7 @@ identical traffic regardless of tier. Swapping loops changes how an action is | `modelDiscovery.js` | Normalises `/api/tags`; falls back to `/api/show` only for incomplete entries. Caches by name+digest. | | `modelCapability.js` | Tier rule, budget matrix, `canPlanTodos`. Indexes budgets through a `Map` so a settings-supplied key cannot reach a prototype member. | | `intentRouter.js` | Decides, before routing, whether a message is conversation or work. Patterns only — no model call. `task` is the default and `chat` needs positive evidence, because a misread greeting costs one loop while a misread request is silently dropped. Consulted in Agent mode only. | +| `commonSense.js` | Reads a request against the workspace before the model sees it. One near-match to a filename that does not exist is repaired with both names stated; two to four are asked about; none is left alone, because that is what creating a file looks like. Patterns and edit distance — no model call, for the same reason as `intentRouter`. | | `promptRouter.js` | Assembles the system prompt and the tool set for the current mode. Plan and Ask modes **omit** the mutating tools rather than refusing them later, and a conversational message in Agent mode gets the same treatment: no loop, no tools, one reply. | | `completionCheck.js` | Consulted when a loop is told the work is finished. Sends the model back **once** if nothing changed on a request that asked for a change, or if a file it just wrote still has an unimplemented function body. | | `contextBuilder.js` | Assembles the prompt under a token budget, by priority. Redacts on the way in. | @@ -123,6 +124,25 @@ its own item, what the earlier steps actually wrote, the files it names — and decides what a step has *demonstrably done*, by comparing the change set against the step's own text. Neither consults the model's account of itself. +`scriptDiagnosis.js`, `errorRecovery.js`, and `clarification.js` are what happens when a +step does not work, and they are three modules rather than one because they answer three +different questions. The diagnosis says *what this failure is* and is pure pattern +matching over what a program printed. Recovery says *whether the model is learning from +it*, by comparing failures across a turn on a normalised signature — line numbers and +absolute paths removed, so the same problem is recognised across attempts and machines. +Clarification says *what the user is asked* once the answer to the second question is no. + +The ladder is short on purpose: guidance on the first failure, the user on the second. +Not a round number — `reactLoop.REPEAT_LIMIT` is 2, so the loop ends a run once the model +has sent the same action twice, and a ladder that waited for a third would never reach +its top rung. The second failure is the last moment at which asking can change anything. + +The rule that keeps this from becoming an agent that interviews its user: **the user is +asked last, and only about something that has already failed with targeted guidance in +between.** A session constructed without an `onClarify` handler — a benchmark, a detached +tab — never asks at all and tops out at guidance, which is why no run can block on a +question nobody can see. + ### `app/security/` — the part that is allowed to say no `permissionGate.js` (chokepoint) · `permissionModes.js` (two independent toggles) · @@ -191,8 +211,8 @@ because a session with nothing on disk yet still owns its number. | Suite | Command | What it proves | |---|---|---| -| Unit (573) | `npm run test:unit` | Logic, guards, parsing, permission decisions — against a stubbed `vscode`. | -| Integration (12) | `npm run test:integration` | Activation, command registration, webview protocol, and a full turn to disk **in a real VS Code**, against a stub Ollama on loopback. | +| Unit (1323) | `npm run test:unit` | Logic, guards, parsing, permission decisions — against a stubbed `vscode`. | +| Integration (16) | `npm run test:integration` | Activation, command registration, webview protocol, and a full turn to disk **in a real VS Code**, against a stub Ollama on loopback. | | Live benchmark | `node tools/bench-agent.js ` | What a real model does to a real workspace. | | Build benchmark | `node tools/bench-build.js --machine ` | Whether a model can build a project from an empty folder — add, read, run, and modify — in Java, JavaScript, and Python. Results land in `benchmarks/results//`. | diff --git a/docs/images/capabilities.png b/docs/images/capabilities.png index 697cc19..b283e57 100644 Binary files a/docs/images/capabilities.png and b/docs/images/capabilities.png differ diff --git a/docs/images/hero-offline-agent.png b/docs/images/hero-offline-agent.png index 2010398..8cc6b28 100644 Binary files a/docs/images/hero-offline-agent.png and b/docs/images/hero-offline-agent.png differ diff --git a/docs/images/src/README.md b/docs/images/src/README.md index d42f10a..3b6ba6f 100644 --- a/docs/images/src/README.md +++ b/docs/images/src/README.md @@ -32,9 +32,12 @@ and no screenshot is written. ## When these need updating -- **The version badge**, on every release. It appears once per file, as `v0.6.1`. -- **The "New in …" line** on the *Knows your machine* card, whenever a release changes - what that card is claiming. +- **The version badge**, on every release. It appears once per file, as `v0.7.0`. +- **The "New in …" line**, whenever a release changes what a card is claiming. It moves + to whichever card the release actually changed rather than staying put — it sat on + *Knows your machine* for 0.6.1 and moved to *Big requests become a checklist* for + 0.7.0, which is the card that stopped being true as written. Only ever one card + carries it; two "New in" tags read as a changelog rather than as a highlight. - **The mock chat transcript** in the hero, if the panel's real layout changes enough that the picture stops being an honest one. It is a mock, not a screenshot — but it should never show something the extension does not do. diff --git a/docs/images/src/capabilities.html b/docs/images/src/capabilities.html index c374249..683f3e7 100644 --- a/docs/images/src/capabilities.html +++ b/docs/images/src/capabilities.html @@ -87,7 +87,7 @@
HirayaCoder
-
v0.6.1
+
v0.7.0
for Visual Studio Code  ·  powered by Ollama
@@ -119,13 +119,13 @@

Agentic on every model

Big requests become a checklist

-

Multi-part work is split into steps, run one at a time, and each step is verified against what it actually wrote before it can close.

+

New in 0.7.0. Steps run one at a time, verified against what they actually wrote — and when one is genuinely stuck, it stops and asks you instead of guessing.

Knows your machine

-

New in 0.6.1. OS, toolchain, and shell rules are detected and stated up front — so the agent never guesses mkdir -p vs md.

+

OS, toolchain, and shell rules are detected and stated up front — so the agent never guesses mkdir -p vs md.

diff --git a/docs/images/src/hero-offline-agent.html b/docs/images/src/hero-offline-agent.html index 5325f64..08557bc 100644 --- a/docs/images/src/hero-offline-agent.html +++ b/docs/images/src/hero-offline-agent.html @@ -152,7 +152,7 @@
HirayaCoder
-
v0.6.1
+
v0.7.0

Your AI pair programmer,
fully offline.

diff --git a/package-lock.json b/package-lock.json index 792011c..210bf0a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "hirayacoder", - "version": "0.6.1", + "version": "0.7.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "hirayacoder", - "version": "0.6.1", + "version": "0.7.0", "license": "MIT", "devDependencies": { "@types/mocha": "^10.0.10", @@ -17,7 +17,7 @@ "eslint": "^9.34.0", "eslint-plugin-no-unsanitized": "^4.1.2", "eslint-plugin-security": "^3.0.1", - "mocha": "^11.7.1", + "mocha": "^11.8.0", "sinon": "^21.0.0" }, "engines": { @@ -2006,9 +2006,9 @@ } }, "node_modules/diff": { - "version": "7.0.0", - "resolved": "https://registry.npmjs.org/diff/-/diff-7.0.0.tgz", - "integrity": "sha512-PJWHUb1RFevKCwaFA9RlG5tCd+FO5iRh9A8HEtkmBH2Li03iJriB6m6JIN4rGz3K3JLawI7/veA1xzRKP6ISBw==", + "version": "9.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-9.0.0.tgz", + "integrity": "sha512-svtcdpS8CgJyqAjEQIXdb3OjhFVVYjzGAPO8WGCmRbrml64SPw/jJD4GoE98aR7r25A0XcgrK3F02yw9R/vhQw==", "dev": true, "license": "BSD-3-Clause", "engines": { @@ -4630,16 +4630,6 @@ ], "license": "MIT" }, - "node_modules/randombytes": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/randombytes/-/randombytes-2.1.0.tgz", - "integrity": "sha512-vYl3iOX+4CKUWuxGi9Ukhie6fsqXqS9FE2Zaic4tNFD2N2QQaXOMFbuKK4QmDHC0JO6B1Zp41J0LpT0oR68amQ==", - "dev": true, - "license": "MIT", - "dependencies": { - "safe-buffer": "^5.1.0" - } - }, "node_modules/rc": { "version": "1.2.8", "resolved": "https://registry.npmjs.org/rc/-/rc-1.2.8.tgz", @@ -4953,13 +4943,13 @@ } }, "node_modules/serialize-javascript": { - "version": "6.0.2", - "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-6.0.2.tgz", - "integrity": "sha512-Saa1xPByTTq2gdeFZYLLo+RFE35NHZkAbqZeWNd3BpzppeVisAqpDjcp8dyf6uIvEqJRd46jemmyA4iFIeVk8g==", + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-7.1.0.tgz", + "integrity": "sha512-RNEqWOyhhUQYN9V1GfHwu9AR/g+NTciH6Z5u3/no6X3/w+04J2lVDL+svFQVXgXrEGBMG2puMVN3gq2SNGuTGw==", "dev": true, "license": "BSD-3-Clause", - "dependencies": { - "randombytes": "^2.1.0" + "engines": { + "node": ">=20.0.0" } }, "node_modules/setimmediate": { @@ -5147,16 +5137,6 @@ "url": "https://opencollective.com/sinon" } }, - "node_modules/sinon/node_modules/diff": { - "version": "8.0.4", - "resolved": "https://registry.npmjs.org/diff/-/diff-8.0.4.tgz", - "integrity": "sha512-DPi0FmjiSU5EvQV0++GFDOJ9ASQUVFh5kD+OzOnYdi7n3Wpm9hWWGfB/O2blfHcMVTL5WkQXSnRiK9makhrcnw==", - "dev": true, - "license": "BSD-3-Clause", - "engines": { - "node": ">=0.3.1" - } - }, "node_modules/slash": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/slash/-/slash-5.1.0.tgz", diff --git a/package.json b/package.json index 2c8b6fb..7e15e77 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "hirayacoder", "displayName": "HirayaCoder", "description": "A fully offline, privacy-first AI coding agent powered by your local Ollama instance. Agentic on every model — even 1B.", - "version": "0.6.1", + "version": "0.7.0", "publisher": "jaymar921", "author": { "name": "jaymar921", @@ -424,8 +424,12 @@ "eslint": "^9.34.0", "eslint-plugin-no-unsanitized": "^4.1.2", "eslint-plugin-security": "^3.0.1", - "mocha": "^11.7.1", + "mocha": "^11.8.0", "sinon": "^21.0.0" }, + "overrides": { + "diff": "^9.0.0", + "serialize-javascript": "^7.1.0" + }, "dependencies": {} } diff --git a/security/sast-report-2026-08-14-0.7.0.md b/security/sast-report-2026-08-14-0.7.0.md new file mode 100644 index 0000000..13897e6 --- /dev/null +++ b/security/sast-report-2026-08-14-0.7.0.md @@ -0,0 +1,171 @@ +# HirayaCoder — SAST Report + +**Date:** 2026-08-14 +**Commit / version:** `0.7.0`, branch `feat/0.7.0` +**Performed by:** 0.7.0 hardening pass +**Platform:** Windows 11 Pro 26100, Node v24.11.1, Python 3.14.7 + +Supersedes `doc/SAST-0.6.1.md`, whose three deferred findings are all closed here. + +## 1. Tooling Run + +| Tool | Command | Scope | Result | +|---|---|---|---| +| ESLint (security rules) | `npm run lint` | `/app`, `/test` | **0 errors, 25 warnings** (all reviewed, §4) | +| npm audit (production) | `npm audit --omit=dev` | shipped tree | **0 vulnerabilities** | +| npm audit (full) | `npm audit` | incl. dev tree | **0 vulnerabilities** — first pass where this is true | +| Semgrep 1.172.0 | `semgrep --config p/javascript --config p/security-audit --metrics=off app` | `/app` | 2 findings, same rule and line (SAST-004, pre-existing) | +| retire.js | — | — | **Not run this pass**; see §6 | +| Manual review | checklist in §5 | `/app`, `/security` | complete | +| Unit suite | `npm run test:unit` | `/app` | 1323 passing | +| Integration suite | `npm run test:integration` | real VS Code host | 16 passing | + +The production dependency tree is still empty (`"dependencies": {}`), which is what makes +the `--omit=dev` result meaningful rather than lucky. + +**Semgrep coverage note.** The scan is limited to files tracked by git, which silently +skips anything newly added. All five modules added in 0.7.0 were committed before the +scan and are in the 77 targets (0.1.0 scanned 50). This was checked rather than assumed — +it is the trap recorded at the end of the 0.1.0 report. + +## 2. Findings Summary + +| Severity | Count | Resolved | Accepted Risk (with justification) | +|---|---|---|---| +| Critical | 0 | 0 | 0 | +| High | 2 | **2** | 0 | +| Medium | 2 | **2** | 0 | +| Low | 1 | **1** | 0 | +| Info | 2 | 0 | 2 — reviewed, §4 and SAST-009 | + +Every finding carried forward from 0.6.1 is closed. Two new findings were opened and +closed within this pass (SAST-007, SAST-008), both found by ESLint warnings that the +previous two passes had reviewed and dismissed as a class — see §4 for why that class +deserved re-examination rather than a third dismissal. + +## 3. Findings Detail + +| ID | Tool | File:Line | Description | Severity | Status | Resolution / Justification | +|---|---|---|---|---|---|---| +| SAST-005 | Manual (0.6.1 §1) | `app/security/scriptRunner.js:72` | `npx` was on the default allow-list and absent from `ALWAYS_CONFIRM`, while `NON_INTERACTIVE_ENV` sets `npm_config_yes` — suppressing npx's own *"Ok to proceed?"*. Under auto-approve-scripts this fetched and executed an arbitrary npm package with no confirmation, in an extension whose headline claim is that it works fully offline. Not theoretical: `.ignore/3.todo-app-0.6.0/.hirayacoder/audit.log:7` records `npx create-vite@latest …` auto-approved on a live run. | **High** | **Resolved** | `ALWAYS_CONFIRM` now covers all six spellings — `npx` (any arguments), and `exec`/`x`/`create` under `npm`, `yarn`, `pnpm` — with the reason *"this downloads and runs a package from the internet"*. Bare `npm init` is deliberately excluded: it writes a manifest and touches no network, and a click for nothing is a click trained away. `init` **with** an initializer (`npm init vite`) does confirm. Nine-command test pins every spelling. | +| SAST-006 | Manual (0.6.1 §2) | `app/core/workspaceBootstrap.js:90`, `app/core/environmentProfile.js:199` | `ensureGitignore` and `environmentProfile.persist` wrote to `path.join(workspaceRoot, …)` directly — the only two writes in the extension that bypassed `pathGuard` and therefore its `realpath` symlink check. A workspace whose `.gitignore` is a symlink pointing outside the tree would have been appended to. | Low | **Resolved** | Both now resolve through `pathGuard.resolvePath` + `assertRealPathSync`, and skip the write with a logged warning on refusal. The guard gained a synchronous twin because activation reads the profile immediately and making that path async would have been a wider change than the check it was adding; the containment decision is shared between the two forms so they cannot drift. Symlink and directory-junction cases tested against both twins on every platform. | +| SAST-002 | npm audit | `node_modules/mocha` → `diff`, `serialize-javascript` | jsdiff DoS (low); serialize-javascript RCE via `RegExp.flags` and CPU-exhaustion DoS (high); mocha transitively (moderate). Open and accepted since the 0.1.0 pass. | High / Medium | **Resolved** | `overrides` pinning `diff@^9.0.0` and `serialize-javascript@^7.1.0`, with mocha at 11.8.0. This is what actually clears the advisories: upstream mocha still declares `diff@^7` and `serialize-javascript@^6`, so neither range reaches a patched release, and `npm audit fix --force` proposed mocha@11.3.0 — a *sideways* move that fixes neither. Both are two majors and one major ahead of what mocha asks for, so the suite was verified three ways: serially (1323 passing), in `--parallel` mode (the `serialize-javascript` path, 1323 passing), and against a deliberately failing assertion to confirm the `diff`-backed reporter still renders. `npm audit` now reports **0 across the full tree**. | +| SAST-007 | ESLint `security/detect-unsafe-regex` | `app/core/commonSense.js` (`DANGLING_REFERENCE`) | Pattern anchored with `^\s*` … `\s*$` around an optional character — two unbounded whitespace runs either side of `[.!?]?`, which is the classic ambiguous shape. A message ending in a long whitespace run could be split between them in quadratically many ways. | Medium | **Resolved** | **Measured before and after, not argued.** 1 ms at 1,000 trailing spaces, 68 ms at 10,000, **1,660 ms at 50,000**. The caller now trims and both anchors are gone: **0 ms at 50,000**. The optional groups that remain each begin with a distinct literal (`please`, `can`, `could`), so any candidate has one way to match. Pinned by a test with a 250 ms budget. | +| SAST-008 | ESLint `security/detect-unsafe-regex` | `app/core/commonSense.js` (`PATH_TOKEN`) | A single match is linear — the nested quantifier's outer group must consume a separator and its character class cannot match one — but the pattern is scanned with `/g`, so a string containing no match gets one scan per start position: O(n²) overall. | Medium | **Resolved** | **3,089 ms on 50,000 characters** of `a/a/a/…`. Input to `referencedPaths` is now bounded to 4,000 characters, the same discipline `todoList` applies to its items. A message longer than that is a paste, not a request naming a mistyped file. Pinned by a test with a 250 ms budget. | +| SAST-004 | Semgrep `javascript.lang.security.detect-child-process` | `app/security/scriptRunner.js:451` | `spawn()` reached from the function arguments `command` and `options` — flagged twice, once per tainted argument. Impact HIGH, **confidence LOW**. Unchanged from 0.1.0 apart from the line number. | High (rule) → Info | **Accepted** | A true positive by pattern and the intended design: this is the only `child_process` call in the project, and the module exists to make it safe. `spawn` receives an argument array with `shell: false`; `argv[0]` must match an allow-list; shell metacharacters are rejected at tokenize time; Windows `.cmd` shims have every argument pre-screened; and the caller must already hold a `permissionGate` approval. 0.7.0 **narrows** this further rather than widening it — see SAST-005. The rule's confidence is LOW precisely because it cannot see that chain. | +| SAST-009 | Manual | `app/features/chatTab.js:_answerClarification` | New in 0.7.0: the webview can now post a `clarify` message whose free text is appended to the model's next prompt and written to session memory. A compromised webview could therefore inject text into a running turn. | Info | **Accepted** | Same trust level as the composer, which has always been able to send arbitrary text to the model — this adds no capability the webview did not have. Three properties bound it: answers are matched by clarification id, so a stale card from an earlier turn cannot answer the current question; an answer arriving with nothing outstanding is logged and dropped; and text written to memory goes through `memoryStore.append` → `normalizeEntry` → `neutralize`, so injection delimiters cannot survive into a later session's prompt. The webview still names no path and opens no file — the boundary the architecture rests on is unchanged. | + +## 4. On the warning class this pass stopped dismissing + +The 0.1.0 and 0.6.1 passes both reviewed `security/detect-unsafe-regex` warnings +individually and accepted all of them, each time with a written argument about why the +pattern was unambiguous. Those arguments were correct for the patterns they were about. + +The failure mode is that "reviewed and dismissed" becomes the default answer for a rule, +and 0.7.0 added two patterns where it would have been wrong. **SAST-007 and SAST-008 were +both real, and both were found by warnings of exactly the class the previous passes had +learned to wave through.** Neither is an attack — the input is the user's own composer, +on their own machine — but a three-second freeze of the extension host is a defect +regardless of who caused it. + +What changed procedurally: a flagged pattern is now **measured on adversarial input** +before it is accepted, and the measurement is recorded in the code beside the pattern. +Arguing from the shape of a regex is how both of these would have been dismissed again. + +The remaining 25 warnings break down as: + +- **`detect-unsafe-regex` (13).** Eleven pre-existing, each `^`-anchored with every + optional group beginning with a literal or a disjoint character class. The two in + `commonSense` are SAST-007 and SAST-008, now bounded, with their measurements written + in place; the linter still flags the shapes, which is why the numbers are in the source + rather than only in this report. +- **`detect-non-literal-regexp` (9).** `projectOverview` and `ignoreRules` build patterns + from workspace filenames, bounded by filename length; the rest are in tests. +- **`detect-object-injection` (3).** All index a fixed, code-owned map or array. The + Damerau-Levenshtein matrix added in 0.7.0 is disabled explicitly with the reason + written in place: every index is a loop counter bounded by one of the two string + lengths, and the matrix is local to the function. + +## 5. Manual Review Checklist + +- [x] **Command injection** — `child_process` is imported in exactly one file + (`security/scriptRunner.js:36`, `spawn` only), confirmed by grep across `/app`. + Unchanged this pass except that the set of commands reaching it without a + confirmation click is now **smaller** (SAST-005). +- [x] **Path traversal** — every mutation resolves through `pathGuard` plus + `assertRealPath`. As of this pass that is now *every* write without exception: + the two that bypassed it are closed (SAST-006). The new synchronous twin shares + its containment check with the async form, and both are asserted to agree on the + same inputs, including a `.gitignore` symlink and a `.hirayacoder` junction. +- [x] **SSRF / non-loopback egress** — `assertLoopbackEndpoint` throws at client + construction, before any socket. Verified in the integration suite against a real + host. Nothing in 0.7.0 opens a socket; the clarification channel is + `postMessage` between the webview and the extension host. +- [x] **Insecure deserialization** — no `eval`, no `new Function` anywhere in `/app`, + re-confirmed by grep. Model output is `JSON.parse`d inside try/catch and then + schema-validated. +- [x] **Webview CSP** — unchanged: `default-src 'none'` with a per-load script nonce and + no remote origins. The new `components/clarificationCard.js` builds every node with + `createElement` + `textContent`; the only `innerHTML` string in `/app` is the + comment in `markdown.js` saying there must never be one. +- [x] **Prototype pollution** — `outputParser` refuses `__proto__`, `constructor`, and + `prototype` and copies into `Object.create(null)`. The new clarification path adds + no object built from model output: option ids are generated by + `clarification.build`, and an answer naming an unknown id resolves to *stop* rather + than to a lookup. +- [x] **Secrets handling** — `redact()` coverage unchanged. Checked specifically for + 0.7.0: `errorRecovery` quotes a failure's first meaningful line into a question and + into memory, and that text has already passed through `runScript`'s `redact()` + before it reaches the observation this reads from. +- [x] **Permission gate coverage** — every mutating tool still calls the gate. The + clarification path grants nothing: its effects are *skip*, *stop*, and *instruct*, + and `instruct` adds text to a prompt. A user answering a question cannot approve a + write, and an answer cannot reach `scriptRunner` at all. +- [x] **Denial of service through a blocked run** — new this pass, because the feature is + new. A run awaiting an answer is parked on a promise and holds its lane in the turn + queue. Every path that removes the card settles it: panel dispose, `cancel()`, and + the turn's `finally`. A session constructed without `onClarify` never asks at all — + the escalation ladder tops out at guidance — which is the configuration benchmarks + and detached runs use. Both directions are tested. +- [x] **Dependency review** — production tree empty; every `devDependency` is a test, + lint, or packaging tool, none bundled into the `.vsix`. The `overrides` added for + SAST-002 affect only the dev tree. + +## 6. Sign-off + +- [x] All Critical/High findings resolved. **For the first time, none are accepted with + a standing risk**: SAST-002 is fixed rather than deferred, SAST-005 is closed, and + SAST-004 is downgraded to Info as the deliberate single `child_process` call behind + the allow-list, the metacharacter screen, and the permission gate. +- [x] Manual checklist reviewed with evidence recorded above rather than asserted. +- [ ] **retire.js was not run this pass.** It is not installed, and fetching it would mean + `npx retire` — downloading and executing a package from the registry, which is the + exact behaviour SAST-005 exists to require a decision about. Running it silently + inside the pass that closed that finding would have been the wrong way to close it. + Its value here is also low: it scans dependencies, the production tree is empty, and + `npm audit` reports zero across the full tree. **To run it deliberately:** + `npm i -D retire && npx retire --path app`. + +**Reviewer signature:** `_______________` +**Date:** `_______________` + +## 7. Reproducing this pass + +```bash +npm ci +npm run lint +npm audit --omit=dev +npm audit +npm run test:unit +npm run test:integration +semgrep --config p/javascript --config p/security-audit --metrics=off app +``` + +`--metrics=off` keeps the Semgrep run from reporting usage back to the registry, which +matters for a project whose main promise is that nothing leaves the machine. The rule +packs are fetched on first use and cached under `~/.semgrep`; point `--config` at that +cache to keep later runs fully offline. + +On this machine Semgrep is installed under Python 3.14 and `python -m semgrep` prints a +deprecation notice and exits **without scanning** — a silent no-op that reads like a +clean run. Invoke the executable directly. diff --git a/test/unit/askingTheUser.test.js b/test/unit/askingTheUser.test.js new file mode 100644 index 0000000..cd54261 --- /dev/null +++ b/test/unit/askingTheUser.test.js @@ -0,0 +1,346 @@ +'use strict'; + +/** + * The 0.7.0 recovery path, end to end against a scripted Ollama and a real workspace. + * + * Three separate mechanisms meet in `AgentSession` and the interesting properties are + * all in how they compose, which is why these are here rather than split across the + * three unit files: + * + * - a request is read before it is routed (`core/commonSense`), + * - a failure that repeats escalates and eventually asks (`agent/errorRecovery`), + * - what the user answers changes the run and the checklist (`agent/clarification`). + * + * The property that matters most is the negative one: **a session with nothing to ask + * must never block.** Every path is exercised with `onClarify` absent as well as + * present, because that is the configuration a benchmark, a detached tab, and every + * pre-0.7.0 caller runs in. + */ + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); + +const { AgentSession } = require('../../app/agent/agentSession'); +const { PermissionGate } = require('../../app/security/permissionGate'); +const { PermissionModes } = require('../../app/security/permissionModes'); +const { AuditLog } = require('../../app/security/auditLog'); +const { MemoryStore } = require('../../app/core/memoryStore'); + +const TIER_B = { tier: 'B', strategy: 'react', label: 'Lite', model: 'llama3.2:1b' }; + +/** @param {Array} replies */ +function scriptedClient(replies) { + return { + calls: 0, + bodies: /** @type {any[]} */ ([]), + async chat(body) { + this.bodies.push(body); + const reply = replies[Math.min(this.calls, replies.length - 1)]; + this.calls += 1; + return { message: typeof reply === 'string' ? { content: reply } : reply }; + }, + }; +} + +const json = (action) => JSON.stringify(action); + +describe('asking the user', () => { + /** @type {string} */ + let root; + /** @type {object[]} */ + let events; + + function makeSession(opts = {}) { + const modes = new PermissionModes({ initial: { autoEdit: true, autoApproveScripts: true } }); + const gate = new PermissionGate({ + workspaceRoot: root, + modes, + auditLog: new AuditLog(root), + confirm: async () => true, + }); + + return new AgentSession({ + client: /** @type {any} */ (opts.client), + model: 'test-model', + capability: TIER_B, + gate, + workspaceRoot: root, + sessionId: '1', + memory: opts.memory, + onClarify: opts.onClarify, + }); + } + + /** @param {object} [options] */ + function run(session, task, options = {}) { + events = []; + return session.run(task, { onEvent: (event) => events.push(event), ...options }); + } + + beforeEach(() => { + root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'hiraya-ask-'))); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync(path.join(root, 'src', 'main.js'), 'export function main() {\n return 1;\n}\n'); + fs.writeFileSync(path.join(root, 'README.md'), '# Test project\n'); + }); + + afterEach(() => { + fs.rmSync(root, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); + }); + + describe('reading the request', () => { + it('opens the file the user meant, not the one they typed', async () => { + const client = scriptedClient([json({ action: 'read_file', path: 'src/main.js' }), json({ action: 'done' })]); + const session = makeSession({ client }); + + const result = await run(session, 'update mian.js to add a comment'); + + const note = events.find((event) => event.type === 'interpretation'); + assert.ok(note, 'the reading was not reported to the panel'); + assert.match(note.note, /mian\.js/); + assert.match(note.note, /src\/main\.js/); + // And the summary says so, so a user can disagree with it. + assert.match(result.summary, /How I read the request/); + }); + + it('records the reading in memory, so the same slip is not re-derived', async () => { + const memory = new MemoryStore(root, 1); + const client = scriptedClient([json({ action: 'done' })]); + const session = makeSession({ client, memory }); + + await run(session, 'update mian.js to add a comment'); + await memory.flush(); + + assert.ok((await memory.readAll()).some((entry) => entry.includes('mian.js'))); + }); + + it('leaves a request that names a real file exactly as it was', async () => { + const client = scriptedClient([json({ action: 'done' })]); + const session = makeSession({ client }); + + const result = await run(session, 'read src/main.js'); + assert.strictEqual(events.some((event) => event.type === 'interpretation'), false); + assert.doesNotMatch(result.summary, /How I read the request/); + }); + + it('asks which file when two are equally plausible', async () => { + fs.writeFileSync(path.join(root, 'src', 'todo.js'), 'export const a = 1;\n'); + fs.writeFileSync(path.join(root, 'src', 'todos.js'), 'export const b = 2;\n'); + + /** @type {object | null} */ + let asked = null; + const client = scriptedClient([json({ action: 'done' })]); + const session = makeSession({ + client, + onClarify: async (request) => { + asked = request; + return { id: request.id, optionId: request.options[0].id }; + }, + }); + + await run(session, 'update todoo.js'); + + assert.ok(asked, 'the run did not ask'); + assert.ok(asked.options.length >= 2 && asked.options.length <= 4); + assert.strictEqual(asked.options.filter((option) => option.recommended).length, 1); + }); + + it('does not ask when there is nobody to ask', async () => { + fs.writeFileSync(path.join(root, 'src', 'todo.js'), 'export const a = 1;\n'); + fs.writeFileSync(path.join(root, 'src', 'todos.js'), 'export const b = 2;\n'); + + const client = scriptedClient([json({ action: 'done' })]); + const session = makeSession({ client }); + + // The assertion is that this resolves at all. + const result = await run(session, 'update todoo.js'); + assert.ok(result.summary); + }); + + it('stops before starting when the user says stop', async () => { + fs.writeFileSync(path.join(root, 'src', 'todo.js'), 'export const a = 1;\n'); + fs.writeFileSync(path.join(root, 'src', 'todos.js'), 'export const b = 2;\n'); + + const client = scriptedClient([json({ action: 'done' })]); + const session = makeSession({ + client, + onClarify: async (request) => ({ id: request.id, cancelled: true }), + }); + + const result = await run(session, 'update todoo.js'); + + assert.strictEqual(result.stopReason, 'cancelled'); + assert.strictEqual(client.calls, 0, 'nothing should have been sent to the model'); + }); + }); + + describe('a failure that will not go away', () => { + /** A script that always fails the same way. */ + function failingScript() { + return json({ action: 'run_script', command: 'node src/broken.js' }); + } + + it('tells the model to change approach when there is nobody to ask', async () => { + const client = scriptedClient([failingScript(), failingScript(), json({ action: 'done' })]); + const session = makeSession({ client }); + + await run(session, 'run the broken script'); + + // The second failure's observation should carry the escalation. It is the only + // channel back to the model on a Tier B loop. + const observations = events.filter((event) => event.type === 'observation'); + const escalated = observations.some((event) => /same failure as before/i.test(event.result.observation)); + assert.ok(escalated, 'the repeat was never called out to the model'); + }); + + it('asks on the second failure, which is before the loop gives up', async () => { + // `reactLoop.REPEAT_LIMIT` is 2: the loop ends a run once the model has sent the + // same action twice. This is the last point at which asking can change anything. + /** @type {object | null} */ + let asked = null; + const client = scriptedClient([failingScript(), failingScript(), json({ action: 'done' })]); + const session = makeSession({ + client, + onClarify: async (request) => { + asked = request; + return { id: request.id, text: 'the entry point is src/main.js, not src/broken.js' }; + }, + }); + + const result = await run(session, 'run the broken script'); + + assert.ok(asked, 'the run never asked, having failed twice'); + assert.strictEqual(asked.kind, 'error'); + assert.match(result.summary, /What you told me when I asked/); + assert.match(result.summary, /src\/main\.js, not src\/broken\.js/); + }); + + it('never blocks a run that has nobody to ask', async () => { + const client = scriptedClient([ + failingScript(), + failingScript(), + failingScript(), + failingScript(), + json({ action: 'done' }), + ]); + const session = makeSession({ client }); + + const result = await run(session, 'run the broken script'); + assert.ok(result.summary); + }); + + it('reports what it kept hitting rather than only what it completed', async () => { + const client = scriptedClient([failingScript(), failingScript(), json({ action: 'done' })]); + const session = makeSession({ client }); + + const result = await run(session, 'run the broken script'); + assert.match(result.summary, /What kept going wrong/); + }); + + it('says nothing extra about a run where nothing went wrong twice', async () => { + const client = scriptedClient([json({ action: 'read_file', path: 'src/main.js' }), json({ action: 'done' })]); + const session = makeSession({ client }); + + const result = await run(session, 'read the main file'); + assert.doesNotMatch(result.summary, /What kept going wrong/); + }); + }); + + describe('the checklist, when the user changes it mid-run', () => { + const TIER_A = { tier: 'A', strategy: 'react', label: 'Agentic', model: 'test', canPlanTodos: true }; + + /** + * A session whose model can hold a checklist, so `_runWithTodos` is the path taken. + * + * @param {object} opts + */ + function planningSession(opts) { + const session = makeSession(opts); + session.capability = TIER_A; + return session; + } + + /** + * The planner replies with a numbered list, then each item runs its own loop. + * + * @param {string[]} items + * @param {string[]} rest + */ + function withPlan(items, rest) { + return scriptedClient([items.map((text, index) => `${index + 1}. ${text}`).join('\n'), ...rest]); + } + + const failing = () => json({ action: 'run_script', command: 'node src/broken.js' }); + + it('records a skip as the user’s decision, not as a failed step', async () => { + const client = withPlan( + ['Update src/broken.js', 'Update README.md'], + [ + failing(), + failing(), + json({ action: 'write_file', path: 'README.md', code: '# Updated\n' }), + json({ action: 'done', summary: 'readme updated' }), + ] + ); + + const session = planningSession({ + client, + onClarify: async (request) => { + const skip = request.options.find((option) => option.effect === 'skip'); + return { id: request.id, optionId: skip.id }; + }, + }); + + const result = await run(session, 'update src/broken.js and update README.md'); + + assert.strictEqual(result.todos[0].status, 'skipped', 'a step the user closed must not read as failed'); + assert.match(result.summary, /you asked me to skip this one/); + // And the rest of the list still ran. + assert.notStrictEqual(result.todos[1].status, 'skipped'); + }); + + it('rewords the running item to carry what the user said', async () => { + const client = withPlan( + ['Update src/broken.js', 'Update README.md'], + [ + failing(), + failing(), + json({ action: 'done', summary: 'gave up' }), + json({ action: 'write_file', path: 'README.md', code: '# Updated\n' }), + json({ action: 'done', summary: 'readme updated' }), + ] + ); + + const session = planningSession({ + client, + onClarify: async (request) => ({ id: request.id, text: 'the entry point is src/main.js' }), + }); + + const result = await run(session, 'update src/broken.js and update README.md'); + + // The item's text is what a retry is briefed on, what stepGuard checks against, + // and what the summary reads back. + assert.match(result.todos[0].text, /the entry point is src\/main\.js/); + assert.match(result.summary, /The checklist changed while it ran/); + assert.match(result.summary, /reworded/); + }); + + it('leaves the checklist alone when nothing was asked', async () => { + const client = withPlan( + ['Update README.md', 'Update src/main.js'], + [ + json({ action: 'write_file', path: 'README.md', code: '# One\n' }), + json({ action: 'done', summary: 'readme' }), + json({ action: 'write_file', path: 'src/main.js', code: 'export function main() {\n return 2;\n}\n' }), + json({ action: 'done', summary: 'main' }), + ] + ); + const session = planningSession({ client }); + + const result = await run(session, 'update README.md and update src/main.js'); + assert.doesNotMatch(result.summary, /The checklist changed/); + }); + }); +}); diff --git a/test/unit/clarification.test.js b/test/unit/clarification.test.js new file mode 100644 index 0000000..6375302 --- /dev/null +++ b/test/unit/clarification.test.js @@ -0,0 +1,135 @@ +'use strict'; + +const assert = require('assert'); + +const clarification = require('../../app/agent/clarification'); + +describe('clarification.build', () => { + /** @param {number} count */ + function options(count) { + return Array.from({ length: count }, (_, index) => ({ + id: `o${index + 1}`, + label: `Option ${index + 1}`, + effect: 'instruct', + recommended: index === 0, + })); + } + + it('builds a question with its options', () => { + const request = clarification.build({ + kind: 'error', + question: 'How should I carry on?', + context: 'It failed three times.', + options: options(3), + }); + + assert.strictEqual(request.question, 'How should I carry on?'); + assert.strictEqual(request.options.length, 3); + assert.ok(request.id); + }); + + it('refuses fewer than two options, because that is not a choice', () => { + assert.throws(() => clarification.build({ question: 'Which?', options: options(1) }), /between 2 and 4/); + }); + + it('refuses more than four, because that is a second task for the user', () => { + assert.throws(() => clarification.build({ question: 'Which?', options: options(5) }), /between 2 and 4/); + }); + + it('refuses a set that recommends two things at once', () => { + const both = options(2).map((option) => ({ ...option, recommended: true })); + assert.throws(() => clarification.build({ question: 'Which?', options: both }), /exactly one/); + }); + + it('promotes the first option rather than failing a run with no recommendation', () => { + // A blocked run must not also throw. "Here are your choices" with nothing marked is + // a worse question, not an unanswerable one. + const none = options(2).map((option) => ({ ...option, recommended: false })); + const request = clarification.build({ question: 'Which?', options: none }); + assert.strictEqual(request.options[0].recommended, true); + assert.strictEqual(request.options[1].recommended, false); + }); + + it('always allows free text, whatever the caller asked for', () => { + // Not settable on purpose: the options are the agent's reading of a situation it + // has just shown it does not fully understand. + const request = clarification.build({ question: 'Which?', options: options(2), allowFreeText: false }); + assert.strictEqual(request.allowFreeText, true); + }); + + it('trims a question and a label down to what a card can hold', () => { + const request = clarification.build({ + question: 'q'.repeat(400), + options: [ + { label: 'l'.repeat(200), effect: 'skip', recommended: true }, + { label: 'Second', effect: 'stop' }, + ], + }); + + assert.ok(request.question.length <= clarification.MAX_QUESTION_CHARS); + assert.ok(request.options[0].label.length <= clarification.MAX_LABEL_CHARS); + assert.ok(request.question.endsWith('…')); + }); + + it('refuses a question with nothing in it', () => { + assert.throws(() => clarification.build({ question: ' ', options: options(2) }), /needs a question/); + }); + + it('gives every question its own id', () => { + const a = clarification.build({ question: 'One?', options: options(2) }); + const b = clarification.build({ question: 'Two?', options: options(2) }); + assert.notStrictEqual(a.id, b.id); + }); +}); + +describe('clarification.resolve', () => { + const request = clarification.build({ + kind: 'error', + question: 'How should I carry on?', + options: [ + { id: 'skip', label: 'Skip it', effect: 'skip', recommended: true, guidance: 'Leave it alone.' }, + { id: 'stop', label: 'Stop', effect: 'stop' }, + ], + }); + + it('resolves a chosen option to its effect and guidance', () => { + const resolved = clarification.resolve(request, { id: request.id, optionId: 'skip' }); + assert.strictEqual(resolved.effect, 'skip'); + assert.strictEqual(resolved.guidance, 'Leave it alone.'); + }); + + it('falls back to the label when an option carries no guidance', () => { + const resolved = clarification.resolve(request, { id: request.id, optionId: 'stop' }); + assert.strictEqual(resolved.guidance, 'Stop'); + }); + + it('prefers what the user typed over what they clicked', () => { + // They meant the words. The option may have been clicked on the way to the box. + const resolved = clarification.resolve(request, { + id: request.id, + optionId: 'skip', + text: 'use the other config file', + }); + assert.strictEqual(resolved.effect, 'instruct'); + assert.strictEqual(resolved.guidance, 'use the other config file'); + }); + + it('stops on a cancelled answer', () => { + assert.strictEqual(clarification.resolve(request, { id: request.id, cancelled: true }).effect, 'stop'); + }); + + it('stops when there is no answer at all', () => { + assert.strictEqual(clarification.resolve(request, null).effect, 'stop'); + }); + + it('stops rather than guessing when the option is unknown', () => { + // The run is blocked, so this has to resolve to something — and picking an option + // on the user's behalf is the one thing it must not do. + assert.strictEqual(clarification.resolve(request, { id: request.id, optionId: 'nope' }).effect, 'stop'); + }); + + it('ignores whitespace-only free text rather than treating it as an instruction', () => { + const resolved = clarification.resolve(request, { id: request.id, optionId: 'skip', text: ' ' }); + assert.strictEqual(resolved.effect, 'skip'); + }); +}); diff --git a/test/unit/commonSense.test.js b/test/unit/commonSense.test.js new file mode 100644 index 0000000..7dfe8ca --- /dev/null +++ b/test/unit/commonSense.test.js @@ -0,0 +1,216 @@ +'use strict'; + +const assert = require('assert'); + +const commonSense = require('../../app/core/commonSense'); + +const FILES = [ + 'src/main.js', + 'src/api.js', + 'src/hooks/useTodos.js', + 'src/components/TodoInput.jsx', + 'README.md', + 'vite.config.js', + 'package.json', +]; + +describe('commonSense.referencedPaths', () => { + it('finds the filenames in a request', () => { + assert.deepStrictEqual(commonSense.referencedPaths('update src/main.js and README.md'), [ + 'src/main.js', + 'README.md', + ]); + }); + + it('is not fooled by prose that names no file', () => { + assert.deepStrictEqual(commonSense.referencedPaths('update the parser so it handles decimals'), []); + }); + + it('ignores a version number', () => { + assert.deepStrictEqual(commonSense.referencedPaths('upgrade to react 18.2'), []); + }); + + it('reads a backtick-quoted path', () => { + assert.deepStrictEqual(commonSense.referencedPaths('fix `src/main.js` please'), ['src/main.js']); + }); + + it('normalises a windows separator', () => { + assert.deepStrictEqual(commonSense.referencedPaths('edit src\\main.js'), ['src/main.js']); + }); +}); + +describe('commonSense.editDistance', () => { + it('charges a transposition one edit, not two', () => { + // The whole reason this is not plain Levenshtein. `mian` for `main` is the + // commonest way a filename gets mistyped, and at two edits it falls below the + // threshold on a seven-character name. + assert.strictEqual(commonSense.editDistance('mian', 'main'), 1); + assert.ok(commonSense.nameSimilarity('mian.js', 'main.js') >= commonSense.TYPO_THRESHOLD); + }); + + it('counts ordinary edits the ordinary way', () => { + assert.strictEqual(commonSense.editDistance('context', 'contxt'), 1); + assert.strictEqual(commonSense.editDistance('todo', 'todos'), 1); + assert.strictEqual(commonSense.editDistance('auth', 'math'), 2); + assert.strictEqual(commonSense.editDistance('same', 'same'), 0); + }); + + it('handles an empty side', () => { + assert.strictEqual(commonSense.editDistance('', 'abc'), 3); + assert.strictEqual(commonSense.editDistance('abc', ''), 3); + assert.strictEqual(commonSense.nameSimilarity('', ''), 0); + }); +}); + +describe('commonSense.nearMatches', () => { + it('finds the file a transposition was meant to name', () => { + assert.deepStrictEqual(commonSense.nearMatches('mian.js', FILES), ['src/main.js']); + }); + + it('matches on the basename, not the folders around it', () => { + assert.deepStrictEqual(commonSense.nearMatches('app/usetodo.js', FILES), ['src/hooks/useTodos.js']); + }); + + it('does not call a right name in a wrong folder a typo', () => { + // Nothing was misspelled. `existsIn` claims this one, and the model resolves it + // from the workspace listing it is already given. + assert.strictEqual(commonSense.existsIn('app/useTodos.js', FILES), true); + assert.strictEqual(commonSense.interpret({ task: 'update app/useTodos.js', files: FILES }).kind, 'ok'); + }); + + it('refuses to call a different extension a typo', () => { + // `main.js` and `main.css` score high on characters and are never the same mistake. + assert.deepStrictEqual(commonSense.nearMatches('main.css', FILES), []); + }); + + it('refuses to judge a name too short to judge', () => { + // `a.js` and `b.js` are one edit apart and have nothing to do with each other. + assert.deepStrictEqual(commonSense.nearMatches('a.js', ['b.js', 'c.js']), []); + }); + + it('finds nothing for a genuinely new name', () => { + assert.deepStrictEqual(commonSense.nearMatches('parser.js', FILES), []); + }); +}); + +describe('commonSense.interpret — a misspelled file', () => { + it('reads a typo as the file that exists', () => { + const reading = commonSense.interpret({ task: 'update mian.js to add a header', files: FILES }); + + assert.strictEqual(reading.kind, 'repaired'); + assert.strictEqual(reading.task, 'update src/main.js to add a header'); + // Both names, so the user can see their request was altered and disagree with it. + assert.match(reading.note, /mian\.js/); + assert.match(reading.note, /src\/main\.js/); + }); + + it('leaves a name the user is inventing alone', () => { + assert.strictEqual(commonSense.interpret({ task: 'create mian.js', files: FILES }).kind, 'ok'); + assert.strictEqual(commonSense.interpret({ task: 'add a new file called mian.js', files: FILES }).kind, 'ok'); + }); + + it('is not put off by the word "add" later in the sentence', () => { + // The verb that governs the filename is the one in front of it. A whole-message + // test reads this as a creation and silently declines to help. + const reading = commonSense.interpret({ task: 'update mian.js to add a header', files: FILES }); + assert.strictEqual(reading.kind, 'repaired'); + }); + + it('corrects only the half of the request that is wrong', () => { + const reading = commonSense.interpret({ task: 'read mian.js and create parser.js', files: FILES }); + assert.strictEqual(reading.kind, 'repaired'); + assert.match(reading.task, /src\/main\.js/); + // The invented name survives untouched. + assert.match(reading.task, /parser\.js/); + }); + + it('says nothing about a file that simply does not exist yet', () => { + assert.strictEqual(commonSense.interpret({ task: 'update parser.js', files: FILES }).kind, 'ok'); + }); + + it('says nothing when the file is right', () => { + assert.strictEqual(commonSense.interpret({ task: 'read src/main.js', files: FILES }).kind, 'ok'); + }); + + it('matches a bare filename against its full path', () => { + // A user naming a file rarely types its folders. + assert.strictEqual(commonSense.interpret({ task: 'read useTodos.js', files: FILES }).kind, 'ok'); + }); + + it('asks when two files are equally plausible', () => { + const files = ['src/todo.js', 'src/todos.js']; + const reading = commonSense.interpret({ task: 'update todoo.js', files }); + + assert.strictEqual(reading.kind, 'ask'); + assert.strictEqual(reading.clarification.options.length, 2); + assert.ok(reading.clarification.options.some((option) => option.label === 'src/todo.js')); + }); + + it('leaves an ambiguous name alone when nothing can ask', () => { + const files = ['src/todo.js', 'src/todos.js']; + const reading = commonSense.interpret({ task: 'update todoo.js', files, canAsk: false }); + assert.strictEqual(reading.kind, 'ok'); + }); + + it('says nothing without a workspace listing to check against', () => { + assert.strictEqual(commonSense.interpret({ task: 'update mian.js', files: [] }).kind, 'ok'); + }); +}); + +describe('commonSense.interpret — a reference to nothing', () => { + it('takes "fix it" as the file the user is looking at', () => { + const reading = commonSense.interpret({ task: 'fix it', files: FILES, editorPath: 'src/api.js' }); + + assert.strictEqual(reading.kind, 'repaired'); + assert.match(reading.task, /src\/api\.js/); + }); + + it('leaves it alone when an earlier turn gives "it" something to mean', () => { + const reading = commonSense.interpret({ + task: 'fix it', + files: FILES, + conversation: [{ role: 'user', text: 'the parser drops decimals' }], + }); + assert.strictEqual(reading.kind, 'ok'); + }); + + it('asks when there is nothing at all for "it" to refer to', () => { + const reading = commonSense.interpret({ task: 'fix it', files: FILES }); + + assert.strictEqual(reading.kind, 'ask'); + assert.match(reading.clarification.question, /fix/); + assert.ok(reading.clarification.options.length >= 2); + }); + + it('does not fire on a message that merely starts with the same verb', () => { + assert.strictEqual(commonSense.interpret({ task: 'fix the parser in src/main.js', files: FILES }).kind, 'ok'); + assert.strictEqual(commonSense.interpret({ task: 'update items in the list', files: FILES }).kind, 'ok'); + }); + + it('still matches when the message is padded with whitespace', () => { + const reading = commonSense.interpret({ task: ' fix it. \n', files: FILES, editorPath: 'src/api.js' }); + assert.strictEqual(reading.kind, 'repaired'); + }); +}); + +describe('commonSense — input that is trying to be expensive', () => { + // Both of these were measured super-linear before 0.7.0 bounded them: the dangling + // pattern at 1,660 ms on 50,000 trailing spaces, and the path scan at 3,089 ms on + // 50,000 characters of `a/a/a/…`. Neither is an attack — it is the user's own + // composer — but three seconds of frozen extension host is a bug either way. + const BUDGET_MS = 250; + + it('reads a message with a huge whitespace tail in linear time', () => { + const task = `fix it${' '.repeat(50000)}X`; + const started = Date.now(); + commonSense.interpret({ task, files: FILES }); + assert.ok(Date.now() - started < BUDGET_MS, `took ${Date.now() - started}ms`); + }); + + it('scans a huge path-like paste in bounded time', () => { + const task = `update ${'a/'.repeat(25000)}${'a'.repeat(50)}!`; + const started = Date.now(); + commonSense.referencedPaths(task); + assert.ok(Date.now() - started < BUDGET_MS, `took ${Date.now() - started}ms`); + }); +}); diff --git a/test/unit/errorRecovery.test.js b/test/unit/errorRecovery.test.js new file mode 100644 index 0000000..6d2f95c --- /dev/null +++ b/test/unit/errorRecovery.test.js @@ -0,0 +1,180 @@ +'use strict'; + +const assert = require('assert'); + +const { ErrorRecovery, signatureOf, headline } = require('../../app/agent/errorRecovery'); + +/** + * @param {object} [overrides] + * @returns {import('../../app/agent/errorRecovery').Failure} + */ +function failure(overrides = {}) { + return { + action: 'run_script', + command: 'node src/main.js', + observation: "ReferenceError: addTodo is not defined\n at Object. (/app/src/main.js:4:11)", + diagnosed: false, + ...overrides, + }; +} + +describe('errorRecovery.signatureOf', () => { + it('treats the same failure at different lines as the same failure', () => { + const a = signatureOf('run_script', 'ReferenceError: x is not defined at /app/src/main.js:4:11'); + const b = signatureOf('run_script', 'ReferenceError: x is not defined at /app/src/main.js:40:2'); + assert.strictEqual(a, b); + }); + + it('treats the same failure on two machines as the same failure', () => { + const posix = signatureOf('run_script', "Cannot find module '/home/jay/app/src/x.js'"); + const win = signatureOf('run_script', "Cannot find module 'C:\\Users\\jay\\app\\src\\x.js'"); + assert.strictEqual(posix, win); + }); + + it('keeps genuinely different failures apart', () => { + assert.notStrictEqual( + signatureOf('run_script', 'ReferenceError: addTodo is not defined'), + signatureOf('run_script', 'ReferenceError: removeTodo is not defined') + ); + }); + + it('separates the same message from two different tools', () => { + assert.notStrictEqual(signatureOf('run_script', 'EACCES'), signatureOf('write_file', 'EACCES')); + }); +}); + +describe('errorRecovery.headline', () => { + it('takes the sentence, not the stack frame under it', () => { + const text = "TypeError: todos.map is not a function\n at App (/app/src/App.jsx:12:20)\n at renderWithHooks"; + assert.strictEqual(headline(text), 'TypeError: todos.map is not a function'); + }); + + it('skips a python traceback header to reach the error', () => { + const text = 'Traceback (most recent call last):\n File "main.py", line 3\nNameError: name \'foo\' is not defined'; + assert.match(headline(text), /NameError/); + }); +}); + +describe('ErrorRecovery.observe', () => { + it('says nothing the first time a diagnosed failure appears', () => { + // scriptDiagnosis already gave it a sentence; a second one saying the same thing + // differently is noise in a context window that has none spare. + const recovery = new ErrorRecovery(); + const decision = recovery.observe(failure({ diagnosed: true })); + assert.strictEqual(decision.kind, 'none'); + }); + + it('tells the model to read the error when nothing recognised it', () => { + const recovery = new ErrorRecovery(); + const decision = recovery.observe(failure()); + assert.strictEqual(decision.kind, 'guidance'); + assert.match(decision.guidance, /read the error above literally/i); + }); + + it('names the count once the same failure comes back with nobody to ask', () => { + const recovery = new ErrorRecovery({ canAsk: false }); + recovery.observe(failure()); + const second = recovery.observe(failure()); + + assert.strictEqual(second.kind, 'guidance'); + assert.strictEqual(second.count, 2); + assert.match(second.guidance, /same failure as before/i); + assert.match(second.guidance, /2 times/); + }); + + it('asks the user the second time, which is the last moment it can', () => { + // `reactLoop.REPEAT_LIMIT` is 2, so the loop ends the run after two identical + // actions. Waiting for a third would mean never asking at all. + const recovery = new ErrorRecovery(); + recovery.observe(failure()); + const second = recovery.observe(failure()); + + assert.strictEqual(second.kind, 'ask'); + assert.ok(second.clarification); + assert.ok(second.clarification.options.length >= 2 && second.clarification.options.length <= 4); + // Recommending another attempt would recommend the thing that has already not + // worked, with guidance in between. + const recommended = second.clarification.options.find((option) => option.recommended); + assert.strictEqual(recommended.effect, 'skip'); + }); + + it('never asks when there is nobody to ask', () => { + const recovery = new ErrorRecovery({ canAsk: false }); + for (let i = 0; i < 6; i += 1) { + const decision = recovery.observe(failure()); + assert.notStrictEqual(decision.kind, 'ask', 'a session with no panel must not block on a question'); + } + }); + + it('asks only once about the same wall', () => { + const recovery = new ErrorRecovery(); + for (let i = 0; i < 2; i += 1) recovery.observe(failure()); + const third = recovery.observe(failure()); + assert.strictEqual(third.kind, 'guidance'); + }); + + it('leaves a refused action alone — that is the permission model, not a wall', () => { + const recovery = new ErrorRecovery(); + for (let i = 0; i < 5; i += 1) { + const decision = recovery.observe( + failure({ action: 'delete_file', error: 'PERMISSION_DENIED', observation: 'The user declined.' }) + ); + assert.strictEqual(decision.kind, 'none'); + } + }); + + it('repeats the user’s answer back rather than asking again', () => { + const recovery = new ErrorRecovery(); + for (let i = 0; i < 2; i += 1) recovery.observe(failure()); + recovery.recordAnswer(failure(), 'import addTodo from ./hooks/useTodos'); + + const next = recovery.observe(failure()); + assert.strictEqual(next.kind, 'guidance'); + assert.match(next.guidance, /the user about this and they said/i); + assert.match(next.guidance, /useTodos/); + }); + + it('does not remember a failure that only happened once', () => { + // A single failure the next step fixes is noise, and the recall window is small + // enough that noise costs something. + const recovery = new ErrorRecovery(); + assert.strictEqual(recovery.observe(failure()).note, ''); + }); + + it('remembers one that keeps happening', () => { + const recovery = new ErrorRecovery(); + recovery.observe(failure()); + const second = recovery.observe(failure()); + assert.match(second.note, /failed 2 times/); + assert.match(second.note, /addTodo is not defined/); + }); + + it('counts two different failures separately', () => { + const recovery = new ErrorRecovery(); + recovery.observe(failure()); + const other = recovery.observe(failure({ observation: 'EADDRINUSE: address already in use' })); + assert.strictEqual(other.count, 1); + }); +}); + +describe('ErrorRecovery.persistent', () => { + it('reports what a run kept hitting, worst first', () => { + const recovery = new ErrorRecovery({ canAsk: false }); + recovery.observe(failure()); + recovery.observe(failure()); + recovery.observe(failure()); + recovery.observe(failure({ observation: 'EADDRINUSE: address already in use' })); + recovery.observe(failure({ observation: 'EADDRINUSE: address already in use' })); + + const stuck = recovery.persistent(); + assert.strictEqual(stuck.length, 2); + assert.strictEqual(stuck[0].count, 3); + assert.match(stuck[0].headline, /addTodo/); + }); + + it('says nothing about a run that hit nothing twice', () => { + const recovery = new ErrorRecovery({ canAsk: false }); + recovery.observe(failure()); + assert.deepStrictEqual(recovery.persistent(), []); + }); +}); diff --git a/test/unit/memoryStore.test.js b/test/unit/memoryStore.test.js index 8e0dc96..e0cb34b 100644 --- a/test/unit/memoryStore.test.js +++ b/test/unit/memoryStore.test.js @@ -392,6 +392,26 @@ describe('superseding stale entries', () => { assert.deepStrictEqual(recalled, await store.readRecent(2)); }); + it('prefers the note that shares more of the subject than the one sharing less', async () => { + // `vite.config.js` and `src/hooks/useTodos.js` both match a subject naming + // useTodos and vite — but only one of them is what the step is about. On a + // one-entry window the count is the only thing that can separate them, and + // before 0.7.0 any single shared token was enough, so the newest weak match won. + const recalled = await store.readRelevant('Wire useTodos into the todos hook for addTodo', 1); + + assert.strictEqual(recalled.length, 1); + assert.ok( + recalled[0].includes('useTodos.js'), + `expected the strongest match, got: ${recalled[0]}` + ); + }); + + it('still breaks a tie by recency', async () => { + // Two notes, one shared token each ("npm"): the newer one wins. + const recalled = await store.readRelevant('npm', 1); + assert.ok(recalled[0].includes('npm run build'), `expected the newer npm note, got: ${recalled[0]}`); + }); + it('fills the remainder of the window with recent notes', async () => { const recalled = await store.readRelevant('Update README.md', 3); assert.strictEqual(recalled.length, 3); diff --git a/test/unit/pathGuard.test.js b/test/unit/pathGuard.test.js index 5443f62..8875899 100644 --- a/test/unit/pathGuard.test.js +++ b/test/unit/pathGuard.test.js @@ -10,6 +10,7 @@ const { resolvePath, resolveForMutation, assertRealPath, + assertRealPathSync, isInside, matchProtectedPrefix, } = require('../../app/security/pathGuard'); @@ -230,3 +231,86 @@ describe('pathGuard.assertRealPath', () => { await assert.doesNotReject(() => assertRealPath(resolvePath(root, 'alias.js'))); }); }); + +// The sync twin exists for `core/workspaceBootstrap`, which runs on a synchronous +// activation path. These tests exist to keep the two from drifting: a difference in +// verdict between them is the bug worth catching, so each case asserts both. +describe('pathGuard.assertRealPathSync', () => { + /** @type {string} */ + let tmp; + + beforeEach(() => { + tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'hiraya-guard-sync-')); + fs.mkdirSync(path.join(tmp, 'workspace', 'src'), { recursive: true }); + fs.writeFileSync(path.join(tmp, 'workspace', 'src', 'app.js'), 'ok'); + fs.mkdirSync(path.join(tmp, 'outside'), { recursive: true }); + fs.writeFileSync(path.join(tmp, 'outside', 'secret.txt'), 'top secret'); + }); + + afterEach(() => { + fs.rmSync(tmp, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 }); + }); + + it('accepts a real file inside the workspace, like its async twin', async () => { + const root = path.join(tmp, 'workspace'); + assert.doesNotThrow(() => assertRealPathSync(resolvePath(root, 'src/app.js'))); + await assert.doesNotReject(() => assertRealPath(resolvePath(root, 'src/app.js'))); + }); + + it('accepts a file that does not exist yet by checking its parent', async () => { + const root = path.join(tmp, 'workspace'); + assert.doesNotThrow(() => assertRealPathSync(resolvePath(root, 'src/brand/new/file.js'))); + await assert.doesNotReject(() => assertRealPath(resolvePath(root, 'src/brand/new/file.js'))); + }); + + it('rejects a .gitignore that is a symlink pointing outside the workspace', async function () { + const root = path.join(tmp, 'workspace'); + try { + fs.symlinkSync(path.join(tmp, 'outside', 'secret.txt'), path.join(root, '.gitignore')); + } catch (err) { + if (/** @type {NodeJS.ErrnoException} */ (err).code === 'EPERM') return this.skip(); + throw err; + } + + // This is the exact shape SAST-0.6.1 §2 described: lexically spotless, and only + // realpath resolution sees that appending to it writes outside the workspace. + const resolved = resolvePath(root, '.gitignore'); + assert.throws( + () => assertRealPathSync(resolved), + (err) => err instanceof PathGuardError && err.code === 'SYMLINK_ESCAPE' + ); + await assert.rejects( + () => assertRealPath(resolved), + (err) => err instanceof PathGuardError && err.code === 'SYMLINK_ESCAPE' + ); + }); + + it('rejects a file created through a linked directory', async () => { + const root = path.join(tmp, 'workspace'); + // Junctions work unprivileged on Windows and realpath resolves them identically, + // so this case gets real coverage on every platform rather than skipping. + const linkType = process.platform === 'win32' ? 'junction' : 'dir'; + fs.symlinkSync(path.join(tmp, 'outside'), path.join(root, '.hirayacoder'), linkType); + + const resolved = resolvePath(root, '.hirayacoder/environment.json'); + assert.throws( + () => assertRealPathSync(resolved), + (err) => err instanceof PathGuardError && err.code === 'SYMLINK_ESCAPE' + ); + await assert.rejects( + () => assertRealPath(resolved), + (err) => err instanceof PathGuardError && err.code === 'SYMLINK_ESCAPE' + ); + }); + + it('accepts a symlink that stays inside the workspace', function () { + const root = path.join(tmp, 'workspace'); + try { + fs.symlinkSync(path.join(root, 'src', 'app.js'), path.join(root, 'alias.js')); + } catch (err) { + if (/** @type {NodeJS.ErrnoException} */ (err).code === 'EPERM') return this.skip(); + throw err; + } + assert.doesNotThrow(() => assertRealPathSync(resolvePath(root, 'alias.js'))); + }); +}); diff --git a/test/unit/scriptRunner.test.js b/test/unit/scriptRunner.test.js index 74a49c0..81daea7 100644 --- a/test/unit/scriptRunner.test.js +++ b/test/unit/scriptRunner.test.js @@ -145,6 +145,43 @@ describe('scriptRunner.requiresExplicitApproval', () => { assert.strictEqual(requiresExplicitApproval(tokenize('git status')), null); assert.strictEqual(requiresExplicitApproval(tokenize('git commit -m wip')), null); }); + + // SAST-0.6.1 §1. `npx` was allow-listed, `npm_config_yes` suppressed its own + // "Ok to proceed?", and nothing here asked — so auto-approve ran arbitrary remote + // code with no click. Every spelling of "fetch a package and run it" is covered. + it('forces confirmation for every spelling of npx', () => { + for (const command of [ + 'npx create-vite@latest todo-glass-app -- --template react', + 'npx cowsay hello', + 'npm exec create-vite', + 'npm x create-vite', + 'npm create vite@latest', + 'yarn dlx create-vite', + 'yarn create vite', + 'pnpm dlx create-vite', + 'pnpm create vite', + ]) { + const reason = requiresExplicitApproval(tokenize(command)); + assert.ok(reason, `${command} should require a click`); + assert.match(reason, /downloads and runs a package from the internet/); + } + }); + + it('confirms an initializer but not a bare init, which touches no network', () => { + assert.ok(requiresExplicitApproval(tokenize('npm init vite'))); + assert.ok(requiresExplicitApproval(tokenize('npm init -y react-app'))); + // Bare `npm init` writes a package.json into the current directory and fetches + // nothing. A click for that is a click trained away. + assert.strictEqual(requiresExplicitApproval(tokenize('npm init')), null); + assert.strictEqual(requiresExplicitApproval(tokenize('npm init -y')), null); + assert.strictEqual(requiresExplicitApproval(tokenize('npm init --yes')), null); + }); + + it('does not let the npx rules swallow ordinary package-manager work', () => { + assert.strictEqual(requiresExplicitApproval(tokenize('npm run build')), null); + assert.strictEqual(requiresExplicitApproval(tokenize('yarn install')), null); + assert.strictEqual(requiresExplicitApproval(tokenize('pnpm test')), null); + }); }); describe('scriptRunner.resolveBinary', () => { diff --git a/test/unit/todoList.test.js b/test/unit/todoList.test.js index 41faf47..821d126 100644 --- a/test/unit/todoList.test.js +++ b/test/unit/todoList.test.js @@ -143,3 +143,47 @@ describe('TodoList', () => { assert.match(todos.render(), /\[x\] 1\. One/); }); }); + +describe('TodoList — changed by the user mid-run', () => { + it('rewords the active item and keeps what it was', () => { + const todos = new TodoList(['Do the thing', 'Two']); + assert.strictEqual(todos.replaceCurrent('Do the specific thing'), true); + + assert.strictEqual(todos.current().text, 'Do the specific thing'); + // The summary has to be able to say what it was, or a user cannot tell that their + // answer is why it succeeded. + assert.match(todos.describeChanges(), /Do the thing.*Do the specific thing/); + }); + + it('ignores a reword that changes nothing', () => { + const todos = new TodoList(['One', 'Two']); + assert.strictEqual(todos.replaceCurrent('One'), false); + assert.strictEqual(todos.replaceCurrent(' '), false); + assert.strictEqual(todos.changes.length, 0); + }); + + it('skips one item without giving up on the rest', () => { + const todos = new TodoList(['One', 'Two', 'Three']); + const next = todos.skipCurrent('you asked me to skip this one'); + + assert.strictEqual(todos.items[0].status, 'skipped'); + assert.strictEqual(next.text, 'Two'); + assert.strictEqual(todos.items[2].status, 'pending', 'skipping one must not abandon the others'); + }); + + it('says nothing about a list that ran as planned', () => { + const todos = new TodoList(['One', 'Two']); + todos.finishCurrent('done'); + assert.strictEqual(todos.describeChanges(), ''); + }); + + it('reports every change it made, in order', () => { + const todos = new TodoList(['One', 'Two']); + todos.replaceCurrent('One, precisely'); + todos.skipCurrent('you asked me to skip this one'); + + const described = todos.describeChanges(); + assert.match(described, /Item 1 reworded/); + assert.match(described, /Item 1 dropped/); + }); +}); diff --git a/test/unit/undefinedErrors.test.js b/test/unit/undefinedErrors.test.js new file mode 100644 index 0000000..6cb7d99 --- /dev/null +++ b/test/unit/undefinedErrors.test.js @@ -0,0 +1,131 @@ +'use strict'; + +/** + * The rules for code that fails because something in it does not exist. + * + * This is the commonest way code written by a small model fails at runtime, and until + * 0.7.0 none of it matched a rule — the model got a stack trace, the generic "the error + * points at a file" fallback, and no mention of the name that was actually missing. It + * then rewrote the file from memory and produced the same error. + * + * What each of these asserts is not "a rule matched" but "the sentence names the + * symbol", because that is the difference between a diagnosis a 1B model can act on and + * one it cannot. + */ + +const assert = require('assert'); + +const { diagnose } = require('../../app/agent/scriptDiagnosis'); + +/** @param {string} stderr */ +function of(stderr) { + return diagnose({ stderr, stdout: '', code: 1 }, { command: 'node src/main.js' }); +} + +describe('scriptDiagnosis — undefined symbols', () => { + it('names the identifier a ReferenceError is about', () => { + const result = of('ReferenceError: addTodo is not defined\n at Object. (/app/src/main.js:4:11)'); + + assert.strictEqual(result.reason, 'UNDEFINED_SYMBOL'); + assert.match(result.summary, /addTodo/); + assert.match(result.fix, /`addTodo`/); + assert.match(result.fix, /import|define/i); + assert.strictEqual(result.fixFirst, true, 'the file must be fixed before the command is run again'); + assert.strictEqual(result.retryable, false); + }); + + it('names the identifier a python NameError is about', () => { + const result = of("Traceback (most recent call last):\n File \"main.py\", line 3\nNameError: name 'render' is not defined"); + + assert.strictEqual(result.reason, 'UNDEFINED_SYMBOL'); + assert.match(result.fix, /`render`/); + }); + + it('names the symbol javac could not find', () => { + const result = of( + 'Main.java:7: error: cannot find symbol\n greeter.greet();\n ^\n symbol: method greet\n location: variable greeter' + ); + + assert.strictEqual(result.reason, 'UNDEFINED_SYMBOL'); + assert.match(result.fix, /`greet`/); + }); + + it('does not mistake an ESM module error for an undefined variable', () => { + // `module is not defined in ES module scope` is a ReferenceError with a completely + // different fix, and MODULE_SYSTEM has to keep claiming it. + const result = of('ReferenceError: module is not defined in ES module scope'); + assert.strictEqual(result.reason, 'MODULE_SYSTEM'); + }); +}); + +describe('scriptDiagnosis — reading a property of nothing', () => { + it('names the property and says the holder is the problem', () => { + const result = of("TypeError: Cannot read properties of undefined (reading 'map')"); + + assert.strictEqual(result.reason, 'UNDEFINED_PROPERTY'); + assert.match(result.fix, /`map`/); + // The point of the sentence: fix what produced the undefined, not the line that + // read it. A model told only "map failed" adds a `?.` and moves on. + assert.match(result.fix, /Fix the source of the undefined/i); + assert.strictEqual(result.fixFirst, true); + }); + + it('handles the older node phrasing too', () => { + const result = of("TypeError: Cannot read property 'length' of null"); + assert.strictEqual(result.reason, 'UNDEFINED_PROPERTY'); + assert.match(result.fix, /`length`/); + assert.match(result.fix, /null/); + }); + + it('explains a python NoneType attribute error as a None, not a typo', () => { + const result = of("AttributeError: 'NoneType' object has no attribute 'title'"); + assert.strictEqual(result.reason, 'UNDEFINED_PROPERTY'); + assert.match(result.fix, /None/); + assert.match(result.fix, /rather than the line that used it/i); + }); + + it('suggests a spelling check for an attribute on a real class', () => { + const result = of("AttributeError: 'Parser' object has no attribute 'parseAll'"); + assert.match(result.fix, /`parseAll`/); + assert.match(result.fix, /spelled differently/i); + }); + + it('reads a NullPointerException as a null that should have been set', () => { + const result = of('Exception in thread "main" java.lang.NullPointerException\n\tat Main.main(Main.java:9)'); + assert.strictEqual(result.reason, 'NULL_DEREFERENCE'); + assert.match(result.fix, /initialise the field|pass the argument/i); + }); +}); + +describe('scriptDiagnosis — calling something that is not a function', () => { + it('names the call and points at the import shape', () => { + const result = of('TypeError: todos.map is not a function'); + + assert.strictEqual(result.reason, 'NOT_A_FUNCTION'); + assert.match(result.fix, /`todos\.map`/); + assert.match(result.fix, /default vs named/i); + }); +}); + +describe('scriptDiagnosis — the rules that were already there', () => { + it('still prefers a missing file over a missing package', () => { + const result = of("Error: Cannot find module '/app/src/main.js'"); + assert.strictEqual(result.reason, 'WRONG_PATH'); + }); + + it('still recognises a missing dependency', () => { + const result = of("Error: Cannot find module 'express'"); + assert.strictEqual(result.reason, 'MISSING_DEPENDENCY'); + }); + + it('still says nothing about output that means nothing', () => { + assert.strictEqual(diagnose({ stderr: '', stdout: 'ok', code: 1 }, { command: 'true' }), null); + }); + + it('gives the same answer twice for the same input', () => { + // The rules are `exec`ed rather than `test`ed now, so a stateful regex would show + // up as a second call returning null. + const text = 'ReferenceError: addTodo is not defined'; + assert.deepStrictEqual(of(text), of(text)); + }); +});