fix(learn): acknowledge /learn on the GUI surfaces and name the created skill - #111316
Open
DavidMetcalfe wants to merge 6 commits into
Open
DavidMetcalfe wants to merge 6 commits into
DavidMetcalfe wants to merge 6 commits into
Conversation
The skill_manage tool result string goes only to the model, and the CLI/TUI completion line fell back to the generic branch, so no surface ever said a skill was created (issue NousResearch#52085). Register a _cute_skill_manage renderer that names the skill with an action-appropriate verb, reusing _cute_trunc and _result_succeeded. Refs NousResearch#52085
The two GUI surfaces route these built-ins through command.dispatch, and the
_prompt_builtin payload carried only {type: send, message: <builder prompt>}:
no notice (so no acknowledgement line, which the CLI and messaging gateway
both print) and no display (so the clients echoed the ~5 KB model-facing
builder prompt as the user's own chat bubble).
Return notice + display alongside message, matching the sibling
_dispatch_skill/_dispatch_bundle precedent. Ack wording is copied verbatim
from gateway/run_inbound.py; display is the slash command as typed. Clients
already render both fields — no ui-tui/desktop changes needed.
Fixes NousResearch#52085
skill_manage's schema advertises ONE call shape — an operations array — but _cute_skill_manage read only the flat top-level action/name, so a batch like [create a, patch b] rendered 'updated skill' with no name at all (the exact gap NousResearch#52085 was about), a sole-delete batch rendered the wrong verb, and a failed batch rendered 'skill skill' from colliding verb/name fallbacks. - Read the first operation from a.get('operations') (flat shape still accepted for legacy transcripts and staged-write replay). - Multi-op batches append ' +N' so the line admits it summarizes a batch. - Failed/unknown results now report the action verb (fallback 'updated', never the 'skill' noun) — kills the 'skill skill' collision — and the _result_succeeded branch covers result=None (absence of evidence is not evidence; the CLI path can pass result=None). - staged: true results (skills.write_approval pending queue) render 'staged <name>' instead of claiming a write that hasn't landed. - Drop the a.get('skill') alias: 'skill' is a cronjob_manage argument, not a skill_manage one. Tests: 6 new cases in TestCuteSkillManage covering batch/multi-op/ sole-delete/failed-batch/staged/unknown-action shapes.
CommandDispatchParams.arg is declared 'str | None' (contracts/tools_commands.py) and
validate_params rejects only extra keys — type/required checks are deferred to handlers
(contracts/registry.py). params.get("arg", "") does not default a present-but-null key,
so an explicit "arg": null reached the new acks and crashed on .strip(): a regression
against the pre-change behavior, where the builders already tolerated null via
(user_arg or ""), and against the messaging gateway, which strips at the source.
Normalize the arg once in _prompt_builtin's closure — (arg or "").strip() — before the
builder, ack, and display use it. This also trims edge whitespace so "/learn " echoes
as "/learn" instead of "/learn " (matching the CLI/gateway); internal whitespace and
newlines in a multi-line request still reach the builder verbatim. No try/except: the
acks are total after normalization.
…ng precedence claim
This was referenced Sep 15, 2026
alt-glitch
added
type/bug
Contributor
|
exact-head 0dd8563 — void if moved KEEP: GUI CHECK: soft overlap with #111198 on |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/learnis a prompt-queueing slash command: it prints an acknowledgement and then does the work as one normal agent turn. Issue #52085 (reported by @sylvesterroos) is that the progress and the result are hard to track. Two concrete gaps:tui_gateway's_prompt_builtinreturned only{"type": "send", "message": <builder prompt>}— omittingnotice(the field both GUI clients use for an ack line) anddisplay(the field they use for the chat bubble text). So the TUI and the desktop app showed nothing on invocation and then rendered the ~5 KB model-facing builder prompt as the user's own message. The classic CLI and the messaging platforms already acked; those two surfaces were the outliers./planand/initshare the same factory and had the identical defect._CUTE_LINEShas noskill_manageentry, so a skill write rendered as the bare argument preview┊ ⚡ skill_man <name>— the tool name truncated, no indication whether the call created, updated, deleted, or failed, and nothing tying it back to the/learnrun that produced it. (skill_manage's own result string,Skill '<name>' created., is returned to the model only.)Changes
tui_gateway/methods_tools.py—_prompt_builtinnow takes a requiredack(arg, message)callable and returnsnotice+displayalongsidemessage; the command argument is normalized once, because the wire contract (CommandDispatchParams.arg: str | None) allowsnulland the previous code tolerated it. Ack wording is reused verbatim from the messaging gateway so every surface says the same thing. No client changes are needed — the TUI and the desktop already render both fields.agent/display.py— new_cute_skill_managerenderer registered in_CUTE_LINES. It names the skill from the tool's advertisedoperationsarray (first op, with+Nwhen the batch is longer), uses an action-appropriate verb (created/updated/wrote/removed/deleted), printsstagedwhen a staged write was not actually saved, and reports the attempted action (with the existing failure suffix appended by_detect_tool_failure) when the call failed.Surfaces, precisely
┊ ⚡ skill_man <name>┊ 📚 skill created <name>(verb, plusstaged/ failure handling)/learn <request>Nothing changes at runtime beyond those rendered strings and the two added payload fields.
Test plan
The 20 failures are pre-existing in this environment (the venv is missing the
anthropicSDK). The identical lane set on the base commit gives11179 passed, 21 failed— the same failure set, plus one flaky file that failed on base and passed here.Notes
/learncan write an unbounded number of files (a knowledge-base run persists one per chapter). Progress continues to rely on the agent's live spinner and per-tool execution stream, framed by the immediate ack and the completion line. A previous attempt at this issue (fix(cli): show /learn progress steps #52136, closed unmerged) added a hardcoded1/3, 2/3, 3/3hint; that shape is why this PR declines to add one./planand/initare fixed in the same commit because they are the same defect in one shared factory; their ack wording is unchanged from the CLI and the gateway./init's ack wording is derived by looking for the markerUPDATE the existing AGENTS.mdinside the built prompt. That sniff now exists in three places (classic CLI, messaging gateway, and the new TUI/desktop ack). An explicit flag frombuild_init_prompt_for_cwdwould be better, but that is a wider change across all three surfaces and is deliberately left out of this PR.Open question: should a
/learn-created skill carry an explicit learn-provenance marker? Today it is recorded like any other foreground create (created_by: None), which makes it invisible to/journeyuntil it has been used once — the learning timeline misses the artifact at the moment a user checks it. Filed separately as #111317, because the fix there is a provenance decision (the naive fix, stampingcreated_by: agent, would make a user's learned skills curator-manageable), not a display change.Closes #52085