feat: Render tool help descriptions from skill parameter tables - #2018
Conversation
Only focus-window cannot have one (it takes no parameters); the six tools that were missing a table were missing it for no structural reason, which left their help text with no source to read from. execute-dynamic-code's prose bullets become the standard table, since keeping both would put two descriptions of one parameter in a single file — the drift this direction exists to remove. --code-file keeps its bullet verbatim below the table: it is a CLI-only flag with no schema property, so no table row can hold it. The hidden CompileOnly property is deliberately absent. The pause-point skill covers five tools, so it gains a Parameters section whose "### <tool-name>" headings carry a description line and one table each. simulate-keyboard and simulate-mouse-input also take their Input System prerequisites into the skill description, which until now existed only in the generated catalog — the one class of fact that must not be lost when the skill becomes the source.
Descriptions lived in three hand-maintained places at once: the skill prose an agent reads, Unity's schema attributes, and the catalog compiled into the CLI. Editing one left the others stale, and help is where that staleness costs the most. The new cli/common/skilldocs package reads the package's own SKILL.md parameter tables at render time and lets them win over every other source, so the file an agent reads and the help an agent runs cannot disagree. - Parses both skill layouts (one skill per tool, and the multi-command pause-point skill), tolerating CRLF, a BOM and escaped pipes. - Matches schema properties through tooldocs.OptionNameForProperty so no second kebab-conversion rule exists. - Falls back silently to the previous descriptions when the package, the file or the table is missing, recording the reason in the CLI vibe log: help that prints stale text is a nuisance, help that fails to print is a broken CLI. - Wires the three paths that print Unity tool descriptions: single-command help, the project-resolved command list, and `uloop list`. - Registers the package in both release-input allow-lists, without which the renderer would never reach a released binary while CI stayed green.
Reading the tables verbatim carried their Markdown into places that cannot render it: `--action` printed "`Press`, `KeyDown`" in terminal help and in list JSON alike. The escape resolution the parser already performed proves a cell-to-plain-text step exists, so backtick removal belongs in that same step rather than in a second one. NormalizeCellText is now the only Markdown-to-text conversion in this package, and the catalog generator and the CI drift check will run cells through it too, so all three surfaces compare and emit the same string. Its scope stays code spans alone: any other Markdown reaching a cell shows up verbatim in help, which is the signal to fix the table.
The skill's parameter table listed three of the five flags record-input accepts, so an agent reading it had no way to learn that recording waits three seconds by default or that the countdown overlay can be turned off. Both rows describe the flag's effect, matching how replay-input documents the same overlay.
…esizing it
A negated boolean's help text was decided by wording: unless the description
began with "Disable" or "Do not" it was thrown away and replaced with a
synthesized "Disable <Property Name>", with two commands special-cased to fixed
strings. So `--no-include-components` printed "Disable include components" while
its reviewed table row read "Exclude component information", and no amount of
editing the table could change the help.
The branch is now on provenance, not wording: a description that came from a
skill parameter table prints verbatim, because those rows are already written
from the flag's point of view. Synthesis survives only for a description with no
skill behind it - a project-local custom command, whose author wrote the property
in the positive sense - so its `--no-<name>` flag still does not claim the
opposite of what it does.
The two special-cased strings are retired; the table rows they duplicated say the
same thing more precisely ("before test execution" rather than "before
execution"). Their predicates stay: the option names and the "auto-save enabled"
default suffixes still need them.
The embedded catalog is what `--help` and `uloop list` print for a user who has no synced project cache, and its descriptions were hand-maintained. Nothing tied them to the skill tables the same options are documented in, so the two drifted independently and neither one was the answer to "where do I edit this text". `scripts/sync-tool-docs.sh` now derives every catalog description from the tables, making the catalog a generated artifact and the tables the single place to edit. `--check` reports staleness without writing, for CI to run. Generation refuses to write a partially documented catalog: a visible option with no table row, or a table row matching no accepted option, stops the run and every such problem is listed at once, since a table that fell behind usually did so for several options. Hidden properties are exempt - they never reach help. Only `focus-window`, which accepts no options, has no table. The edit is a byte-level replacement of the description string literals rather than a decode-edit-encode round trip, which would drop zero-value defaults, reorder properties, and add empty schema objects to every tool that has no parameters. This run rewrites 75 description lines and nothing else. The regenerated text changed two assertions: help output now shows the table wording for the two negated booleans that were special-cased, and simulate-keyboard's `--key` states the digit-key rule as accepted ranges rather than one example.
The table cell ended in "see \"When to use --force-recompile\" below", which works in the skill file but not in `compile --help`, where there is no "below" to look at - a caller was pointed at a section they cannot reach. Now that help renders from this cell, the cell has to be self-contained. It now states the reason not to use the flag directly: a plain compile already detects externally edited files, and the forced reload can freeze large projects and return COMPILE_RESULT_UNKNOWN. The skill's detailed section stays for readers of the skill itself.
Help, `uloop list` and the embedded catalog all render parameter descriptions from the skill tables now, so a parameter with no row is a parameter no agent can discover - and nothing prevented adding one. The generator catches the opposite direction (a row matching no option), but it reads the catalog, not the schema types, so only a C# test can see a property that was just added. The guard walks every live tool in the shipped catalog and requires a kebab-matching row for each visible property. Hidden properties are skipped by reading the catalog's own `hidden` flag rather than a second list here, so this test and the generator cannot disagree about which options reach the command line. The second test requires a non-empty tool description, which is the one-line summary `uloop list` prints. The kebab rule is duplicated from cli/common/tooldocs on purpose: it has to run on both sides of the language boundary, and a divergence is caught from either direction. Table parsing here is smaller than the Go parser - it locates the table and reads its first column, never a description - so the duplicated understanding of the file format is limited to what a missing row depends on. The registry's test double moves out of DefaultToolsCatalogDriftTests so both guards share it.
Nothing checked that the committed catalog still matched the tables it is generated from, so a skill edit without a regeneration would ship help text that disagrees with the documentation an agent reads. The step runs the generator in check mode, which reports the mismatch and names the script that fixes it without writing anything. The repository root is passed explicitly because the step runs inside the module directory, where the catalog's relative path does not resolve.
Forgetting to regenerate the catalog after editing a skill table is caught by CI, but only after a push, which costs a round trip. The hook regenerates it whenever a skill file is staged and adds the result to the same commit, so the mistake stops being possible to make locally. A table that disagrees with its schema fails the generator and blocks the commit, which is where that conflict is cheapest to fix. The hook was never wired up (core.hooksPath is unset in a fresh clone), and wiring it as it stood would have run the full Go check - lint, tests and a binary rebuild - on every commit touching Go. It now runs only gofmt and go vet, seconds rather than minutes; the full check stays where it was, in PR preparation and CI. The one-line opt-in and the catalog rule are documented for agents, who read that file every session.
The embedded catalog was classified with contract.json as a release-please stamp target, so a change to it counted as no release input at all. That was true while the descriptions were hand-edited rarely; now that the catalog is generated from the skill tables, a pull request that improves a tool's help text and nothing else would have passed the trigger guard and never reached a binary - the improved text would sit in the repository and never arrive on anyone's machine. The catalog now counts as a shared release input for both binaries, which is what it is: `//go:embed` compiles it into each one. contract.json stays excluded, since release-please writes it. The stamp script's file selection is meant to mirror the guard, so it moves in step, and a case there proves the catalog reaches the stamps.
The glossary defined a skill as instructions for an agent. It is now also where the tool and parameter descriptions the CLI prints come from, read at render time and compiled into the embedded catalog. Naming that in the definition is what keeps "edit the description in the skill" discoverable from the vocabulary rather than only from the code.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR makes ChangesSkill documentation and parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Skill as SKILL.md
participant Parser as skilldocs/parse
participant Discover as skilldocs/discover
participant Help as Help/List
participant Sync as sync-tool-docs
participant Catalog as default-tools.json
Skill->>Parser: Parse parameter tables
Parser->>Discover: Return tool docs
Discover->>Help: Load and apply at runtime
Help->>Help: Render with skill text
Skill->>Sync: Extract descriptions
Sync->>Catalog: Replace via JSON editor
Sync->>Sync: Validate schema/table drift
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/project-runner/internal/projectrunner/list_output_test.go (1)
251-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated skill-fixture test helper across two packages.
writeSkillFixtureProjectandwriteSkillDocsFixturePackagewrite the identical fake uloop package (package.jsonmanifest + simulate-keyboardSKILL.mdwith the same frontmatter/table) purely to test that installed-skill descriptions win over embedded ones; if the skill fixture format ever needs to change, both copies must be updated in lockstep.
cli/project-runner/internal/projectrunner/list_output_test.go#L251-L279: extract this logic into a small shared internal test-support package (e.g. undercli/common/skilldocsor a newtestsupportpackage) exposing aWriteSkillFixturePackage(t, root, durationDescription)helper, and have this file call it.cli/dispatcher/internal/dispatcher/command_help_skill_docs_test.go#L82-L108: replacewriteSkillDocsFixturePackagewith a call to the same shared helper instead of maintaining a second copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/project-runner/internal/projectrunner/list_output_test.go` around lines 251 - 279, The skill fixture creation logic is duplicated across two test packages. Extract the shared package-manifest and simulate-keyboard SKILL.md setup from writeSkillFixtureProject into a shared test-support helper such as WriteSkillFixturePackage(t, root, durationDescription), then replace writeSkillFixtureProject in cli/project-runner/internal/projectrunner/list_output_test.go:251-279 and writeSkillDocsFixturePackage in cli/dispatcher/internal/dispatcher/command_help_skill_docs_test.go:82-108 with calls to that helper; both sites require direct changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/glossary.md`:
- Around line 94-98: Update the glossary passage describing description sources
to qualify the “nowhere else” claim: skill parameter tables remain authoritative
for skill-backed tools, while fallback metadata remains authoritative for tools
without skills. Preserve the existing explanation of --help, uloop list, and the
generated embedded catalog.
---
Nitpick comments:
In `@cli/project-runner/internal/projectrunner/list_output_test.go`:
- Around line 251-279: The skill fixture creation logic is duplicated across two
test packages. Extract the shared package-manifest and simulate-keyboard
SKILL.md setup from writeSkillFixtureProject into a shared test-support helper
such as WriteSkillFixturePackage(t, root, durationDescription), then replace
writeSkillFixtureProject in
cli/project-runner/internal/projectrunner/list_output_test.go:251-279 and
writeSkillDocsFixturePackage in
cli/dispatcher/internal/dispatcher/command_help_skill_docs_test.go:82-108 with
calls to that helper; both sites require direct changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72ec4086-35a1-4104-902b-7e7e6961ca3e
⛔ Files ignored due to path filters (3)
.husky/pre-commitis excluded by none and included by noneAssets/Tests/Editor/AlwaysEnabledToolSettingsPort.cs.metais excluded by none and included by noneAssets/Tests/Editor/SkillParameterTableCoverageTests.cs.metais excluded by none and included by none
📒 Files selected for processing (55)
.agents/skills/uloop-compile/SKILL.md.agents/skills/uloop-execute-dynamic-code/SKILL.md.agents/skills/uloop-pause-point/SKILL.md.agents/skills/uloop-record-input/SKILL.md.agents/skills/uloop-simulate-keyboard/SKILL.md.agents/skills/uloop-simulate-mouse-input/SKILL.md.claude/skills/uloop-compile/SKILL.md.claude/skills/uloop-execute-dynamic-code/SKILL.md.claude/skills/uloop-pause-point/SKILL.md.claude/skills/uloop-record-input/SKILL.md.claude/skills/uloop-simulate-keyboard/SKILL.md.claude/skills/uloop-simulate-mouse-input/SKILL.md.github/workflows/build-and-test.ymlAGENTS.mdAssets/Tests/Editor/AlwaysEnabledToolSettingsPort.csAssets/Tests/Editor/DefaultToolsCatalogDriftTests.csAssets/Tests/Editor/SkillParameterTableCoverageTests.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/Compile/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/SimulateKeyboard/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/SimulateMouseInput/Skill/SKILL.mdcli/common/skilldocs/apply.gocli/common/skilldocs/apply_test.gocli/common/skilldocs/discover.gocli/common/skilldocs/parse.gocli/common/skilldocs/parse_test.gocli/common/skilldocs/skill_docs.gocli/common/tooldocs/tool_option_help.gocli/common/tooldocs/tool_option_help_test.gocli/common/tools/catalog.gocli/common/tools/default-tools.jsoncli/common/tools/description_fallback.gocli/common/tools/description_fallback_test.gocli/common/tools/types.gocli/dispatcher/internal/dispatcher/command_help.gocli/dispatcher/internal/dispatcher/command_help_skill_docs_test.gocli/dispatcher/internal/dispatcher/help_test.gocli/dispatcher/internal/dispatcher/run_help.gocli/dispatcher/shared-inputs-stamp.jsoncli/project-runner/internal/projectrunner/list_output.gocli/project-runner/internal/projectrunner/list_output_test.gocli/project-runner/internal/projectrunner/run.gocli/project-runner/shared-inputs-stamp.jsoncli/release-automation/cmd/sync-tool-docs/main.gocli/release-automation/internal/automation/release_trigger_guard.gocli/release-automation/internal/automation/release_trigger_guard_test.gocli/release-automation/internal/automation/tool_docs_json_editor.gocli/release-automation/internal/automation/tool_docs_sync.gocli/release-automation/internal/automation/tool_docs_sync_test.godocs/glossary.mdscripts/stamp-release-inputs.shscripts/sync-tool-docs.shscripts/test-stamp-release-inputs.sh
💤 Files with no reviewable changes (1)
- Assets/Tests/Editor/DefaultToolsCatalogDriftTests.cs
| A skill is also the single source of truth for the tool and parameter descriptions the CLI | ||
| prints. `--help` and `uloop list` read the parameter table out of the installed package's | ||
| skill at render time, and the embedded catalog (`cli/common/tools/default-tools.json`) is | ||
| generated from those same tables. Descriptions are therefore edited in the skill and nowhere | ||
| else. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Qualify the “nowhere else” source-of-truth claim.
Fallback descriptions remain supported for tools without skills, so this wording incorrectly implies that every description must be edited only in a skill. Clarify that skill tables own skill-backed descriptions while fallback metadata remains authoritative for tools without skills.
Proposed wording
-Descriptions are therefore edited in the skill and nowhere else.
+Skill-backed descriptions are edited in the skill and nowhere else; tools
+without skills continue to use their fallback descriptions.This follows the PR objective to preserve fallback descriptions for tools without skills.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| A skill is also the single source of truth for the tool and parameter descriptions the CLI | |
| prints. `--help` and `uloop list` read the parameter table out of the installed package's | |
| skill at render time, and the embedded catalog (`cli/common/tools/default-tools.json`) is | |
| generated from those same tables. Descriptions are therefore edited in the skill and nowhere | |
| else. | |
| A skill is also the single source of truth for the tool and parameter descriptions the CLI | |
| prints. `--help` and `uloop list` read the parameter table out of the installed package's | |
| skill at render time, and the embedded catalog (`cli/common/tools/default-tools.json`) is | |
| generated from those same tables. Skill-backed descriptions are edited in the skill and nowhere | |
| else; tools without skills continue to use their fallback descriptions. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/glossary.md` around lines 94 - 98, Update the glossary passage
describing description sources to qualify the “nowhere else” claim: skill
parameter tables remain authoritative for skill-backed tools, while fallback
metadata remains authoritative for tools without skills. Preserve the existing
explanation of --help, uloop list, and the generated embedded catalog.
Generating descriptions from the tables made the tables authoritative, and fifteen cells were thinner than the catalog text they replaced - so help and `uloop list` lost information that had been there. What went missing was the part a caller cannot guess. Every enum cell had been reduced to a bare list of member names: `Press`, `KeyDown`, `KeyUp`, `ReleaseAll` says nothing about what any of them do, and in particular nothing about `ReleaseAll` being the way out of inconsistent key state after a pause point interrupts PlayMode. The same happened to simulate-mouse-input, simulate-mouse-ui, replay-input, record-input and control-play-mode, where `Step`, `Status` and `Resume` had no explanation at all. Elsewhere it was the fact behind the number or the flag: which direction a positive `--scroll-y` scrolls, that drag origins are top-left, that rendering screenshots return the formula for converting raw pixels, that `--annotate-elements` also returns element metadata. Each cell now carries what its catalog counterpart carried, so the regenerated descriptions read the same as before this change for the reader of `--help` while staying the single place the text is edited.
Two ways the hook could produce a commit nobody wrote: Regenerating overwrites the catalog, so an unstaged hand edit to it was swept into the commit as though the generator had produced it. The hook now refuses to run in that state and says what to do, because that edit is lost either way - the next run overwrites it - and losing it silently inside someone else's commit is the worse of the two. The generator reads the working tree, so staging a skill file with `git add -p` produced a catalog describing text the commit does not contain. A partially staged skill file now stops the commit rather than being resolved by guesswork about which version was meant. The skill-file pattern also no longer names the two CLI-only skill directories that happen to exist today; the reader it feeds scans all of them.
Deleting either assignment - the one in the skill reader or the one that carries the flag through the embedded fallback - left every test green, so nothing was holding the behaviour they exist for: a negated boolean printing its documented wording instead of a synthesized "Disable <name>". One test covers each path. The reader's test documents a property the embedded catalog does not know, so only the skill can describe it; without the assignment the help output falls back to the synthesized summary. The fallback's test starts from Unity's placeholder and checks the description it picks up survives the same way.
The parser normalized line endings and a BOM once for the table and then handed the raw file to the frontmatter reader, so a file containing lone CRs would have been understood two different ways by one function. Git never writes that, but "the normalization happens in one place" is this feature's own rule, and the frontmatter path was outside it. The C# guard's table splitter gains a note on why it does not resolve the "\|" escape: it reads only the first column. Reading a description there needs the same handling the Go splitter has.
7b020ec
into
feature/cli-discoverability-integration
Summary
uloop <command> --helpanduloop listnow describe every option in the same words the agent skill documents it with, instead of the placeholder or synthesized text they printed before.User Impact
Before, three different sources could supply an option's help text, and the weakest one usually won:
[Description]printedParameter: MaxHistory— the property name spelled differently.truehad its description thrown away and replaced with a synthesizedDisable <property name>, so--no-include-componentsprinted "Disable include components" while its reviewed documentation said "Exclude component information". Editing the documentation could not change the help.compile --force-recompilepointed at a section that only exists in the skill file:see "When to use --force-recompile" below. In--helpthere was no "below".record-input's five flags were documented; a caller had no way to learn that recording waits three seconds before it starts.Now help,
uloop listand the fallback catalog all print the reviewed table text, and each of the above reads as a sentence about what the flag does. Descriptions also follow the installed package version, so a project on an older package sees the help that matches the package it actually has.Changes
cli/common/skilldocsreads the installed package'sSKILL.mdparameter tables and overlays them onto the tool catalog at render time, for both help paths anduloop list. Tools with no skill — a project's own custom commands — pass through untouched.Disable <name>synthesis survives only for a negated boolean with no skill behind it, where the author wrote the property in the positive sense. Two hardcoded command-specific strings are retired.scripts/sync-tool-docs.shgenerates the embedded catalog's descriptions from the same tables, making it a generated artifact. It refuses to write a partially documented catalog: a visible option with no row, or a row matching no accepted option, stops the run and every such problem is listed at once. Generation replaces description string literals byte-for-byte, so zero-value defaults, property order and hidden flags survive untouched — this run changes 75 description lines and nothing else.--checkreports drift without writing, and CI runs it. A pre-commit hook regenerates the catalog whenever a skill file is staged, so the mistake is hard to make locally. It refuses to run when the catalog has unstaged edits or a skill file is only partially staged, rather than committing a catalog that describes text the commit does not contain.gofmtandgo vet. The hook was never wired up (core.hooksPathis unset in a fresh clone), and it ran the fullscripts/check-go-cli.sh— lint, tests and a binary rebuild, tens of seconds to minutes. Wiring that to every commit touching Go is not something anyone would keep enabled, and the point of this change is a hook people actually run. The full check keeps its two homes:scripts/check-go-cli.shbefore a pull request, and CI.contract.jsonas a release-please stamp target, which meant a pull request that only improved help text would have shipped no binary and reached nobody.record-input's countdown and overlay flags), and sixteen cells were rewritten so the generated text carries what the old catalog carried: every enum cell explains what its members do rather than listing their names, and--force-recompileno longer points at a section that exists only inside the skill file.Verification
sh scripts/check-go-cli.sh→ exit 0,0 issues.per module.sh scripts/test-stamp-release-inputs.sh→ passed. The new catalog case was confirmed to fail when the script's input selection is reverted.go run ./cmd/sync-tool-docs --check→ catalog matches; a second generation run reports no change (idempotent); a temporarily edited table makes it exit non-zero.uloop compile→ 0 errors.uloop run-tests(EditMode): the two new guard tests and the existing catalog drift test pass; adding a dummy schema property makes the guard fail withcompile --temporary-drift-probe: no row in Compile/Skill/SKILL.md, and it passes again once reverted.--no-include-components Exclude component information,--stop-on-external-scene-changes Stop before compilation if open Scene files changed externally instead of auto-reloading them, and both newrecord-inputflags.v3-beta.ToolSkillSynchronizerTests.DetectTargets_WhenGroupedLayoutRequested_DetectsEmptyFlatManagedDirectoriesfails identically on the base branch (51/52 in that fixture, with and without these commits).