Skip to content

feat(cli): make import apply/commit/cancel agent-drivable#1402

Open
martzoukos wants to merge 5 commits into
mainfrom
martzoukos/ai-445-pending-mappings-on-pr
Open

feat(cli): make import apply/commit/cancel agent-drivable#1402
martzoukos wants to merge 5 commits into
mainfrom
martzoukos/ai-445-pending-mappings-on-pr

Conversation

@martzoukos

Copy link
Copy Markdown
Contributor

What

Makes the import lifecycle commands agent-drivable (non-interactive) so a remote AI runner (or CI) can reserve imported resources without a human at a TTY. This is the checkly-cli slice of the AI runner → PR handoff (AI-446); the backend/runner pieces live in the monorepo repo.

All changes are additive and backwards compatible — interactive TTY behavior is unchanged.

Changes

  • --plan-id <id> on import apply, import commit, and import cancel — target a specific plan without interactive selection.
  • --force/-f on import apply — apply only and skip the trailing "commit now?" prompt, deferring finalization to the user's post-merge checkly deploy.
  • Non-interactive auto-resolution: in agent/CI sessions (detected via the existing detectCliMode), a single candidate plan is auto-selected; an ambiguous multi-plan selection errors and asks for --plan-id (exit 1). Previously these commands silently no-op'd in non-TTY sessions.
  • Shared resolution logic extracted to src/helpers/import-plan-selection.ts (resolvePlanNonInteractively, selectPlanOrExit, PlanSelectionError). Deliberately kept out of commands/import/ because oclif auto-registers every file in a command directory as a command.
  • Reused this repo's established forceFlag() / confirmOrAbort / detectCliMode convention (as in checks delete, env rm) rather than a bespoke --yes.

Scoped import — verified, no change needed

checkly import plan check:<id> already builds filters = [{exclude-all}, {include check:<id>}] and sends it as the backend payload, so the named-resource path is scoped end-to-end and does not fall back to a bulk import.

Testing

  • helpers/__tests__/import-plan-selection.spec.ts — plan resolution matrix (by id, ambiguous non-interactive error, single-plan auto-select, interactive fallback).
  • commands/import/__tests__/apply.spec.ts — apply auto-selects/targets a plan and does not commit in agent mode / with --force; ambiguous selection exits 1.
  • tsc --build ✅ · eslint ✅ · 147 tests pass (incl. command-metadata) · --help renders the new flags with no oclif warnings.

Out of scope / follow-ups

  • The deploy-side pending→committed finalization and other backend/runner work (separate monorepo tickets in the 444→447 family).
  • import plan's own interactive prompts (project-init, existing-plan handling) are untouched — a fully headless import plan in a fresh project may warrant a follow-up.

🤖 Generated with Claude Code

martzoukos and others added 2 commits July 13, 2026 17:41
Add non-interactive support to the import lifecycle commands so a remote
agent (or CI) can reserve imported resources without a human at a TTY,
which is the CLI slice of the AI runner -> PR handoff (AI-446).

- Add `--plan-id <id>` to `import apply`, `import commit`, and `import
  cancel` to target a plan without interactive selection.
- Add `--force`/`-f` to `import apply`: apply only and skip the trailing
  "commit now?" prompt, deferring finalization to a post-merge deploy.
- In non-interactive sessions (agent/CI, via detectCliMode) a single
  candidate plan is auto-selected; an ambiguous multi-plan selection
  errors asking for `--plan-id` (exit 1). Interactive behavior is
  unchanged.
- Extract the shared resolution logic into
  helpers/import-plan-selection.ts (kept out of commands/import/ so oclif
  does not register it as a command).

All changes are additive and backwards compatible. Scoped
`import plan check:<id>` already sends a scoped filter payload, so no
change was needed there.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
When `checkly import` finds no importable resources, a resource the user
expected may simply be hidden because it's reserved by a pending import.
Add an info tip pointing them at the scoped `checkly import check:<id>`
form, which surfaces such resources.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@martzoukos martzoukos marked this pull request as ready for review July 14, 2026 15:18
Making import cancel agent-drivable exposed a footgun: with both --all and
--plan-id set, --all silently won and the requested plan id was ignored.
For the one destructive command in the import set, driven by agents that
assemble flags programmatically, silently doing the opposite of an explicit
--plan-id is a hazard.

Reject the contradictory combination with a non-zero exit instead. The
--all / --plan-id resolution (plus the previously duplicated
PlanSelectionError -> exit tail) is unified into a new selectPlansOrExit
helper, and cancel's command-level wiring now has test coverage
(agent single-plan, --all, ambiguous exit 1, and the new guard).

Co-Authored-By: Claude Opus 4.8 <[email protected]>

@thebiglabasky thebiglabasky 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.

Review Summary

Verdict: REQUEST CHANGES
Intent (as understood): Make the import lifecycle targetable and observable from agents/CI while preserving the existing interactive flow.
Overview: The selection extraction is readable and the happy-path coverage is solid, but the new non-interactive contract currently bypasses the CLI's mutation-safety convention and has two user-visible flag/exit-status problems.

Triage

  • Diff: 9 files, +476/-9 at 216e9420
  • Personas: Generalist & Domain, Correctness (new lifecycle behavior), Architecture (shared helper/public CLI contract), Security (permanent/destructive mutations driven by external flags)
  • Skipped: DB and Performance; no triggered surface

Important Issues

  1. Do not treat agent/CI detection as authorization for permanent or destructive mutations (confidence 92/100)

    packages/cli/src/helpers/import-plan-selection.ts:46 auto-selects the sole plan solely because the ambient mode is non-interactive. import commit then immediately performs the permanent commit, and import cancel immediately deletes the selected plan. Neither command exposes an explicit confirmation/force step. Verified against the current AuthCommand contract: agent/CI mutations emit a preview and exit 2 until explicitly forced. The repo rule is: “Prefer the newest command with the same interaction model as the precedent.” Please keep headless plan resolution, but require explicit authorization before commit/cancel and set their destructive metadata intentionally.

  2. Return failure for explicit targets and invalid flag combinations even when the candidate list is empty (confidence 98/100)

    packages/cli/src/commands/import/cancel.ts:51 returns before selectPlansOrExit, so import cancel --all --plan-id x is not rejected when zero plans exist. The same ordering in apply and commit makes an explicit --plan-id x print a fatal-looking message and return normally when no candidates exist; CommandStyle.fatal only logs, so agent/CI observes exit 0 although nothing happened. Validate mutually exclusive flags before fetching/short-circuiting, exit non-zero when an explicit action cannot run, and add the required empty-path coverage. Applicable repo rule: “Add tests for REST parameter mapping, JSON output shape, empty/error paths.”

  3. Do not give --force the prompt's negative semantics (confidence 96/100)

    packages/cli/src/commands/import/apply.ts:63 returns before performCommitAction when --force is set. But the shared flag is documented as “Skip confirmation prompt”, and the current convention interprets that as proceeding without prompting. Here it is equivalent to answering “no” to “Would you like to commit?”, so the help text can make users expect the opposite final state. The desired pending-only behavior is valid, but it needs an explicit name/help contract such as --no-commit/--defer-commit, rather than reusing forceFlag().

What's Done Well

  • User-supplied plan IDs are matched against server-returned candidates before mutation.
  • Ambiguous multi-plan resolution fails non-zero, and --all/ --plan-id conflict handling is centralized.
  • The helper stays outside the oclif command tree, and the interactive fallback remains structurally isolated.

Verification Story

  • Focused Vitest: 5 files / 164 tests passed.
  • Changed-file ESLint, prepare:dist, prepack, and git diff --check: passed.
  • Help smoke tests for import apply, commit, and cancel: passed and confirmed the published flag wording.
  • Live GitHub CI: all Ubuntu/Windows unit and E2E gates green.
  • Security check: no direct use of unvalidated plan IDs, secrets, or new dependency surface.

Addresses review feedback on the agent-drivable import lifecycle.

Require explicit authorization for commit and cancel. Agent/CI detection
resolved a plan and then immediately mutated: commit permanently, cancel
by deleting. Both now take --force/--dry-run and route through
confirmOrAbort, matching `checks delete`. Agents get a JSON preview and
exit 2 until they re-run with --force. The resolved plan ID is pinned
into confirmCommand so the confirming run cannot resolve to a different
plan than the one previewed. cancel is marked destructive; commit is
deliberately not (it deletes nothing) but is still gated, as
confirmOrAbort covers every non-read-only command.

Make the failure paths honest. style.fatal only logs, so an explicit
--plan-id that could not be satisfied printed a red error and exited 0.
Flag conflicts were also detected after the fetch, so
`cancel --all --plan-id x` passed silently when zero plans existed.
Conflicts are now validated before the API call, an unsatisfiable
explicit target exits 1, and genuine nothing-to-do exits 0 with plain
informational text instead of red.

Rename apply's --force to --no-commit. forceFlag() is documented as
"Skip confirmation prompt", but on apply it returned before the commit
step — equivalent to answering "no". The deferred-commit behavior is
unchanged; only the name and help text now describe it. --force on apply
is introduced by this PR and unreleased, so this is not a breaking
rename. Applying now also prints the exact command to commit later.

List candidate plan IDs when selection is ambiguous. The error told
agents to re-run with --plan-id <id> but named no IDs, and there is no
plan listing command, so the instruction was unfollowable.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@martzoukos

Copy link
Copy Markdown
Contributor Author

Thanks — all three land, and chasing #2 turned up a fourth problem. Fixed in 4a5c739.

1. Agent/CI detection is no longer authorization

commit and cancel now take --force/--dry-run and route through confirmOrAbort, same as checks delete. Headless plan resolution is unchanged; only the mutation is gated.

$ checkly import commit          # agent mode, one applied plan
{
  "status": "confirmation_required",
  "command": "import commit",
  "classification": { "readOnly": false, "destructive": false, "idempotent": false },
  "changes": [
    "Permanently commit import plan abc123",
    "Imported resources become fully managed by the Checkly CLI",
    "This cannot be undone — the plan can no longer be cancelled"
  ],
  "confirmCommand": "checkly import commit --plan-id=\"abc123\" --force"
}
$ echo $?
2

One thing worth a look. Every existing confirmOrAbort caller takes its target as a required arg, so the agent already knew the ID and confirmCommand just echoed it back. import commit is the first case where the CLI resolves the target itself — so the preview is telling the agent something it had no other way to know. Passing the raw parsed flags through would have produced checkly import commit --force, and the confirming run would re-resolve independently: if another plan got applied in between, the agent permanently commits something it never previewed. The resolved ID is pinned instead:

flags: { ...flags, 'plan-id': plan.id },

cancel pins the same way for a single plan. --all deliberately does not — it means "whatever is uncommitted", so re-resolving is the correct semantics.

On metadata: cancel is now destructive = true. commit is deliberately destructive = false — it deletes nothing — but it is still gated, since confirmOrAbort covers every non-read-only command. The comment in the file records that reasoning so it doesn't read as an oversight.

Interactive commit now shows the preview before the existing "Would you like to commit the plan now?" prompt (reused via interactiveConfirm, so the wording is unchanged). That is a small interactive change, and I think it is the right one for an irreversible step — happy to revert if you disagree.

2. Failure paths now report failure

Both halves fixed. Flag conflicts are validated before the fetch, so remote state can no longer mask them:

# before: exit 0 when zero plans existed, exit 1 when some existed
# after:  exit 1 always, and no API call at all
$ checkly import cancel --all --plan-id abc123
--all and --plan-id cannot be used together.
$ echo $?
1

And the message severity now matches the exit code. An explicit target that cannot be satisfied fails:

$ checkly import commit --plan-id abc123    # no plans exist
No plan available to commit with ID "abc123".
$ echo $?
1

Genuine nothing-to-do is not an error, so it stops using red fatal text:

$ checkly import cancel
Nothing to cancel — no uncommitted import plans found.
$ echo $?
0

Empty-path coverage added for all three commands, including a regression test asserting findImportPlans is never called when the flags conflict.

3. --force on apply renamed to --no-commit

You were right that the name was the problem, not the behavior. --force was equivalent to answering "no" to the commit prompt while advertising "Skip confirmation prompt". Behavior is unchanged; it now says what it does:

$ checkly import apply --plan-id abc123 --no-commit
✔ Your import plan has been applied!

  The plan is applied but not committed. To commit it, run:

    npx checkly import commit --plan-id abc123

--force on apply was introduced by this PR and never shipped, so this is not a breaking rename. Apply stays ungated — it is the reversible step, and cancel undoes it.

4. Ambiguous selection was a dead end (not in the review)

Found while working on #2. The ambiguity error correctly exits 1 — but it told the agent to re-run with --plan-id <id> and named no IDs. There is no plan listing command (import plan creates one), so the instruction was unfollowable: the agent's only options were to call the REST API directly or ask a human, which defeats the point of the PR. Interactive users never hit it, since they just get the menu.

$ checkly import commit          # agent mode, two applied plans
Found 2 plans available to commit. Re-run with --plan-id <id> to choose one:

  abc123  (created 2026-07-14T10:12:00Z, applied 2026-07-14T10:15:00Z)
  def456  (created 2026-07-15T09:03:00Z, applied 2026-07-15T09:05:00Z)

$ echo $?
1

Same principle as the pinning in #1: where the CLI knows something the agent cannot discover, it has to hand it back.

One latent trap found

buildConfirmCommand renders any false boolean flag as --no-<flag>. Existing callers never hit this because their only booleans are force/dry-run, both in OMITTED_FLAGS. cancel --all is the first real boolean to flow through, and all: false would have produced checkly import cancel --no-all --plan-id="abc" --force — which does not parse, since all has no allowNo. Worked around locally by dropping the flag rather than passing false, with a test pinning it. Left command-preview.ts alone as out of scope, but it will bite the next command that adds a boolean flag.

Verification

Unit tests are not enough here — they call run.call(ctx) directly and bypass oclif — so I drove the real binary against a stub API (CHECKLY_SKIP_AUTH=1, CHECKLY_API_URL pointed at a local server that logs any mutation it receives):

  • commit with no flags → exit 2, no mutation reaches the API
  • replaying the emitted confirmCommand → commit succeeds, POST .../commit reaches the API
  • commit --dry-run → exit 0, no mutation
  • cancel with no flags → exit 2, no mutation
  • ambiguous (2 plans) → exit 1, both IDs listed, no mutation
  • --plan-id unsatisfiable → exit 1; nothing-to-do → exit 0
  • --all --plan-id with zero plans → exit 1, zero API calls
  • apply --no-commitPOST .../apply only, no commit

Also: 1541 unit tests pass, eslint clean, tsc --build clean, and --help renders the new flags with no oclif warnings.

@martzoukos martzoukos requested a review from thebiglabasky July 15, 2026 09:32
@martzoukos martzoukos dismissed thebiglabasky’s stale review July 15, 2026 14:01

Applied review fixes.

This branch routes `import commit` and `import cancel` through
`confirmOrAbort`, so both now return exit code 2 and a
`confirmation_required` envelope in agent sessions. The protocol reference
listed the confirmation-gated write commands as a closed set —
`incidents create/update/resolve`, `deploy`, `destroy` — which no longer
matches the CLI. Add the two import commands to that list.

Also document the pinning behavior the same commands introduce. Both write
their resolved plan back into the `confirmCommand`, so a bare
`checkly import commit` confirms as:

  checkly import commit --plan-id="abc123" --force

carrying a flag the caller never passed. That is deliberate — it stops the
approved run from re-resolving to a plan the user was never shown — but it
looks like noise to an agent that assumes the confirmCommand only ever
repeats what it typed, and the reference gave it no reason to think
otherwise. Say so explicitly, and say not to strip the flag.

Reference-only change: `skills/checkly/SKILL.md` carries the action list,
not reference bodies, so no `sync:skills` regeneration is needed.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
martzoukos added a commit that referenced this pull request Jul 15, 2026
The confirmation docs promised that the `confirmCommand` is the command you
ran plus `--force`, and that it "echoes back only the flags that were
actually passed". That is true of every command on main today: the two
callers that reshape their preview flags (`members delete`, `members
update`) only drop empty ones, they never add.

It stops being true the moment #1402 lands. `import commit` and `import
cancel` resolve their plan and pin it into the preview, so a bare
`checkly import commit` confirms as:

  checkly import commit --plan-id="abc123" --force

The pin is the right call — it stops the approved run from re-resolving to
a plan the user was never shown — but it makes an absolute claim in this
reference false, and #1402 merges first. Widen the contract to allow a
resolved target, and keep "treat every flag you see there as deliberate",
which was always the operative rule and covers the pinned flag too.

Worded to hold under either merge order: it describes the mechanism without
pointing at the import section that arrives with #1402.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
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