Skip to content

fix(sync): coerce absent dry-run flag to bool in task SG ingress step#93

Merged
stevethomas merged 1 commit into
mainfrom
fix/sync-task-sg-dryrun-null
Jun 5, 2026
Merged

fix(sync): coerce absent dry-run flag to bool in task SG ingress step#93
stevethomas merged 1 commit into
mainfrom
fix/sync-task-sg-dryrun-null

Conversation

@stevethomas
Copy link
Copy Markdown
Member

Hey, I made a thing! 🥳

Great! Now please answer the following questions to help out your assigned reviewer:

What problems are you solving?

  • yolo sync <env> fatal'd mid-apply with a TypeError at the task security group step on a green-field app (hit on the convictrecords CON-538 cutover).
  • Root cause: dropping the --dry-run CLI option (aafd817) removed the dry-run key from the options array entirely. The plan pass still injects dry-run => true, but the apply pass now carries no key at all, so Arr::get($options, 'dry-run') returns null instead of the old false. Every sibling step casts (bool); SyncTaskSecurityGroupStep was the lone outlier feeding the raw nullable straight into a bool-typed param.
  • Fix: (bool) cast at the call site — restores the null → false behaviour and matches the prevailing idiom across the sync steps.
  • Added SyncTaskSecurityGroupStepTest covering the apply pass (no dry-run key — the regression), the already-authorised no-op, the manifest custom-managed branch, and the dry-run no-write.

Is there anything the reviewer needs to know to deploy this?

  • Only the apply pass was affected (the plan pass injects dry-run => true, so previews always rendered fine) — which is why the crash hit after the confirm gate, not before. Nothing was written before the gate.
  • Grepped every dry-run reference in src/: this was the only strict-typed site receiving the raw nullable, so there's no second landmine from aafd817.
  • Resuming a partially-applied sync is safe: it's create-or-sync idempotent, so already-created resources sync and the rest create. The task SG that was created before the crash picks up its missing load-balancer ingress rule on the next run.
  • 610 Pest green, pint clean. Pure bugfix — no command/manifest/scope-model surface change, so no docs update.

🤖 Generated with Claude Code

Dropping the `--dry-run` CLI option (aafd817) removed the `dry-run` key
from the apply pass options entirely. The plan pass still injects it as
`true`, but on apply `Arr::get($options, 'dry-run')` now returns `null`
instead of `false`. Every sibling step casts `(bool)`; this one fed the
raw nullable straight into a `bool`-typed param, so `sync` fatal'd with a
TypeError mid-apply at the task security group step.

Cast at the call site to restore the old `null -> false` behaviour, plus
a regression test covering the apply pass (no `dry-run` key), the
already-authorised no-op, the manifest custom-managed branch, and the
dry-run no-write.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@stevethomas stevethomas merged commit dab2561 into main Jun 5, 2026
5 checks passed
@stevethomas stevethomas deleted the fix/sync-task-sg-dryrun-null branch June 5, 2026 05:12
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.

1 participant