Skip to content

Avoid repeating the skill installation safety warning - #14784

Merged
babakks merged 1 commit into
trunkfrom
tommaso-moro-skill-install-warnings
Oct 22, 2026
Merged

babakks merged 1 commit into
trunkfrom
tommaso-moro-skill-install-warnings

Conversation

@tommaso-moro

Copy link
Copy Markdown
Contributor

Description

Installing a skill prints a safety disclaimer before installation and repeats essentially the same warning afterward. Keep the full pre-install disclaimer and make the post-install message plain review guidance instead, for both local and remote installs.

How did you test this change?

I ran make before and after the change; both builds passed. With isolated home, configuration, and destination directories, I used the newly built bin/gh skill install with --from-local --dir to install the same synthetic example skill. Both installs exited 0 and created example/SKILL.md. I saw two safety warnings before the change and one afterward, with the original pre-install disclaimer unchanged.

The post-install stderr excerpts were:

Before:

! Skills may contain prompt injections or malicious scripts.
  Review the installed files before use.

After:

Review the installed files before use.

Key points

The safety disclaimer remains before installation rather than being removed entirely. Remote installs retain the gh skill preview commands pinned to the installed commit, including --allow-hidden-dirs when needed. Local installs retain their file-review reminder. Review guidance stays on stderr.

Notes for reviewers

Start with printReviewHint in pkg/cmd/skills/install/install.go. Regression coverage checks that local and remote installs print the warning once in both TTY and non-TTY modes; exact-output tests preserve preview arguments and formatting. Remote behavior was covered with mocked API responses, not a live remote installation.

Automated check Result
go test ./pkg/cmd/skills/... Passed
go fix ./pkg/cmd/skills/install Passed; no edits
go test ./... Passed
make lint Passed with the repository-pinned golangci-lint v2.14.0 built using Go 1.27.2

Authorship and follow-up

Who wrote this:

  • A human wrote it.
  • An agent wrote it under close human direction.
  • An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

  • @tommaso-moro will read and reply directly.
  • An agent will draft replies and @tommaso-moro will read them before they are posted.
  • Nobody has explicitly committed to replying.

Keep the pre-install disclaimer and present post-install review guidance without a second warning. Preserve pinned preview commands and cover local and remote output in TTY and non-TTY modes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c5ee8a8a-f0e8-432e-8ebb-62ff72712b41
@tommaso-moro
tommaso-moro requested review from a team as code owners October 21, 2026 17:32
@tommaso-moro
tommaso-moro requested review from BagToad and a balanced review from Copilot October 21, 2026 17:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Contribution eligibility remains unverified and needs confirmation from @cli/code-reviewers.

No open issues 🎉

What changed in this PR

Simplifies GitHub CLI skill installation output while preserving the pre-install safety disclaimer.

Changes:

  • Replaces the repeated warning with plain review guidance on stderr.
  • Preserves remote preview commands and adds local/remote regression coverage for TTY and non-TTY output.
File Description
pkg/​cmd/​skills/​install/​install.go Simplifies post-install guidance for both installation paths.
pkg/​cmd/​skills/​install/​install_test.go Checks warning count, output routing, and exact guidance formatting.

Review effort: Balanced

@babakks
babakks merged commit 5056c0a into trunk Oct 22, 2026
19 checks passed
@babakks
babakks deleted the tommaso-moro-skill-install-warnings branch October 22, 2026 09:36

@dl909-ai dl909-ai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evidence

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants