Skip to content

Revert "Make Ctrl+C triggered during the skills-install prompt dismiss it permanently (#14172)"#14222

Merged
dario-piotrowicz merged 1 commit into
mainfrom
dario/revert-skill-install-crtl-c
Jun 8, 2026
Merged

Revert "Make Ctrl+C triggered during the skills-install prompt dismiss it permanently (#14172)"#14222
dario-piotrowicz merged 1 commit into
mainfrom
dario/revert-skill-install-crtl-c

Conversation

@dario-piotrowicz

@dario-piotrowicz dario-piotrowicz commented Jun 8, 2026

Copy link
Copy Markdown
Member

This reverts PR #14172

My reasoning for reverting the PR is that this feature is becoming more complex and problematic than initially thought, as commented by @petebacondarwin #14199 (review)

(On top of that the user that we wanted to support with this turned out not to be a fan of this approach)

Additionally seems like edge cases and issues with this feature keep popping up (in particular in scenarios where TTY is not super clear). Also various users (like @MattIPv4 and @Cherry) have shared various concerns with it.

So here I want to revert the latest change so that it doesn't get released tomorrow, afterwards we should regroup (@MattieTK) and decide whether we want to double down on this feature or consider alternative options.

(Note: #14220 should get the feature in a good enough position, however as CherryJimbo put it, this might also still include other edge cases that are not obvious now and create a cat-and-mouse game with maintenance)


Open in Devin Review

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Jun 8, 2026
@workers-devprod workers-devprod requested review from a team and james-elicx and removed request for a team June 8, 2026 13:16
@workers-devprod

workers-devprod commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@ask-bonk

ask-bonk Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This resolves the backward-compat concern. The prompt-skip logic at line 60-64 checks only for the existence of the metadata file (existingConfig !== undefined), not the accepted value. So a user with a stale accepted: "SIGINT" file will still be skipped (never re-prompted) — the desired behavior is preserved.

For the telemetry path (line 653, 700), a stale "SIGINT" value would now be truthy and treated as accepted, but this only affects a telemetry classification edge case for a feature that shipped briefly (or possibly never reached release, given the PR's intent to prevent tomorrow's release). This is a minor, acceptable telemetry-only effect for a revert designed to prevent release.

The revert is clean and exact:

  • Code in agents-skills-install.ts is restored byte-for-byte to the pre-14172 state.
  • The changeset is removed.
  • The added tests are removed.
  • Prompt-skip behavior on existing metadata is preserved (existence check, not value check), so users won't be re-prompted.

No logic bugs, no security issues, no API-behavior regressions. The revert accomplishes its stated goal without introducing problems.

LGTM

github run

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 2 potential issues.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment thread .changeset/sigint-dismisses-skills-prompt.md
Comment thread packages/wrangler/src/agents-skills-install.ts
@pkg-pr-new

pkg-pr-new Bot commented Jun 8, 2026

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@14222

@cloudflare/deploy-helpers

npm i https://pkg.pr.new/@cloudflare/deploy-helpers@14222

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@14222

miniflare

npm i https://pkg.pr.new/miniflare@14222

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@14222

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@14222

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@14222

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@14222

@cloudflare/workers-auth

npm i https://pkg.pr.new/@cloudflare/workers-auth@14222

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@14222

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@14222

wrangler

npm i https://pkg.pr.new/wrangler@14222

@cloudflare/wrangler-bundler

npm i https://pkg.pr.new/@cloudflare/wrangler-bundler@14222

commit: c7b5562

@dario-piotrowicz dario-piotrowicz added the ci:skip-pr-description-validation Skip validation of the required PR description format label Jun 8, 2026
@petebacondarwin petebacondarwin removed the request for review from james-elicx June 8, 2026 13:55

@workers-devprod workers-devprod 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.

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Jun 8, 2026
@dario-piotrowicz dario-piotrowicz merged commit 75e652a into main Jun 8, 2026
76 of 79 checks passed
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Jun 8, 2026
@dario-piotrowicz dario-piotrowicz deleted the dario/revert-skill-install-crtl-c branch June 8, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:skip-pr-description-validation Skip validation of the required PR description format

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants