Skip to content

fix: resume queued prompts after slash commands - #1069

Open
mikemikimike wants to merge 27 commits into
Nano-Collective:mainfrom
mikemikimike:fix/issue-1060-compact-queue
Open

fix: resume queued prompts after slash commands#1069
mikemikimike wants to merge 27 commits into
Nano-Collective:mainfrom
mikemikimike:fix/issue-1060-compact-queue

Conversation

@mikemikimike

@mikemikimike mikemikimike commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • forward slash-command completion through the app handler lifecycle
  • drain queued prompts only after generation, tool execution, cancellation, decision prompts, and modal modes are idle
  • make queue draining idempotent across delayed command completion and retried turns
  • keep queued-message editing and removal available while idle
  • add regression coverage for generation and modal guards, delayed completion, multiple queued prompts, and /retry

Closes #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 passed
  • pnpm run test:format — passed
  • pnpm run test:lint — passed
  • pnpm run test:types and pnpm run test:types:vscode — passed
  • pnpm exec tsc, pnpm exec tsc-alias, and the Windows-equivalent asset copy step — passed
  • pnpm run test:knip — passed
  • git diff --cached --check — passed

The 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 audit endpoint is unavailable on the repository's npm mirror, and the package build script uses Unix cp, so those two checks were documented with their Windows limitations.

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 second handleChatMessage on top of the live one. There is no re-entrancy guard, so setAbortController (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-89 awaits onHandleChatMessage and then calls onCommandComplete. The awaited turn already drains via onConversationComplete, so the handler's own call drains a second message while the first is starting. create-handler.ts has 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.onCommandComplete is missing from handleMessageSubmit's dependency array (useAppHandlers.tsx:718-751). It lints clean only because bare props is 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, :638 and :669 gate the key handling on isBusy while 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.

mikemikimike and others added 24 commits August 31, 2026 05:00
…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
@mikemikimike

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. I addressed the requested issues in 5642f223:

  • Queue draining now waits until generation, tool execution, cancellation, abort-controller, confirmation/question, subagent approval, and modal/settings state are idle. The drain is guarded against re-entry and schedules another pass after each dispatched turn settles.
  • /retry no longer emits a second completion signal after its awaited chat turn, so it cannot start a second queued turn.
  • Queued messages can be selected, edited, or removed while idle.
  • Added regression coverage for generating and modal states, delayed command completion, multiple queued prompts, and /retry completion ownership.

The PR is rebased through a normal merge commit onto the current upstream main; no history was force-pushed. Focused validation passes with 42 tests, and format, lint, type checks, the Windows-equivalent build steps, and knip pass. The full AVA and audit limitations are recorded in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] After compaction, queued messages do not fire and cannot be edited/deleted