agents: add --revoke-previous to triggers rotate-secret

[sdharavath]

Summary

Replaces the --allow-grace flag this branch previously carried. The server-side default is not changing after all, so the CLI shouldn't imply it does.

rotate-secret keeps its grace window by default: the provider side holds only one secret (a GitHub webhook has a single Secret field), so it keeps signing with the old value until a human pastes the new one in. Revoking on the call would turn every routine rotation into a delivery outage lasting as long as that handoff takes.

--revoke-previous is the breach path instead — it retires the old secret immediately and accepts that gap.

Either way the command now states the outcome: the expiry instant, or that the old secret is already dead. "Rotated" on its own leaves an operator unable to tell whether a leaked secret still works.

$ doctl agents triggers rotate-secret TRIGGER_ID
  Webhook secret ...
  Old secret stops working at: 2026-08-12T12:05:00Z

$ doctl agents triggers rotate-secret TRIGGER_ID --revoke-previous
  Webhook secret ...
  Old secret revoked: deliveries still signed with it will fail.

In -o json, exactly one of previous_secret_expires_at / previous_secret_revoked is present, so scripts never infer the outcome from a missing field.

Depends on

Server-side contract: cthulhu#172453 (MARSOHS-1078), which is additive — ?revoke_previous=true plus previous_secret_revoked in the response. The default response shape is unchanged.

Testing

go build ./..., go vet, and go test ./commands/... ./do/... pass. New TestAgentTriggersRotateSecret_RevokePrevious covers the breach path; the existing rotate tests now assert the expiry is surfaced on the default path.

Made with Cursor

[SSharma-10]

LGTM
Only thing I’d flag: the godo update is currently patched directly in vendor/. Before this goes to feat/agents-subcommands, we should land the change in digitalocean/godo and bump the dependency so we don’t lose it on the next vendor refresh

[sdharavath]

Thanks @SSharma-10 — agreed on the vendor point, and it's now the only thing gating this PR.

Heads up that the change has also changed shape since your review. After thinking through the availability side, flipping the default was the wrong call: the provider side holds only one secret, so it keeps signing with the old value until a human pastes the new one in. Immediate revocation by default would turn every routine rotation into a delivery outage lasting as long as that handoff takes.

So the default stays as-is and breach response is opt-in instead. --allow-grace is gone, replaced by --revoke-previous. Server-side that's ?revoke_previous=true plus a previous_secret_revoked response field — purely additive, no change to the default response shape. Description updated.

On the vendor patch: that means the godo change to land upstream is now revokePrevious rather than allowGrace, plus PreviousSecretRevoked on HostedAgentTriggerRotateSecretResponse. Sequencing I'd propose, unless you'd rather do it differently:

1. cthulhu#172453 merges and fixes the wire contract.
2. PR that exact signature into digitalocean/godo, cut a beta (we're on v1.204.0-beta.4 here).
3. Bump go.mod + re-vendor on this branch and drop the hand-patched vendor/ hunk.
4. Then merge into feat/agents-subcommands.

That keeps the vendor patch from ever reaching the base branch, which is what you were flagging. Happy to open the godo PR as soon as the contract is locked — just didn't want to land a signature upstream that the API PR might still move.

[sdharavath]

@SSharma-10 the godo side is now up: digitalocean/godo#1092, based on OHS_endpoints (that's where the hosted-agents surface lives — it has never been on godo main).

One thing worth a second look, because it changed from what you reviewed here. Upstream I made RotateSecret take an options pointer instead of the bare bool this PR was carrying:

RotateSecret(ctx, triggerID string, opt *HostedAgentTriggerRotateSecretOptions)

Two reasons. It matches how List, ListExecutions, and ListReusableSessions already work in that same file (options struct → addOptions), and nil reads as "server defaults", which is exactly the semantics we want since grace is the default. A bare bool also would have meant a second breaking signature change the next time a knob shows up. Since that surface has only ever shipped on the beta tags, taking the break now is cheap.

I've pushed 0714ec2 here so the vendored copy and the call site match #1092 byte for byte. do/agent_triggers.go absorbs the difference, so doctl's own HostedAgentTriggersService interface, the mocks, and every test are untouched — the only other adjustment was PreviousSecretExpiresAt becoming *godo.Timestamp (nil when revoked) instead of a string.

Sequencing from here, as you called out: #1092 merges → bump the dependency here → drop the vendor patch. I'll do that bump as the last commit on this PR, so nothing merges to feat/agents-subcommands with a hand-edited vendor/.

For completeness on the SDK question: pydo needs nothing. It's AutoRest-generated from digitalocean/openapi, and that spec has no hosted-agents surface at all yet — no /v2/agents/triggers, no rotate-secret. Whenever that surface does get published, revoke_previous comes along with it, since it's already in the trigger.swagger.json the bindings are generated from.

[sdharavath]

Correction to my last comment: pydo did need a change — I was wrong.

I'd only checked the generated seam (the public openapi spec still has no hosted-agents surface, so no Autorest regen is involved) and stopped there. I missed that the triggers surface in pydo is hand-authored in src/pydo/agents/custom_triggers.py on OHS_endpoints — @SSharma-10's #680 — where rotate_secret() took no options and the docstring said the old secret "stays valid briefly", which is the exact wording MARSOHS-1078 was filed about.

Fixed in digitalocean/pydo#714, based on OHS_endpoints so #680 picks it up. Keyword-only revoke_previous=False on both the sync and async clients, plus corrected docstrings. 127 passed / 1 pre-existing skip, black --check clean.

Full set for this ticket, all additive and all defaulting to the existing grace behaviour:

Repo	PR	Base
cthulhu (harness-trigger)	#172453	master
godo	digitalocean/godo#1092	OHS_endpoints
pydo	digitalocean/pydo#714	OHS_endpoints
doctl	this PR	feat/agents-subcommands

[sdharavath]

Note for the latest review pass: this branch is at e25dec3, not 0714ec2 — the sentinel finding was fixed in the commit after the one reviewed.

do/agent_triggers.go no longer infers anything from a missing expiry. It returns a HostedAgentTriggerRotateSecretResult carrying PreviousRevoked read straight from resp.PreviousSecretRevoked, and commands/agent_triggers.go has three branches rather than two:

• revoked → "Old secret revoked: deliveries still signed with it will fail."
• expiry present → "Old secret stops working at: T"
• neither → "Old secret status not reported by the API; assume it is still valid until you can confirm."

The third branch is the point. The server guarantees exactly one field, so "neither" shouldn't happen — but if it does (older server, proxy dropping a field), the reading an operator would act on is the dangerous one, and guessing "revoked" on a live secret is the guess that gets someone hurt. TestAgentTriggersRotateSecret_NeitherOutcomeReported asserts that path never renders as a dead secret.

The matching godo change is digitalocean/godo#1092; this PR needs the dependency bump once that merges.

[sdharavath]

@SSharma-10 re-review request. Your earlier LGTM was on a revision whose default has since been reversed, so it's worth another pass rather than carrying that approval forward.

What changed since: the flag is --revoke-previous (grace stays the default, revocation is opt-in) rather than the earlier --allow-grace, and the service layer reads previous_secret_revoked from the API instead of inferring it from a missing expiry.

Blocked on godo#1092 merging first for the dependency bump.

[SSharma-10]

LGTM