fix: resume queued prompts after slash commands - #1069
Conversation
will-lamerton
left a comment
There was a problem hiding this comment.
Right diagnosis and the right seam: /compact never goes through the chat handler, so the queue drain was unreachable. The problem is that onCommandComplete is a shared terminal callback for essentially every slash command, and the drain has no busy/mode guard (App.tsx:252-279 only checks client and toolManager).
Blocking issues:
- Concurrent turns. Slash commands intentionally bypass the queue while busy (
user-input.tsx:448) and the input stays enabled during generation (chat-input.tsx:240). Queue a prompt mid-turn, type/status, and its completion drains the queue into a secondhandleChatMessageon top of the live one. There is no re-entrancy guard, sosetAbortController(useChatHandler.tsx:374) clobbers the in-flight turn's controller and Escape only reaches the newer one. - Double drain on
/retry.retry-handler.ts:85-89awaitsonHandleChatMessageand then callsonCommandComplete. The awaited turn already drains viaonConversationComplete, so the handler's own call drains a second message while the first is starting.create-handler.tshas the same await-then-complete shape on some paths. - Drain behind a modal mode.
/model,/explorer,/ide,/tune(app-util.ts:271-276) and/settings(:309-311) enter a mode and complete immediately, so a queued prompt starts generating underneath the picker.
Suggested fix: guard the drain on "nothing in flight" (a generating ref plus activeMode === null), or drop the callback plumbing and drain from an effect on the busy to idle transition. cancellable in interactive-app.tsx:173-181 already computes that signal, it covers /compact naturally, and it cannot fire mid-generation or behind a modal by construction.
Test coverage: setup() defaults to client: null, messages: [], so /compact takes the "No messages to compact" branch (compact-handler.ts:147-151) which completes synchronously. The new JSDoc says "including delayed completions", but no DELAY_COMMAND_COMPLETE_MS path is exercised, and those are what every real compaction uses (:226, :265). Please add a delayed-completion case and a negative case asserting no drain while a turn is generating.
Smaller notes:
props.onCommandCompleteis missing fromhandleMessageSubmit's dependency array (useAppHandlers.tsx:718-751). It lints clean only because barepropsis listed; every other prop is enumerated.- The changeset says "after manual context compaction completes", but the change affects all slash commands. The title is accurate, worth matching them up.
- "Fixes #1060" is only half of the issue. The second symptom, queued messages not being selectable or deletable once idle, is untouched:
user-input.tsx:610,:638and:669gate the key handling onisBusywhile the list plus its "↑/↓ select, Enter edit, Del remove" hint render whenever the queue is non-empty (:1077). Any queue that fails to drain is still stranded and still advertises dead controls. Either fix that gating or downgrade to "Refs #1060".
Verified pnpm exec ava source/hooks/useAppHandlers.spec.tsx passes on the branch (15 tests). No security or performance concerns, and the style matches the codebase.
…llective#928) * feat: migrate file search to a ripgrep-backed implementation Replace the hand-rolled JS file-search walker with one backed by the real `rg` binary (via @vscode/ripgrep) for a large speed improvement, per Nano-Collective#889. - find_files and search_file_contents now shell out to rg for file listing and content search; file-autocomplete gets the same speedup for free via the shared walkProjectEntries. - Glob matching (find_files' pattern arg) uses a hand-written DP-table matcher instead of a compiled regex - the previous regex-based approach allowed catastrophic backtracking on adversarial patterns. - No --follow: a symlink checked into a project can point anywhere on disk: search must stay scoped to the project directory. - .gitignore handled via rg's own native discovery (--no-ignore-parent, --no-require-git), including nested .gitignore files; the supplementary empty-directory walker mirrors this with its own prefixed-pattern nested-.gitignore support. - Result limits enforced by streaming rg's own --json output and killing the process once enough matches are seen, rather than trusting rg's --max-count (which can overshoot with --context) or buffering unbounded output before truncating in JS. - Abort signals are wired to a manual child.kill() rather than spawn's own `signal` option, avoiding a race where Node's built-in abort handling drops the caller's abort reason. - Empty/whitespace-only queries and non-positive maxResults are rejected before ever reaching rg. Fixes two pre-existing upstream bugs needed to get a clean local install/test run: AVA 8's extensions option must be an array, and TypeScript 7 removed baseUrl (paths now use explicit ./ prefixes). * fix: stop a failed ripgrep spawn from stalling the process for the full timeout spawn({timeout}) only clears its timer on 'exit'. A spawn-time OS failure (bad cwd, missing binary) only emits 'error', so the timer stayed armed and delayed process exit by up to 30s. Own the timeout manually instead, cleared on both 'error' and 'close'. * chore: add changeset for ripgrep file search migration * fix: drop system rg probe, add --no-config to close 3 blocking review issues * fix: stop excluding binary-extension files from find_files/autocomplete * fix: make file-search respect .nanocoderignore * chore: update changeset * fix: address ripgrep-backed file search review items Path validation, PCRE2 dialect detection, empty-dir walk skip, brace-expansion DoS cap, raw-scan bound, context headroom. * fix: dedupe ignore-rule construction and clarify walkProjectEntries API * fix: use ripgrep's --engine auto for PCRE2 detection instead of hand-rolled regex * fix: stop runRipgrep's stdout from overshooting maxLines/maxMatches within a chunk * test: shrink the stdout-overshoot regression test to 200 async-written files * fix: widen rgMaxCount headroom to contextLines, not a flat +1 * fix: bound globTokenCache by total token count, not entry count * fix: stream findMatchingPaths results instead of sorting then buffering * docs: replace the sorted-option comment's implied guarantee with the measured distribution * test: add resetRipgrepPathCache and proper test isolation for ripgrep-path.spec.ts * fix: match directory-only .gitignore patterns in walkEmptyDirectories
…sage (Nano-Collective#956) * feat(export): auto-generate descriptive filenames from first user message Replaces generic timestamp-based export filenames with descriptive slugs derived from the first 4 words of the user's first message. Before: nanocoder-chat-2026-08-25T09-54-14.353Z.md After: fix-my-button-2026-08-25.md - Add generateExportFilename() utility with word-boundary truncation - Add uniqueFilename() to handle collisions by appending counter - Add isUnsafeFilename() to block path traversal attacks - Add 16 unit tests covering edge cases and security - Update export command to use shared utilities - Closes Nano-Collective#934 * chore: add changeset for export filename feature * fix(export): reuse isValidFilePath for path safety and render export errors correctly - Delete isUnsafeFilename, which weakened the existing validator and rejected valid subdirectory exports (basename(f) === f rejected any path with a separator) - Validate with isValidFilePath(process.cwd()) from path-validation.ts, the same validator used by read_file/write_file/string_replace; restores subdirectory export while still blocking traversal, ~, null bytes, and absolute path escapes - Render path-traversal rejections with ErrorMessage instead of feeding 'Error: invalid filename' through SuccessMessage, which lied that the chat was exported - Assert on rendered text in the rejection test rather than only React.isValidElement * fix(export): harden uniqueFilename, keep overwrite for explicit names, fix i18n slugs - uniqueFilename now falls back to a timestamp-suffixed name after a bounded number of attempts instead of returning the original path that fs.writeFile would clobber, preserving the 'never overwrites' guarantee - Keep overwrite semantics for user-typed filenames; only generated names get auto-suffixed, so repeated /export notes.md overwrites as before - Preserve CJK, Cyrillic, and other non-ASCII in slugs via \\p{L}\\p{N} with the u flag instead of stripping to empty and falling back, which previously caused date-only collisions for non-ASCII users - Add tests for CJK/Cyrillic slugs, emoji stripping, the exhaustion fallback, and user-provided overwrite semantics * test(export): cover subdirectory, null-byte, and home-shorthand paths - Subdirectory export (reports/chat.md) must work now that validation is segment-aware via isValidFilePath, guarding the regression the old basename-based check would have caused - Null-byte and ~ home-shorthand paths must be rejected outright - Assertions are path-separator agnostic so they pass on Windows * fix(export): make generated-name writes atomic and surface write errors Previously a generated filename wrote to the target chosen by a separate check-then-act access(); two concurrent /export commands for the same slug could both pass the existence check and one would clobber the other, violating the never-overwrite guarantee. Replace the TOCTOU-prone uniqueFilename with writeUniqueFile, which creates the file atomically with the exclusive flag ('wx') and retries the next collision suffix on EEXIST until the bounded attempts are exhausted, then falls back to a timestamp-suffixed name. It never falls through to overwriting an existing export. The handler now routes the final write through the same atomic path for generated names while explicit user filenames keep overwrite semantics at the exact path the user typed. A failed write (ENOSPC, EACCES, EPERM) is caught and surfaced as an error message instead of being swallowed by the command dispatcher's catch-less try/finally and leaving the user with no feedback. Tests cover the free-path, counter-collision, exhaustion, and concurrent- writer cases, plus the write-failure surface, asserting on rendered output. * fix(export): enforce project containment and byte-aware slug limits The export path was resolved with path.resolve and validated only lexically, so a subdirectory export pointing at an in-project symlink could redirect the write outside the project, diverging from read_file/write_file/string_replace which defend at the resolve layer. Validate and resolve exports through resolveFilePath(process.cwd()), the same symlink-aware containment check used by the file tools. This both rejects traversal and absolute escapes that leave the project, and keeps subdirectory exports working, consistent with the rest of the codebase. Slug truncation was character-count based, which let 40 multi-byte CJK characters produce a ~120-byte slug. Truncation now also enforces a UTF-8 byte budget so the full filename stays well under the 255-byte filesystem limit. Tests cover traversal, absolute-path escape, and a byte-limited multi-byte slug, asserting on rendered output. * fix(export): resolve paths via session cwd and project root, surface clear errors Follow the read_file/write_file convention of resolving relative paths against the session cwd (which honours bash cd) and enforcing containment against the project root (which does not shrink as cd descends), instead of pinning to the static process.cwd(). Also surface a readable 'Parent directory does not exist' error for user-typed paths (writeUniqueFile already handled generated names) and route all error formatting through formatError so non-Error throws do not leak as '[object Object]'. Export feedback now echoes the file relative to the project root so subdirectory exports are recognisable. * fix(export): drop byte-budget guard and give clear missing-directory errors The manual UTF-8 byte-trim (MAX_SLUG_BYTES=96) guarded against a case that cannot occur: even a 40-char CJK slug plus the fixed date suffix lands far below the 255-byte filesystem limit, and the 40-char bound already caps the slug. Remove the ~25 lines of branchy byte-loop and keep the simple word-boundary character truncation, which is provably sufficient. In writeUniqueFile, translate ENOENT into an explicit 'Parent directory does not exist' error instead of leaking the raw fs path so generated-name exports fail with an actionable message. * chore(export): annotate path.join sinks that semgrep flags as false positives writeUniqueFile receives an absolute path already validated and containment-checked by resolveFilePath in the export handler, and derives dir/base/ext via path.dirname/path.basename/path.extname, so the generated collision candidates built with path.join can never escape dir. Add nosemgrep comments (matching the repo's existing convention in source/config/index.ts) to the two flagged lines to clear the blocking semgrep-scan CI job without silencing a real vulnerability. * fix(export): use rule-specific nosemgrep to fully suppress remaining semgrep finding The trailing inline '// nosemgrep' cleared the loop's path.join but not the timestamp-suffix line. Use the documented form -- '// nosemgrep: <full-rule-id>' on its own line directly above each path.join call -- applied uniformly so both sinks are suppressed. Path safety still originates from resolveFilePath in the handler; this join only appends a counter/timestamp to an already validated basename inside an already contained directory.
* feat(ui): colour syntax highlighting with the active theme Every cli-highlight call site passed `theme: 'default'`, a string where the library expects a token-to-formatter map. The option was silently discarded, so code always rendered in cli-highlight's own palette no matter which of the 50 themes was selected. Derive the map from the active theme instead. getSyntaxTheme() builds it from a palette — keywords take `primary`, built-ins and declarations `tool`, strings `success`, numbers `warning`, comments `secondary`, attributes and variables `info`, and everything else the theme's body `text` — covering every token cli-highlight styles itself, so nothing falls back to the library default. It is memoised per palette, since the diff and file previews highlight line by line. All five sites now pass it: markdown code blocks, both string_replace diff-context branches, the write_file preview, and the file explorer preview. Closes Nano-Collective#935. * feat(config): add syntaxTheme to give code a palette of its own Code follows `selectedTheme` as of the previous commit, which is the right default. Someone whose terminal already wears Dracula or Nord may want code coloured to match it while the rest of the UI stays where they put it, so read an optional `syntaxTheme` from preferences and let it win. It names any of the 50 existing themes rather than introducing a second registry, so the two cannot drift apart. An unknown or misspelt name falls back to the UI theme instead of dropping the styling. Resolved once and re-resolved only when NANOCODER_CONFIG_DIR moves, since the diff and file previews highlight line by line. `@/config/preferences` imports `@/config/index`, which imports this module. Neither touches the other at module scope, so the cycle resolves — verified against the built output for the three entry orders that reach it. The specs now pin NANOCODER_CONFIG_DIR at a directory of their own: a contributor who sets `syntaxTheme` must not change what they assert. * fix(themes): re-resolve syntaxTheme on write, and reject inherited keys Addresses review feedback on Nano-Collective#959. The override cache keyed on NANOCODER_CONFIG_DIR alone, which only ever moves under tests. In a real session syntaxTheme was read once per process, so a later preferences write was ignored until restart. It now keys on getPreferencesVersion() as well - a monotonic counter bumped on every write and free to read - so a change lands on the next highlight. The dir stays in the key because that is what the spec repoints. Required merging main, which is where getPreferencesVersion lives; the branch was 101 commits behind. `preset in themes` is now Object.hasOwn. `themes` comes from JSON.parse and carries Object.prototype, so a syntaxTheme of "constructor", "toString", "valueOf" or "__proto__" passed the `in` check and resolved to a non-theme whose .colors was undefined, with the ?? fallback rescuing it by accident. Also from the review: - An unknown syntaxTheme warns once, naming the value, instead of silently rendering as though the preference were unset. Through the structured logger rather than logWarning: @/utils/message-queue reaches @/components/message-box -> useTheme -> back to this module, a cycle it is deliberately kept out of, and getSyntaxTheme is called from render, where queueing a message is a state update during another component's render. - The file explorer keeps the plain source and derives the highlight in a memo keyed on the palette, so switching theme with a preview open recolours it instead of stranding it until reselect. - RenderPalette names the shared subset at its declaration; Colors stays as a deprecated alias so the markdown-parser call sites and its re-export are untouched. The markdown-parser spec comment is corrected but keeps the distinct palette object, because that part is load-bearing. chalk re-checks level per call for whether to emit codes, but bakes the colour MODEL in when the builder is made: chalk.hex() picks ansi16 / ansi256 / truecolor from chalk.level at creation. getSyntaxTheme memoises per palette identity, so reusing mockColors reuses builders frozen at the runner's default level - \x1b[91m against the assertion's \x1b[38;2;... Removing the trick fails the test; the comment now says why it is there. Cycle re-verified against the built output for five entry orders, including the new utils/logging edge.
…ano-Collective#1102) runRipgrep only rejected on exit > 1 when stderr also matched one of five hardcoded patterns. Anything unenumerated - an unreadable search root, a rejected argument, a build without PCRE2 - fell through to resolve(''), which find_files, search_file_contents, @-autocomplete and /repomap all report as a genuine "no results". A silent, permanent zero-result search is worse than a loud failure. Reject whenever rg exits > 1 with nothing on stdout. Exit 2 with output is still treated as a recoverable mid-scan warning, so one unreadable subdirectory in an otherwise readable tree keeps its results. FATAL_RIPGREP_ERROR_PATTERNS and isFatalRipgrepError are now unreachable, and their only remaining caller was a test, so both are removed rather than left as a test-only island.
…le out Follow-up to Nano-Collective#956. - `/export` rejections all rendered the same "outside the project directory" string regardless of cause, which was simply wrong for a null byte or `~`. Re-derive the specific reason so the message names what was wrong and, for `~`, what to use instead. - Move `writeUniqueFile` to `source/utils/write-unique-file.ts`. It is a generic path helper with nothing export-specific about it, so `generate-export-filename.ts` was a hard place to find it. Its tests move with it. - Record that the export date is deliberately UTC rather than local. - Two export specs created directories inside the working tree and cleaned up with a trailing `fs.rm`, leaving them behind on a mid-test failure. Use `t.teardown` so cleanup is registered up front. - Bump the changeset to minor (it is a feature) and document the deliberate containment narrowing: `~` is not expanded and absolute paths outside the project root are refused, matching read_file / write_file / string_replace.
…d visitor (Nano-Collective#1104) Two follow-ups from the Nano-Collective#928 review. .nanocoderignore was only applied as a JS filter over rg's output, so ignored paths were still traversed and still spent budget against maxRawFilesScanned. A large ignored fixtures directory sorting before src/ could consume the whole 50k cap and crowd real files out of the results. Pass the file to rg as --ignore-file so it prunes during traversal; rg applies those rules after .gitignore, which is the layering loadGitignore already documents. The JS filter stays as a backstop since rg's matcher and the `ignore` package are separate implementations. Omitted when the file is absent - rg warns on a missing path. walkProjectEntries' "onEntry must be synchronous when sorted: false" contract existed only in a throw message. Overloads make it a compile error, and the option is documented. Writing the test for that surfaced a worse problem: emitEntrySync throws from the stdout data handler, which runs on the stream's event loop turn rather than inside the promise executor, so the throw escaped as an uncaught exception and took the process down instead of rejecting the search. Route any throw from chunk consumption to reject.
…ve#1031) * fix: spawn cmd.exe with /c for custom tools on Windows execute_bash already used /c; custom tools always passed -c. Closes Nano-Collective#1028. * link quoting follow-up
* feat(init): add bundled preset configurations * fix(init): address preset review feedback --------- Co-authored-by: raheeb-gill <raheeb@loudlydev.com>
* chore: label PRs by changed path * fix labeler review * fix labeler followup
The generated .nanocoder/commands/check.md declared aliases: [verify] in all three presets. CustomCommandLoader registers aliases with a plain Map set, so the last writer wins silently: initializing a project would hijack an existing user command aliased verify with no warning. The command is still reachable as /check, and users can add their own alias if they want one. Pins the absence with a spec over every supported preset.
…tep (Nano-Collective#1092) * fix(vscode): migrate to Tailwind v4 @theme and add CSS verification step * docs: add changeset for vscode tailwind fix * fix(ci): create assets directory before building vscode extension * fix(ci): suppress semgrep regex false positive in verify-theme-css
) Wire FAIL_ON_COVERAGE_DROP to a local c8 comparison against the PR base. Keep the 80% floor. Closes Nano-Collective#1052.
…ff_edit (Nano-Collective#1086) Both tools passed the model's replacement straight to String.prototype.replace. That second argument is a substitution template, not a literal, so the engine ran GetSubstitution over it and rewrote four token sequences before the bytes reached disk: `$$` collapsed to a single `$`, `$&` injected the matched text, and the backtick and quote forms injected everything before / after the match. The last two duplicate a whole half of the file into the middle of the edit, so the damage scaled with file size. Those are ordinary characters in shell scripts, Makefiles, docker-compose files, CI YAML and anything that builds a regex. The tool reported success and nothing warned the model or the user, so the approval gate was bypassed by construction: the confirmation renders old_str / new_str directly, so the diff the user approved was not the diff that landed. replaceFirstLiteral splices by index instead. That sidesteps substitution parsing entirely and avoids re-scanning the string for a second pass. Both write paths use it, as do the two previews that synthesize post-edit content - the VS Code diff in the string_replace formatter and the ACP whole-file diff - so what is shown stays what is written. Every call site is guarded by an existing uniqueness check, so the helper's not-found branch is a defensive no-op rather than a silent skipped edit. Closes Nano-Collective#1057
* feat: add semantic memory storage foundation (Nano-Collective#649) * feat: add semantic memory storage foundation * fix: align semantic memory schema with phase 1 * feat: add manual semantic memory creation * test: cover semantic memory error paths * feat: inject semantic memories into prompts * fix semantic memory * feat: add semantic memory management * fix semantic memory category (Nano-Collective#709) * feat: add semantic memory setting (Nano-Collective#711) * feat: add provenance and warnings to semantic memory proposals (Nano-Collective#716) * fix: address review feedback on semantic memory * fix: address round-4 review feedback on semantic memory (Nano-Collective#882) * fix: close leftover semantic memory review gaps (Nano-Collective#991) Subagent/daemon recall, cross-instance write locking, and a 500-entry store cap. Refs Nano-Collective#619 * fix: format settings and memory files * fix: drop dynamic regex from reversal detection * fix: keep /clear from wiping tasks and loosen recall * fix: cache memory managers and repair store edge cases * fix: drop a dead ranking branch and log recall failures properly --------- Co-authored-by: Sk Akram <skcodewizard786@gmail.com> Co-authored-by: Luis Edward Miranda <36224337+llupRisinglll@users.noreply.github.com> Co-authored-by: Will Lamerton <89926355+will-lamerton@users.noreply.github.com>
…ve#1080) A tool call made inside a dispatched sub-agent goes through the global approval slot in tool-approval-queue. Only the Ink TUI installs a handler there, so under ACP the slot fell back to denying, and the sub-agent got "Tool execution was denied by the user." without the client ever being asked. A client that gates writes saw and decided every top-level call and nothing a sub-agent did, so delegated work could only write by bypassing approval. Install a handler for the turn that forwards these to the same session/request_permission channel the top-level calls use. The call is announced first, because a permission request naming a tool call the client has not seen is rejected as invalid params, and the title carries the sub-agent name so the client can tell the two sources apart. A denied or cancelled decision marks the announced call failed. The terminal status after an approval is not emitted, because the sub-agent layer does not report its tool results back to the ACP conversation. That gap predates this change. Closes Nano-Collective#1019
The changeset shipped with Nano-Collective#1080 said "the handler is scoped to the turn that installs it, so concurrent sessions cannot answer each other's approvals". They still can. turnActive is per session (acp-session.ts, guarded in acp-agent.ts), so two sessions can be mid-turn at once, and the approval slot is a process-wide singleton with last-writer-wins semantics. For the overlap the later installer answers the earlier session's approvals against the wrong session id and abort controller - the exact mis-routing the disposer was added to address. What the disposer does fix is the leak past the turn: a finished turn's session, connection and aborted controller no longer stay reachable, and the restore is LIFO-correct, so routing rights hand back when the later turn ends. Say only that, in both the release note and the comment, and name what closing the window would take: keying the slot by session id, or threading an approval channel through SubagentExecutor. Also record the other half-truth. An approved sub-agent call is marked completed at approval time rather than when it runs, because the sub-agent layer does not report results back, so a client sees completed for a tool that may still fail. That was a deliberate trade against leaving the card spinning forever, but it was undocumented. Restore the cancelled-permission test dropped when the sub-agent tests were rewritten to signal from inside the turn. Nothing covered the permission === 'cancelled' branch, so the distinct "Cancelled by user" output was free to regress into the deny path; both messages are now asserted.
* fix(subagents): preserve object tool output * chore: add changeset for structured subagent output
|
Thanks for the detailed review. I addressed the requested issues in
The PR is rebased through a normal merge commit onto the current upstream |
Summary
/retryCloses #1060.
Validation
pnpm exec ava source/app/sections/interactive-app.spec.tsx source/hooks/useAppHandlers.spec.tsx source/app/utils/handlers/retry-handler.spec.ts— 42 passedpnpm run test:format— passedpnpm run test:lint— passedpnpm run test:typesandpnpm run test:types:vscode— passedpnpm exec tsc,pnpm exec tsc-alias, and the Windows-equivalent asset copy step — passedpnpm run test:knip— passedgit diff --cached --check— passedThe full AVA suite was attempted. It reported 79 unrelated Windows/path/network/platform failures and 292 tests pending after timeouts; the changed queue and retry suites passed. The configured
pnpm auditendpoint is unavailable on the repository's npm mirror, and the packagebuildscript uses Unixcp, so those two checks were documented with their Windows limitations.