feat: Improve uloop CLI discoverability after E2E verification rounds - #2023
Merged
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 149 files, which is 49 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (149)
You can disable this status message by setting the ✨ Finishing Touches🧪 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 |
…n order Both of these were already load-bearing for day-to-day validation, and both were written down in a form that let a reader draw the wrong conclusion. The excludedCommands entry for the dev binary was documented as an unverified suggestion, so nobody knew which command shapes it covers. Measuring it showed that an absolute path needs its own anchored entry, and that a shell variable expanded into an env-var prefix defeats every entry - including the plain `uloop *` one, which is why the rule belongs here rather than in a dev-binary footnote. The runner resolution order was described only from the override's point of view, which reads as if the override is what makes a dogfooding run use the local build. It is not: `dist/<platform>/uloop` takes its sibling runner without it. A verifier who did not know that attributed a local-build run to the version cache. Version numbers cannot settle the question either - a local build and the released runner both answer 3.0.0-beta.58 while their SHA-256 differ - so the section says plainly that hashing is currently the only way to tell which binary served a run.
The previous commit left "write env-prefix values literally" as the only way out, which reads as if a dogfooding shell cannot take the runner path from a variable at all. Measuring the export form shows the rule is narrower: the match is only lost when the expansion happens in the same command as uloop. Remedy 3 hands the runner path through exactly that env prefix, so it now says which of the two forms to use rather than leaving the reader to find the table.
…er them Both references existed but sent the reader somewhere the answer no longer was. The sandbox reference was written as a post-mortem step - read it once a "Unity not reachable" symptom appears - while what the document now records is which command shapes survive the sandbox exclusion, which is only useful before typing the command. Its parenthetical "misreported as an i/o timeout" also became wrong when the refused-connect report landed: the fixed CLI reports the denial verbatim, and only a pre-fix runner still turns it into a timeout. The project runner reference implied the env override is what makes a run use a local build. In this checkout it is not, and since no response field names the runner that served a command, a verification result can be silently about the released binary instead.
The previous commit called it a property of Claude Code's exclusion matching
that this repository cannot do anything about. That closed off the option that
actually works: an excludedCommands entry naming the variable makes the
expanded form match again, verified for both
"ULOOP_PROJECT_RUNNER_PATH=* uloop *"
"ULOOP_PROJECT_RUNNER_PATH=* dist/*/uloop *"
with a control run confirming a different variable name is still denied, so
the entries are what changed the outcome.
Also weakens the warning against a leading wildcard. Matching turns out to
happen per sub-command - an absolute-path invocation after `echo hello;` is
still excluded - so the stated reason (an arbitrary prefix riding into the
exclusion) is not supported by anything measured here. What remains is that
the unanchored form was not measured at all, which is reason enough to prefer
another anchored entry, and the text now says only that.
The matching model this section infers is labelled as inferred. It predicts
every row in the table, but it was read off the measurements rather than off
the implementation, and a reader should not extend it without measuring.
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
1 participant
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
uloop --helpanduloop listnow describe what a command actually does, read from the skill files of the package installed in that project, so the help matches the version you have.--key 3pressing Tab,--keys 3filtering Tab — now fail with a message naming the right key.--triggerthat cannot run, a connection the OS refused, and a Unity process check that failed all report what happened instead of making the caller wait out a timeout for guidance that does not apply.Why this work exists
Three verification agents drove
uloop-pause-pointthrough a round of E2E work and reported eleven usable findings. Interviewing them afterwards put the failures in three layers:Parameter: Durationplaceholders, and the examples were so regular (--key Space) that agents extrapolated--key 3.--key 3returnedSuccess: truehaving pressed Tab; a malformed--triggerargument burned the full 60-second wait.The principle this branch applies: put facts or instructions — not references — where the reader's eye already is: the always-visible description, the document they are currently reading, and the error message. Layer 1 is treated as unfixable; layers 2 and 3 have to absorb it.
Design principle: prose belongs to the skill, structure belongs to the code, hand-written copies are zero
Help must describe the package installed in this project, not the newest description someone wrote. Since
SKILL.mdships inside the package, it is version-matched by construction — so help reads it directly at render time instead of transporting generated descriptions through the Unity schema..uloop/tools.jsonSKILL.mdOne generated artifact remains: the embedded catalog's descriptions (
cli/common/tools/default-tools.json), used only outside a project where neither schema nor skills exist. CI fails on drift.Four design decisions changed after measuring the code
.uloop/tools.jsondescriptions are not empty — they areParameter: Xxxplaceholders (99 of 102 properties). "Fall back when empty" would never fire, so the CLI treats placeholders as missing.--key:Enum.TryParsealso accepts+3," 3 ",Space,Enter(OR-ed into Tab), and undefined ordinals like300that later throw from the keyboard indexer. Only names defined on theKeyenum resolve.uloop-breaks immediately (enable-pause-pointhas nouloop-enable-pause-point; one skill covers four commands). An explicit table with a guard test replaced it.What changed
Digit3with the digit rule stated inline in all three placessimulate-keyboard --keyaccepts only definedKeynames;3,+3,300,Space,Enterall fail, and3suggestsDigit3andNumpad3--triggerrejected before it ran fails fast, keeps the marker armed, re-pauses Play if this command resumed it, and returns a copy-pasteable recovery command with the remaining lifetimepause-point-status --expect, clearer capture diagnostics, and top-level visibility when a trigger was rejected by preflightrecord-input --keysapplies the same name rule; entries that name no key fail the command instead of silently filtering something else#2019 and #2020 came from two diagnostic incidents found while verifying this branch, not from the original eleven findings.
Four documentation-only commits sit on top of the merged children.
docs/claude-code-sandbox.mdgains a measured table of which command shapes the sandbox exclusion actually covers (an absolute path needs its own anchored entry; a shell variable expanded into an env-var prefix defeats every entry), anddocs/project-runner-pin.mdgains a "Which runner actually ran" section covering the four-step resolution order, the fact that runningdist/<platform>/uloopuses the local build without any override, and the fact that a local build and the released runner both answer3.0.0-beta.58so only SHA-256 tells them apart. Both were written from measurements taken while verifying this branch, because both had already misled a verification round.AGENTS.mdthen follows: its sandbox reference now sends the reader to that document before running dev-binary commands rather than after a symptom appears, its "(misreported as an i/o timeout)" parenthetical is corrected — #2019 made it wrong, since the fixed CLI reports the denial verbatim and only a pre-fix runner still turns it into a timeout — and itsULOOP_PROJECT_RUNNER_PATHparagraph no longer implies the override is what makes a run local. A final commit corrects an over-claim made in the first: an env-prefix value taken from a shell variable is not something only Claude Code can fix — anexcludedCommandsentry naming the variable restores the match, verified with a control run showing a different variable name is still denied. The matching model the section infers is labelled as inferred rather than stated as fact.Deliberately not done
--key 3toDigit3. Guessing which key the caller meant is how the original bug felt harmless.SKILL.mdfrom C# at runtime, and a new frontmatter field for help text: both add a second parser or a second source for the same sentence.--helpfor Unity tools to the pinned runner. Deferred, not rejected on principle.Known limits
(required except ReleaseAll).hiddenanditemsare lost on the cached schema path. Onlyexecute-dynamic-codehas hidden properties today and it is covered; adding a hidden property to another tool would leak it into in-project help..uloop/tools.jsonkeeps its placeholder descriptions. Help never shows them, but a direct consumer of that file would still see them.--trigger: they carry no structured rejection field, and treating everySuccess: falseas an argument error would cut short legitimate runtime failures.Returnis accepted as an alias forEnterwithout being documented.Verification
scripts/check-go-cli.sh: format, vet, lint (0 issues.for all four modules), all module tests, binary rebuild.uloop compile: 0 errors, 0 warnings.uloop run-tests(EditMode, full suite): 2398 tests, 2391 passed, 0 failed, 7 skipped. The one pre-existing failure is fixed by chore: Fix the skill-target layout test that pointed at a removed directory #2022.uloop run-tests(PlayMode, full suite): 108 tests, 106 passed. The two failures areSimulateMouseInputTests.Click_WhenPausePointMarkerHits_Should_ReturnMarkerDetailsandClick_WhenUnityPausesDuringObservation_Should_CompleteAsPausePointInterruption; both reproduce unchanged at the base commit (cbbf9df6) and are unrelated to this branch. No Unity test suite runs on pull requests — the EditMode workflow isworkflow_dispatchplus a nightly schedule, and PR CI validates package structure rather than running tests — so both suites above were run locally.scripts/sync-tool-docs.shthengit diff --exit-code cli/common/tools/default-tools.json: clean, so the catalog is idempotent.uloop skills install --claude --agentsthengit status --short: clean, so the generated skill copies match their sources.