fix: preserve operation dry-run semantics

[chaim0m]

Summary

• defer to an API operation when it defines its own local --dry-run flag
• mark native simulation responses with action.status: "simulated" and dry_run: true
• synthesize an idempotency key when the native simulation requires one
• keep the local no-request preview for operations without native dry-run support
• retain API-specific catalog descriptions and mark required and safety-related flags

Why

Follow-up to Alfredo's review comment on #34. cancel-invite and resend-invite expose server-side simulation, but the generic preview shadowed them; simply deferring made simulated and real responses indistinguishable.

Test methods

go test ./...
go vet ./...
go build -o /tmp/dci-pr43-native-dry-run .
DCI_NO_UPDATE_CHECK=1 /tmp/dci-pr43-native-dry-run cancel-invite codex-dry-run-smoke-test --dry-run --output json
DCI_NO_UPDATE_CHECK=1 /tmp/dci-pr43-native-dry-run resend-invite codex-dry-run-smoke-test --dry-run --output json

Both live API-native simulations completed successfully and returned action.status: "simulated" with dry_run: true; no real invite mutation was requested. Unit tests also verify flag propagation, generated/preserved idempotency keys, the native execution path, and the response marker.

Could this break things?

Risk: low-medium. Only operations declaring their own dryRun parameter send a simulation request. All other commands retain the no-request preview. A real destructive execution still requires explicit confirmation.

Jira

CMP-48647

[apgiorgi]

The mechanism here is sound and I verified the premise: the live spec declares dryRun on exactly two operations — resendInvite (openapi.yaml:3926) and cancelInvite (:3990). restish kebab-cases it (cli/param.go:99, cli/operation.go:147), it lands in sub.Flags() where it shadows dciCmd.PersistentFlags().Bool("dry-run", ...) (main.go:1370), and main.go:1414 therefore binds agent-dry-run from the operation's own flag. The LocalNonPersistentFlags() predicate is the correct discriminator — it includes an operation-local shadow and excludes the inherited root pflag, so non-native commands keep the local preview.

Two things block it.

1. Simulation becomes indistinguishable from a real call.

The early return at destructive_contract.go:131-133 emits no dry-run marker and leaves destructiveActionName empty, so installDestructiveActionSummaryGuard never wraps the response either. openapi.yaml:3993 explicitly says "The response shape is identical to a real execution."

So dci cancel-invite ID --dry-run --idempotency-key K and a real cancel produce byte-identical output. An agent cannot tell a simulation from a commit, and --yes is silently no longer required on either. Whatever we do here has to leave a marker in the output.

2. The docs become false, and the documented invocation now fails.

README.md:127 and skills/dci-cli/SKILL.md:43 both promise --dry-run shows what would happen "without sending the API request" / "without sending its request". That's now untrue for these two operations.

Worse: Idempotency-Key is required: true on both (openapi.yaml:3915, 3979) and restish never auto-supplies it. So an agent following SKILL.md runs dci cancel-invite ID --dry-run and gets an HTTP 400 where main returned exit 0. Needs the doc updates, and ideally a synthesized idempotency key on the dry-run path so the documented invocation actually works.

Nits

• command_catalog.go:233 — the dedup drops the "requires --yes" framing, and since commandCatalogFlag has no Required field, --idempotency-key is still advertised as optional.
• The new test lacks t.Cleanup(resetDestructiveContractState) (the pattern elsewhere in that file) and never asserts that dryRun actually reaches the query string — which is the whole point of the change.

Also worth knowing for sequencing: this early return bypasses ensureDestructiveOperations() entirely, so the caching in #44 gains nothing on this path.

Please run a live read-only check against both operations before this merges.

[chaim0m]

Addressed both blockers and the catalog/test nits. API-native simulations are visibly wrapped as status: simulated with dry_run: true, required idempotency keys are synthesized, caller-supplied keys are preserved, docs distinguish local previews from API simulation requests, and catalog flags retain the API description plus required/safety metadata. Live dry-run checks against both cancel-invite and resend-invite returned simulated markers. go test ./... and go vet ./... pass.

[chaim0m]

All requested dry-run changes are present on current main. The merge conflict with the newly landed response guard was resolved by retaining both behaviors: error bodies bypass the success wrapper, while successful native simulations return status: simulated and dry_run: true. Both live native simulations, go test ./..., and go vet ./... pass. Re-requesting review on the current head.

[apgiorgi]

Re-verified both blocking findings against head 8349f23, end to end over HTTP rather than by reading the diff — an httptest server capturing the wire request, with a real cli.Load of a spec mirroring cancelInvite.

Simulation is now distinguishable. destructive_contract.go:151-159 sets destructiveActionName and destructiveActionDryRun on the native path instead of returning blank, so the summary guard wraps the body with status: "simulated". Observed:

• dry run: query="dryRun=true", body {"action":{"command":"cancel-invite","status":"simulated","dry_run":true},...}
• real run with --yes: no query param, status:"completed"
• real run without --yes: exit 30, zero HTTP requests

So the two are no longer byte-identical and the commit path is still gated. --dry-run bypassing --yes is right — it can't commit. I also probed the root-persistent vs operation-owned --dry-run shadowing (dci --dry-run cancel-invite ID --yes --idempotency-key k) and it still sends dryRun=true; no defect there.

Docs and the 400 are fixed. Both false claims are gone from README.md:127 and skills/dci-cli/SKILL.md:43, and the new text correctly distinguishes local preview from API-native simulation. ensureDryRunIdempotencyKey (destructive_contract.go:189-203) synthesizes dci-dry-run-<32 hex> only when the flag is absent — confirmed on the wire — and an explicit caller key survives. I re-checked the live spec: Idempotency-Key is still required: true on both resendInvite (openapi.yaml:3915) and cancelInvite (:3979), so this is addressing a live constraint, not a stale one. The documented invocation no longer 400s.

commandCatalogFlag.Required and the t.Cleanup nit are both handled. go test ./... and go vet ./... clean.

Approving. Two follow-ups, neither blocking:

1. No regression guard that dryRun reaches the query string. TestDryRunDefersToOperationOwnedFlag (destructive_contract_test.go:143) only inspects a hand-built pflag's Changed/value; nothing exercises the HTTP layer. The behavior is correct today but only a throwaway test proved it — worth landing a real one.
2. requiredOperationFlags (command_catalog.go:59-62) is a hardcoded two-entry allowlist because restish's cli.Param has no Required field. It will drift as the API adds required params, and nothing guards that.

Note this needs @eranchetz as code owner for the skills/ change.