Importer: de-duplicate parameters into CLI-shared parameters#37
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…orted 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
… 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The SnipCommand importer now de-duplicates parameters and promotes the shared ones up to CLI-scoped shared parameters (
Cli.Parameters), so recurring parameters live once at the CLI level instead of being copied onto every snip.Matching rules (as requested)
Why this is safe
ParameterResolver.Resolveonly surfaces a CLI-shared parameter when the snip's template actually references its token — so stripping the promoted locals doesn't make unrelated inputs appear on other snips.Guards (added after review)
{source}and{dest}) are not collapsed into one token.Behaviour / flags
--no-share-parameterskeeps every parameter local.Design
ParameterSharer(Analyzecomputes a plan,Applymutates), invoked fromStoreMerger.Plan/Apply.MergeOptionsgainsShareParameters(default off, so existing call sites are unaffected); the command sets it from--no-share-parameters.Testing
ParameterSharerTestscovering the matching rules, the two collision guards, purity-until-apply; plus aStoreMergerintegration test). Core: 214 pass. All on Linux.mpt-appgets 7 CLI-shared parameters (theauthIdchoice with all 7 options + 6 recurring text params); the 21 snips that use them inherit them and only 4 single-use-param snips keep locals.Review note:
codex reviewcouldn't run in this environment (its bubblewrap sandbox can't create namespaces here); findings came from an agent-based review and are addressed above.🤖 Generated with Claude Code