fix: hide unsupported /todos slash command from menu - #8829
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All evidence is in. The fix reuses the existing First-Principles-Verdict: PASS Removes a menu entry that always errors, using the existing blocklist and its documented frontend mirror — nothing new is invented. What this change shipsIntent: stop the dashboard from offering
Watch
[FIRST-PRINCIPLES-REVIEWED] 21231ad |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Analysis complete. The fix reuses the existing Design-Verdict: CONCERNS A kiro-specific capability gap is fixed with a harness-global block, downgrading Watch
[DESIGN-REVIEWED] 21231ad |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS Removing the dead The blind reader parsed both post-removal notices accurately ("nothing… starts with 'tod', and pressing Enter will send it as plain text") and found shots 2/4 indistinguishable — which is the fix working, since fallback and live menus are meant to mirror. The reader's unease that "Couldn't load commands" "sounds like something went wrong" concerns a pre-existing string this diff does not touch ( [UX-REVIEWED] 21231ad |
48746b9 to
ca15a11
Compare
ca15a11 to
8f5233e
Compare
8f5233e to
d5e7e69
Compare
|
Design finding 1 — "a global blocked set now encodes one harness's gap, stripping a live-advertised command from the claude backend": partially rebutted, partially accepted-and-deferred to #8939.
The menu half does not hold. cc_commands = ["compact", "clear", "context", "help", "init",
"review", "security-review", "usage"]plus the appended The runtime half does hold. This PR's commit message already records the global block as a deliberate choice |
|
Design finding 2 — "~600KB of PNGs ... once on
The committed-under- The "permanent history" premise is also incorrect.
So these four files are pruned on a timer within days of merge, and the PR body's Keeping the code as-is. The 628KB is four PNGs of a menu at two states on two menu |
Inherited
|
| revision | files referencing the key |
|---|---|
31bd900ff |
2 |
2d18f68f2 — #8816, "route error states through ErrorNotice (batch pages-rest-1)" |
1 |
b75889c69 — #8859, "route error states through ErrorNotice (batch components-1)" |
0 |
Two ErrorNotice routing batches replaced bespoke error copy with a shared component;
the call site went away while the catalog entry stayed. That is the failure mode the
test's own message names — "copy deleted without its key".
Measured, not inferred
An offline replication of the test's own dead computation (flatten the catalog, then
count keys with no quoted literal occurrence in website/src/**/*.{ts,tsx}), validated
by reproducing CI's 30 on this head before comparing anything:
| revision | dead.length |
|---|---|
d5e7e691 — this PR's head at the time |
30 |
cb3aa2c5b — origin/main at the time |
30 |
d580bcb2e — this PR's rebase base, pure main |
30 |
a8205e559 — this PR's pre-rebase base |
29 |
The rebase base already measured 30 with none of this PR's changes in it.
Why this diff cannot move the count
- It touches no
locales/,i18n/, catalog orpluralKeys.jsonfile. - It adds and removes zero quoted dotted catalog keys.
components.slashCommandMenu.desc_todosis still literally referenced at
SlashCommandMenu.tsx:73('/todos': 'components.slashCommandMenu.desc_todos'), and
is confirmed absent from the full 30-key dead list. The printed list is
dead.slice(0, 12)and is sorted, so that key would have appeared at position 11 —
betweencomponents.runInTerminalBtn.run_anywayandpages.artifactDetailPage.iterate—
had it been dead.
Why it surfaced here first
#8859 merged at 07:54:57Z. This PR's rebase base d580bcb2e is dated 07:55:20Z,
so the regression was already in the base 23 seconds later. main's own runs around
07:55 were largely cancelled by concurrency, so this PR was the first place the red
became visible. It was never this PR's defect.
Resolution
02fa7c201 — "fix(i18n): drop the dead pages.chatPage.dismiss_upload_error key",
on main at 08:34:35Z — removed the key from the catalogs, which is the remedy the
test's doctrine calls for (ratchet down when keys are removed; never raise BASELINE).
origin/main now measures dead=29 against BASELINE = 29, key count 12600 → 12599.
This PR rebased onto it: head bc64cfbaf74d5377e6e011060068e5c87116c287 measures
dead=29. The code diff is byte-identical across the rebase, still one commit on base.
BASELINE was deliberately not touched from this PR — bending a repo-wide ratchet
inside a "hide an unsupported slash command" fix would have absorbed another PR's
regression under an unrelated title and left the ratchet blind to the next orphaned key.
Note for anyone hitting this class again: consume a main-side fix with a rebase.
gh run rerun re-tests the original merge-ref snapshot and fails identically.
Tracked at #8959.
Second inherited red on head bc64cfbaf — Backend Tests (3.12, 4)
The rebase cleared the deadKeys cascade above. What remains is a different
main-side red, also not reachable from this diff:
Backend Tests (3.12, 4) -> test/test_snapshot.py::TestNotificationCopyWhenNoLiveFileExists
Coverage Gate -> fail-closes off it
PR Readiness -> red, and it is the sole required check
The failing assertion is
test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery —
a delivery on a fresh gateway ran concurrently with the copy: 'no pool' was read as 'no writer'.
test/test_snapshot.py is not in this PR's diff, and a slash-command frozenset plus a
frontend menu list have no coupling to snapshot notification-copy ordering. The class
passes deterministically in isolation locally (25 passed, 4 consecutive runs).
It fails on main itself, alternating between the class's two ordering assertions:
| run | head | failing method |
|---|---|---|
34022725589 08:45Z |
8aef8fe3f (main) |
test_a_FRESH_gateway_still_orders_the_copy_against_a_delivery — same method and assertion as here |
34026408948 10:04Z |
d5955744f (main) |
test_a_note_delivered_during_the_copy_survives_the_READER |
Interleaved with green main runs (d3e9cfe93 08:12Z success, d4c2cbf22 09:15Z
success), so the class reds shard 4 on main roughly half the time. The class arrived in
be7de18fb (#8576, "fix(snapshot): validate notification records the copy branch
installs").
Already tracked as #8893 — "flaky: TestNotificationCopyWhenNoLiveFileExists ordering
asserts race on loaded CI shards" (open), with #8915 and #8976 closed against the same
symptom.
One targeted gh run rerun --failed was spent on it (attempt 2 of run 34024712213) and
it failed identically. Note that a --failed rerun re-tests the original merge-ref
snapshot, so it re-rolls runner timing but not the tree — with the class failing ~50% of
the time on main, further reruns are a coin flip rather than a fix, so this PR stops
here rather than burning CI on them.
This PR needs no content change for it. Once #8893 lands on main, consume it with a
rebase — never gh run rerun.
d5e7e69 to
bc64cfb
Compare
|
Design finding 1 — "it ships 600KB of
This shape is what the repo's own gate instructs, not an author preference. The "never cleanly deleted" premise is the part that does not hold, and the repo
So the hotlink is not what traps the files; it is what lets them be pruned without That property is also what rules out the suggested alternative: a GitHub Keeping the code as-is: 628KB is four PNGs covering two menu paths in two query states, |
|
Design finding 2 — "a claude-backend harness that actually implements
The "hidden from the menu" half does not hold on the shipped codeThe claude branch of cc_commands = ["compact", "clear", "context", "help", "init",
"review", "security-review", "usage"]plus the appended The "refused at execution time" half does hold
The decision this finding asks for, put to a maintainerThe question is narrow: does the Claude Code harness accept
Not resolving it inside this PR, deliberately: scoping the block on an unverified premise |
The dashboard slash-command menu offered /todos, but the kiro-cli ACP harness does not implement it and rejects the request with an "unknown variant" error, so selecting the entry always failed. /todos was listed in _SLASH_COMMANDS (so the menu advertised it) but not in _BLOCKED_SLASH_COMMANDS, the set that hides TUI-only / unsupported commands from GET /api/slash-commands and refuses them at execution time. Add it there. The frontend keeps a hand-maintained FALLBACK_COMMAND_NAMES list that is documented as "_SLASH_COMMANDS minus _BLOCKED_SLASH_COMMANDS" and is rendered before the API resolves (or when it fails); drop /todos from it too so both menu surfaces agree, and pin its absence in the offline fallback test. Deliberately a harness-global block with no provider-capability gating; a per-backend blocked set is the follow-up shape if a second such command appears. Screenshot evidence for the visible menu change is under temp-screenshots/slash-menu-no-todos/ (fallback menu and live-API menu, each with a "/tod" query that now matches nothing). Co-authored-by: Kiro Crew <noreply@kiro.dev>
bc64cfb to
21231ad
Compare
|
Design finding — "a kiro-specific capability gap is fixed with a harness-global block, downgrading
Both cited call sites check out against source
This is a better-aimed argument than the previous round's, and it is accepted as such: it identifies the actual mechanism rather than the menu payload, which is where my earlier response showed the claim did not hold ( The miscount is concededThe commit message's "if a second harness-specific gap appears" is wrong: What the decision turns on, and why it is still a decisionThe repository cannot establish whether the Claude Code SDK implements
That asymmetry is why this is not something to resolve by guessing: scoping the block on an unverified premise would re-introduce the class of bug this PR fixes — a forwarded command the harness does not implement — on the claude path instead. Putting the question to a maintainer, which is what the finding asks for. #8939 owns the implementation either way and starts by making the claude command list harness-derived rather than hardcoded, which is what would let a gate answer this mechanically instead of by inspection. |
Problem / Motivation
The dashboard slash-command menu offered
/todos, but the kiro-cli ACP harness does not implement it and rejects the request with an "unknown variant" error. Selecting the menu item therefore always failed.Why it matters
Every user who picks
/todosfrom the menu gets an error instead of a result. Advertising a command that never works teaches a gesture that does not work, and the same inert entry was visible on the frontend's offline fallback list too.What changed (motivation → approach → change)
Symptom:
/todosin the menu → "unknown variant" error from the harness.Root cause:
/todoswas listed in_SLASH_COMMANDS(so the menu advertised it) but not in_BLOCKED_SLASH_COMMANDS, the set that hides TUI-only / unsupported commands fromGET /api/slash-commandsand refuses them at execution time. The frontend'sFALLBACK_COMMAND_NAMES(rendered before the API resolves, or when it fails) is documented as_SLASH_COMMANDSminus_BLOCKED_SLASH_COMMANDSand still listed/todosas well.Change:
src/kiro_crew/dashboard/chat_utils.py: add/todosto_BLOCKED_SLASH_COMMANDS, with a comment explaining why. This reuses the existing mechanism (the/tangentprecedent).website/src/components/SlashCommandMenu.tsx: remove/todosfromFALLBACK_COMMAND_NAMESso the fallback path hides it too, and update the comment.Pre-existing drift left alone:
/goalis in_SLASH_COMMANDSbut absent fromFALLBACK_COMMAND_NAMES; that is the same mirror-drift class but unrelated to/todos, so it is left for a follow-up rather than widening this fix.Scope cut, stated explicitly:
_BLOCKED_SLASH_COMMANDSis harness-global, so/todosis also hidden and refused on the claude backend. This PR deliberately does not add per-provider capability gating; if a second harness-specific gap appears, a per-backend blocked set is the right follow-up shape.Tests
test/test_api_slash_commands.py: extendedtest_blocked_commands_absent_from_suggestionsto assert/todosis not advertised byGET /api/slash-commands.website/src/test/SlashCommandMenu.test.tsx: added/todosto theBLOCKEDlist pinned absent on the API-failure fallback path (fails with/todosstill inFALLBACK_COMMAND_NAMES, passes without it).pytest test/test_api_slash_commands.py→ 4 passed;vitest run src/test/SlashCommandMenu.test.tsx→ 14 passed;flake8andeslintclean on touched files.Manual verification
Built SPA, menu opened with
/. Before:/todoswas listed on both surfaces. After (this branch):Fallback menu (API stubbed to 503) -- no
/todosrow; typing/todmatches nothing:Live API menu (payload from the fixed backend) -- same result:
Related Issues
no linked issue: reported directly, no tracked issue exists for this defect.
Pattern harvest
Rule candidate: review-prompt
Pattern: a hand-maintained frontend fallback list documented as "mirrors backend set X minus Y" drifts silently when Y changes in a backend-only PR — a change to
_BLOCKED_SLASH_COMMANDSshould always be checked againstFALLBACK_COMMAND_NAMES.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Backport: please include in release/0.6.0.