Context

Review feedback from PRs #13167, #13168, #13170, and #13171 that was deferred to post-merge follow-ups on the sm/add-skills-command branch.

Follow-up tasks

PR #13168 - Refactor HTTP client injection to use factory pattern

• Remove direct opts.client injection in publish command. Use the standard factory pattern (opts.HttpClient returning a stubbed client) instead of injecting a client field directly on the options struct. This keeps test-awareness out of command code.
  • Reviewer: babakks
  • Comment link

  • "We do not inject HTTP clients for testing in this way. Basically, the test function creates a stubbed HTTP client and make the factory function (opts.HttpClient) return that. That way you never need to involve the test-aware mindset in the command code."

PR #13170 - Search deduplication improvements

• Rename testName field to name in test struct. In discovery_test.go, the test struct field testName should follow the standard naming convention.

  • Reviewer: babakks (nitpick)
  • Comment link

  • "I would rather change testName to either name or about."

• Investigate case-sensitivity in skill name deduplication. Determine what should happen when two skill names differ only in casing (e.g. MySkill vs myskill).

  • Reviewer: babakks (question)
  • Comment link

  • "What if two skill names only differ in casing?"

• Fix whitespace indentation. Line 578 in search.go is indented with spaces instead of tabs.

  • Reviewer: babakks (nitpick)
  • Comment link

  • "Indented with whitespaces."

• Refactor dedup map to use typed struct keys. Replace string-concatenated map keys with a typed struct key for the deduplication map. More idiomatic Go and avoids potential key collision issues.

  • Reviewer: babakks (nitpick)
  • Comment link

  • "The nicer/idiomatic way is to avoid string keys (and string-based collisions) and use typed map keys."

PR #13171 - Auto-push and test infrastructure

• Replace real git usage in tests with command stubs. The publish tests use real git commands which is not the standard pattern. Use run.CommandStubber to register expected git commands and their outputs instead.

  • Reviewer: babakks
  • Comment link

  • "This is real git in tests and we do not do it for many reasons. Instead you should do what is done in other commands, which is to create a command stub, and then register the commands and the stubbed outputs."

• Simplify remote selection using gitClient.Remotes ordering. The Remotes method already returns remotes in a preferred order (upstream, github, origin, rest). The manual origin-preference logic could be simplified.

  • Reviewer: babakks
  • Comment link

  • "gitClient.Remotes returns an ordered list of remotes with this ordering (if they exist): upstream, github, origin, (the rest). If that ordering makes sense for you to follow, you can simplify this function."

• Fix push icon timing. The push icon is shown before the push operation completes. Either remove the icon or show SuccessIcon only after push succeeds.

  • Reviewer: babakks
  • Comment link

  • "Maybe with no icon, as the operation may fail for any reason."