fix(cli): confirm command echoes only the flags the user typed [ship]

[martzoukos]

Affected Components

• CLI
• Test
• Docs

What

In agent mode, write commands print a confirmCommand for the user to approve. buildConfirmCommand serialized every entry of oclif's parsed flags — including everything filled in from default: — so a bare npx checkly deploy confirmed as:

checkly deploy --no-preview --no-verbose --schedule-on-deploy --no-preserve-resources \
  --no-cancel-in-progress-deployment --verify-runtime-dependencies --no-debug-bundle \
  --debug-bundle-output-file="./debug-bundle.json" --force

That command does not run. --no-x is only a registered flag when the definition sets allowNo: true, and on deploy only schedule-on-deploy and verify-runtime-dependencies do. oclif rejects the rest with Nonexistent flags: --no-preview, .... So we printed a command, told agents to run it verbatim, and it errored out. It also leaked two hidden: true debug flags and buried whichever flag the user had actually typed.

Fix

Pass oclif's metadata.flags through the preview and skip flags marked setFromDefault:

$ npx checkly deploy --preserve-resources
  before: checkly deploy --no-preview --no-verbose --schedule-on-deploy --preserve-resources
          --no-cancel-in-progress-deployment --verify-runtime-dependencies --no-debug-bundle
          --debug-bundle-output-file="./debug-bundle.json" --force
  after : checkly deploy --preserve-resources --force

buildConfirmCommand was written for incidents create, where every flag is user-typed and string-valued, then reused verbatim for deploy, whose flags are mostly booleans with defaults. deploy and destroy were the two commands producing unrunnable output; incidents create/update/resolve were only noisy. members/delete|update already dodged this with a || undefined coercion.

flagMetadata is optional on CommandPreview, so call sites that don't pass it behave exactly as before.

Docs

The confirmation protocol lived only in references/communicate.md (the incidents/status-page doc), so an agent running a deploy never read it. The Deploying section of references/configure.md now covers the two-stage confirm, --preserve-resources, and the delete guard.

Also resolved a contradiction: the skill said "never auto-append --force" while buildConfirmCommand always appends it, making the rule read as unfollowable. It now says the confirmCommand is meant to run verbatim.

Tests

Added a round-trip test that feeds the generated confirmCommand back through oclif's parser and asserts it parses. It fails with the exact Nonexistent flags error when the fix is disabled — verified, not assumed. Full suite green (1576 passing), lint clean.

Notes for the Reviewer

• Reported by Timo in #ask-engineering. The "before" line above is character-for-character his paste.
• Not addressed here: his other point — that agent mode is auto-detected from env markers like CODEX_THREAD_ID without asking. That's preexisting and a product decision. Gating agent mode on !process.stdout.isTTY would kill this exact false positive, but any agent that allocates a pty would fall back to interactive and hang on the prompt — worse than noise. Worth a separate discussion.

🤖 Generated with Claude Code

[martzoukos]

This seems like a preexisting bug in the confirm path, but it seems like a too obvious thing to have been like that for a while. I'd appreciate your 2c here, guys.

[timohuovinen]

Review: checkly-cli PR #1410

HEAD SHA: f181e5bc0da82110399371848f4e3d7c935b44ac
Reviewed: 2026-07-15 (Checkly Stack review, 4 personas)

---

Review Summary

Verdict: REQUEST CHANGES — the core fix is correct and genuinely well-tested; the changes requested are small and mostly in the shipped docs.

Intent (as understood): buildConfirmCommand serialized every parsed oclif flag including default:-filled ones, so a bare npx checkly deploy emitted a confirmCommand containing --no-preview, --no-verbose etc. Those --no-x forms only parse when a flag sets allowNo: true, so the command the CLI told agents to run verbatim errored with Nonexistent flags. The fix threads oclif's metadata.flags through CommandPreview and skips flags marked setFromDefault. Docs were relocated/expanded so a deploying agent actually reads the confirmation protocol.

Overview: The diagnosis is right, the fix is minimal and correctly targeted, and the round-trip test is the best kind — it asserts the property under repair (the emitted command parses) through the real oclif parser, not a golden string. I had a specialist disable the fix and confirm the test fails with precisely the claimed Nonexistent flags error, so the regression lock is real, not assumed. What holds it back is that the docs now ship a guarantee to agents that no command in the tree implements, and the fix is threaded by hand through an optional field with nothing preventing the next command from regressing.

Triage

• Diff: 15 files / +161 / −17 (~178 LOC)
• Personas spawned: Generalist & Domain (mandatory; deploy/destroy semantics and the agent confirm protocol); Correctness (bug-fix commit, logic change to a generator whose output is executed, new test claims); Architecture (cap-exempt: public contract change — new exported type, widened exported signature); Security (cap-exempt: user-controlled values rendered into a string agents execute verbatim; alters a human-in-the-loop confirmation boundary)
• Personas skipped: Performance & Reliability — no trigger fires. The <200 LOC cap would have allowed only one specialist beyond Generalist; Architecture and Security were admitted under the cap-exempt list, not in spite of it.

---

Critical Issues

None. Security proposed one (shell injection via unescaped interpolation) — I demoted it; see Important #4 for the reasoning and verification.

---

Important Issues

1. The docs promise target-pinning that no command implements

packages/cli/src/ai-context/references/communicate.md:28 tells agents "a command that resolves a target for you pins that target into the confirmCommand, so the approved run cannot drift onto a different one."

I verified this against every command that passes args to a preview (members/delete.ts:61, members/update.ts:88, incidents/resolve.ts:52, incidents/update.ts:73, checks/delete.ts:52): all five pass the raw user input, never a resolved value. members delete is the only command that resolves anything — and it confirms with args: { member: args.member }, the raw email, while members/delete.ts:48-52 actively drops the email/id disambiguation. So checkly members delete foo@bar.com --force re-resolves at run time — precisely the drift the sentence says is prevented.

Commit f181e5bc's own message concedes the pinning behavior arrives only with unmerged PR #1402. Three specialists flagged this independently. Metadata-only, so capped at Important — but these files ship to agents, and the next sentence ("Treat every flag you see there as deliberate") builds agent trust on it.

Fix: either drop the clause until #1402 lands, or move it into #1402. Same claim at skill.md:39 and skills/checkly/SKILL.md:39.

Relatedly: the PR body never mentions #1402. AGENTS.md asks that unmerged dependencies be "call[ed] out in the PR body with a direct link" — that rule is scoped to backend/API PRs, so this is its spirit rather than its letter, but a reviewer shouldn't have to read commit messages to learn the docs are coupled to unmerged work.

2. flagMetadata is optional with nothing enforcing it, leaving two mechanisms for one problem

packages/cli/src/helpers/command-preview.ts:18 makes the field optional, so six call sites pass it and two don't (members/delete.ts:60, members/update.ts:87), which instead hand-roll email: flags.email || undefined.

That coercion is correct today only by coincidence — email/id are the only non-omitted booleans and neither sets allowNo — but any new Flags.boolean({ default: false }) on those commands, or on a new write command that forgets the field, silently reproduces the exact bug this PR fixes, with no type error and no failing test. The PR frames "call sites that don't pass it behave exactly as before" as a safety property, but the preserved behavior is the bug. AGENTS.md is direct here: "do not spread the inconsistency to new commands."

Fix: making flagMetadata required on CommandPreview (keeping the buildConfirmCommand parameter optional as the test seam) turns this into a compile error at exactly the two members call sites, and lets both previewFlags blocks be deleted.

3. The round-trip proof covers the two easiest commands and would not catch a live defect in the ones it skips

The new tests cover only deploy and destroy — the two commands with no positional args and no free-text flags. incidents create/resolve/update are modified by this diff to pass flagMetadata and are exactly the commands with positional args and a free-form --message; their specs still assert against hand-written mocks and never re-parse.

Correctness ran the real helper and found a live instance of the same defect class this PR exists to fix:

buildConfirmCommand('incidents update', { message: 'He said "ship it"' }, { id: 'inc-1' })
→ checkly incidents update inc-1 --message="He said "ship it"" --force

A shell splits that into --message=He said, ship, it → rejected with Unexpected arguments.

Separately, the flagArgv tokenizer at confirm-flow-deploy.spec.ts:50-52 matches only -- tokens, so positional args pushed via bare String(value) are silently dropped rather than tested — the harness is load-bearing by accident and would keep passing when pointed at a command where it drops the evidence.

AGENTS.md asks for tests on "any generated follow-up commands shown to users."

Fix: extend confirmCommandFor to the incidents specs.

4. Unescaped interpolation into an agent-executed string — pre-existing, not introduced

command-preview.ts:62 wraps values in " with no escaping of ", $, or backticks, and line 44 pushes positional args entirely unquoted; changes (what the human approves) and confirmCommand (what runs) are derived independently, so a value can inject tokens that never appear in the approved text.

Security rated this Critical. I demoted it, and logged why: git diff confirms both interpolation lines are untouched by this PR, and git show origin/main:…/communicate.md shows the old rule already read "only run the confirmCommand after the user explicitly approves" — verbatim execution was already the contract, so the "Run verbatim" rewording clarifies rather than widens it.

This is a real issue on a real threat model (an agent drafting --message from an alert payload or customer ticket), but it predates the PR and blocking on it would be scope creep. Worth a follow-up issue; if the author wants to fix the quoting here, the existing round-trip test extends to cover it naturally.

---

Suggestions

Env-sourced flags slip past the setFromDefault filter

Three specialists independently read the installed parser (@oclif/core@4.11.14/lib/parser/parse.js:352-377) and confirmed the env branch returns without metadata, so a value from env: is unmarked and gets echoed. For deploy.ts:94-99's verify-runtime-dependencies this is harmless (it sets allowNo: true, so it re-parses) and arguably desirable, since echoing pins the value.

Three small consequences worth a look:

• the FlagMetadata JSDoc's binary framing ("from default: rather than typed by the user") doesn't admit the third source;
• the test named "echoes only the flags the user typed" overclaims;
• expect(await confirmCommandFor([])).toBe('checkly deploy --force') would fail for anyone with CHECKLY_VERIFY_RUNTIME_DEPENDENCIES exported in their shell.

A future env: boolean without allowNo would reintroduce the original bug.

flagMetadata on deploy's second guard looks inert

deploy.ts:320 — the field is consumed only by formatPreviewForAgent, but the second guard is gated on !force, and in agent mode with !force the first guard already exit(2)s. Interactive mode reaches it but renders via formatPreviewForTerminal, which ignores the field. Harmless and arguably good symmetry; noting rather than asking for a change.

---

What's Done Well

• The round-trip test is the right shape and is genuinely red-locked. Feeding the generated confirmCommand back through the real Parser with the real Deploy.flags tests the property that matters (is this runnable?) rather than the current output shape, so it survives refactors of the helper and would have caught the original bug. A specialist verified the claim by disabling the fix: it fails with exactly Nonexistent flags: --no-preserve-resources, --no-cancel-in-progress-deployment, then 50/50 green on restore.
• The skip-branch comment at command-preview.ts:51-53 earns its place — it states the rationale and the concrete failure it prevents, which is the non-obvious oclif constraint a future reader would otherwise rediscover the hard way.
• Documenting the delete-guard gap honestly ("An agent following the confirmation protocol never sees that list") rather than papering over it. Generalist traced it and confirmed it's accurate: bare deploy → guard 1 exits 2 → the approved re-run carries --force → deploy.ts:288 skips the entire itemised-deletion block.
• Deleting "Never auto-append --force" fixes a real contradiction — the helper always appends it, so the old rule made a compliant agent's only sanctioned action look forbidden.
• Skill-file sync is correct, not accidental duplication: skills/checkly/SKILL.md is generated from ai-context/skill.md via prepare-ai-context.ts + sync:skills, the two now differ only at the <!-- SKILL_COMMANDS --> placeholder, and CLAUDE.md confirms "CI checks this."
• Typing FlagMetadata as a narrow structural subset rather than importing oclif's parser types keeps the helper decoupled while still accepting metadata.flags directly.

---

Verification Story

• Tests reviewed: yes — all three affected suites run green (31/31 and 50/50 across specialists). The round-trip lock was verified by inversion, not taken on trust; worktree restored to HEAD afterward.
• Build verified: not run. Suites pass; prepare:dist/prepack from the AGENTS.md checklist were not exercised this pass.
• Security checked: yes — injection surface probed with hostile values against the real helper; all eight confirmOrAbort callers audited for secret exposure (none currently carries one; note env add/env update take secret values and don't yet use confirmOrAbort — if they adopt it, the secret lands in the echoed string).

---

Out of Scope This Pass

• Performance & Reliability — no trigger.
• The pre-existing quoting/escaping defect as a fix (flagged above as a follow-up).
• E2E and packaging smoke tests.

---

Conflicts / Open Questions

• Is documenting the delete-guard bypass sufficient? Generalist called it "the right call" — documenting a real gap honestly and routing agents to --preview. Security called it "not mitigating it," arguing the human approves "deletion is possible" and gets silent deletion of named resources plus run history, with --preview-first being advisory prose that relies on the same agent discipline the PR is tightening. Both traced the same code and agree on the mechanics; they disagree on whether prose is enough. Security's structural suggestion (a single-purpose token satisfying guard 1 only, so guard 2 can emit its own envelope with the itemised list) is a sound design but well beyond this PR. This is a product decision — the PR body already flags a neighbouring one (agent-mode auto-detection) as deliberately deferred, and this may belong in the same conversation.
• Is leaving members/* on the || undefined coercion intentional? The PR body mentions it dodged the bug but doesn't say whether that's a deliberate keep. If deliberate, one line in the description settles it; if not, Important feat: setup global config / init #2 applies.

[timohuovinen]

LGTM, the /review bits can be addressed now or as a follow-up

[martzoukos]

Awesome, thanks Timo! I addressed #1 from your findings as it was a genuine hole in the thing I intended to fix. Merging now.

[github-actions]

Auto-approved: ship/show PR from a same-repo branch.