Skip to content

fix(cli): reject multi-colon hosts and dead deploy.yml keys in the deploy Validator - #3144

Merged
bpamiri merged 2 commits into
developfrom
peter/issue-3086-3088-deploy-validator
Jun 12, 2026
Merged

fix(cli): reject multi-colon hosts and dead deploy.yml keys in the deploy Validator#3144
bpamiri merged 2 commits into
developfrom
peter/issue-3086-3088-deploy-validator

Conversation

@bpamiri

@bpamiri bpamiri commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Fixes #3086 and Fixes #3088.

Two deploy Validator.cfc defects in one PR, both verified to still reproduce on current develop (bb98ffecd) before coding.

#3086 — host validator under-counts adjacent colons

$validateHost counted colons via arrayLen(listToArray(host, ":", false, true)) - 1. includeEmptyFields=false collapses adjacent/leading delimiters, so ::1:22 tokenized to ['1','22'] → colonCount 1 → accepted, despite the source comment and config-reference.mdx promising rejection. Red-phase run confirmed: expected '::1:22' to be rejected failed pre-fix.

Fix: count colons directly — len(host) - len(replace(host, ":", "", "all")). [::1]:22, deploy@1.2.3.4:2222, and bare hosts remain accepted; ::1:22, :a:b, a::b, h:1:2 are rejected.

#3088 — allowlist trimmed to implemented keys

Per the issue direction, the top-level allowlist is trimmed to the keys the runtime actually reads, so dead keys now fail loudly with unknown top-level key: '<key>' (allowed keys: …) instead of being accepted-and-ignored. No dead features were implemented.

Kept (each has a Config.cfc accessor with downstream consumers): service, image, servers, registry, builder, env, ssh, proxy, accessories.

Removed, with evidence (grep over cli/lucli/services/deploy/** + Module.cfc; Config.cfc is the only raw-struct reader — no commands//cli/ code touches config.raw() directly):

Removed key Evidence
boot No Config accessor; DeployMainCli.deploy() is a plain sequential host loop (issue cfg-04). All grep hits for "boot" are proxy/accessory boot verbs, not the config key
logging Zero hits outside the allowlist; no --log-driver/--log-opt ever emitted (cfg-06)
retain_containers Zero readers; prune keep-count comes only from the --keep= CLI flag (cfg-08)
minimum_version Zero readers; 99.0.0 accepted silently (cfg-09)
asset_path, require_destination, allow_empty_roles, run_directory, readiness_delay Zero readers outside the allowlist (cfg-10…14)
hooks HookCommands.hookPath() hardcodes .kamal/hooks/; no Config.hooks() accessor — the hooks feature works by convention, only the config key was dead (issue hooks.path row)
healthcheck (top-level) Only proxy.healthcheck is consumed (Proxy.cfc:35-43 reads the proxy sub-block; Config.proxy() is constructed from raw.proxy). The top-level key has zero readers
volumes, labels (top-level) Beyond the issue's table but same defect class: only accessory volumes:/directories: are mounted (Accessory.cfc:46-49AccessoryCommands.cfc:83) and app containers get only the four fixed labels (AppCommands.$labelArgs, lines 87-92). No top-level reader exists for either

Safety checks before trimming:

  • templates/deploy/init/deploy.yml.mustache (the wheels deploy init scaffold) uses only kept keys — fresh scaffolds still validate.
  • The Kamal upstream fixture tests/_fixtures/deploy/configs/full.yml carried readiness_delay: 0; the line is removed with an explanatory comment (intentional divergence: unimplemented Kamal keys now fail loudly).
  • No other spec fixture or in-CLI doc (wheels deploy docs) references a removed key.

Dead sub-keys inside kept blocks (builder.arch/args/remote, ssh.proxy/keys_only, proxy.forward_headers/buffering, role options:/labels:/env:, accessory files:) are out of scope here — the Validator has no sub-key schema, and config-reference.mdx already flags each as parsed-but-ignored under #3088.

Docs

config-reference.mdx is updated in a separate commit so the shipped guide doesn't contradict the validator (issue acceptance (c): "removed from the allowlist and the docs"): the allowlist block now lists the nine implemented keys, the eleven "accepted but currently ignored" sections collapse into one "Rejected Kamal keys" table with migration hints (healthcheckproxy.healthcheck:, retain_containersprune --keep=<n>), and the #3086 known-gap caveats are removed.

Tests (TDD red-first, Lucee 7 Docker harness, /wheels/cli/tests)

  • Baseline (pre-change): 926 pass / 1 fail / 2 error — residuals are the environment-dependent SshClientSpec / SshPoolSpec / ServerCommandsSpec bundles.
  • Red: new specs fail exactly as predicted (::1:22 accepted, boot accepted, no allowed-keys list in the error).
  • Green (post-fix): 931 pass / 1 fail / 2 error — identical residual set to baseline; all 5 new ValidatorSpec cases pass (multi-colon rejection loop, bracketed-IPv6/single-colon acceptance, 13-dead-key rejection loop, every-implemented-key acceptance, allowed-keys-in-error).

🤖 Generated with Claude Code

bpamiri and others added 2 commits June 12, 2026 10:35
…ploy Validator

Two validator defects:

- $validateHost counted colons via listToArray(includeEmptyFields=false),
  which collapses adjacent/leading delimiters, so unbracketed '::1:22'
  under-counted to 1 colon and passed validation (#3086). Count colons
  directly with len()-replace() instead.

- The top-level allowlist accepted 13 keys the runtime never reads
  (boot, healthcheck, hooks, volumes, labels, logging, retain_containers,
  minimum_version, asset_path, require_destination, allow_empty_roles,
  run_directory, readiness_delay), turning documented config into silent
  no-ops (#3088). Trim the allowlist to the keys Config.cfc and its
  consumers actually read and list the allowed keys in the unknown-key
  error so dead keys fail loudly.

The Kamal upstream fixture full.yml drops its readiness_delay line to
match the stricter allowlist.

Signed-off-by: Peter Amiri <peter@alurium.com>
…or allowlist

Replace the per-key 'accepted but currently ignored' sections with a
single Rejected Kamal keys table, shrink the allowlist block to the nine
implemented keys, and update the host-validation and validation-error
copy now that adjacent colons are counted (#3086) and dead keys are
rejected (#3088).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Peter Amiri <peter@alurium.com>
@github-actions github-actions Bot added the docs label Jun 12, 2026

@wheels-bot wheels-bot Bot 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.

Wheels Bot — Reviewer

TL;DR: This PR fixes the deploy Validator.cfc host colon under-count (#3086) and trims the top-level deploy.yml allowlist to the nine keys the runtime actually reads (#3088), with red-first specs, a rewritten config-reference guide, and changelog fragments. I independently re-verified every load-bearing claim in the PR body against the code rather than taking it on faith, and found no correctness, cross-engine, security, convention, or commitlint issues. Verdict: approve.

Correctness

No findings. Verification notes on the two fixes:

  • Colon counting (cli/lucli/services/deploy/config/Validator.cfc:84): len(arguments.host) - len(replace(arguments.host, ":", "", "all")) counts every colon, including adjacent ones the old listToArray(..., false, true) collapsed. I traced the acceptance set by hand: [::1]:22 (bracket prefix short-circuits), deploy@1.2.3.4:2222 (1 colon), and bare hosts (0 colons) all still pass; ::1:22, :a:b, a::b, h:1:2 are now rejected. The empty-string edge is safe — colonCount > 1 short-circuits before left(host, 1). One adjacent tightening worth noting (correct, same defect class): a trailing-colon multi-colon host like a:b: was previously under-counted to 1 and accepted; it is now rejected, which matches the documented rule.
  • Allowlist trim: I confirmed Config.cfc is the only raw-struct reader and its accessors cover exactly the nine kept keys (service/image at Config.cfc:23-27, env:39, builder:46, registry:53, proxy:60, ssh:67, servers:74, accessories:111). Every grep hit for the removed keys is a verb or sub-block reader, not a top-level config read: Proxy.cfc:35-42 reads healthcheck from the proxy sub-block, Accessory.cfc:46-49 reads accessory volumes:, HookCommands.cfc:30 hardcodes .kamal/hooks/, and AppCommands.cfc $labelArgs (lines 87-94) emits only the four fixed labels. The wheels deploy init scaffold (cli/lucli/templates/deploy/init/deploy.yml.mustache) uses only kept keys, so fresh scaffolds still validate.

One non-blocking, pre-existing observation (out of scope for this PR, fine to leave): the bracket exemption checks only the leading character (left(arguments.host, 1) != "[" at Validator.cfc:85), so a malformed bracketed host like [::1:22 (no closing bracket) still passes. That leniency predates this change and is not worsened by it — flagging it only as a possible follow-up.

Tests

cli/lucli/tests/specs/deploy/config/ValidatorSpec.cfc adds five cases covering both rejection loops (multi-colon hosts with per-host failure messages, all 13 dead keys with message assertions), the acceptance paths (bracketed IPv6, single-colon user@host:port, a config exercising every implemented key), and the new allowed-keys-in-error contract. The var state = {threw: false, ...} catch pattern matches Cross-Engine Invariant 11 and the file's existing idiom, and ##3086/##3088 are correctly escaped in comments. The only fixture carrying a removed key was cli/lucli/tests/_fixtures/deploy/configs/full.yml (readiness_delay: 0), now removed with an explanatory comment — I confirmed its two consumers (ConfigLoaderSpec.cfc:34-36, AccessoryCommandsSpec.cfc:39) remain valid and no other fixture references a removed key.

Docs

config-reference.mdx is consistent with the new behavior: the allowlist block lists exactly the nine kept keys, the per-key "accepted but currently ignored" sections collapse into the "Rejected Kamal keys" table with migration hints, and the #3086 known-gap caveats are gone. The #rejected-kamal-keys anchor and the migrating-from-kamal / hooks page links all resolve. I also swept the rest of web/sites/guides/.../deployment/*.mdx and .ai/wheels/deploy.md for stale claims about the removed keys — the only healthcheck: hits elsewhere are docker-compose service healthchecks in docker-deployment.mdx, unrelated to deploy.yml. Changelog fragments (changelog.d/deploy-validator-host-colons.fixed.md, deploy-validator-allowlist-trim.changed.md) use the correct types and there is no direct CHANGELOG.md edit.

Commits

Both commits conform to commitlint.config.js: valid types (fix, docs), headers under 100 chars, sentence-case subjects, bodies explaining the why, and DCO sign-offs matching the author. The behavioral fix and the guide rewrite are cleanly separated into their own commits.

@bpamiri
bpamiri merged commit 5bd8c6b into develop Jun 12, 2026
15 checks passed
@bpamiri
bpamiri deleted the peter/issue-3086-3088-deploy-validator branch June 12, 2026 18:19
bpamiri added a commit that referenced this pull request Jun 12, 2026
…laim from deploy guides (#3151)

Review findings on #3144: the validator trim turned two doc claims false.

- rollback.mdx + the three prune pages still presented retain_containers:
  in deploy.yml as the retention control; adding that key now fails every
  command with 'unknown top-level key'. Reworded all four to the real
  control, the --keep=<n> prune flag (default 5), and documented the flag
  in the prune all / prune containers synopses and flag tables.
- migrating-from-kamal.mdx claimed ERB was the one schema exception. Added
  a 'Divergence 2: rejected top-level keys' section (allowlist, rejected
  Kamal keys, per-key migration hints), a matching intro aside, and a
  remove-rejected-keys step in the switch-over checklist. Aligned the same
  single-divergence wording in deployment/index.mdx, architecture.mdx, and
  config-reference.mdx.

Guides site builds clean (433 pages); new heading anchors verified in the
built HTML.

Signed-off-by: Peter Amiri <peter@alurium.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment