From 1162df4ad77873c24cf127671b32fb8a15134d78 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 11:30:50 +0900 Subject: [PATCH 1/2] Reject key filter entries that name no key in record-input --keys went through Enum.TryParse, which also accepts enum ordinals, so "--keys 3" silently became a filter for Key.Tab. The same hole was closed for simulate-keyboard's --key by matching defined Key enum names; this applies that rule to the filter as well. The name-to-Key whitelist moves to the shared Common/InputSystem assembly both tools already reference, so the rule lives in one place instead of being copied. Entries that name no key are now reported rather than skipped. Skipping them records a different set of keys than the caller asked for, and skipping all of them falls back to recording every key - the opposite of the request - with nothing in the response to distinguish it from omitting --keys entirely. --- .agents/skills/uloop-record-input/SKILL.md | 2 +- .claude/skills/uloop-record-input/SKILL.md | 2 +- .../Editor/InputRecordingKeyFilterTests.cs | 66 +++++++++++ .../InputRecordingKeyFilterTests.cs.meta | 11 ++ Assets/Tests/Editor/KeyNameResolverTests.cs | 111 ++++++++++++++++++ .../Tests/Editor/KeyNameResolverTests.cs.meta | 11 ++ .../InputRecordingFileHelper.cs | 24 ++-- .../InputRecording/KeyFilterParseResult.cs | 27 +++++ .../KeyFilterParseResult.cs.meta | 11 ++ .../Common/InputSystem/KeyNameResolver.cs | 68 +++++++++++ .../InputSystem/KeyNameResolver.cs.meta | 11 ++ .../RecordInput/RecordInputUseCase.cs | 18 ++- .../RecordInput/Skill/SKILL.md | 2 +- .../SimulateKeyboardUseCase.cs | 39 +----- cli/common/tools/default-tools.json | 2 +- 15 files changed, 355 insertions(+), 50 deletions(-) create mode 100644 Assets/Tests/Editor/InputRecordingKeyFilterTests.cs create mode 100644 Assets/Tests/Editor/InputRecordingKeyFilterTests.cs.meta create mode 100644 Assets/Tests/Editor/KeyNameResolverTests.cs create mode 100644 Assets/Tests/Editor/KeyNameResolverTests.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs create mode 100644 Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs.meta create mode 100644 Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs create mode 100644 Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs.meta diff --git a/.agents/skills/uloop-record-input/SKILL.md b/.agents/skills/uloop-record-input/SKILL.md index 40859135fa..1dcf2a1235 100644 --- a/.agents/skills/uloop-record-input/SKILL.md +++ b/.agents/skills/uloop-record-input/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter (for example `W,A,S,D,Space`). Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/.claude/skills/uloop-record-input/SKILL.md b/.claude/skills/uloop-record-input/SKILL.md index 40859135fa..1dcf2a1235 100644 --- a/.claude/skills/uloop-record-input/SKILL.md +++ b/.claude/skills/uloop-record-input/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter (for example `W,A,S,D,Space`). Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs new file mode 100644 index 0000000000..4d465c32bf --- /dev/null +++ b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs @@ -0,0 +1,66 @@ +#if ULOOP_HAS_INPUT_SYSTEM +using System.Collections.Generic; +using NUnit.Framework; +using UnityEngine.InputSystem; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Test fixture that verifies the record-input key filter accepts only defined key names. + /// + public sealed class InputRecordingKeyFilterTests + { + /// + /// Tests that named keys are accepted case-insensitively and no name is reported invalid. + /// + [Test] + public void ParseKeyFilter_WhenGivenKeyNames_KeepsThemAll() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter("space, w"); + + Assert.IsEmpty(result.InvalidKeyNames); + Assert.IsNotNull(result.Filter); + Assert.AreEqual(new HashSet { Key.Space, Key.W }, result.Filter); + } + + /// + /// Tests that a bare digit is reported invalid instead of silently filtering the key whose + /// enum ordinal it happens to be. + /// + [Test] + public void ParseKeyFilter_WhenGivenAnOrdinal_ReportsItInvalid() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter("3"); + + Assert.AreEqual(new[] { "3" }, result.InvalidKeyNames); + Assert.IsNull(result.Filter); + } + + /// + /// Tests that an invalid entry alongside a valid one is still reported, so a partially + /// applied filter is never mistaken for the requested one. + /// + [Test] + public void ParseKeyFilter_WhenOneEntryIsInvalid_ReportsThatEntry() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter("W, 3"); + + Assert.AreEqual(new[] { "3" }, result.InvalidKeyNames); + } + + /// + /// Tests that no filter and no invalid name is reported when the parameter is omitted. + /// + [Test] + public void ParseKeyFilter_WhenGivenNothing_ReportsNoFilter() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter(""); + + Assert.IsEmpty(result.InvalidKeyNames); + Assert.IsNull(result.Filter); + } + } +} +#endif diff --git a/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs.meta b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs.meta new file mode 100644 index 0000000000..fe4c257054 --- /dev/null +++ b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 8afa56a901fe140609ebf4a9c0e8a193 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/KeyNameResolverTests.cs b/Assets/Tests/Editor/KeyNameResolverTests.cs new file mode 100644 index 0000000000..c4fe5df597 --- /dev/null +++ b/Assets/Tests/Editor/KeyNameResolverTests.cs @@ -0,0 +1,111 @@ +#if ULOOP_HAS_INPUT_SYSTEM +using NUnit.Framework; +using UnityEngine.InputSystem; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Test fixture that verifies key names resolve only through defined Key enum names. + /// + public sealed class KeyNameResolverTests + { + /// + /// Tests that a defined key name resolves regardless of the casing it was written in. + /// + [Test] + public void Resolve_WhenGivenADefinedNameInAnyCasing_ResolvesTheKey() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("space"); + + Assert.IsTrue(resolved); + Assert.AreEqual(Key.Space, key); + } + + /// + /// Tests that surrounding whitespace does not stop a defined name from resolving. + /// + [Test] + public void Resolve_WhenGivenAPaddedName_ResolvesTheKey() + { + (bool resolved, Key key) = KeyNameResolver.Resolve(" W "); + + Assert.IsTrue(resolved); + Assert.AreEqual(Key.W, key); + } + + /// + /// Tests that "Return" keeps resolving to Enter, the alias callers already relied on. + /// + [Test] + public void Resolve_WhenGivenTheReturnAlias_ResolvesEnter() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("Return"); + + Assert.IsTrue(resolved); + Assert.AreEqual(Key.Enter, key); + } + + /// + /// Tests that a bare digit is rejected instead of being read as an enum ordinal. + /// + [Test] + public void Resolve_WhenGivenAnOrdinal_IsRejected() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("3"); + + Assert.IsFalse(resolved); + Assert.AreEqual(Key.None, key); + } + + /// + /// Tests that a signed ordinal is rejected: Enum.TryParse accepted it as a value too. + /// + [Test] + public void Resolve_WhenGivenASignedOrdinal_IsRejected() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("-1"); + + Assert.IsFalse(resolved); + Assert.AreEqual(Key.None, key); + } + + /// + /// Tests that an ordinal outside the enum is rejected rather than producing an undefined key. + /// + [Test] + public void Resolve_WhenGivenAnUndefinedOrdinal_IsRejected() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("300"); + + Assert.IsFalse(resolved); + Assert.AreEqual(Key.None, key); + } + + /// + /// Tests that comma-separated names are rejected instead of being OR-ed into one value. + /// + [Test] + public void Resolve_WhenGivenCommaSeparatedNames_IsRejected() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("Space,Enter"); + + Assert.IsFalse(resolved); + Assert.AreEqual(Key.None, key); + } + + /// + /// Tests that the placeholder None value is rejected: it names no physical key. + /// + [Test] + public void Resolve_WhenGivenNone_IsRejected() + { + (bool resolved, Key key) = KeyNameResolver.Resolve("None"); + + Assert.IsFalse(resolved); + Assert.AreEqual(Key.None, key); + } + } +} +#endif diff --git a/Assets/Tests/Editor/KeyNameResolverTests.cs.meta b/Assets/Tests/Editor/KeyNameResolverTests.cs.meta new file mode 100644 index 0000000000..1a6fa3479f --- /dev/null +++ b/Assets/Tests/Editor/KeyNameResolverTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b1157c682c6684c189f34872ba911a8a +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs index 90e5df343e..55035071fb 100644 --- a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs +++ b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs @@ -92,14 +92,20 @@ public static string ResolveLatestRecording(string inputPath) return files.OrderByDescending(f => File.GetLastWriteTimeUtc(f)).First(); } - public static HashSet? ParseKeyFilter(string keys) + /// + /// Parses the comma-separated key filter into the keys to record. Entries that name no key + /// are reported rather than skipped: dropping them silently would record a different set of + /// keys than the caller asked for, and dropping all of them would record every key. + /// + public static KeyFilterParseResult ParseKeyFilter(string keys) { if (string.IsNullOrEmpty(keys)) { - return null; + return new KeyFilterParseResult(null, System.Array.Empty()); } HashSet filter = new(); + List invalidKeyNames = new(); string[] parts = keys.Split(','); for (int i = 0; i < parts.Length; i++) @@ -110,17 +116,17 @@ public static string ResolveLatestRecording(string inputPath) continue; } - if (Enum.TryParse(trimmed, ignoreCase: true, out Key key) && key != Key.None) - { - filter.Add(key); - } - else + (bool resolved, Key key) = KeyNameResolver.Resolve(trimmed); + if (!resolved) { - Debug.LogWarning($"[InputRecordingFileHelper] Unknown key name in filter: '{trimmed}'"); + invalidKeyNames.Add(trimmed); + continue; } + + filter.Add(key); } - return filter.Count > 0 ? filter : null; + return new KeyFilterParseResult(filter.Count > 0 ? filter : null, invalidKeyNames); } } } diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs new file mode 100644 index 0000000000..0c1abaa2e2 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs @@ -0,0 +1,27 @@ +#if ULOOP_HAS_INPUT_SYSTEM +#nullable enable +using System.Collections.Generic; +using UnityEngine.InputSystem; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Outcome of parsing a comma-separated key filter: the keys to record, plus every entry that + /// named no key. Why carry the rejected entries instead of dropping them: a filter that lost + /// entries records something other than what was asked for, and the caller cannot tell that + /// from a filter that was never given. + /// + internal sealed class KeyFilterParseResult + { + public KeyFilterParseResult(HashSet? filter, IReadOnlyList invalidKeyNames) + { + Filter = filter; + InvalidKeyNames = invalidKeyNames; + } + + public HashSet? Filter { get; } + + public IReadOnlyList InvalidKeyNames { get; } + } +} +#endif diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs.meta b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs.meta new file mode 100644 index 0000000000..e816f6faac --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/KeyFilterParseResult.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: e6ed0f03d8bd4407691333639c5611fa +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs b/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs new file mode 100644 index 0000000000..3de90c343c --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs @@ -0,0 +1,68 @@ +#if ULOOP_HAS_INPUT_SYSTEM +using System; +using System.Collections.Generic; +using UnityEngine.InputSystem; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Resolves Input System Key values from the key names tools accept, so every tool applies the + /// same rule for what counts as a key name. + /// + internal static class KeyNameResolver + { + // Immutable name-to-value map of the Input System Key enum, so key resolution never falls + // back to Enum.TryParse's ordinal and flag-combination behavior. + private static readonly IReadOnlyDictionary DefinedKeysByName = BuildDefinedKeysByName(); + + /// + /// Resolves a raw key name to its Key value, reporting whether it named a key at all. + /// + public static (bool resolved, Key key) Resolve(string keyName) + { + // Why not Enum.TryParse: it also accepts ordinals ("3"), signed ordinals ("+3"), + // whitespace-padded input, comma-separated names OR-ed together ("Space,Enter"), and + // undefined ordinals ("300") that later throw from the keyboard indexer. Only a name + // that is defined on the Key enum may resolve to a key. + string normalizedKey = NormalizeKeyName(keyName); + if (!DefinedKeysByName.TryGetValue(normalizedKey, out Key key) || key == Key.None) + { + return (false, Key.None); + } + + return (true, key); + } + + /// + /// Trims the raw name and applies the Return alias, giving callers the form the whitelist + /// is keyed by so their diagnostics can describe the same value that was looked up. + /// + public static string NormalizeKeyName(string keyName) + { + // Why trim here rather than at the whitelist comparison: Enum.TryParse used to accept + // whitespace-padded names, so padded correct input already worked. Blocking ambiguous + // input must not narrow correct input, and the alias has to see the padded form too. + string trimmed = keyName.Trim(); + if (string.Equals(trimmed, "Return", StringComparison.OrdinalIgnoreCase)) + { + return Key.Enter.ToString(); + } + + return trimmed; + } + + private static IReadOnlyDictionary BuildDefinedKeysByName() + { + string[] names = Enum.GetNames(typeof(Key)); + Array values = Enum.GetValues(typeof(Key)); + Dictionary keysByName = new(names.Length, StringComparer.OrdinalIgnoreCase); + for (int index = 0; index < names.Length; index++) + { + keysByName[names[index]] = (Key)values.GetValue(index); + } + + return keysByName; + } + } +} +#endif diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs.meta b/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs.meta new file mode 100644 index 0000000000..79bba0de6f --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/InputSystem/KeyNameResolver.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: e3c265151ed404bb1aa2e42f44205e01 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs index 2ca4bc94f9..dda4f02e88 100644 --- a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs @@ -141,7 +141,23 @@ private static async Task ExecuteStartAsync( } int delaySeconds = Mathf.Clamp(request.DelaySeconds, RecordInputConstants.MIN_DELAY_SECONDS, RecordInputConstants.MAX_DELAY_SECONDS); - HashSet? keyFilter = InputRecordingFileHelper.ParseKeyFilter(request.Keys); + KeyFilterParseResult keyFilterResult = InputRecordingFileHelper.ParseKeyFilter(request.Keys); + if (keyFilterResult.InvalidKeyNames.Count > 0) + { + // Why reject instead of recording what did parse: a recording is taken once, and a + // filter that quietly lost entries produces a file that looks like the requested + // one. Entries that all failed would record every key, the opposite of the request. + return new RecordInputResponse + { + Success = false, + Message = + $"Invalid key name(s) in the keys filter: {string.Join(", ", keyFilterResult.InvalidKeyNames)}. " + + "Use Input System Key enum names (e.g. \"W\", \"Space\", \"LeftShift\", \"Digit3\").", + Action = RecordInputAction.Start.ToString() + }; + } + + HashSet? keyFilter = keyFilterResult.Filter; if (request.ShowOverlay) { diff --git a/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md b/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md index 40859135fa..1dcf2a1235 100644 --- a/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md +++ b/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter (for example `W,A,S,D,Space`). Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs index 3520090e25..146948b3ae 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs @@ -1,5 +1,4 @@ #nullable enable -using System; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; @@ -78,12 +77,9 @@ public async Task ExecuteAsync( }; } - // Why not Enum.TryParse alone: it also accepts ordinals ("3"), signed ordinals ("+3"), - // whitespace-padded input, comma-separated names OR-ed together ("Space,Enter"), and - // undefined ordinals ("300") that later throw from the keyboard indexer. Only a name - // that is defined on the Key enum may resolve to a key. - string normalizedKey = NormalizeKeyName(parameters.Key); - if (!DefinedKeysByName.TryGetValue(normalizedKey, out Key key) || key == Key.None) + string normalizedKey = KeyNameResolver.NormalizeKeyName(parameters.Key); + (bool resolved, Key key) = KeyNameResolver.Resolve(parameters.Key); + if (!resolved) { // Suggest from the normalized form so padding does not degrade the candidates, // while the message below still reports the raw input verbatim. @@ -242,23 +238,6 @@ private static void EnsureOverlayExists() OverlayCanvasFactory.EnsureExists(); } - // Immutable name-to-value map of the Input System Key enum, so key resolution never falls - // back to Enum.TryParse's ordinal and flag-combination behavior. - private static readonly IReadOnlyDictionary DefinedKeysByName = BuildDefinedKeysByName(); - - private static IReadOnlyDictionary BuildDefinedKeysByName() - { - string[] names = Enum.GetNames(typeof(Key)); - Array values = Enum.GetValues(typeof(Key)); - Dictionary keysByName = new(names.Length, StringComparer.OrdinalIgnoreCase); - for (int index = 0; index < names.Length; index++) - { - keysByName[names[index]] = (Key)values.GetValue(index); - } - - return keysByName; - } - /// /// Reports whether the raw key input is the numeric form that Enum.TryParse used to accept /// as an enum ordinal, so the rejection can explain what earlier runs actually pressed. @@ -289,18 +268,6 @@ private static bool LooksLikeNumericKeyInput(string keyName) return true; } - private static string NormalizeKeyName(string keyName) - { - // Why trim here rather than at the whitelist comparison: Enum.TryParse used to accept - // whitespace-padded names, so padded correct input already worked. Blocking ambiguous - // input must not narrow correct input, and the alias has to see the padded form too. - string trimmed = keyName.Trim(); - if (string.Equals(trimmed, "Return", StringComparison.OrdinalIgnoreCase)) - { - return Key.Enter.ToString(); - } - return trimmed; - } #endif } diff --git a/cli/common/tools/default-tools.json b/cli/common/tools/default-tools.json index 6f3eb5e7e9..36da5a4554 100644 --- a/cli/common/tools/default-tools.json +++ b/cli/common/tools/default-tools.json @@ -654,7 +654,7 @@ }, "Keys": { "type": "string", - "description": "Comma-separated key filter (for example W,A,S,D,Space). Empty records all common game keys", + "description": "Comma-separated key filter of Input System Key enum names (for example W,A,S,D,Space). Case-insensitive. Digit keys use Digit0-Digit9 or Numpad0-Numpad9, not bare 0-9; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys", "default": "" }, "DelaySeconds": { From e4babfb84635be8247ec862f88e7fa59729d1c27 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 11:44:10 +0900 Subject: [PATCH 2/2] Address review: stamp shared release inputs and pin the rejection contract - Run scripts/stamp-release-inputs.sh: the regenerated tool catalog is a shared release input, so both stamp files must move with it or the release trigger guard fails. - Say "fails the command" rather than "is rejected" in the --keys documentation: rejected reads as if only that name is dropped while the rest still records. - A filter made only of empty entries ("," or " ") no longer falls through to recording every key with a response indistinguishable from no filter at all. A trailing comma beside a named key stays harmless. - Add the PlayMode test for the rejection itself: without it, deleting the check in RecordInputUseCase left every test green. - Move normalizedKey into the failure branch that is now its only user. --- .agents/skills/uloop-record-input/SKILL.md | 2 +- .claude/skills/uloop-record-input/SKILL.md | 2 +- .../Editor/InputRecordingKeyFilterTests.cs | 25 ++++++++ .../RecordInputKeyFilterRejectionTests.cs | 57 +++++++++++++++++++ ...RecordInputKeyFilterRejectionTests.cs.meta | 11 ++++ .../InputRecordingFileHelper.cs | 11 +++- .../RecordInput/Skill/SKILL.md | 2 +- .../SimulateKeyboardUseCase.cs | 2 +- cli/common/tools/default-tools.json | 2 +- cli/dispatcher/shared-inputs-stamp.json | 2 +- cli/project-runner/shared-inputs-stamp.json | 2 +- 11 files changed, 110 insertions(+), 8 deletions(-) create mode 100644 Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs create mode 100644 Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs.meta diff --git a/.agents/skills/uloop-record-input/SKILL.md b/.agents/skills/uloop-record-input/SKILL.md index 1dcf2a1235..d3481797b3 100644 --- a/.agents/skills/uloop-record-input/SKILL.md +++ b/.agents/skills/uloop-record-input/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key fails the command instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/.claude/skills/uloop-record-input/SKILL.md b/.claude/skills/uloop-record-input/SKILL.md index 1dcf2a1235..d3481797b3 100644 --- a/.claude/skills/uloop-record-input/SKILL.md +++ b/.claude/skills/uloop-record-input/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key fails the command instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs index 4d465c32bf..f62b1bdbce 100644 --- a/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs +++ b/Assets/Tests/Editor/InputRecordingKeyFilterTests.cs @@ -50,6 +50,31 @@ public void ParseKeyFilter_WhenOneEntryIsInvalid_ReportsThatEntry() Assert.AreEqual(new[] { "3" }, result.InvalidKeyNames); } + /// + /// Tests that a filter made only of empty entries is reported invalid: it would otherwise + /// record every key while looking like no filter was requested. + /// + [Test] + public void ParseKeyFilter_WhenEveryEntryIsEmpty_ReportsTheRawInputInvalid() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter(", ,"); + + Assert.AreEqual(new[] { ", ," }, result.InvalidKeyNames); + Assert.IsNull(result.Filter); + } + + /// + /// Tests that a trailing comma is harmless once another entry names a key. + /// + [Test] + public void ParseKeyFilter_WhenAnEntryIsEmptyBesideANamedKey_KeepsTheKey() + { + KeyFilterParseResult result = InputRecordingFileHelper.ParseKeyFilter("W,"); + + Assert.IsEmpty(result.InvalidKeyNames); + Assert.AreEqual(new HashSet { Key.W }, result.Filter); + } + /// /// Tests that no filter and no invalid name is reported when the parameter is omitted. /// diff --git a/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs b/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs new file mode 100644 index 0000000000..ff989663d6 --- /dev/null +++ b/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs @@ -0,0 +1,57 @@ +#if ULOOP_HAS_INPUT_SYSTEM +#nullable enable +using System.Collections; +using System.Threading; +using System.Threading.Tasks; +using NUnit.Framework; +using UnityEngine.TestTools; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; +using io.github.hatayama.UnityCliLoop.ToolContracts; + +namespace io.github.hatayama.UnityCliLoop.Tests.PlayMode +{ + /// + /// Test fixture that verifies record-input refuses to start when the key filter names no key. + /// Runs in PlayMode because the rejection sits behind the PlayMode preflight. + /// + public sealed class RecordInputKeyFilterRejectionTests + { + [TearDown] + public void TearDown() + { + // A regression here starts a real recording, which would leak into the next test. + if (InputRecorder.IsRecording) + { + InputRecorder.StopRecording(); + } + } + + /// + /// Tests that a key filter naming no key fails the command instead of recording every key. + /// + [UnityTest] + public IEnumerator RecordInput_WhenTheKeyFilterNamesNoKey_FailsWithoutRecording() + { + RecordInputSchema request = new() + { + Action = RecordInputAction.Start, + Keys = "3", + DelaySeconds = 0, + ShowOverlay = false + }; + + Task execution = new RecordInputUseCase().RecordInputAsync(request, CancellationToken.None); + while (!execution.IsCompleted) + { + yield return null; + } + + RecordInputResponse response = execution.Result; + Assert.IsFalse(response.Success, response.Message); + StringAssert.Contains("Invalid key name(s) in the keys filter: 3", response.Message); + Assert.IsFalse(InputRecorder.IsRecording); + } + } +} +#endif diff --git a/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs.meta b/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs.meta new file mode 100644 index 0000000000..476c7bb13d --- /dev/null +++ b/Assets/Tests/PlayMode/RecordInputKeyFilterRejectionTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 2181da05a0170455b82212942745e63d +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs index 55035071fb..f7c77af1f0 100644 --- a/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs +++ b/Packages/src/Editor/FirstPartyTools/Common/InputRecording/InputRecordingFileHelper.cs @@ -101,7 +101,7 @@ public static KeyFilterParseResult ParseKeyFilter(string keys) { if (string.IsNullOrEmpty(keys)) { - return new KeyFilterParseResult(null, System.Array.Empty()); + return new KeyFilterParseResult(null, Array.Empty()); } HashSet filter = new(); @@ -126,6 +126,15 @@ public static KeyFilterParseResult ParseKeyFilter(string keys) filter.Add(key); } + if (filter.Count == 0 && invalidKeyNames.Count == 0) + { + // Every entry was empty (for example "," or " "), so the filter would fall back to + // recording every key while the response looked like no filter was ever given. + // Why not reject the empty entries themselves: a trailing comma in "W," is harmless + // once at least one entry names a key. + invalidKeyNames.Add(keys); + } + return new KeyFilterParseResult(filter.Count > 0 ? filter : null, invalidKeyNames); } } diff --git a/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md b/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md index 1dcf2a1235..d3481797b3 100644 --- a/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md +++ b/Packages/src/Editor/FirstPartyTools/RecordInput/Skill/SKILL.md @@ -30,7 +30,7 @@ uloop record-input --action Stop --output-path scripts/my-play.json |-----------|------|---------|-------------| | `--action` | enum | `Start` | `Start` - begin recording input, `Stop` - stop recording and save to file | | `--output-path` | string | auto | Save path for the recording JSON. When empty, auto-generates under `.uloop/outputs/InputRecordings/` | -| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys | +| `--keys` | string | `""` | Comma-separated key filter of Input System Key enum names (for example `W,A,S,D,Space`). Case-insensitive. Digit keys use `Digit0`-`Digit9` or `Numpad0`-`Numpad9`, not bare `0`-`9`; a name that matches no key fails the command instead of being dropped from the filter. Empty records all common game keys | | `--delay-seconds` | integer | `3` | Countdown delay in seconds before recording starts (0-10). Gives time to switch focus to Game View. | | `--no-show-overlay` | flag | - | Hide the recording countdown and REC indicator overlay | diff --git a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs index 146948b3ae..5fbce21030 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs @@ -77,12 +77,12 @@ public async Task ExecuteAsync( }; } - string normalizedKey = KeyNameResolver.NormalizeKeyName(parameters.Key); (bool resolved, Key key) = KeyNameResolver.Resolve(parameters.Key); if (!resolved) { // Suggest from the normalized form so padding does not degrade the candidates, // while the message below still reports the raw input verbatim. + string normalizedKey = KeyNameResolver.NormalizeKeyName(parameters.Key); IReadOnlyList suggestions = KeyboardKeyNameSuggester.Suggest(normalizedKey); string suggestionText = suggestions.Count == 0 ? string.Empty diff --git a/cli/common/tools/default-tools.json b/cli/common/tools/default-tools.json index 36da5a4554..df36cb6ffd 100644 --- a/cli/common/tools/default-tools.json +++ b/cli/common/tools/default-tools.json @@ -654,7 +654,7 @@ }, "Keys": { "type": "string", - "description": "Comma-separated key filter of Input System Key enum names (for example W,A,S,D,Space). Case-insensitive. Digit keys use Digit0-Digit9 or Numpad0-Numpad9, not bare 0-9; a name that matches no key is rejected instead of being dropped from the filter. Empty records all common game keys", + "description": "Comma-separated key filter of Input System Key enum names (for example W,A,S,D,Space). Case-insensitive. Digit keys use Digit0-Digit9 or Numpad0-Numpad9, not bare 0-9; a name that matches no key fails the command instead of being dropped from the filter. Empty records all common game keys", "default": "" }, "DelaySeconds": { diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index 1ea01b792d..5750f15ebe 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "b9bc288d3a76099a1fe4cee8843e510bf5bbe632" + "sharedInputsHash": "0313515f1add4e1c394e1957c0aff7215e39e678" } diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index 1c3005f88e..086c6efc93 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "f6f1ad4c29722943c9a66a64cfc68d706ff8b141" + "sharedInputsHash": "b85b6271eda91daf75fb620f635266c7a709f90b" }