From 0ebb6466803a24fc55bbc2d63ef912be693b94c6 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 05:07:23 +0000 Subject: [PATCH 1/9] Importer: de-duplicate parameters into CLI-shared parameters When the same parameter recurs across two or more snips imported into a CLI, promote it to a CLI-scoped shared parameter (Cli.Parameters) and strip the local copies, so the resolver inherits it by token name. Single-use parameters stay local. Matching: - Choice parameters match on their option set (order-independent); the name is not part of the match and the most common name wins. Snips that used a different name have their template token rewritten to the winning name. - Text parameters match by name; the most common default wins (empty included). Guards against pathological inputs: two distinct same-option choices in one snip are not collapsed into one token, and a name promoted by one group (e.g. a Choice) is never added a second time by another (e.g. a Text), avoiding duplicate CLI-scoped names. On by default; --no-share-parameters keeps everything local. The dry-run lists the shared parameters per CLI. Adds ParameterSharer (pure Analyze + Apply) with unit tests and a StoreMerger integration test. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 6 +- .../ParameterSharerTests.cs | 197 +++++++++++++ .../StoreMergerTests.cs | 58 ++++ .../Commands/ImportCommandSettings.cs | 4 + .../Commands/SnipCommandImportCommand.cs | 6 +- .../Merge/ParameterSharer.cs | 272 ++++++++++++++++++ tools/Snipdeck.Importer/Merge/StoreMerger.cs | 60 +++- .../Output/DryRunRenderer.cs | 21 ++ tools/Snipdeck.Importer/README.md | 8 + 9 files changed, 626 insertions(+), 6 deletions(-) create mode 100644 tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs create mode 100644 tools/Snipdeck.Importer/Merge/ParameterSharer.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index cdbc3b7..61f65d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,7 +52,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 structured parameters, and defaults to a safe dry-run — `--write` backs the store up first, mints fresh identifiers, creates CLIs on demand and skips duplicates (`--allow-duplicates` to override). `--store`, `--cli` and `--into` - control the target. See [`tools/Snipdeck.Importer/README.md`](tools/Snipdeck.Importer/README.md). + control the target. It also de-duplicates parameters used by two or more snips in + a CLI, promoting them to CLI-scoped shared parameters (Choice params match on their + option set regardless of name/order, most common name wins; Text params match by + name, most common default wins) — `--no-share-parameters` disables it. See + [`tools/Snipdeck.Importer/README.md`](tools/Snipdeck.Importer/README.md). - **Move a snip to a different CLI.** The snip editor now has a **CLI** selector, so a snip can be re-homed to another CLI (useful after a bulk import lands snips in a fallback CLI). diff --git a/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs new file mode 100644 index 0000000..1803f80 --- /dev/null +++ b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs @@ -0,0 +1,197 @@ +using Snipdeck.Core.Models; +using Snipdeck.Importer.Merge; + +namespace Snipdeck.Importer.Tests +{ + public class ParameterSharerTests + { + private static Snip TextSnip(string template, params (string name, string? def)[] ps) + { + return new Snip + { + Title = template, + CommandTemplate = template, + Parameters = [.. ps.Select(p => new Parameter { Name = p.name, Type = ParameterType.Text, Default = p.def })], + }; + } + + private static Parameter Choice(string name, string? def, params string[] options) + { + return new Parameter { Name = name, Type = ParameterType.Choice, Options = [.. options], Default = def }; + } + + private static void AnalyzeAndApply(Cli cli, params Snip[] snips) + { + var plan = ParameterSharer.Analyze(cli.Parameters, snips); + ParameterSharer.Apply(cli, plan); + } + + [Fact] + public void A_text_parameter_used_by_two_snips_is_promoted_and_stripped_from_the_snips() + { + var cli = new Cli { Name = "mpt-app" }; + var a = TextSnip("mpt-app x {id}", ("id", "1005")); + var b = TextSnip("mpt-app y {id}", ("id", "1005")); + + AnalyzeAndApply(cli, a, b); + + var shared = Assert.Single(cli.Parameters); + Assert.Equal("id", shared.Name); + Assert.Equal(ParameterType.Text, shared.Type); + Assert.Equal("1005", shared.Default); + Assert.Empty(a.Parameters); + Assert.Empty(b.Parameters); + // Templates are unchanged for text (the token already matches the shared name). + Assert.Equal("mpt-app x {id}", a.CommandTemplate); + } + + [Fact] + public void A_single_use_parameter_is_left_local() + { + var cli = new Cli { Name = "mpt-app" }; + var a = TextSnip("mpt-app x {only}", ("only", "v")); + + AnalyzeAndApply(cli, a); + + Assert.Empty(cli.Parameters); + Assert.Single(a.Parameters); + } + + [Fact] + public void Text_promotion_uses_the_most_common_default_even_when_empty() + { + var cli = new Cli { Name = "mpt-app" }; + // Two snips with empty (null) default, one with "X" -> empty wins. + var a = TextSnip("a {agreement}", ("agreement", null)); + var b = TextSnip("b {agreement}", ("agreement", null)); + var c = TextSnip("c {agreement}", ("agreement", "X")); + + AnalyzeAndApply(cli, a, b, c); + + var shared = Assert.Single(cli.Parameters); + Assert.Equal("agreement", shared.Name); + Assert.Null(shared.Default); + } + + [Fact] + public void Choice_parameters_match_by_option_set_regardless_of_order() + { + var cli = new Cli { Name = "mpt-app" }; + var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "x", "x", "y", "z")] }; + var b = new Snip { Title = "b", CommandTemplate = "b {authId}", Parameters = [Choice("authId", "z", "z", "y", "x")] }; + + AnalyzeAndApply(cli, a, b); + + var shared = Assert.Single(cli.Parameters); + Assert.Equal("authId", shared.Name); + Assert.Equal(ParameterType.Choice, shared.Type); + Assert.Equal(["x", "y", "z"], shared.Options.OrderBy(o => o, StringComparer.Ordinal).ToList()); + Assert.Empty(a.Parameters); + Assert.Empty(b.Parameters); + } + + [Fact] + public void Choice_promotion_picks_the_most_common_name_and_rewrites_other_template_tokens() + { + var cli = new Cli { Name = "mpt-app" }; + // Same option set, two snips name it "authId", one names it "auth" -> "authId" wins. + var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "p", "p", "q")] }; + var b = new Snip { Title = "b", CommandTemplate = "b {authId}", Parameters = [Choice("authId", "p", "p", "q")] }; + var c = new Snip { Title = "c", CommandTemplate = "c {auth}", Parameters = [Choice("auth", "q", "q", "p")] }; + + AnalyzeAndApply(cli, a, b, c); + + var shared = Assert.Single(cli.Parameters); + Assert.Equal("authId", shared.Name); + // The odd-named snip's template token was rewritten to the winning name. + Assert.Equal("c {authId}", c.CommandTemplate); + Assert.Empty(c.Parameters); + } + + [Fact] + public void Existing_compatible_shared_choice_is_reused_not_duplicated() + { + var cli = new Cli { Name = "mpt-app", Parameters = [Choice("authId", "x", "x", "y")] }; + var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "y", "y", "x")] }; + var b = new Snip { Title = "b", CommandTemplate = "b {authId}", Parameters = [Choice("authId", "x", "x", "y")] }; + + AnalyzeAndApply(cli, a, b); + + // No new shared param added; the locals are stripped to inherit the existing one. + Assert.Single(cli.Parameters); + Assert.Empty(a.Parameters); + Assert.Empty(b.Parameters); + } + + [Fact] + public void Existing_incompatible_name_leaves_parameters_local() + { + // CLI already has a Text "authId"; imported choices named "authId" must not be merged into it. + var cli = new Cli { Name = "mpt-app", Parameters = [new Parameter { Name = "authId", Type = ParameterType.Text }] }; + var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "x", "x", "y")] }; + var b = new Snip { Title = "b", CommandTemplate = "b {authId}", Parameters = [Choice("authId", "x", "x", "y")] }; + + AnalyzeAndApply(cli, a, b); + + Assert.Single(cli.Parameters); // unchanged + Assert.Single(a.Parameters); // left local + Assert.Single(b.Parameters); + } + + [Fact] + public void Two_distinct_same_option_choices_in_one_snip_are_not_collapsed() + { + // {source} and {dest} happen to share an option set but are different arguments. + var cli = new Cli { Name = "cp" }; + var a = new Snip { Title = "a", CommandTemplate = "cp {source} {dest}", Parameters = [Choice("source", "x", "x", "y"), Choice("dest", "x", "x", "y")] }; + var b = new Snip { Title = "b", CommandTemplate = "cp {source} {dest}", Parameters = [Choice("source", "x", "x", "y"), Choice("dest", "x", "x", "y")] }; + + AnalyzeAndApply(cli, a, b); + + // The two tokens must remain distinct, not merged into "{source} {source}". + Assert.Equal("cp {source} {dest}", a.CommandTemplate); + Assert.Equal("cp {source} {dest}", b.CommandTemplate); + // "source" is promoted; "dest" stays local on each snip. + Assert.Single(cli.Parameters); + Assert.Equal("source", cli.Parameters[0].Name); + Assert.Equal("dest", Assert.Single(a.Parameters).Name); + Assert.Equal("dest", Assert.Single(b.Parameters).Name); + } + + [Fact] + public void A_choice_and_a_text_param_with_the_same_name_do_not_both_promote() + { + var cli = new Cli { Name = "tool" }; + // Choice "fmt" used by 2 snips; Text "fmt" used by 2 other snips. + var c1 = new Snip { Title = "c1", CommandTemplate = "c1 {fmt}", Parameters = [Choice("fmt", "json", "json", "yaml")] }; + var c2 = new Snip { Title = "c2", CommandTemplate = "c2 {fmt}", Parameters = [Choice("fmt", "json", "json", "yaml")] }; + var t1 = TextSnip("t1 {fmt}", ("fmt", "raw")); + var t2 = TextSnip("t2 {fmt}", ("fmt", "raw")); + + AnalyzeAndApply(cli, c1, c2, t1, t2); + + // Exactly one CLI param named "fmt" (the Choice, promoted first); no duplicate name. + var shared = Assert.Single(cli.Parameters); + Assert.Equal("fmt", shared.Name); + Assert.Equal(ParameterType.Choice, shared.Type); + // The text snips keep their local "fmt" rather than clashing with the choice. + Assert.Single(t1.Parameters); + Assert.Single(t2.Parameters); + } + + [Fact] + public void Analyze_does_not_mutate_until_apply() + { + var cli = new Cli { Name = "mpt-app" }; + var a = TextSnip("a {id}", ("id", "1")); + var b = TextSnip("b {id}", ("id", "1")); + + var plan = ParameterSharer.Analyze(cli.Parameters, [a, b]); + + // Pure analysis: nothing changed yet. + Assert.Empty(cli.Parameters); + Assert.Single(a.Parameters); + Assert.False(plan.IsEmpty); + } + } +} diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index 1317e48..e77e632 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -185,6 +185,64 @@ public void Into_is_used_only_for_unconfident_suggestions() Assert.Equal("fallback", plan.Items[1].TargetCliName); } + [Fact] + public void ShareParameters_promotes_duplicated_params_to_the_created_cli_and_strips_the_snips() + { + var doc = new SnipStoreDocument(); + var a = new Snip + { + Title = "A", + CommandTemplate = "mpt-app a {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["x", "y"], Default = "x" }], + }; + var b = new Snip + { + Title = "B", + CommandTemplate = "mpt-app b {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["y", "x"], Default = "y" }], + }; + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan( + doc, + [new SnippetCandidate("mpt-app", true, a), new SnippetCandidate("mpt-app", true, b)], + options); + + Assert.Equal(1, plan.SharedParameterCount); + + StoreMerger.Apply(doc, plan); + + var cli = Assert.Single(doc.Clis); + var shared = Assert.Single(cli.Parameters); + Assert.Equal("authId", shared.Name); + // Both snips now inherit the shared parameter instead of carrying their own. + Assert.All(doc.Snips, s => Assert.Empty(s.Parameters)); + } + + [Fact] + public void Sharing_is_off_by_default_so_params_stay_local() + { + var doc = new SnipStoreDocument(); + var a = TextParamSnip("A", "mpt-app a {id}"); + var b = TextParamSnip("B", "mpt-app b {id}"); + + var plan = StoreMerger.Plan(doc, [a, b], _defaults); + StoreMerger.Apply(doc, plan); + + Assert.Empty(Assert.Single(doc.Clis).Parameters); + Assert.All(doc.Snips, s => Assert.Single(s.Parameters)); + } + + private static SnippetCandidate TextParamSnip(string title, string template) + { + return new SnippetCandidate("mpt-app", true, new Snip + { + Title = title, + CommandTemplate = template, + Parameters = [new Parameter { Name = "id", Type = ParameterType.Text, Default = "1" }], + }); + } + [Fact] public void Unconfident_with_no_into_falls_back_to_a_generic_bucket() { diff --git a/tools/Snipdeck.Importer/Commands/ImportCommandSettings.cs b/tools/Snipdeck.Importer/Commands/ImportCommandSettings.cs index 6281cc6..72080e6 100644 --- a/tools/Snipdeck.Importer/Commands/ImportCommandSettings.cs +++ b/tools/Snipdeck.Importer/Commands/ImportCommandSettings.cs @@ -33,5 +33,9 @@ public class ImportCommandSettings : CommandSettings [CommandOption("--allow-duplicates")] [Description("Import snips even if one with the same title and command already exists.")] public bool AllowDuplicates { get; init; } + + [CommandOption("--no-share-parameters")] + [Description("Keep every parameter on its snip instead of promoting duplicates to CLI-shared parameters.")] + public bool NoShareParameters { get; init; } } } diff --git a/tools/Snipdeck.Importer/Commands/SnipCommandImportCommand.cs b/tools/Snipdeck.Importer/Commands/SnipCommandImportCommand.cs index e44dbea..7485892 100644 --- a/tools/Snipdeck.Importer/Commands/SnipCommandImportCommand.cs +++ b/tools/Snipdeck.Importer/Commands/SnipCommandImportCommand.cs @@ -38,7 +38,11 @@ protected override async Task ExecuteAsync( var store = new JsonSnipStore(targets.StorePath); var document = await store.LoadAsync(cancellationToken).ConfigureAwait(false); - var options = new MergeOptions(settings.Cli, settings.Into, settings.AllowDuplicates); + var options = new MergeOptions( + settings.Cli, + settings.Into, + settings.AllowDuplicates, + ShareParameters: !settings.NoShareParameters); var plan = StoreMerger.Plan(document, candidates, options); DryRunRenderer.Render(source.DisplayName, targets.StorePath, plan, settings.Write); diff --git a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs new file mode 100644 index 0000000..becb46c --- /dev/null +++ b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs @@ -0,0 +1,272 @@ +using Snipdeck.Core.Models; + +namespace Snipdeck.Importer.Merge +{ + /// + /// De-duplicates the parameters of the snips imported into a single CLI and promotes the + /// shared ones up to CLI-scoped parameters (), so the resolver + /// inherits them by token name. Only parameters used by two or more snips are promoted; + /// single-use parameters stay local. + /// + /// Matching rules: + /// + /// Choice parameters match when their option sets are equal (order-independent); + /// the name is not part of the match, and the most common name wins. Snips that used a different + /// name have their template token rewritten to the winning name. + /// Text parameters match by name; the most common default value wins (including an + /// empty/absent default). + /// + /// Ties are broken by first appearance. Analysis is pure; performs the mutation. + /// + /// + public static class ParameterSharer + { + private const char _separator = '\u0001'; + + public static CliSharePlan Analyze(IReadOnlyList existingCliParameters, IReadOnlyList snips) + { + ArgumentNullException.ThrowIfNull(existingCliParameters); + ArgumentNullException.ThrowIfNull(snips); + + // Record every (snip, parameter) occurrence with a stable order for deterministic ties. + var occurrences = new List(); + var order = 0; + foreach (var snip in snips) + { + foreach (var parameter in snip.Parameters) + { + occurrences.Add(new Occurrence(snip, parameter, order++)); + } + } + + var existingByName = new Dictionary(StringComparer.Ordinal); + foreach (var p in existingCliParameters) + { + _ = existingByName.TryAdd(p.Name, p); + } + + var sharedToAdd = new List(); + // Names already promoted this run, so a second group (e.g. a Text param sharing a + // name with a promoted Choice) can't add a duplicate CLI-scoped name. + var claimed = new HashSet(StringComparer.Ordinal); + // snip -> (local names to remove, token renames) + var edits = new Dictionary Remove, Dictionary Renames)>(); + + void RecordEdit(Snip snip, string removeName, string? renameTo) + { + if (!edits.TryGetValue(snip, out var edit)) + { + edit = ([], new Dictionary(StringComparer.Ordinal)); + edits[snip] = edit; + } + + edit.Remove.Add(removeName); + if (renameTo is not null && !string.Equals(removeName, renameTo, StringComparison.Ordinal)) + { + edit.Renames[removeName] = renameTo; + } + } + + // --- Choice parameters: grouped by option-set (order-independent) --- + foreach (var group in occurrences + .Where(o => o.Parameter.Type == ParameterType.Choice) + .GroupBy(o => OptionSetKey(o.Parameter.Options))) + { + var members = group.ToList(); + if (DistinctSnipCount(members) < 2) + { + continue; + } + + var winningName = MostCommonName(members); + if (claimed.Contains(winningName)) + { + // Name already promoted by an earlier group — can't share it twice; leave local. + continue; + } + + var options = MostCommonOptions(members); + + if (existingByName.TryGetValue(winningName, out var existing)) + { + // Reuse only a compatible existing shared parameter; otherwise leave these local. + if (existing.Type != ParameterType.Choice || !SameOptionSet(existing.Options, options)) + { + continue; + } + } + else + { + sharedToAdd.Add(new Parameter + { + Name = winningName, + Type = ParameterType.Choice, + Options = [.. options], + Default = MostCommonDefault(members), + }); + } + + _ = claimed.Add(winningName); + + // Merge at most one parameter per snip — two distinct same-option choices in one + // snip (e.g. {source} and {dest}) are different arguments and must not collapse + // into a single token. Prefer the occurrence already named the winning name; skip + // a rename that would collide with another token already in the snip's template. + foreach (var bySnip in members.GroupBy(m => m.Snip)) + { + var target = bySnip + .OrderByDescending(m => string.Equals(m.Parameter.Name, winningName, StringComparison.Ordinal)) + .ThenBy(m => m.Order) + .First(); + + if (!string.Equals(target.Parameter.Name, winningName, StringComparison.Ordinal) + && TemplateContainsToken(target.Snip.CommandTemplate, winningName)) + { + continue; + } + + RecordEdit(target.Snip, target.Parameter.Name, winningName); + } + } + + // --- Text parameters: grouped by name --- + foreach (var group in occurrences + .Where(o => o.Parameter.Type == ParameterType.Text) + .GroupBy(o => o.Parameter.Name, StringComparer.Ordinal)) + { + var members = group.ToList(); + if (DistinctSnipCount(members) < 2) + { + continue; + } + + var name = group.Key; + if (claimed.Contains(name)) + { + // Name already promoted (e.g. as a Choice) — don't add a clashing CLI param. + continue; + } + + var winningDefault = MostCommonDefault(members); + + if (existingByName.TryGetValue(name, out var existing)) + { + if (existing.Type != ParameterType.Text) + { + continue; + } + } + else + { + sharedToAdd.Add(new Parameter + { + Name = name, + Type = ParameterType.Text, + Default = winningDefault, + }); + } + + _ = claimed.Add(name); + + foreach (var member in members) + { + RecordEdit(member.Snip, name, renameTo: null); + } + } + + var snipEdits = edits + .Select(kvp => new SnipParameterEdit(kvp.Key, kvp.Value.Remove, kvp.Value.Renames)) + .ToList(); + return new CliSharePlan(sharedToAdd, snipEdits); + } + + public static void Apply(Cli cli, CliSharePlan plan) + { + ArgumentNullException.ThrowIfNull(cli); + ArgumentNullException.ThrowIfNull(plan); + + cli.Parameters.AddRange(plan.SharedToAdd); + + foreach (var edit in plan.SnipEdits) + { + foreach (var (oldName, newName) in edit.TokenRenames) + { + edit.Snip.CommandTemplate = edit.Snip.CommandTemplate.Replace( + "{" + oldName + "}", "{" + newName + "}", StringComparison.Ordinal); + } + + var remove = new HashSet(edit.RemoveLocalNames, StringComparer.Ordinal); + _ = edit.Snip.Parameters.RemoveAll(p => remove.Contains(p.Name)); + } + } + + private static int DistinctSnipCount(IEnumerable members) + { + return members.Select(m => m.Snip).Distinct().Count(); + } + + private static string OptionSetKey(IEnumerable options) + { + return string.Join(_separator, options.Distinct(StringComparer.Ordinal).OrderBy(o => o, StringComparer.Ordinal)); + } + + private static bool SameOptionSet(IEnumerable a, IEnumerable b) + { + return new HashSet(a, StringComparer.Ordinal).SetEquals(b); + } + + private static string MostCommonName(IEnumerable members) + { + return members + .GroupBy(m => m.Parameter.Name, StringComparer.Ordinal) + .Select(g => (g.Key, Count: g.Count(), First: g.Min(m => m.Order))) + .OrderByDescending(g => g.Count) + .ThenBy(g => g.First) + .First().Key; + } + + private static string? MostCommonDefault(IEnumerable members) + { + return members + .GroupBy(m => m.Parameter.Default) + .Select(g => (g.Key, Count: g.Count(), First: g.Min(m => m.Order))) + .OrderByDescending(g => g.Count) + .ThenBy(g => g.First) + .First().Key; + } + + /// The most common option ordering among a choice group; ties broken by first appearance. + private static List MostCommonOptions(IEnumerable members) + { + return members + .GroupBy(m => string.Join(_separator, m.Parameter.Options)) + .Select(g => + { + var picked = g.OrderBy(m => m.Order).First(); + return (Count: g.Count(), picked.Order, picked.Parameter.Options); + }) + .OrderByDescending(g => g.Count) + .ThenBy(g => g.Order) + .First().Options; + } + + private static bool TemplateContainsToken(string template, string name) + { + return template.Contains("{" + name + "}", StringComparison.Ordinal); + } + + private readonly record struct Occurrence(Snip Snip, Parameter Parameter, int Order); + } + + /// Parameters to add to a CLI plus the per-snip edits that promotion requires. + public sealed record CliSharePlan(IReadOnlyList SharedToAdd, IReadOnlyList SnipEdits) + { + public bool IsEmpty => SharedToAdd.Count == 0 && SnipEdits.Count == 0; + } + + /// The local parameters to drop from a snip and the token renames its template needs. + public sealed record SnipParameterEdit( + Snip Snip, + IReadOnlyList RemoveLocalNames, + IReadOnlyDictionary TokenRenames); +} diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 9aed541..36113c2 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -67,7 +67,41 @@ public static MergePlan Plan( } } - return new MergePlan(items, clisToCreate); + var sharePlans = options.ShareParameters + ? BuildSharePlans(target, items) + : new Dictionary(StringComparer.OrdinalIgnoreCase); + + return new MergePlan(items, clisToCreate, sharePlans); + } + + private static Dictionary BuildSharePlans( + SnipStoreDocument target, + IReadOnlyList items) + { + var existingClisByName = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var cli in target.Clis) + { + _ = existingClisByName.TryAdd(cli.Name, cli); + } + + var plans = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var group in items + .Where(i => !i.IsDuplicateSkip) + .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) + { + var existingParameters = existingClisByName.TryGetValue(group.Key, out var existingCli) + ? existingCli.Parameters + : (IReadOnlyList)[]; + var snips = group.Select(i => i.Candidate.Snip).ToList(); + + var plan = ParameterSharer.Analyze(existingParameters, snips); + if (!plan.IsEmpty) + { + plans[group.Key] = plan; + } + } + + return plans; } public static void Apply(SnipStoreDocument target, MergePlan plan) @@ -102,6 +136,16 @@ public static void Apply(SnipStoreDocument target, MergePlan plan) snip.CliId = cli.Id; target.Snips.Add(snip); } + + // Promote duplicated parameters to CLI-shared parameters (after snips are attached, + // so the shared definitions and any template-token rewrites land on the real objects). + foreach (var (cliName, sharePlan) in plan.SharePlansByCli) + { + if (clisByName.TryGetValue(cliName, out var cli)) + { + ParameterSharer.Apply(cli, sharePlan); + } + } } private static (string, string, string) DedupeKey(string cliName, string title, string commandTemplate) @@ -130,16 +174,24 @@ private static string ResolveCliName(SnippetCandidate candidate, MergeOptions op } /// Knobs that shape a merge, mapped from the CLI options. - public sealed record MergeOptions(string? ForceCli, string? Into, bool AllowDuplicates); + public sealed record MergeOptions(string? ForceCli, string? Into, bool AllowDuplicates, bool ShareParameters = false); /// One candidate's planned outcome. public sealed record MergePlanItem(SnippetCandidate Candidate, string TargetCliName, bool IsDuplicateSkip); - /// The full plan: per-candidate decisions and the CLIs that would be created. - public sealed record MergePlan(IReadOnlyList Items, IReadOnlyList ClisToCreate) + /// + /// The full plan: per-candidate decisions, the CLIs that would be created, and the + /// parameter-sharing plan per target CLI (keyed by CLI name; empty when sharing is off). + /// + public sealed record MergePlan( + IReadOnlyList Items, + IReadOnlyList ClisToCreate, + IReadOnlyDictionary SharePlansByCli) { public int ImportCount => Items.Count(i => !i.IsDuplicateSkip); public int SkipCount => Items.Count(i => i.IsDuplicateSkip); + + public int SharedParameterCount => SharePlansByCli.Values.Sum(p => p.SharedToAdd.Count); } } diff --git a/tools/Snipdeck.Importer/Output/DryRunRenderer.cs b/tools/Snipdeck.Importer/Output/DryRunRenderer.cs index b3c365b..6ed77f3 100644 --- a/tools/Snipdeck.Importer/Output/DryRunRenderer.cs +++ b/tools/Snipdeck.Importer/Output/DryRunRenderer.cs @@ -60,6 +60,27 @@ public static void Render(string sourceName, string storePath, MergePlan plan, b AnsiConsole.WriteLine(); AnsiConsole.MarkupLineInterpolated( $"CLIs to create: {plan.ClisToCreate.Count} Snips to import: {plan.ImportCount} Skipped (duplicates): {plan.SkipCount}"); + + RenderSharedParameters(plan); + } + + private static void RenderSharedParameters(MergePlan plan) + { + if (plan.SharePlansByCli.Count == 0) + { + return; + } + + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLineInterpolated( + $"Shared parameters (scoped to their CLI): {plan.SharedParameterCount}"); + foreach (var (cliName, share) in plan.SharePlansByCli + .Where(kvp => kvp.Value.SharedToAdd.Count > 0) + .OrderBy(kvp => kvp.Key, StringComparer.OrdinalIgnoreCase)) + { + var names = string.Join(", ", share.SharedToAdd.Select(p => p.Name)); + AnsiConsole.MarkupLineInterpolated($" {cliName}: {names}"); + } } private static string Escape(string value) diff --git a/tools/Snipdeck.Importer/README.md b/tools/Snipdeck.Importer/README.md index c21ff1d..d30dda8 100644 --- a/tools/Snipdeck.Importer/README.md +++ b/tools/Snipdeck.Importer/README.md @@ -36,6 +36,7 @@ is JSON — point the importer straight at it. | `--cli ` | — | Force every imported snip into this CLI, overriding auto-detection. | | `--into ` | — | Fallback CLI for snips whose CLI could not be confidently auto-detected. | | `--allow-duplicates` | off | Import snips even if one with the same title and command already exists. | +| `--no-share-parameters` | off | Keep every parameter on its snip instead of promoting duplicates to CLI-shared parameters. | ### Dry-run first @@ -64,6 +65,13 @@ snipdeck-importer snipcommand snipcommand.db --write parameter list (Choice with options, or Text), with a sensible default carried across. Variable names containing spaces or punctuation are slugified into legal token names. +- **Shares duplicated parameters.** When a parameter recurs across two or more snips in the + same CLI, it is promoted to a **CLI-scoped shared parameter** and removed from the + individual snips (which then inherit it by token name). Choice parameters match on their + option *set* (order-independent) — names need not match, the most common name wins, and + snips that used a different name have their template token rewritten to it. Text parameters + match by name, and the most common default value wins (including an empty default). + Single-use parameters stay on their snip. Pass `--no-share-parameters` to disable. - **Carries metadata.** Title, description, tags and favourite flag come across; usage counts and last-used timestamps are preserved when present. Trashed entries are skipped. From b1d755bbf685eb2264f47aeb9cef379d9062c7e4 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 05:40:12 +0000 Subject: [PATCH 2/9] Importer: dedupe against the post-share command template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parameter sharing can rewrite a choice token to the winning name, which happens after the duplicate check ran — so a rename could turn an imported snip into an exact duplicate of an existing store snip without being caught (when --allow-duplicates is off). Compute sharing first and run the duplicate check against the post-share template. Adds a regression test. Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../StoreMergerTests.cs | 44 ++++++++++++++++ tools/Snipdeck.Importer/Merge/StoreMerger.cs | 50 ++++++++++++++----- 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index e77e632..dd67163 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -219,6 +219,50 @@ public void ShareParameters_promotes_duplicated_params_to_the_created_cli_and_st Assert.All(doc.Snips, s => Assert.Empty(s.Parameters)); } + [Fact] + public void A_snip_that_becomes_a_duplicate_after_a_share_rename_is_skipped() + { + // Existing store snip already uses the winning token name. + var cli = new Cli { Name = "x" }; + var doc = new SnipStoreDocument + { + Clis = { cli }, + Snips = { new Snip { CliId = cli.Id, Title = "Dup", CommandTemplate = "x run {authId}" } }, + }; + + // Imported: "Dup" uses {auth}; two others use {authId}, so "authId" wins the choice and + // {auth} is rewritten to {authId} — turning "Dup" into a duplicate of the existing snip. + static Snip MakeChoice(string token) + { + return new Snip + { + Title = token == "auth" ? "Dup" : token, + CommandTemplate = $"x run {{{token}}}", + Parameters = [new Parameter { Name = token, Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }; + } + + var dup = new SnippetCandidate("x", true, MakeChoice("auth")); + var c2 = new SnippetCandidate("x", true, MakeChoice("authId")); + var c3 = new SnippetCandidate("x", true, new Snip + { + Title = "Three", + CommandTemplate = "x three {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }); + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan(doc, [dup, c2, c3], options); + + // The renamed "Dup" candidate is recognised as a duplicate and skipped. + var dupItem = plan.Items.Single(i => ReferenceEquals(i.Candidate, dup)); + Assert.True(dupItem.IsDuplicateSkip); + + StoreMerger.Apply(doc, plan); + // Only one "Dup"-titled snip remains (the pre-existing one). + Assert.Single(doc.Snips, s => s.Title == "Dup"); + } + [Fact] public void Sharing_is_off_by_default_so_params_stay_local() { diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 36113c2..f2d9b78 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -39,15 +39,47 @@ public static MergePlan Plan( target.Clis.Select(c => c.Name), StringComparer.OrdinalIgnoreCase); + // Resolve each candidate's target CLI up front. + var resolved = new List<(SnippetCandidate Candidate, string CliName)>(candidates.Count); + foreach (var candidate in candidates) + { + resolved.Add((candidate, ResolveCliName(candidate, options))); + } + + // Compute parameter sharing first: it can rewrite a choice token to the winning name, + // so the duplicate check must run against the POST-share template — otherwise a rename + // could silently turn an imported snip into a duplicate of an existing one. + var sharePlans = options.ShareParameters + ? BuildSharePlans(target, resolved) + : new Dictionary(StringComparer.OrdinalIgnoreCase); + + var renamesBySnip = sharePlans.Values + .SelectMany(p => p.SnipEdits) + .Where(e => e.TokenRenames.Count > 0) + .ToDictionary(e => e.Snip, e => e.TokenRenames); + + string DedupeTemplate(Snip snip) + { + var template = snip.CommandTemplate; + if (renamesBySnip.TryGetValue(snip, out var renames)) + { + foreach (var (oldName, newName) in renames) + { + template = template.Replace("{" + oldName + "}", "{" + newName + "}", StringComparison.Ordinal); + } + } + + return template; + } + var items = new List(candidates.Count); var clisToCreate = new List(); var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); var plannedKeys = new HashSet<(string, string, string)>(); - foreach (var candidate in candidates) + foreach (var (candidate, cliName) in resolved) { - var cliName = ResolveCliName(candidate, options); - var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); + var key = DedupeKey(cliName, candidate.Snip.Title, DedupeTemplate(candidate.Snip)); var isDuplicate = !options.AllowDuplicates && (existing.Contains(key) || plannedKeys.Contains(key)); @@ -67,16 +99,12 @@ public static MergePlan Plan( } } - var sharePlans = options.ShareParameters - ? BuildSharePlans(target, items) - : new Dictionary(StringComparer.OrdinalIgnoreCase); - return new MergePlan(items, clisToCreate, sharePlans); } private static Dictionary BuildSharePlans( SnipStoreDocument target, - IReadOnlyList items) + IReadOnlyList<(SnippetCandidate Candidate, string CliName)> resolved) { var existingClisByName = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var cli in target.Clis) @@ -85,14 +113,12 @@ private static Dictionary BuildSharePlans( } var plans = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var group in items - .Where(i => !i.IsDuplicateSkip) - .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) + foreach (var group in resolved.GroupBy(r => r.CliName, StringComparer.OrdinalIgnoreCase)) { var existingParameters = existingClisByName.TryGetValue(group.Key, out var existingCli) ? existingCli.Parameters : (IReadOnlyList)[]; - var snips = group.Select(i => i.Candidate.Snip).ToList(); + var snips = group.Select(r => r.Candidate.Snip).ToList(); var plan = ParameterSharer.Analyze(existingParameters, snips); if (!plan.IsEmpty) From 516e37d977971885aa78346a94399ad0a7437662 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 05:47:25 +0000 Subject: [PATCH 3/9] Importer: build the share plan only from imported snips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Parameter sharing was computed over every resolved candidate, but the plan is applied (CLI params added, snips mutated) only for imported snips — so a duplicate-only import could still add shared parameters to an existing CLI, changing how its existing bare {token} templates resolve. Restructure StoreMerger.Plan into phases: (1) detect duplicates on the as-imported template, (2) build the share plan from imported snips only, (3) re-check duplicates against the post-share template (keeps the earlier rename-aware dedupe fix), (4) derive CLIs-to-create from the final imported set and drop share plans for CLIs that end up receiving nothing. Adds a regression test for the duplicate-only case. Found by codex review. (Its second finding — unescaped CLI name in the dry-run summary — is a non-issue: MarkupLineInterpolated auto-escapes interpolated arguments, unlike the .Title()/.AddRow() paths that use manual Escape.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../StoreMergerTests.cs | 38 ++++++ tools/Snipdeck.Importer/Merge/StoreMerger.cs | 127 ++++++++++++------ 2 files changed, 123 insertions(+), 42 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index dd67163..1619721 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -263,6 +263,44 @@ static Snip MakeChoice(string token) Assert.Single(doc.Snips, s => s.Title == "Dup"); } + [Fact] + public void A_duplicate_only_import_does_not_mutate_the_existing_clis_shared_parameters() + { + var cli = new Cli { Name = "x" }; + var doc = new SnipStoreDocument + { + Clis = { cli }, + Snips = + { + new Snip { CliId = cli.Id, Title = "A", CommandTemplate = "x a {p}" }, + new Snip { CliId = cli.Id, Title = "B", CommandTemplate = "x b {p}" }, + }, + }; + + // Re-import the same two snips (exact duplicates), each carrying a {p} parameter. + static SnippetCandidate Dup(string title, string template) + { + return new SnippetCandidate("x", true, new Snip + { + Title = title, + CommandTemplate = template, + Parameters = [new Parameter { Name = "p", Type = ParameterType.Text, Default = "v" }], + }); + } + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan(doc, [Dup("A", "x a {p}"), Dup("B", "x b {p}")], options); + + Assert.Equal(0, plan.ImportCount); + Assert.Equal(0, plan.SharedParameterCount); + + StoreMerger.Apply(doc, plan); + + // Nothing imported, and the existing CLI gained no shared parameters. + Assert.Equal(2, doc.Snips.Count); + Assert.Empty(cli.Parameters); + } + [Fact] public void Sharing_is_off_by_default_so_params_stay_local() { diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index f2d9b78..664a71f 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -39,72 +39,65 @@ public static MergePlan Plan( target.Clis.Select(c => c.Name), StringComparer.OrdinalIgnoreCase); - // Resolve each candidate's target CLI up front. - var resolved = new List<(SnippetCandidate Candidate, string CliName)>(candidates.Count); + // Phase 1 — duplicate detection against the as-imported template. + var items = new List(candidates.Count); + var plannedKeys = new HashSet<(string, string, string)>(); foreach (var candidate in candidates) { - resolved.Add((candidate, ResolveCliName(candidate, options))); + var cliName = ResolveCliName(candidate, options); + var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); + var isDuplicate = !options.AllowDuplicates + && (existing.Contains(key) || plannedKeys.Contains(key)); + items.Add(new MergePlanItem(candidate, cliName, isDuplicate)); + if (!isDuplicate) + { + _ = plannedKeys.Add(key); + } } - // Compute parameter sharing first: it can rewrite a choice token to the winning name, - // so the duplicate check must run against the POST-share template — otherwise a rename - // could silently turn an imported snip into a duplicate of an existing one. + // Phase 2 — build the share plan ONLY from snips that will actually be imported, so a + // duplicate-only import never mutates an existing CLI's shared parameters. var sharePlans = options.ShareParameters - ? BuildSharePlans(target, resolved) + ? BuildSharePlans(target, items) : new Dictionary(StringComparer.OrdinalIgnoreCase); - var renamesBySnip = sharePlans.Values - .SelectMany(p => p.SnipEdits) - .Where(e => e.TokenRenames.Count > 0) - .ToDictionary(e => e.Snip, e => e.TokenRenames); - - string DedupeTemplate(Snip snip) + // Phase 3 — sharing can rewrite a choice token to the winning name; re-check duplicates + // against the post-share template so a rename can't slip a duplicate past Phase 1. + if (options.ShareParameters && !options.AllowDuplicates) { - var template = snip.CommandTemplate; - if (renamesBySnip.TryGetValue(snip, out var renames)) - { - foreach (var (oldName, newName) in renames) - { - template = template.Replace("{" + oldName + "}", "{" + newName + "}", StringComparison.Ordinal); - } - } - - return template; + items = RededupeAfterRenames(items, sharePlans, existing); } - var items = new List(candidates.Count); + // Phase 4 — derive CLIs to create from the final imported set, and drop share plans for + // CLIs that (after all skips) receive no snips, so they are neither created nor mutated. var clisToCreate = new List(); var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); - var plannedKeys = new HashSet<(string, string, string)>(); - - foreach (var (candidate, cliName) in resolved) + var importedCliNames = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var item in items) { - var key = DedupeKey(cliName, candidate.Snip.Title, DedupeTemplate(candidate.Snip)); - - var isDuplicate = !options.AllowDuplicates - && (existing.Contains(key) || plannedKeys.Contains(key)); - - items.Add(new MergePlanItem(candidate, cliName, isDuplicate)); - - if (isDuplicate) + if (item.IsDuplicateSkip) { continue; } - _ = plannedKeys.Add(key); - - if (!existingCliNames.Contains(cliName) && seenNewClis.Add(cliName)) + _ = importedCliNames.Add(item.TargetCliName); + if (!existingCliNames.Contains(item.TargetCliName) && seenNewClis.Add(item.TargetCliName)) { - clisToCreate.Add(cliName); + clisToCreate.Add(item.TargetCliName); } } + foreach (var cliName in sharePlans.Keys.Where(k => !importedCliNames.Contains(k)).ToList()) + { + _ = sharePlans.Remove(cliName); + } + return new MergePlan(items, clisToCreate, sharePlans); } private static Dictionary BuildSharePlans( SnipStoreDocument target, - IReadOnlyList<(SnippetCandidate Candidate, string CliName)> resolved) + IReadOnlyList items) { var existingClisByName = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var cli in target.Clis) @@ -113,12 +106,14 @@ private static Dictionary BuildSharePlans( } var plans = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var group in resolved.GroupBy(r => r.CliName, StringComparer.OrdinalIgnoreCase)) + foreach (var group in items + .Where(i => !i.IsDuplicateSkip) + .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) { var existingParameters = existingClisByName.TryGetValue(group.Key, out var existingCli) ? existingCli.Parameters : (IReadOnlyList)[]; - var snips = group.Select(r => r.Candidate.Snip).ToList(); + var snips = group.Select(i => i.Candidate.Snip).ToList(); var plan = ParameterSharer.Analyze(existingParameters, snips); if (!plan.IsEmpty) @@ -130,6 +125,54 @@ private static Dictionary BuildSharePlans( return plans; } + private static List RededupeAfterRenames( + List items, + IReadOnlyDictionary sharePlans, + HashSet<(string, string, string)> existingKeys) + { + var renamesBySnip = sharePlans.Values + .SelectMany(p => p.SnipEdits) + .Where(e => e.TokenRenames.Count > 0) + .ToDictionary(e => e.Snip, e => e.TokenRenames); + if (renamesBySnip.Count == 0) + { + return items; + } + + var seen = new HashSet<(string, string, string)>(existingKeys); + var result = new List(items.Count); + foreach (var item in items) + { + if (item.IsDuplicateSkip) + { + result.Add(item); + continue; + } + + var template = item.Candidate.Snip.CommandTemplate; + if (renamesBySnip.TryGetValue(item.Candidate.Snip, out var renames)) + { + foreach (var (oldName, newName) in renames) + { + template = template.Replace("{" + oldName + "}", "{" + newName + "}", StringComparison.Ordinal); + } + } + + var key = DedupeKey(item.TargetCliName, item.Candidate.Snip.Title, template); + if (seen.Contains(key)) + { + result.Add(item with { IsDuplicateSkip = true }); + } + else + { + _ = seen.Add(key); + result.Add(item); + } + } + + return result; + } + public static void Apply(SnipStoreDocument target, MergePlan plan) { ArgumentNullException.ThrowIfNull(target); From 8f2d4bc34dca1721ecf30861b9f08c0b0cb85c94 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 05:55:07 +0000 Subject: [PATCH 4/9] Importer: normalise choice names before dedupe, promote after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decouple choice-token renaming from promotion to remove the order-dependency codex flagged (sharing decided over a set that later shrinks via post-rename duplicate skips). New flow in StoreMerger.Plan: 1. NormalizeChoiceNames per CLI — for each option set used by >=2 snips, rewrite every snip's template token AND local parameter name to the most common name. Normalisation only; it never promotes or strips. 2. De-duplicate once, on the now-normalised templates. 3. Promote shared parameters only over the snips that are actually imported. This is terminating (no rebuild/refixpoint) and makes the earlier findings moot: a rename can't slip a duplicate past dedup (renames happen first), and a duplicate-only or shrunk import never promotes params it no longer justifies. Removes the RededupeAfterRenames pass. Existing regression tests still cover the rename-becomes-duplicate and duplicate-only cases. Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Merge/ParameterSharer.cs | 55 +++++++++ tools/Snipdeck.Importer/Merge/StoreMerger.cs | 113 +++++------------- 2 files changed, 86 insertions(+), 82 deletions(-) diff --git a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs index becb46c..63017cb 100644 --- a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs +++ b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs @@ -23,6 +23,61 @@ public static class ParameterSharer { private const char _separator = '\u0001'; + /// + /// Unifies choice-parameter names across the given snips: for each option set used by two + /// or more snips, the most common name wins and each snip's template token and local + /// parameter name are rewritten to it. This is normalisation only — it does not promote or + /// strip anything — so that duplicate detection and later promotion agree on token names. + /// Mutates the snips. Run it before de-duplication. + /// + public static void NormalizeChoiceNames(IReadOnlyList snips) + { + ArgumentNullException.ThrowIfNull(snips); + + var occurrences = new List(); + var order = 0; + foreach (var snip in snips) + { + foreach (var parameter in snip.Parameters) + { + occurrences.Add(new Occurrence(snip, parameter, order++)); + } + } + + foreach (var group in occurrences + .Where(o => o.Parameter.Type == ParameterType.Choice) + .GroupBy(o => OptionSetKey(o.Parameter.Options))) + { + var members = group.ToList(); + if (DistinctSnipCount(members) < 2) + { + continue; + } + + var winningName = MostCommonName(members); + + // At most one occurrence per snip, preferring one already named the winner; never + // rename onto a token the snip already uses (would merge two distinct arguments). + foreach (var bySnip in members.GroupBy(m => m.Snip)) + { + var target = bySnip + .OrderByDescending(m => string.Equals(m.Parameter.Name, winningName, StringComparison.Ordinal)) + .ThenBy(m => m.Order) + .First(); + + if (string.Equals(target.Parameter.Name, winningName, StringComparison.Ordinal) + || TemplateContainsToken(target.Snip.CommandTemplate, winningName)) + { + continue; + } + + target.Snip.CommandTemplate = target.Snip.CommandTemplate.Replace( + "{" + target.Parameter.Name + "}", "{" + winningName + "}", StringComparison.Ordinal); + target.Parameter.Name = winningName; + } + } + } + public static CliSharePlan Analyze(IReadOnlyList existingCliParameters, IReadOnlyList snips) { ArgumentNullException.ThrowIfNull(existingCliParameters); diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 664a71f..328d4ef 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -39,58 +39,55 @@ public static MergePlan Plan( target.Clis.Select(c => c.Name), StringComparer.OrdinalIgnoreCase); - // Phase 1 — duplicate detection against the as-imported template. - var items = new List(candidates.Count); - var plannedKeys = new HashSet<(string, string, string)>(); + // Resolve each candidate's target CLI up front. + var resolved = new List<(SnippetCandidate Candidate, string CliName)>(candidates.Count); foreach (var candidate in candidates) { - var cliName = ResolveCliName(candidate, options); - var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); - var isDuplicate = !options.AllowDuplicates - && (existing.Contains(key) || plannedKeys.Contains(key)); - items.Add(new MergePlanItem(candidate, cliName, isDuplicate)); - if (!isDuplicate) - { - _ = plannedKeys.Add(key); - } + resolved.Add((candidate, ResolveCliName(candidate, options))); } - // Phase 2 — build the share plan ONLY from snips that will actually be imported, so a - // duplicate-only import never mutates an existing CLI's shared parameters. - var sharePlans = options.ShareParameters - ? BuildSharePlans(target, items) - : new Dictionary(StringComparer.OrdinalIgnoreCase); - - // Phase 3 — sharing can rewrite a choice token to the winning name; re-check duplicates - // against the post-share template so a rename can't slip a duplicate past Phase 1. - if (options.ShareParameters && !options.AllowDuplicates) + // Normalise choice token/parameter names per CLI BEFORE de-duplication, so renaming and + // dedup agree: a snip whose choice token is unified to the winning name is compared (and + // promoted) under that final name. Normalisation only renames; it never promotes, so the + // promotion decision below is driven purely by what is actually imported. + if (options.ShareParameters) { - items = RededupeAfterRenames(items, sharePlans, existing); + foreach (var group in resolved.GroupBy(r => r.CliName, StringComparer.OrdinalIgnoreCase)) + { + ParameterSharer.NormalizeChoiceNames([.. group.Select(r => r.Candidate.Snip)]); + } } - // Phase 4 — derive CLIs to create from the final imported set, and drop share plans for - // CLIs that (after all skips) receive no snips, so they are neither created nor mutated. + var items = new List(candidates.Count); var clisToCreate = new List(); var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); - var importedCliNames = new HashSet(StringComparer.OrdinalIgnoreCase); - foreach (var item in items) + var plannedKeys = new HashSet<(string, string, string)>(); + + foreach (var (candidate, cliName) in resolved) { - if (item.IsDuplicateSkip) + var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); + var isDuplicate = !options.AllowDuplicates + && (existing.Contains(key) || plannedKeys.Contains(key)); + + items.Add(new MergePlanItem(candidate, cliName, isDuplicate)); + + if (isDuplicate) { continue; } - _ = importedCliNames.Add(item.TargetCliName); - if (!existingCliNames.Contains(item.TargetCliName) && seenNewClis.Add(item.TargetCliName)) + _ = plannedKeys.Add(key); + + if (!existingCliNames.Contains(cliName) && seenNewClis.Add(cliName)) { - clisToCreate.Add(item.TargetCliName); + clisToCreate.Add(cliName); } } - foreach (var cliName in sharePlans.Keys.Where(k => !importedCliNames.Contains(k)).ToList()) - { - _ = sharePlans.Remove(cliName); - } + // Promote shared parameters only over the snips that will actually be imported. + var sharePlans = options.ShareParameters + ? BuildSharePlans(target, items) + : new Dictionary(StringComparer.OrdinalIgnoreCase); return new MergePlan(items, clisToCreate, sharePlans); } @@ -125,54 +122,6 @@ private static Dictionary BuildSharePlans( return plans; } - private static List RededupeAfterRenames( - List items, - IReadOnlyDictionary sharePlans, - HashSet<(string, string, string)> existingKeys) - { - var renamesBySnip = sharePlans.Values - .SelectMany(p => p.SnipEdits) - .Where(e => e.TokenRenames.Count > 0) - .ToDictionary(e => e.Snip, e => e.TokenRenames); - if (renamesBySnip.Count == 0) - { - return items; - } - - var seen = new HashSet<(string, string, string)>(existingKeys); - var result = new List(items.Count); - foreach (var item in items) - { - if (item.IsDuplicateSkip) - { - result.Add(item); - continue; - } - - var template = item.Candidate.Snip.CommandTemplate; - if (renamesBySnip.TryGetValue(item.Candidate.Snip, out var renames)) - { - foreach (var (oldName, newName) in renames) - { - template = template.Replace("{" + oldName + "}", "{" + newName + "}", StringComparison.Ordinal); - } - } - - var key = DedupeKey(item.TargetCliName, item.Candidate.Snip.Title, template); - if (seen.Contains(key)) - { - result.Add(item with { IsDuplicateSkip = true }); - } - else - { - _ = seen.Add(key); - result.Add(item); - } - } - - return result; - } - public static void Apply(SnipStoreDocument target, MergePlan plan) { ArgumentNullException.ThrowIfNull(target); From 69bd4044882073878696ce584ee2652c25a081ba Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 06:04:34 +0000 Subject: [PATCH 5/9] Importer: make de-duplication rename-invariant under sharing Root-cause fix for the recurring sharing/dedupe interaction: when sharing is on, key each Choice token in the duplicate-detection template by its option SET rather than its name (CanonicalTemplate). De-duplication is then independent of choice-name unification, which removes the order-dependency entirely: - A snip can't be turned into (or hidden from) a duplicate by a rename. - The rename basis and promotion both run AFTER dedupe, over imported snips only, so skipped duplicates can neither skew the winning name nor drop or rename a genuinely-importable snip. Existing snips are canonicalised via ParameterResolver.Resolve so a shared choice param is recognised. Adds a regression test for the skipped-duplicate scenario and updates the rename-becomes-duplicate test to a realistic store (existing snip carries its choice param). Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../StoreMergerTests.cs | 59 +++++++++++++++- tools/Snipdeck.Importer/Merge/StoreMerger.cs | 70 ++++++++++++++----- 2 files changed, 109 insertions(+), 20 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index 1619721..3344005 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -222,12 +222,21 @@ public void ShareParameters_promotes_duplicated_params_to_the_created_cli_and_st [Fact] public void A_snip_that_becomes_a_duplicate_after_a_share_rename_is_skipped() { - // Existing store snip already uses the winning token name. + // Existing store snip already uses the winning token name, with its choice parameter. var cli = new Cli { Name = "x" }; var doc = new SnipStoreDocument { Clis = { cli }, - Snips = { new Snip { CliId = cli.Id, Title = "Dup", CommandTemplate = "x run {authId}" } }, + Snips = + { + new Snip + { + CliId = cli.Id, + Title = "Dup", + CommandTemplate = "x run {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }, + }, }; // Imported: "Dup" uses {auth}; two others use {authId}, so "authId" wins the choice and @@ -263,6 +272,52 @@ static Snip MakeChoice(string token) Assert.Single(doc.Snips, s => s.Title == "Dup"); } + [Fact] + public void A_skipped_duplicate_does_not_rename_or_drop_a_genuinely_importable_snip() + { + // Existing store snip uses {authId} (with its choice param). + var cli = new Cli { Name = "x" }; + var doc = new SnipStoreDocument + { + Clis = { cli }, + Snips = + { + new Snip + { + CliId = cli.Id, + Title = "Existing", + CommandTemplate = "x run {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }, + }, + }; + + // A genuinely-new snip uses {auth}; plus a re-import of the existing {authId} snip (a dup). + var real = new SnippetCandidate("x", true, new Snip + { + Title = "Real", + CommandTemplate = "x do {auth}", + Parameters = [new Parameter { Name = "auth", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }); + var dup = new SnippetCandidate("x", true, new Snip + { + Title = "Existing", + CommandTemplate = "x run {authId}", + Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }); + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan(doc, [real, dup], options); + StoreMerger.Apply(doc, plan); + + // The re-imported existing snip is skipped; "Real" is imported and NOT renamed to {authId} + // (the skipped duplicate must not drive a rename), and stays local (no sharing group of 2). + Assert.True(plan.Items.Single(i => ReferenceEquals(i.Candidate, dup)).IsDuplicateSkip); + Assert.Equal("x do {auth}", real.Snip.CommandTemplate); + Assert.Equal("auth", Assert.Single(real.Snip.Parameters).Name); + Assert.Empty(cli.Parameters); + } + [Fact] public void A_duplicate_only_import_does_not_mutate_the_existing_clis_shared_parameters() { diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 328d4ef..cd52b4e 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -1,3 +1,4 @@ +using Snipdeck.Core.Engine; using Snipdeck.Core.Models; using Snipdeck.Importer.Sources; @@ -24,15 +25,23 @@ public static MergePlan Plan( // De-duplication is scoped to the CLI a snip lands in: CLI is Snipdeck's organising // axis, so the same (Title, CommandTemplate) under two different CLIs is legitimate. - var cliNameById = target.Clis.ToDictionary(c => c.Id, c => c.Name); + // When sharing is on, the dedupe key is rename-invariant — each Choice token is keyed by + // its option SET rather than its name — so the choice-name unification below can neither + // create nor hide a duplicate, and the rename basis is unaffected by what gets skipped. + var clisById = target.Clis.ToDictionary(c => c.Id); var existing = new HashSet<(string, string, string)>(); foreach (var snip in target.Snips) { - if (!snip.IsTrash) + if (snip.IsTrash) { - var owningCli = cliNameById.TryGetValue(snip.CliId, out var name) ? name : string.Empty; - _ = existing.Add(DedupeKey(owningCli, snip.Title, snip.CommandTemplate)); + continue; } + + var owningCli = clisById.GetValueOrDefault(snip.CliId); + var template = options.ShareParameters + ? CanonicalTemplate(snip.CommandTemplate, ParameterResolver.Resolve(snip, owningCli, target.GlobalParameters)) + : snip.CommandTemplate; + _ = existing.Add(DedupeKey(owningCli?.Name ?? string.Empty, snip.Title, template)); } var existingCliNames = new HashSet( @@ -46,18 +55,6 @@ public static MergePlan Plan( resolved.Add((candidate, ResolveCliName(candidate, options))); } - // Normalise choice token/parameter names per CLI BEFORE de-duplication, so renaming and - // dedup agree: a snip whose choice token is unified to the winning name is compared (and - // promoted) under that final name. Normalisation only renames; it never promotes, so the - // promotion decision below is driven purely by what is actually imported. - if (options.ShareParameters) - { - foreach (var group in resolved.GroupBy(r => r.CliName, StringComparer.OrdinalIgnoreCase)) - { - ParameterSharer.NormalizeChoiceNames([.. group.Select(r => r.Candidate.Snip)]); - } - } - var items = new List(candidates.Count); var clisToCreate = new List(); var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); @@ -65,7 +62,10 @@ public static MergePlan Plan( foreach (var (candidate, cliName) in resolved) { - var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); + var template = options.ShareParameters + ? CanonicalTemplate(candidate.Snip.CommandTemplate, candidate.Snip.Parameters) + : candidate.Snip.CommandTemplate; + var key = DedupeKey(cliName, candidate.Snip.Title, template); var isDuplicate = !options.AllowDuplicates && (existing.Contains(key) || plannedKeys.Contains(key)); @@ -84,7 +84,19 @@ public static MergePlan Plan( } } - // Promote shared parameters only over the snips that will actually be imported. + // Now that the imported set is fixed (dedupe is rename-invariant), unify choice names + // across the IMPORTED snips of each CLI, then promote shared parameters over them. Both + // steps see only imported snips, so skipped duplicates can't influence either. + if (options.ShareParameters) + { + foreach (var group in items + .Where(i => !i.IsDuplicateSkip) + .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) + { + ParameterSharer.NormalizeChoiceNames([.. group.Select(i => i.Candidate.Snip)]); + } + } + var sharePlans = options.ShareParameters ? BuildSharePlans(target, items) : new Dictionary(StringComparer.OrdinalIgnoreCase); @@ -166,6 +178,28 @@ public static void Apply(SnipStoreDocument target, MergePlan plan) } } + /// + /// Rewrites each Choice token in a template to a marker keyed by its option SET (sorted, + /// de-duplicated) instead of its name, so two snips that differ only in a choice token's + /// name compare equal. Text tokens are left untouched. + /// + private static string CanonicalTemplate(string template, IReadOnlyList effectiveParameters) + { + var result = template; + foreach (var parameter in effectiveParameters) + { + if (parameter.Type != ParameterType.Choice) + { + continue; + } + + var options = string.Join('|', parameter.Options.Distinct(StringComparer.Ordinal).OrderBy(o => o, StringComparer.Ordinal)); + result = result.Replace("{" + parameter.Name + "}", "{choice:" + options + "}", StringComparison.Ordinal); + } + + return result; + } + private static (string, string, string) DedupeKey(string cliName, string title, string commandTemplate) { // CLI name matches case-insensitively (Cli reuse is case-insensitive); title/command are exact. From 8f96fe4f803ccad912a71dedc0098032f2494c8b Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 06:12:30 +0000 Subject: [PATCH 6/9] Importer: dedupe on the literal template, share over imported only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the option-set canonical dedupe key, which had a false positive: two distinct same-option choices in one command (e.g. "cp {source} {dest}" vs "cp {dest} {source}") canonicalised equal and a valid import was dropped. De-duplication now uses the literal (CLI, Title, CommandTemplate) — positional and exact, so it never drops a snip that merely differs in a choice token's name or position. Choice-name unification and promotion run afterwards over the imported snips only, so skipped duplicates can't skew the winning name, drop, or rename an importable snip. Deliberate trade-off (favouring no false positives over no false negatives): a pathological re-import that uses an inconsistent choice-token name for a command already in the store can leave a duplicate after unification. That is benign (an extra snip, never a dropped one or lost data) and only on re-import of an inconsistently-named export. Adds a regression test for the swapped-choice case. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../StoreMergerTests.cs | 59 ++++++------------- tools/Snipdeck.Importer/Merge/StoreMerger.cs | 51 +++------------- 2 files changed, 27 insertions(+), 83 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index 3344005..1e735ab 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -220,56 +220,33 @@ public void ShareParameters_promotes_duplicated_params_to_the_created_cli_and_st } [Fact] - public void A_snip_that_becomes_a_duplicate_after_a_share_rename_is_skipped() + public void Swapped_same_option_choices_are_not_treated_as_duplicates() { - // Existing store snip already uses the winning token name, with its choice parameter. - var cli = new Cli { Name = "x" }; - var doc = new SnipStoreDocument - { - Clis = { cli }, - Snips = - { - new Snip - { - CliId = cli.Id, - Title = "Dup", - CommandTemplate = "x run {authId}", - Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], - }, - }, - }; - - // Imported: "Dup" uses {auth}; two others use {authId}, so "authId" wins the choice and - // {auth} is rewritten to {authId} — turning "Dup" into a duplicate of the existing snip. - static Snip MakeChoice(string token) + // Two distinct choices that happen to share an option set must keep positional identity: + // "cp {source} {dest}" and "cp {dest} {source}" are different commands, not duplicates. + var doc = new SnipStoreDocument(); + static Snip Swapped(string a, string b) { return new Snip { - Title = token == "auth" ? "Dup" : token, - CommandTemplate = $"x run {{{token}}}", - Parameters = [new Parameter { Name = token, Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + Title = "Copy", + CommandTemplate = $"cp {{{a}}} {{{b}}}", + Parameters = + [ + new Parameter { Name = a, Type = ParameterType.Choice, Options = ["x", "y"], Default = "x" }, + new Parameter { Name = b, Type = ParameterType.Choice, Options = ["x", "y"], Default = "x" }, + ], }; } - - var dup = new SnippetCandidate("x", true, MakeChoice("auth")); - var c2 = new SnippetCandidate("x", true, MakeChoice("authId")); - var c3 = new SnippetCandidate("x", true, new Snip - { - Title = "Three", - CommandTemplate = "x three {authId}", - Parameters = [new Parameter { Name = "authId", Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], - }); var options = _defaults with { ShareParameters = true }; - var plan = StoreMerger.Plan(doc, [dup, c2, c3], options); - - // The renamed "Dup" candidate is recognised as a duplicate and skipped. - var dupItem = plan.Items.Single(i => ReferenceEquals(i.Candidate, dup)); - Assert.True(dupItem.IsDuplicateSkip); + var plan = StoreMerger.Plan( + doc, + [new SnippetCandidate("cp", true, Swapped("source", "dest")), new SnippetCandidate("cp", true, Swapped("dest", "source"))], + options); - StoreMerger.Apply(doc, plan); - // Only one "Dup"-titled snip remains (the pre-existing one). - Assert.Single(doc.Snips, s => s.Title == "Dup"); + // Both are imported — neither is wrongly collapsed into the other. + Assert.Equal(2, plan.ImportCount); } [Fact] diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index cd52b4e..16aaedb 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -1,4 +1,3 @@ -using Snipdeck.Core.Engine; using Snipdeck.Core.Models; using Snipdeck.Importer.Sources; @@ -25,23 +24,17 @@ public static MergePlan Plan( // De-duplication is scoped to the CLI a snip lands in: CLI is Snipdeck's organising // axis, so the same (Title, CommandTemplate) under two different CLIs is legitimate. - // When sharing is on, the dedupe key is rename-invariant — each Choice token is keyed by - // its option SET rather than its name — so the choice-name unification below can neither - // create nor hide a duplicate, and the rename basis is unaffected by what gets skipped. - var clisById = target.Clis.ToDictionary(c => c.Id); + // The key is the literal template (positional, exact) so dedup never drops a snip that + // merely differs in a choice token's name or position. + var cliNameById = target.Clis.ToDictionary(c => c.Id, c => c.Name); var existing = new HashSet<(string, string, string)>(); foreach (var snip in target.Snips) { - if (snip.IsTrash) + if (!snip.IsTrash) { - continue; + var owningCli = cliNameById.TryGetValue(snip.CliId, out var name) ? name : string.Empty; + _ = existing.Add(DedupeKey(owningCli, snip.Title, snip.CommandTemplate)); } - - var owningCli = clisById.GetValueOrDefault(snip.CliId); - var template = options.ShareParameters - ? CanonicalTemplate(snip.CommandTemplate, ParameterResolver.Resolve(snip, owningCli, target.GlobalParameters)) - : snip.CommandTemplate; - _ = existing.Add(DedupeKey(owningCli?.Name ?? string.Empty, snip.Title, template)); } var existingCliNames = new HashSet( @@ -62,10 +55,7 @@ public static MergePlan Plan( foreach (var (candidate, cliName) in resolved) { - var template = options.ShareParameters - ? CanonicalTemplate(candidate.Snip.CommandTemplate, candidate.Snip.Parameters) - : candidate.Snip.CommandTemplate; - var key = DedupeKey(cliName, candidate.Snip.Title, template); + var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); var isDuplicate = !options.AllowDuplicates && (existing.Contains(key) || plannedKeys.Contains(key)); @@ -84,9 +74,8 @@ public static MergePlan Plan( } } - // Now that the imported set is fixed (dedupe is rename-invariant), unify choice names - // across the IMPORTED snips of each CLI, then promote shared parameters over them. Both - // steps see only imported snips, so skipped duplicates can't influence either. + // Over the IMPORTED snips of each CLI only (skipped duplicates excluded, so they can't + // skew the result): unify choice names, then promote shared parameters. if (options.ShareParameters) { foreach (var group in items @@ -178,28 +167,6 @@ public static void Apply(SnipStoreDocument target, MergePlan plan) } } - /// - /// Rewrites each Choice token in a template to a marker keyed by its option SET (sorted, - /// de-duplicated) instead of its name, so two snips that differ only in a choice token's - /// name compare equal. Text tokens are left untouched. - /// - private static string CanonicalTemplate(string template, IReadOnlyList effectiveParameters) - { - var result = template; - foreach (var parameter in effectiveParameters) - { - if (parameter.Type != ParameterType.Choice) - { - continue; - } - - var options = string.Join('|', parameter.Options.Distinct(StringComparer.Ordinal).OrderBy(o => o, StringComparer.Ordinal)); - result = result.Replace("{" + parameter.Name + "}", "{choice:" + options + "}", StringComparison.Ordinal); - } - - return result; - } - private static (string, string, string) DedupeKey(string cliName, string title, string commandTemplate) { // CLI name matches case-insensitively (Cli reuse is case-insensitive); title/command are exact. From 946c21f6127d79552dd174fb6e7395d2b1bfaba9 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 06:25:01 +0000 Subject: [PATCH 7/9] Importer: re-check duplicates after choice-name unification; keep imported defaults Two fixes from codex review: - Re-check duplicates after NormalizeChoiceNames. Choice-name unification can rewrite two imported snips (or an imported and an existing one) to the same title/template; a post-normalisation dedup pass on the normalised templates now skips those instead of importing duplicates under --allow-duplicates=off. The pass is positional/literal, so two distinct same-option choices in one command stay distinct. CLI creation and promotion now run over the final imported set. - Preserve imported parameter defaults. When the target CLI already shares a parameter of the same name but a DIFFERENT default (or, for choices, a different option set), imported snips are left with their local parameter instead of being stripped to silently inherit the existing default. Adds regression tests for both; updates the reuse test to use a matching default. Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ParameterSharerTests.cs | 21 +++++- .../StoreMergerTests.cs | 28 +++++++ .../Merge/ParameterSharer.cs | 15 +++- tools/Snipdeck.Importer/Merge/StoreMerger.cs | 73 +++++++++++++++---- 4 files changed, 116 insertions(+), 21 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs index 1803f80..39bb546 100644 --- a/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs +++ b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs @@ -111,8 +111,10 @@ public void Choice_promotion_picks_the_most_common_name_and_rewrites_other_templ [Fact] public void Existing_compatible_shared_choice_is_reused_not_duplicated() { + // Existing shared default ("x") matches the imported group's default, with the same + // option set in a different order — so it's reused, not duplicated. var cli = new Cli { Name = "mpt-app", Parameters = [Choice("authId", "x", "x", "y")] }; - var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "y", "y", "x")] }; + var a = new Snip { Title = "a", CommandTemplate = "a {authId}", Parameters = [Choice("authId", "x", "y", "x")] }; var b = new Snip { Title = "b", CommandTemplate = "b {authId}", Parameters = [Choice("authId", "x", "x", "y")] }; AnalyzeAndApply(cli, a, b); @@ -123,6 +125,23 @@ public void Existing_compatible_shared_choice_is_reused_not_duplicated() Assert.Empty(b.Parameters); } + [Fact] + public void Existing_text_param_with_a_different_default_is_not_reused() + { + // CLI already shares {env} defaulting to "prod"; imported snips default it to "dev". + // They must keep their own local {env} rather than silently inheriting "prod". + var cli = new Cli { Name = "deploy", Parameters = [new Parameter { Name = "env", Type = ParameterType.Text, Default = "prod" }] }; + var a = TextSnip("deploy a {env}", ("env", "dev")); + var b = TextSnip("deploy b {env}", ("env", "dev")); + + AnalyzeAndApply(cli, a, b); + + // The CLI's shared param is untouched, and the imported snips keep their "dev" default. + Assert.Equal("prod", Assert.Single(cli.Parameters).Default); + Assert.Equal("dev", Assert.Single(a.Parameters).Default); + Assert.Equal("dev", Assert.Single(b.Parameters).Default); + } + [Fact] public void Existing_incompatible_name_leaves_parameters_local() { diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index 1e735ab..6fbd13c 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -219,6 +219,34 @@ public void ShareParameters_promotes_duplicated_params_to_the_created_cli_and_st Assert.All(doc.Snips, s => Assert.Empty(s.Parameters)); } + [Fact] + public void Two_imported_snips_that_unify_to_the_same_template_are_deduped() + { + // {auth} and {authId} (same options, same title) become identical after choice-name + // unification; the second must be skipped rather than imported as a duplicate. + var doc = new SnipStoreDocument(); + static Snip Named(string token) + { + return new Snip + { + Title = "Run", + CommandTemplate = $"x run {{{token}}}", + Parameters = [new Parameter { Name = token, Type = ParameterType.Choice, Options = ["a", "b"], Default = "a" }], + }; + } + // Two name "authId" so it wins; one "auth" that unifies to {authId} -> duplicate of them. + var c1 = new SnippetCandidate("x", true, Named("authId")); + var c2 = new SnippetCandidate("x", true, Named("auth")); + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan(doc, [c1, c2], options); + StoreMerger.Apply(doc, plan); + + // Only one "Run" snip lands (they were the same command once unified). + Assert.Single(doc.Snips); + Assert.Equal("x run {authId}", doc.Snips[0].CommandTemplate); + } + [Fact] public void Swapped_same_option_choices_are_not_treated_as_duplicates() { diff --git a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs index 63017cb..34acc22 100644 --- a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs +++ b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs @@ -141,11 +141,15 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) } var options = MostCommonOptions(members); + var chosenDefault = MostCommonDefault(members); if (existingByName.TryGetValue(winningName, out var existing)) { - // Reuse only a compatible existing shared parameter; otherwise leave these local. - if (existing.Type != ParameterType.Choice || !SameOptionSet(existing.Options, options)) + // Reuse only a fully-compatible existing shared parameter (same option set AND + // default); otherwise leave these local so imported snips keep their own default. + if (existing.Type != ParameterType.Choice + || !SameOptionSet(existing.Options, options) + || !string.Equals(existing.Default, chosenDefault, StringComparison.Ordinal)) { continue; } @@ -157,7 +161,7 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) Name = winningName, Type = ParameterType.Choice, Options = [.. options], - Default = MostCommonDefault(members), + Default = chosenDefault, }); } @@ -206,7 +210,10 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) if (existingByName.TryGetValue(name, out var existing)) { - if (existing.Type != ParameterType.Text) + // Reuse only when the existing shared default matches; otherwise leave these + // local so imported snips keep their own default rather than silently adopting it. + if (existing.Type != ParameterType.Text + || !string.Equals(existing.Default, winningDefault, StringComparison.Ordinal)) { continue; } diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 16aaedb..5be97c4 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -48,42 +48,53 @@ public static MergePlan Plan( resolved.Add((candidate, ResolveCliName(candidate, options))); } + // Phase 1 — duplicate detection on the literal (as-imported) template. Exact duplicates + // (including re-imports of existing snips) are excluded here, so they can't skew the + // choice-name unification below. var items = new List(candidates.Count); - var clisToCreate = new List(); - var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); var plannedKeys = new HashSet<(string, string, string)>(); - foreach (var (candidate, cliName) in resolved) { var key = DedupeKey(cliName, candidate.Snip.Title, candidate.Snip.CommandTemplate); var isDuplicate = !options.AllowDuplicates && (existing.Contains(key) || plannedKeys.Contains(key)); - items.Add(new MergePlanItem(candidate, cliName, isDuplicate)); - - if (isDuplicate) - { - continue; - } - - _ = plannedKeys.Add(key); - - if (!existingCliNames.Contains(cliName) && seenNewClis.Add(cliName)) + if (!isDuplicate) { - clisToCreate.Add(cliName); + _ = plannedKeys.Add(key); } } - // Over the IMPORTED snips of each CLI only (skipped duplicates excluded, so they can't - // skew the result): unify choice names, then promote shared parameters. if (options.ShareParameters) { + // Phase 2 — unify choice names across the imported snips of each CLI (mutates them). foreach (var group in items .Where(i => !i.IsDuplicateSkip) .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) { ParameterSharer.NormalizeChoiceNames([.. group.Select(i => i.Candidate.Snip)]); } + + // Phase 3 — unification can make two imported snips (or an imported and an existing + // one) identical; re-check duplicates on the normalised templates. This is positional + // (literal), so two distinct same-option choices in one command stay distinct. + if (!options.AllowDuplicates) + { + items = RededupeOnNormalisedTemplates(items, existing); + } + } + + // Phase 4 — CLIs to create and shared-parameter promotion, both over the FINAL imported set. + var clisToCreate = new List(); + var seenNewClis = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var item in items) + { + if (!item.IsDuplicateSkip + && !existingCliNames.Contains(item.TargetCliName) + && seenNewClis.Add(item.TargetCliName)) + { + clisToCreate.Add(item.TargetCliName); + } } var sharePlans = options.ShareParameters @@ -167,6 +178,36 @@ public static void Apply(SnipStoreDocument target, MergePlan plan) } } + private static List RededupeOnNormalisedTemplates( + List items, + HashSet<(string, string, string)> existingKeys) + { + var seen = new HashSet<(string, string, string)>(existingKeys); + var result = new List(items.Count); + foreach (var item in items) + { + if (item.IsDuplicateSkip) + { + result.Add(item); + continue; + } + + // The template was mutated in place by NormalizeChoiceNames. + var key = DedupeKey(item.TargetCliName, item.Candidate.Snip.Title, item.Candidate.Snip.CommandTemplate); + if (seen.Contains(key)) + { + result.Add(item with { IsDuplicateSkip = true }); + } + else + { + _ = seen.Add(key); + result.Add(item); + } + } + + return result; + } + private static (string, string, string) DedupeKey(string cliName, string title, string commandTemplate) { // CLI name matches case-insensitively (Cli reuse is case-insensitive); title/command are exact. From fb042e300cf76737ed94ad60b3ca2c13b26e3808 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 06:32:00 +0000 Subject: [PATCH 8/9] Importer: reuse existing same-option-set CLI choices, don't duplicate Choice matching is name-independent, so when importing into a CLI that already shares a choice with the same option set under a different name, don't add a second CLI-scoped choice. Reuse the existing one only when fully compatible (same name and default); otherwise leave the imported snips' parameters local rather than accumulating duplicate shared parameters. Adds a regression test. Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../ParameterSharerTests.cs | 17 +++++++++++++++++ .../Merge/ParameterSharer.cs | 19 +++++++++++++------ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs index 39bb546..9de14c6 100644 --- a/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs +++ b/tools/Snipdeck.Importer.Tests/ParameterSharerTests.cs @@ -125,6 +125,23 @@ public void Existing_compatible_shared_choice_is_reused_not_duplicated() Assert.Empty(b.Parameters); } + [Fact] + public void An_existing_same_option_choice_under_a_different_name_is_not_duplicated() + { + // CLI already shares a [x,y] choice named "authId"; imported snips use "auth" (same set). + var cli = new Cli { Name = "mpt-app", Parameters = [Choice("authId", "x", "x", "y")] }; + var a = new Snip { Title = "a", CommandTemplate = "a {auth}", Parameters = [Choice("auth", "x", "x", "y")] }; + var b = new Snip { Title = "b", CommandTemplate = "b {auth}", Parameters = [Choice("auth", "x", "x", "y")] }; + + AnalyzeAndApply(cli, a, b); + + // No second [x,y] choice is added to the CLI; imported snips keep their local "auth". + Assert.Single(cli.Parameters); + Assert.Equal("authId", cli.Parameters[0].Name); + Assert.Equal("auth", Assert.Single(a.Parameters).Name); + Assert.Equal("auth", Assert.Single(b.Parameters).Name); + } + [Fact] public void Existing_text_param_with_a_different_default_is_not_reused() { diff --git a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs index 34acc22..4a1ea09 100644 --- a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs +++ b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs @@ -143,13 +143,20 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) var options = MostCommonOptions(members); var chosenDefault = MostCommonDefault(members); - if (existingByName.TryGetValue(winningName, out var existing)) + // Choice matching is name-independent: if the CLI already shares a choice with the + // same option set (under ANY name), this parameter already exists there — don't add a + // duplicate. Reuse it only when fully compatible (same name AND default); otherwise, + // or if the winning name is already taken by another param, leave the imported snips' + // parameters local rather than duplicating or silently changing them. + var existingEquivalent = existingCliParameters.FirstOrDefault(p => + p.Type == ParameterType.Choice && SameOptionSet(p.Options, options)); + + if (existingEquivalent is not null || existingByName.ContainsKey(winningName)) { - // Reuse only a fully-compatible existing shared parameter (same option set AND - // default); otherwise leave these local so imported snips keep their own default. - if (existing.Type != ParameterType.Choice - || !SameOptionSet(existing.Options, options) - || !string.Equals(existing.Default, chosenDefault, StringComparison.Ordinal)) + var compatible = existingEquivalent is not null + && string.Equals(existingEquivalent.Name, winningName, StringComparison.Ordinal) + && string.Equals(existingEquivalent.Default, chosenDefault, StringComparison.Ordinal); + if (!compatible) { continue; } From ce4d277ca07513d12bff98c63d64c135d3abdac5 Mon Sep 17 00:00:00 2001 From: Stuart Meeks Date: Mon, 1 Jun 2026 06:38:53 +0000 Subject: [PATCH 9/9] Importer: don't let promotion rebind pre-existing snips in the target CLI When importing into an existing CLI, promoting a new CLI-scoped parameter whose name is used by a pre-existing snip there (as a bare token, or one it inherits from a global) would silently change how that unrelated snip resolves/prompts. Compute the set of such "protected" names per CLI and skip promoting them, leaving the imported snips' parameters local instead. Reusing an already-present CLI parameter is still fine (no new binding). Adds a regression test. Found by codex review. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../StoreMergerTests.cs | 33 +++++++++++++++ .../Merge/ParameterSharer.cs | 19 ++++++++- tools/Snipdeck.Importer/Merge/StoreMerger.cs | 41 +++++++++++++++++-- 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs index 6fbd13c..3baaf12 100644 --- a/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs +++ b/tools/Snipdeck.Importer.Tests/StoreMergerTests.cs @@ -361,6 +361,39 @@ static SnippetCandidate Dup(string title, string template) Assert.Empty(cli.Parameters); } + [Fact] + public void Promotion_does_not_rebind_a_pre_existing_snip_in_the_target_cli() + { + // The CLI already has a snip that uses a bare {env} token (no local/CLI/global definition). + var cli = new Cli { Name = "deploy" }; + var doc = new SnipStoreDocument + { + Clis = { cli }, + Snips = { new Snip { CliId = cli.Id, Title = "Existing", CommandTemplate = "deploy --env {env}" } }, + }; + + // Importing two snips that would otherwise promote a shared {env} must NOT add it, + // because that would start binding the pre-existing "Existing" snip's {env}. + static SnippetCandidate Env(string title) + { + return new SnippetCandidate("deploy", true, new Snip + { + Title = title, + CommandTemplate = $"deploy {title} {{env}}", + Parameters = [new Parameter { Name = "env", Type = ParameterType.Text, Default = "dev" }], + }); + } + var options = _defaults with { ShareParameters = true }; + + var plan = StoreMerger.Plan(doc, [Env("a"), Env("b")], options); + StoreMerger.Apply(doc, plan); + + // No shared {env} added to the CLI; imported snips keep it local. + Assert.Empty(cli.Parameters); + Assert.Equal(3, doc.Snips.Count); + Assert.All(doc.Snips.Where(s => s.Title is "a" or "b"), s => Assert.Single(s.Parameters)); + } + [Fact] public void Sharing_is_off_by_default_so_params_stay_local() { diff --git a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs index 4a1ea09..85d4299 100644 --- a/tools/Snipdeck.Importer/Merge/ParameterSharer.cs +++ b/tools/Snipdeck.Importer/Merge/ParameterSharer.cs @@ -78,11 +78,18 @@ public static void NormalizeChoiceNames(IReadOnlyList snips) } } - public static CliSharePlan Analyze(IReadOnlyList existingCliParameters, IReadOnlyList snips) + public static CliSharePlan Analyze( + IReadOnlyList existingCliParameters, + IReadOnlyList snips, + IReadOnlySet? protectedNames = null) { ArgumentNullException.ThrowIfNull(existingCliParameters); ArgumentNullException.ThrowIfNull(snips); + // Names a new CLI-scoped parameter must not take, because a pre-existing snip in the CLI + // would newly bind to (or have an inherited global overridden by) it. + var offLimits = protectedNames ?? new HashSet(StringComparer.Ordinal); + // Record every (snip, parameter) occurrence with a stable order for deterministic ties. var occurrences = new List(); var order = 0; @@ -161,6 +168,11 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) continue; } } + else if (offLimits.Contains(winningName)) + { + // Promoting this name would rebind a pre-existing snip in the CLI; leave local. + continue; + } else { sharedToAdd.Add(new Parameter @@ -225,6 +237,11 @@ void RecordEdit(Snip snip, string removeName, string? renameTo) continue; } } + else if (offLimits.Contains(name)) + { + // Promoting this name would rebind a pre-existing snip in the CLI; leave local. + continue; + } else { sharedToAdd.Add(new Parameter diff --git a/tools/Snipdeck.Importer/Merge/StoreMerger.cs b/tools/Snipdeck.Importer/Merge/StoreMerger.cs index 5be97c4..3885587 100644 --- a/tools/Snipdeck.Importer/Merge/StoreMerger.cs +++ b/tools/Snipdeck.Importer/Merge/StoreMerger.cs @@ -1,3 +1,4 @@ +using Snipdeck.Core.Engine; using Snipdeck.Core.Models; using Snipdeck.Importer.Sources; @@ -119,12 +120,16 @@ private static Dictionary BuildSharePlans( .Where(i => !i.IsDuplicateSkip) .GroupBy(i => i.TargetCliName, StringComparer.OrdinalIgnoreCase)) { - var existingParameters = existingClisByName.TryGetValue(group.Key, out var existingCli) - ? existingCli.Parameters - : (IReadOnlyList)[]; + var existingCli = existingClisByName.GetValueOrDefault(group.Key); + var existingParameters = existingCli?.Parameters ?? (IReadOnlyList)[]; var snips = group.Select(i => i.Candidate.Snip).ToList(); - var plan = ParameterSharer.Analyze(existingParameters, snips); + // Names that pre-existing snips in this CLI resolve from outside their own locals + // (a bare token, or an inherited global). Promoting a new CLI parameter with such a + // name would silently rebind those unrelated snips, so it is left off-limits. + var protectedNames = ProtectedNames(target, existingCli); + + var plan = ParameterSharer.Analyze(existingParameters, snips, protectedNames); if (!plan.IsEmpty) { plans[group.Key] = plan; @@ -134,6 +139,34 @@ private static Dictionary BuildSharePlans( return plans; } + private static HashSet ProtectedNames(SnipStoreDocument target, Cli? cli) + { + var names = new HashSet(StringComparer.Ordinal); + if (cli is null) + { + return names; + } + + foreach (var snip in target.Snips) + { + if (snip.IsTrash || snip.CliId != cli.Id) + { + continue; + } + + var localNames = snip.Parameters.Select(p => p.Name).ToHashSet(StringComparer.Ordinal); + foreach (var token in SubstitutionEngine.ExtractTokens(snip.CommandTemplate)) + { + if (!localNames.Contains(token)) + { + _ = names.Add(token); + } + } + } + + return names; + } + public static void Apply(SnipStoreDocument target, MergePlan plan) { ArgumentNullException.ThrowIfNull(target);