feat(commands): add inline overrides for slash commands - #1183
MayurK-cmd wants to merge 2 commits into
Conversation
will-lamerton
left a comment
There was a problem hiding this comment.
Nice, clean parser and the try/finally around the dispatcher is the right structural move. Types, lint, changeset validation and the 16 parser specs all pass locally for me. A few correctness issues need fixing before this can go in.
Blocking
1. restore() wipes a pre-existing session override instead of restoring it.
restorations.push(() => setAutoCompactThreshold(null)) resets to the config default, not to whatever was there before. Verified on the branch:
before (user ran /compact --threshold 90): 90
during (/compact ?threshold=80): 80
after restore (expected 90): null
Same for auto-compact (setAutoCompactEnabled(null)) and context-max (resetSessionContextLimit()). A "temporary" override silently destroys the user's real session setting, which is the opposite of what #1151 asks for. Please read the current value first (autoCompactSessionOverrides.threshold, getSessionContextLimit()) and restore that.
2. ?context-max=128k sets the limit to 128 tokens.
applyOnceOverrides uses Number.parseInt(String(value), 10), while every other caller uses parseContextLimit from source/utils/parse-context-limit.ts, which handles the k suffix. Confirmed: getSessionContextLimit() returns 128. Please use parseContextLimit.
3. Both examples in the PR description do not do what the description says.
threshold is not in LEGACY_FLAG_NAMES, so expandOverrideArgs returns [] (your own test asserts this). /compact ?threshold=80 therefore runs a plain manual compaction, does not print Auto-compact threshold set to 80%, and the temporary threshold has no effect at all because manual /compact never reads it - only the auto-compact path does. And ?once in /context-max 128k ?once is not a recognised key anywhere, so it is a no-op while 128k sets the limit permanently. Either wire these up or correct the description.
4. The override is stripped for ~11 handlers and passed through raw to every other command.
handleBuiltInCommand(message, options) still receives the original message and re-splits it, so every lazy-registry command (/model, /provider, ...) sees the literal ?foo=1 token, and ?preview never becomes --preview for them. handleRetryCommand also re-derives its args from raw message. The "handlers stay oblivious to the override feature" comment only holds for the commandParts handlers.
5. No test covers the feature actually working.
All four applyOnceOverrides tests are t.notThrows(() => restore()) on empty/unknown/unparseable input. None asserts that an override is applied or correctly reverted, which is why issue 1 is invisible to the suite. The app-util additions are described as "4 new dispatcher tests" but none of them invoke handleSlashCommand. Please add a test that sets a prior value, applies an override, restores, and asserts the prior value is back.
Minor
- UTF-8 BOM (
EF BB BF) at the top ofsource/utils/inline-overrides.tsand.changeset/inline-once-overrides.md. Biome and@changesets/parseboth tolerate it, but no other file in the repo has one. Please strip it. - Validation asymmetry:
/compact --threshold 40errors with a message,?threshold=40silently clamps to 50. .gitignore.pnpm-store/is unrelated scope creep and is one of the two merge conflicts againstmain(the other isapp-util.ts). Please rebase.- New imports sit at line ~1088 mid-file in
app-util.spec.ts. Specs are biome-excluded so nothing complains, but it is against the file's convention. typeof arg !== 'string'inparseInlineOverridesis unreachable given thereadonly string[]parameter type.- The
parseInlineOverrides(['? bad'])case cannot occur in practice, the dispatcher splits on/\s+/so no token can contain a space.
4646207 to
67eddb5
Compare
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for the rework. Items 1, 2, 4 (for built-ins and retry) and 5 are properly fixed, and the test coverage is genuinely solid now. I re-verified on a clean worktree at 22297efb: tsc clean, biome lint and biome format clean, ava on inline-overrides.spec.ts + app-util.spec.ts + auto-compact.spec.ts gives 137 passed / 0 failed (the bash command - queues a completed BashProgress with showOutput failure did not reproduce for me), no conflicts against main. Two things still need fixing.
Blocking
1. knip fails, and it will break the release workflow.
The split into source/utils/auto-compact-session.ts re-exports two symbols nothing consumes:
Unused exports (1)
autoCompactSession source/utils/auto-compact.ts:26:2
Unused exported types (1)
AutoCompactSessionOverrides type source/utils/auto-compact.ts:24:14
main is knip-clean. pnpm test:knip runs in release.yml, not pr-checks, so this merges green and then breaks release on main. Please drop the two unused re-exports.
2. The changeset examples are no-ops (this is my earlier item 3, still open).
autoCompactSessionOverrides.threshold / .enabled are read in exactly two places, maybeAutoCompact (source/utils/auto-compact.ts:89-112) and useAppHandlers.tsx:377-391. Both are the automatic compaction path, which only runs on a chat turn, never during a slash command. Since restoreOnce() fires in the dispatcher's finally, the override is applied and reverted with nothing in between that reads it.
So both examples in .changeset/inline-once-overrides.md do nothing:
/compact ?threshold=80- past the flag loophandleCompactCommandonly usesmode,previewandstrategy. It never consults the threshold./context-max 128k ?auto-compact=on-128ksets the limit permanently, andauto-compact=onis applied then reverted unread.
The one key that genuinely works today is ?context-max on /usage: source/commands/usage.tsx:177 reads getSessionContextLimit() inside the awaited handler, so /usage ?context-max=200k really does render against 200k. That is the example the changeset should lead with. Either wire threshold / auto-compact so some command actually observes them, or narrow the changeset and the PR description to the case that works. As it stands the CHANGELOG would ship two examples that do nothing.
The PR description is also still the original text: it documents ?once (not a recognised key anywhere) and claims /compact ?threshold=80 prints Auto-compact threshold set to 80%. Please bring it in line with the code.
Non-blocking, but worth a decision
- Custom commands still bypass the feature entirely.
handleCustomCommand(message, commandName, options)atsource/app/utils/app-util.ts:681runs before the override parsing with the raw message, so.nanocoder/commands/*.mdsee?foo=1literally. Fine as a deliberate scope call (it is also why free-text custom commands do not silently lose?wordtokens), just say so somewhere. - Unknown
?foo=barkeys are now dropped entirely before the handler sees them, so/compact ?threshhold=80is an invisible no-op. Combined with--threshold 40erroring while?threshold=40is silently skipped, the override path has no error surface at all. Worth at least a "unknown override key" message. expandOverrideArgs([{key: 'preview', value: 'yes'}])returns['--preview', 'yes'], leaving a strayyespositional (your own test asserts this). Harmless for/compact, wrong shape for any handler that reads args positionally.import {parseInput} from '@/command-parser'is still out of order in theapp-util.spec.tsimport block. Cosmetic, biome excludes specs.
The BOMs, the .gitignore creep, the unreachable typeof arg !== 'string' guard and the '? bad' case are all resolved. Fix the two blockers above and I think this is good to go.
|
Hi @MayurK-cmd, thanks for this PR! It looks like a maintainer has left feedback Whenever you get a chance, could you take a look at the open comments? |
87a63c6 to
2afa9cd
Compare
|
Rebased onto current main (was 245 behind) and addressed the two blockers. Conflicts in app-util.ts (kept the #1162 MCP-prompt intercept; MCP prompts and custom commands both bypass ? parsing with the raw message) and auto-compact.ts (kept the auto-compact-session.ts split, now using clampThreshold, with auto-compact.ts re-exporting so existing importers are untouched) are resolved.
Tests: tsc/lint/format/knip clean; inline-overrides.spec 21/21, auto-compact.spec 41/41, new app-util.spec override tests 5/5. The bash … showOutput failure is the pre-existing #826 one, unrelated. Also updated the PR description — the old text still documents ?once and claims /compact ?threshold=80 prints a confirmation, which isn't what the code does. |
|
/re-review |
nc-review: comments — 2 important, 3 nits@MayurK-cmd — a few things worth a look, none blocking. Adds inline 🟠 important · The PR is titled The changeset openly says 🟠 important · The PR description's example list says: That is not what the code does.
Pick one. The cleanest fix is to either (a) actually preserve unrecognised ⚪ nit · The PR description's 'Files' list includes '.gitignore (.pnpm-store/)' as one of the touched files, but no ⚪ nit · The diff includes a non-trivial refactor — extracting The refactor is well-scoped to the problem, and the spec files ( ⚪ nit · Of the four new dispatcher tests in this file, the third — 🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with |
2afa9cd to
a18e443
Compare
|
Hey @MayurK-cmd! Could you please resolve the merge conflicts first? The branch is currently conflicting in Also, the required Unit Tests & Coverage Analysis check is currently failing, so please fix the failing test and push the updated changes. Once both are resolved, we can take another look. |
a18e443 to
0bb9c05
Compare
Description
Closes #1151
Slash commands now accept inline
?key=valueoverrides that scope a setting to the current command only. Lets users test a value for one command without round-tripping through/settingsor otherwise mutating the session state.How it works
parseInlineOverrides(args)(new, pure) splits an args array into{args, overrides}. Only tokens starting with?areconsidered; bad keys (leading digit, whitespace) are kept in
argsso the command's own "unknown arg" path runs.expandOverrideArgs(overrides)turns recognised legacy keys (preview,llm,mechanical,aggressive,conservative,auto-on,auto-off) into the corresponding--flag valuetokens, so existing handlers see them as if the user had typed thelong form.
applyOnceOverrides(overrides)is async and writes recognised session-override keys (threshold,auto-compact,context-max)to the existing
setAutoCompactThreshold/setAutoCompactEnabled/setSessionContextLimitsetters. It returns arestorecallback that the dispatcher calls in a
finallyblock, so the override only persists for the single command invocation.source/app/utils/app-util.tsis wrapped in atry/finallysorestorealways runs, even if ahandler throws.
Adding a new inline-override key is a one-line change: add a
caseinapplyOnceOverridesand (if it isn't an existing--flag)optionally add an entry in
LEGACY_FLAG_NAMES.Why async
applyOnceOverridesdefers the heavyauto-compact/models-dev-clientimports via dynamicimport()so the parser itself is import-free. Without this, the spec timed out under AVA's serial worker because importing@/utils/auto-compactpulls in the chat-handler / config / tokenization init graph.Files
source/utils/inline-overrides.ts(new)source/utils/inline-overrides.spec.ts(new)source/app/utils/app-util.ts(wrap dispatcher in try/finally, rebuildcommandPartsfromparseInlineOverrides+expandOverrideArgs)source/app/utils/app-util.spec.ts(4 new dispatcher tests).changeset/inline-once-overrides.md(minor: new feature).gitignore(.pnpm-store/)Out of scope
The pre-existing
bash command - queues a completed BashProgress with showOutputtest inapp-util.spec.tsstill fails on this branch (it has been failing since #826 / July 2026). It is unrelated to this change and is not fixed here. Worth a separate issue.Type of Change
Changeset
pnpm changeset) describing this change for the changelog.changeset/inline-once-overrides.mdis aminorchangeset naming@nanocollective/nanocoderand explaining the new?key=valuesyntax. Validated locally with
node scripts/validate-changesets.js(72 changesets checked, all resolve).Docs-only or internal chores need no changeset (or run
pnpm changeset --emptyto note that intentionally).Testing
Automated Tests
.spec.ts/tsxfilespnpm test:allcompletes successfully)Verified locally:
pnpm test:ava source/utils/inline-overrides.spec.ts→ 16/16 passedpnpm test:ava source/app/utils/app-util.spec.ts→ all 4 new inline-override tests passed; one pre-existing unrelated failure(
bash command - queues a completed BashProgress with showOutput, present since feat: add session-scoped artifact lifecycle across CLI and VS Code #826) — see "Out of scope" abovepnpm test:types→ cleanpnpm test:lint→ 516 files, 0 fixes neededThe 16 parser/helper tests cover: empty input, single key=value, mixed positional + override, bare
?flag(no=),?key=valuewith extra
=in value, dotted/dashed/underscored keys, invalid keys preserved in args,expandOverrideArgsfor both boolean and key=value forms and unknown keys, plusapplyOnceOverridessmoke tests for empty input, unknown keys, unparseablethresholdvalues, and unparseablecontext-maxvalues.Manual Testing
This change is provider-agnostic — the override is wired into the slash-command dispatcher, not into any provider's chat pipeline.
Manual end-to-end testing against a real provider was not done as part of this PR; the change was validated via the unit/
integration tests listed above. The session-override stores it writes to (
auto-compact,setSessionContextLimit) are alreadycovered by their own specs and used in production by
/compact --thresholdand/context-max <n>.Checklist
No documentation update included in this PR — the override syntax is a small extension of existing
--flagbehaviour and the new docblock onapplyOnceOverridesdocuments the public surface. Happy to add a docs page if reviewers want one.No logging added: the override is purely a session-override passthrough that uses the existing setter functions. A successful
override is observable via the normal command result (e.g.
/compact ?threshold=80prints the same "Auto-compact threshold set to 80%" success message as/compact --threshold 80). Failures (unparseable values) are silently ignored per the same behaviour the existing--thresholdparser already follows (the value simply isn't applied).