From ba0b989c98bfacc34796327aa4d1d2bd5c9cf73f Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:39:33 +0200 Subject: [PATCH 1/2] Add git-commit @file route for multi-line messages (#340) The @file payload parser leaked syntax brackets into field names (git-commit:::MSG[:::PATH...] -> fields msg[, path...]), so git-commit:@- 404'd with "missing required field 'msg['" and any real subject+body commit had to drop to raw git commit -F -- which skips the op's auto Co-Authored-By trailer and forced a follow-up --amend. Generalize the field parser: a token in a trailing [...] group is optional, a ... token is variadic (payload list expands to multiple positional args). git-commit syntax relabeled MESSAGE[:::PATHS...] so the payload keys are message (required) + paths (optional list); the trailer is appended on both routes. Colon-CLI one-liner and the builtin edit/replace/paste/vim @file routes are unchanged. Co-Authored-By: Max --- CHANGELOG.md | 1 + docs/presets/git.md | 2 +- presets/git.json | 4 +- presets/git/commit.py | 2 +- presets/git/resolve.py | 2 +- supertool.py | 92 +++++++++++++++++++++++++------------ tests/test_at_file_route.py | 81 ++++++++++++++++++++++++++------ 7 files changed, 137 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c02b8..5415471 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **`git-status` gained a `:full` mode** — `git-status:full` (alias `:porcelain`) uncaps every list in the dashboard (staged/unstaged/untracked files, other branches, stashes), printing the complete untruncated set instead of the default `... (N more)` markers. The default view stays capped (20 staged/unstaged, 10 untracked/branches, 5 stashes) — a cheap overview that answers ahead/behind + dirty + MR. The bug wasn't the truncation (correct for the common case) but the lack of an escape hatch: driving precise staging — e.g. excluding a few pre-existing untracked items from a large commit — needs the full machine-readable list, which previously forced a drop back to raw `git status --porcelain`. Closes [#330](https://github.com/Digital-Process-Tools/claude-supertool/issues/330). - **`gl-job` / `gh-job` gained a `:fail` suffix** — `gl-job:ID:fail` (alias `:errors`) prints only the matched failure blocks with no tail, the tight triage view. It applies the same per-job pattern table as the default mode (rector → diff lines, phpstan → 🪪/type markers, unit → `FAILURES!`/`Failed asserting`), so a red job shows just its actionable failures instead of the default's blocks-plus-80-line-tail. This names the discoverable front door for a behavior that previously only existed as the undocumented `:errors` mode on `gl-job` (and was entirely absent on `gh-job`); `:errors` stays as a back-compat alias. The default (no suffix), `:grep:PATTERN`, and `:raw` are unchanged. Closes [#326](https://github.com/Digital-Process-Tools/claude-supertool/issues/326). - **`grep` gained a `:no-auto-read` flag** — `grep:PATTERN:PATH:no-auto-read` suppresses the single-small-file auto-read so only the matching line(s) are emitted, mirroring `glob`'s existing flag. Default behavior (auto-read a concrete file < 20KB on a match) is unchanged. The flag is order-independent with `:count` and any `LIMIT`/`CONTEXT` args. Avoids silently dumping 10-18KB of unrequested file content when the caller only wants the hit. Closes [#320](https://github.com/Digital-Process-Tools/claude-supertool/issues/320). +- **`git-commit` gained an `@file` payload route for multi-line messages** — `git-commit:@-` (stdin) or `git-commit:@msg.toml` reads a `message` field (subject + blank line + body) and an optional `paths` list, so a real subject+body commit no longer forces a drop to raw `git commit -F file` — which skipped the op's auto `Co-Authored-By` trailer and needed a follow-up `--amend`. The trailer is still appended on the payload route. The colon-CLI one-liner (`git-commit:::MESSAGE`) is unchanged. Mechanically this generalizes the `@file` field parser: a syntax token in a trailing `[...]` group is now optional, and a `...` token is variadic (a payload list expands into multiple positional args) — previously the parser leaked the `[`/`...]` brackets into the field names (`msg[`, `path...]`), so the route 404'd with `missing required field 'msg['`. Closes [#340](https://github.com/Digital-Process-Tools/claude-supertool/issues/340). ### Fixed diff --git a/docs/presets/git.md b/docs/presets/git.md index 0b52b44..87e0384 100644 --- a/docs/presets/git.md +++ b/docs/presets/git.md @@ -20,7 +20,7 @@ Git investigation and workflow ops. Replaces the 4-6 raw `git` calls you'd norma | `git-merge` | `git-merge:REF` | Merge REF — on conflict surfaces the UU file list, conflict markers, and ours/theirs SHAs | | `git-conflicts` | `git-conflicts` | List all UU files + every conflict block + abort hint | | `git-resolve` | `git-resolve:::SIDE:::PATH[,PATH...][:::BLOCKS]` | Pick `ours`/`theirs`/`both` for one file, a comma-separated list, or `all` — stages and prints the continue command. `both` is a union: it strips the conflict markers and keeps both sides (ours then theirs), like git's `merge=union` driver — use it when both branches added different non-overlapping lines. Optional **`BLOCKS`** selector (e.g. `1,3`) resolves only those 1-indexed conflict blocks of a **single** file, numbered exactly as `git-conflicts` lists them; one side per call (mixed sides → run twice). A **partial** resolve leaves the other blocks' markers in place by design, so the file stays conflicted and **unstaged** — the receipt reads `N of M block(s) resolved, file still conflicted`; only when the selector covers every block does the file go clean and get staged. **Self-verifies before staging:** a leftover `<<<<<<<` / `>>>>>>>` is a hard fail (file left unstaged), and each resolved file's receipt carries a warn-only validator digest (`markers: clean \| validate: ok`) | -| `git-commit` | `git-commit:::MSG[:::PATH...]` | Stage PATHs (or all staged if omitted) and commit with MSG — surfaces hook errors, shows HEAD before/after. Use `MSG=--no-edit` to reuse MERGE_MSG/CHERRY_PICK_HEAD during an in-progress merge or cherry-pick. Auto-appends a `Co-Authored-By:` trailer when the message lacks one (default `Max `) — configurable via `.supertool.json` (`ops.git-commit.coauthor`) or the `SUPERTOOL_COAUTHOR` env var; disable with an empty value or `none`/`off`/`false` | +| `git-commit` | `git-commit:::MESSAGE[:::PATHS...]` | Stage PATHs (or all staged if omitted) and commit with MESSAGE — surfaces hook errors, shows HEAD before/after. Use `MESSAGE=--no-edit` to reuse MERGE_MSG/CHERRY_PICK_HEAD during an in-progress merge or cherry-pick. **Multi-line body:** use the `@file` route — `git-commit:@-` (stdin) or `git-commit:@msg.toml` — with a `message` field (subject + blank line + body) and an optional `paths` list, instead of dropping to raw `git commit -F` (which skips the trailer below). Auto-appends a `Co-Authored-By:` trailer when the message lacks one (default `Max `), on both the colon-CLI and `@file` routes — configurable via `.supertool.json` (`ops.git-commit.coauthor`) or the `SUPERTOOL_COAUTHOR` env var; disable with an empty value or `none`/`off`/`false` | | `git-push` | `git-push[:force-with-lease][:no-verify]` | Push the current branch (sets upstream on first push) — remote SHA before/after with commits pushed, ahead/behind vs upstream, and the open MR/PR + pipeline status. For **updating** an already-open MR; use the `mr` op for push+create. **Non-fast-forward** is handled in-op: it fetches, surfaces the **incoming remote commits** (SHA, author, subject) so you can see whose work you'd be rebasing over, then rebases your work onto the remote and re-pushes; on conflict it leaves the rebase **paused**, warns to check the incoming authors before forcing, and points you at `git-conflicts` + the keep-both/cancel/force paths — never auto-forced, never silently rewritten. A **pre-push hook that amends HEAD and pushes** the fixed commit itself (exiting non-zero) is reported as `PUSHED`, not `REJECTED`, since the live remote ref already matches HEAD. The **post-push receipt** carries the next-decision signals (all on calls already made): MR **mergeability** (warns if it now `cannot_be_merged` with target), **stale base** (`N behind origin/`), **uncommitted leftovers** (changes not in this push), the **pipeline id + url**, and a ready `watch:gitlab-mr:` command. `:force-with-lease` also reports **what it discarded** (author + subject of overwritten remote commits). Flags: `:force-with-lease` (safe force — overwrite only if the remote hasn't moved; skips the auto-rebase, and lists discarded commits), `:no-verify` (skip the local pre-push hook, e.g. when a local formatter diverges from CI), `:watch` (spawn a background pipeline poller instead of just recommending the command) | ## Common workflows diff --git a/presets/git.json b/presets/git.json index cfbbd11..5232f59 100644 --- a/presets/git.json +++ b/presets/git.json @@ -65,8 +65,8 @@ "git-commit": { "cmd": "{python} {path}git/commit.py {args}", "timeout": 60, - "description": "Commit MSG (stages PATHS) — HEAD before/after, hook errors surfaced. MSG=--no-edit reuses MERGE_MSG/CHERRY_PICK msg (merge or cherry-pick must be in progress). Auto-appends a `Co-Authored-By:` trailer when absent; identity via .supertool.json ops.git-commit.coauthor (default 'Max '; set '' / 'none' to disable).", - "syntax": "git-commit:::MSG[:::PATH...]" + "description": "Commit MESSAGE (stages PATHS) — HEAD before/after, hook errors surfaced. MESSAGE=--no-edit reuses MERGE_MSG/CHERRY_PICK msg (merge or cherry-pick must be in progress). Auto-appends a `Co-Authored-By:` trailer when absent; identity via .supertool.json ops.git-commit.coauthor (default 'Max '; set '' / 'none' to disable). Multi-line body: use the @file route — git-commit:@- (stdin) or git-commit:@msg.toml — with a `message` field (subject + blank line + body) and optional `paths` list; the trailer is still appended.", + "syntax": "git-commit:::MESSAGE[:::PATHS...]" }, "git-push": { "cmd": "{python} {path}git/push.py {args}", diff --git a/presets/git/commit.py b/presets/git/commit.py index a09b963..0fe7a80 100644 --- a/presets/git/commit.py +++ b/presets/git/commit.py @@ -133,7 +133,7 @@ def main() -> int: # Pre-commit staged check staged = _git(["diff", "--cached", "--name-only"]) if staged.returncode != 0 or not staged.stdout.strip(): - print("ERROR: nothing staged. Use `git-commit:::MSG:::PATHS` or stage manually first.") + print("ERROR: nothing staged. Use `git-commit:::MESSAGE:::PATHS` or stage manually first.") return 1 staged_files = [l for l in staged.stdout.splitlines() if l.strip()] diff --git a/presets/git/resolve.py b/presets/git/resolve.py index 576929a..9becf01 100644 --- a/presets/git/resolve.py +++ b/presets/git/resolve.py @@ -475,7 +475,7 @@ def main() -> int: elif exists(join(gd, "CHERRY_PICK_HEAD")): print("Next: git cherry-pick --continue") else: - print("Next: ./supertool 'git-commit:::MSG' to commit the resolution.") + print("Next: ./supertool 'git-commit:::MESSAGE' to commit the resolution.") return 0 if not failed else 1 diff --git a/supertool.py b/supertool.py index d7f1fb7..f92fd1f 100755 --- a/supertool.py +++ b/supertool.py @@ -10686,37 +10686,53 @@ def _load_at_file(ref: str) -> Any: # Dynamic @file field registry — built lazily from op syntax strings. # Maps op name → ordered list of JSON field names (positional parts[1..N]). # Populated on first dispatch call via _build_at_file_registry(). -_AT_FILE_REGISTRY: Dict[str, List[str]] = {} +_AT_FILE_REGISTRY: Dict[str, List[Tuple[str, bool, bool]]] = {} _AT_FILE_REGISTRY_BUILT: bool = False -def _fields_from_syntax(syntax: str) -> List[str]: - """Derive positional field names from a syntax string using ':::' separator. +def _fields_from_syntax(syntax: str) -> List[Tuple[str, bool, bool]]: + """Derive field specs from a syntax string using ':::' separator. - Takes the first alternative (before ' | '), splits on ':::', drops the - first token (op name), and lowercases the rest. + Returns a list of (name, optional, variadic) tuples: + - name: lowercased field name, stripped of [ ] ... and whitespace + - optional: field sits inside a trailing [...] optional group + - variadic: field token carried '...' — payload value may be a list, + expanded into multiple positional parts - Returns [] if the syntax has no ':::' (read-only op — no @file route). + Takes the first alternative (before ' | '), splits on ':::', drops the + first token (op name). Returns [] if the syntax has no ':::' (read-only + op — no @file route). Optional groups are assumed trailing (the only + shape used across the op syntaxes), so a field is optional once any '[' + has opened. Examples: - 'edit:::OLD:::NEW:::PATH' → ['old', 'new', 'path'] - 'paste:::PATH:::CONTENT' → ['path', 'content'] - 'read:PATH' → [] + 'edit:::OLD:::NEW:::PATH' → [('old',F,F),('new',F,F),('path',F,F)] + 'git-commit:::MESSAGE[:::PATHS...]' → [('message',F,F),('paths',T,T)] + 'read:PATH' → [] """ first_alt = re.split(r"\s*\|\s*", syntax)[0] if ":::" not in first_alt: return [] - tokens = first_alt.split(":::") - return [t.strip().lower() for t in tokens[1:]] - - -_AT_FILE_BUILTIN_DEFAULTS: Dict[str, List[str]] = { - "edit": ["old", "new", "path"], - "replace": ["old", "new", "path"], - "replace_dry": ["old", "new", "path"], - "replace_lines": ["path", "start", "end", "content"], - "paste": ["path", "content"], - "vim": ["path", "script"], + specs: List[Tuple[str, bool, bool]] = [] + seen_open = False + for tok in first_alt.split(":::")[1:]: + variadic = "..." in tok + optional = seen_open + if "[" in tok: + seen_open = True + name = (tok.replace("[", "").replace("]", "") + .replace("...", "").strip().lower()) + specs.append((name, optional, variadic)) + return specs + + +_AT_FILE_BUILTIN_DEFAULTS: Dict[str, List[Tuple[str, bool, bool]]] = { + "edit": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace_dry": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace_lines": [("path", False, False), ("start", False, False), ("end", False, False), ("content", False, False)], + "paste": [("path", False, False), ("content", False, False)], + "vim": [("path", False, False), ("script", False, False)], } @@ -10733,7 +10749,7 @@ def _build_at_file_registry() -> None: global _AT_FILE_REGISTRY, _AT_FILE_REGISTRY_BUILT if _AT_FILE_REGISTRY_BUILT: return - registry: Dict[str, List[str]] = dict(_AT_FILE_BUILTIN_DEFAULTS) + registry: Dict[str, List[Tuple[str, bool, bool]]] = dict(_AT_FILE_BUILTIN_DEFAULTS) config = _load_config() for section in ("builtin-ops", "ops"): for op_name, info in config.get(section, {}).items(): @@ -10749,12 +10765,21 @@ def _build_at_file_registry() -> None: _AT_FILE_REGISTRY_BUILT = True -def _at_file_fields(op: str) -> List[str]: - """Return the field list for *op*, or [] if the op has no @file route.""" +def _at_file_specs(op: str) -> List[Tuple[str, bool, bool]]: + """Return (name, optional, variadic) specs for *op*, or [] if no @file route.""" _build_at_file_registry() return _AT_FILE_REGISTRY.get(op, []) +def _at_file_fields(op: str) -> List[str]: + """Return the field NAMES for *op*, or [] if the op has no @file route. + + Kept name-only for the truthiness/sub-op callers; field semantics + (optional, variadic) live in _at_file_specs. + """ + return [name for name, _opt, _var in _at_file_specs(op)] + + def _reorder_batch_for_snapshot(batch_ops: List[Any]) -> Tuple[List[Any], str]: """Reorder replace_lines ops within a batch so line numbers refer to the original file state (snapshot semantics), not the file as mutated by @@ -10838,18 +10863,27 @@ def _at_file_to_parts(op: str, payload: Any) -> Tuple[List[str], bool]: f"@file payload for op '{op}' must be a JSON object, " f"got {type(payload).__name__}" ) - fields = _at_file_fields(op) - if not fields: + specs = _at_file_specs(op) + if not specs: raise ValueError(f"@file route not supported for op '{op}'") # Case-insensitive key lookup — normalise payload keys once. lower_payload = {k.lower(): v for k, v in payload.items()} parts = [op] - for field in fields: - if field not in lower_payload: + for name, optional, variadic in specs: + if name not in lower_payload: + if optional: + continue raise ValueError( - f"@file payload for op '{op}' missing required field '{field}'" + f"@file payload for op '{op}' missing required field '{name}'" ) - parts.append(str(lower_payload[field])) + value = lower_payload[name] + if variadic: + # Accept a single scalar or a list; each element becomes one + # positional part (e.g. git-commit paths → PATH PATH ...). + items = value if isinstance(value, list) else [value] + parts.extend(str(v) for v in items) + else: + parts.append(str(value)) replace_all = bool(lower_payload.get("replace_all", False)) return parts, replace_all diff --git a/tests/test_at_file_route.py b/tests/test_at_file_route.py index 5326fff..eb52fa7 100644 --- a/tests/test_at_file_route.py +++ b/tests/test_at_file_route.py @@ -180,13 +180,16 @@ def test_read_at_file_not_intercepted(self, tmp_path: Path) -> None: class TestFieldsFromSyntax: @pytest.mark.parametrize("syntax,expected", [ - ("edit:::OLD:::NEW:::PATH", ["old", "new", "path"]), - ("replace:::OLD:::NEW:::PATH", ["old", "new", "path"]), - ("replace_lines:::PATH:::START:::END:::CONTENT", ["path", "start", "end", "content"]), - ("paste:::PATH:::CONTENT", ["path", "content"]), - ("vim:::PATH:::SCRIPT", ["path", "script"]), + ("edit:::OLD:::NEW:::PATH", [("old", False, False), ("new", False, False), ("path", False, False)]), + ("replace:::OLD:::NEW:::PATH", [("old", False, False), ("new", False, False), ("path", False, False)]), + ("replace_lines:::PATH:::START:::END:::CONTENT", [("path", False, False), ("start", False, False), ("end", False, False), ("content", False, False)]), + ("paste:::PATH:::CONTENT", [("path", False, False), ("content", False, False)]), + ("vim:::PATH:::SCRIPT", [("path", False, False), ("script", False, False)]), + # Trailing optional group → optional; '...' → variadic. Brackets/ellipsis stripped from names. + ("git-commit:::MESSAGE[:::PATHS...]", [("message", False, False), ("paths", True, True)]), + ("op:::A[:::B]", [("a", False, False), ("b", True, False)]), # First alternative is used when ' | ' separates alternatives - ("op:::A:::B | op:::X:::Y", ["a", "b"]), + ("op:::A:::B | op:::X:::Y", [("a", False, False), ("b", False, False)]), # Read-only op (no :::) → empty list ("read:PATH", []), ("grep:PATTERN:PATH", []), @@ -195,17 +198,21 @@ def test_fields_from_syntax(self, syntax: str, expected: list) -> None: assert supertool._fields_from_syntax(syntax) == expected def test_builtin_defaults_cover_all_write_ops(self) -> None: - """_AT_FILE_BUILTIN_DEFAULTS must derive the same field lists as the hardcoded table.""" + """_AT_FILE_BUILTIN_DEFAULTS must derive the same field specs as the hardcoded table.""" expected = { - "edit": ["old", "new", "path"], - "replace": ["old", "new", "path"], - "replace_dry": ["old", "new", "path"], - "replace_lines": ["path", "start", "end", "content"], - "paste": ["path", "content"], - "vim": ["path", "script"], + "edit": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace_dry": [("old", False, False), ("new", False, False), ("path", False, False)], + "replace_lines": [("path", False, False), ("start", False, False), ("end", False, False), ("content", False, False)], + "paste": [("path", False, False), ("content", False, False)], + "vim": [("path", False, False), ("script", False, False)], } assert supertool._AT_FILE_BUILTIN_DEFAULTS == expected + def test_at_file_fields_returns_names_only(self) -> None: + """_at_file_fields stays name-only for the truthiness/sub-op callers.""" + assert supertool._at_file_fields("edit") == ["old", "new", "path"] + def test_registry_populated_after_first_dispatch(self, tmp_path: Path) -> None: """After any dispatch call the registry must contain the builtin write ops.""" # Force a dispatch so the registry is built @@ -529,3 +536,51 @@ def test_different_files_no_cross_overlap_check(self) -> None: ] new, err = supertool._reorder_batch_for_snapshot(ops) assert err == "" + + +# --------------------------------------------------------------------------- +# Optional + variadic fields (#340 — git-commit:@- multi-line message route) +# --------------------------------------------------------------------------- + +class TestAtFileOptionalVariadic: + """_at_file_to_parts honours optional fields and expands variadic lists. + + Uses the same (name, optional, variadic) spec shape the registry builds + for `git-commit:::MESSAGE[:::PATHS...]` — message required, paths optional + and list-valued — without depending on a preset config being loaded. + """ + + _SPECS = [("message", False, False), ("paths", True, True)] + + def _patch(self, monkeypatch) -> None: + monkeypatch.setattr( + supertool, "_at_file_specs", + lambda op: self._SPECS if op == "git-commit" else [], + ) + + def test_message_only_omits_optional_paths(self, monkeypatch) -> None: + self._patch(monkeypatch) + parts, replace_all = supertool._at_file_to_parts( + "git-commit", {"message": "subject\n\nbody"} + ) + assert parts == ["git-commit", "subject\n\nbody"] + assert replace_all is False + + def test_paths_list_expands_to_multiple_parts(self, monkeypatch) -> None: + self._patch(monkeypatch) + parts, _ = supertool._at_file_to_parts( + "git-commit", {"message": "m", "paths": ["a.txt", "b.txt"]} + ) + assert parts == ["git-commit", "m", "a.txt", "b.txt"] + + def test_paths_scalar_becomes_single_part(self, monkeypatch) -> None: + self._patch(monkeypatch) + parts, _ = supertool._at_file_to_parts( + "git-commit", {"message": "m", "paths": "only.txt"} + ) + assert parts == ["git-commit", "m", "only.txt"] + + def test_missing_required_message_raises(self, monkeypatch) -> None: + self._patch(monkeypatch) + with pytest.raises(ValueError, match="missing required field 'message'"): + supertool._at_file_to_parts("git-commit", {"paths": ["a.txt"]}) From f6ffb4244c9aa14587e30f310715332a31d35263 Mon Sep 17 00:00:00 2001 From: Florian DAVID <150798857+fdaviddpt@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:00:15 +0200 Subject: [PATCH 2/2] Harden @file field parser: drop prose syntaxes, depth-based optional, null-safe variadic Review follow-ups on the #340 generalization. - _fields_from_syntax now returns [] when any derived field name is not a clean identifier, so a syntax string carrying inline prose/punctuation (git-resolve: comma list + parenthesized notes) no longer registers a falsely-named, non-functional @file route. The registry is now actually clean across all presets, as the #340 docstring claimed. - Optionality tracked by [ ] bracket depth instead of a one-way seen-open flag, so a required field after a closed optional group stays required. - Variadic expansion drops null values/elements, so paths:null / paths:[] omit cleanly rather than emitting a literal "None" positional arg. Tests: git-resolve real syntax -> [], middle-optional, empty/null paths. Co-Authored-By: Max --- supertool.py | 34 ++++++++++++++++++++++++---------- tests/test_at_file_route.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 10 deletions(-) diff --git a/supertool.py b/supertool.py index f92fd1f..a60c57b 100755 --- a/supertool.py +++ b/supertool.py @@ -10701,9 +10701,15 @@ def _fields_from_syntax(syntax: str) -> List[Tuple[str, bool, bool]]: Takes the first alternative (before ' | '), splits on ':::', drops the first token (op name). Returns [] if the syntax has no ':::' (read-only - op — no @file route). Optional groups are assumed trailing (the only - shape used across the op syntaxes), so a field is optional once any '[' - has opened. + op — no @file route). Optionality is tracked by '[' / ']' bracket depth, + so a field is optional whenever an unclosed group is open at its position + (correct even for a non-trailing optional group). + + Returns [] when any derived field name is not a clean identifier + ([a-z][a-z0-9_]*). That guards against syntax strings carrying inline + prose or punctuation a payload key could never match — e.g. git-resolve's + 'PATH[,PATH...][:::BLOCKS] (SIDE: ...)'. Such ops simply have no @file + route rather than a falsely-registered, non-functional one. Examples: 'edit:::OLD:::NEW:::PATH' → [('old',F,F),('new',F,F),('path',F,F)] @@ -10714,14 +10720,15 @@ def _fields_from_syntax(syntax: str) -> List[Tuple[str, bool, bool]]: if ":::" not in first_alt: return [] specs: List[Tuple[str, bool, bool]] = [] - seen_open = False + depth = 0 for tok in first_alt.split(":::")[1:]: variadic = "..." in tok - optional = seen_open - if "[" in tok: - seen_open = True + optional = depth > 0 + depth += tok.count("[") - tok.count("]") name = (tok.replace("[", "").replace("]", "") .replace("...", "").strip().lower()) + if not re.fullmatch(r"[a-z][a-z0-9_]*", name): + return [] specs.append((name, optional, variadic)) return specs @@ -10879,9 +10886,16 @@ def _at_file_to_parts(op: str, payload: Any) -> Tuple[List[str], bool]: value = lower_payload[name] if variadic: # Accept a single scalar or a list; each element becomes one - # positional part (e.g. git-commit paths → PATH PATH ...). - items = value if isinstance(value, list) else [value] - parts.extend(str(v) for v in items) + # positional part (e.g. git-commit paths → PATH PATH ...). A null + # value or null elements are dropped, so paths:null / paths:[] + # cleanly omit rather than emitting a literal "None" arg. + if value is None: + items: List[Any] = [] + elif isinstance(value, list): + items = value + else: + items = [value] + parts.extend(str(v) for v in items if v is not None) else: parts.append(str(value)) replace_all = bool(lower_payload.get("replace_all", False)) diff --git a/tests/test_at_file_route.py b/tests/test_at_file_route.py index eb52fa7..8657f1c 100644 --- a/tests/test_at_file_route.py +++ b/tests/test_at_file_route.py @@ -188,8 +188,15 @@ class TestFieldsFromSyntax: # Trailing optional group → optional; '...' → variadic. Brackets/ellipsis stripped from names. ("git-commit:::MESSAGE[:::PATHS...]", [("message", False, False), ("paths", True, True)]), ("op:::A[:::B]", [("a", False, False), ("b", True, False)]), + # Bracket depth, not "seen any '['": a required field AFTER a closed + # optional group stays required. + ("op:::A[:::B]:::C", [("a", False, False), ("b", True, False), ("c", False, False)]), # First alternative is used when ' | ' separates alternatives ("op:::A:::B | op:::X:::Y", [("a", False, False), ("b", False, False)]), + # Syntax carrying prose/punctuation a payload key can't match → no @file + # route at all (git-resolve's real syntax: comma list + inline prose). + ("git-resolve:::SIDE:::PATH[,PATH...][:::BLOCKS] (SIDE: ours|theirs|both)", []), + ("op:::A:::B WITH WORDS", []), # Read-only op (no :::) → empty list ("read:PATH", []), ("grep:PATTERN:PATH", []), @@ -580,6 +587,28 @@ def test_paths_scalar_becomes_single_part(self, monkeypatch) -> None: ) assert parts == ["git-commit", "m", "only.txt"] + def test_paths_empty_list_omits(self, monkeypatch) -> None: + self._patch(monkeypatch) + parts, _ = supertool._at_file_to_parts( + "git-commit", {"message": "m", "paths": []} + ) + assert parts == ["git-commit", "m"] + + def test_paths_null_omits_no_literal_none(self, monkeypatch) -> None: + """paths:null (JSON) must not emit a literal 'None' positional arg.""" + self._patch(monkeypatch) + parts, _ = supertool._at_file_to_parts( + "git-commit", {"message": "m", "paths": None} + ) + assert parts == ["git-commit", "m"] + + def test_paths_list_drops_null_elements(self, monkeypatch) -> None: + self._patch(monkeypatch) + parts, _ = supertool._at_file_to_parts( + "git-commit", {"message": "m", "paths": ["a.txt", None, "b.txt"]} + ) + assert parts == ["git-commit", "m", "a.txt", "b.txt"] + def test_missing_required_message_raises(self, monkeypatch) -> None: self._patch(monkeypatch) with pytest.raises(ValueError, match="missing required field 'message'"):