Skip to content

fix: separate confirmation from action labels - #498

Merged
cjol merged 1 commit into
mainfrom
codex/confirmation-action-labels
Nov 3, 2026
Merged

cjol merged 1 commit into
mainfrom
codex/confirmation-action-labels

Conversation

@petebacondarwin

Copy link
Copy Markdown
Collaborator

Commands marked with x-forge-require-confirmation currently inherit Deleting/Deleted labels even when they update settings, restore data, or process a mixed batch. Generate action labels from the finalized HTTP method: Updating/Updated for PUT/PATCH, Deleting/Deleted for DELETE, and neutral Processing/Done for confirmed POST actions. Keep read and validation labels, operation-specific warnings, and --force behavior.

Check for missing required --body input before authentication and confirmation, while accepting file and multipart inputs. Regenerate the tracked commands, update the architecture reference, and add a patch changeset.

Validation:

  • pnpm check
  • pnpm test src/__tests__/generator/plugin from packages/cli
  • pnpm test src/__tests__/generator/plugin/multipart-files.test.ts from packages/cli after adding upload regression coverage
  • Reviewed all 123 changed generated files: 52 label changes and 72 relocated body checks, with one file containing both

Addresses the generator findings in the review on #477.

@petebacondarwin
petebacondarwin requested a review from a team as a code owner November 3, 2026 16:57
@pkg-pr-new

pkg-pr-new Bot commented Nov 3, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/cf@498

commit: 5123bae

@ask-bonk

ask-bonk Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

cf --help startup

mean stddev min max
PR 263.9 ms 7.0 ms 255.4 ms 278.6 ms
base 268.1 ms 9.4 ms 257.5 ms 292.5 ms

Δ (PR vs base): ↓ -4.2 ms (-1.6%)

Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.

@clanky-the-clanker clanky-the-clanker Bot 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.

✅ Approved — No findings: the label derivation and the earlier required-body check are correct, and the generated output, tests, docs and changeset are consistent with them.

The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.


✅ All checks passed

🚀 Release: ✅ LGTM

Risk: low. A patch changeset is appropriate for this fix.


🔒 Security: ✅ LGTM

The confirmation and --force behaviour is unchanged. The missing-body check now runs before authentication, which adds no exposure.


📖 AGENTS.md: ✅ LGTM

The change follows the repository conventions: generated output was regenerated rather than edited by hand, labels are derived from the HTTP method, and a changeset is included.


📋 Code Quality: ✅ LGTM

The generator changes are small and well tested. The required-body check moved from plainCall to the start of the handler, and it now covers file and multipart inputs.


⚡ Performance: ✅ LGTM

No performance impact.


📝 Documentation: ✅ LGTM

The architecture reference has been updated to describe the new label behaviour.


Reviewed commit: 5123bae3

@cjol
cjol added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit b5c5756 Nov 3, 2026
12 checks passed
@cjol
cjol deleted the codex/confirmation-action-labels branch November 3, 2026 17:11
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.

2 participants