Skip to content

fix(cli): add wheels deploy fetch-secrets/extract-secrets/print-secrets flat aliases (#2697)#2699

Merged
bpamiri merged 4 commits into
developfrom
claude/dreamy-cerf-2a6543
May 15, 2026
Merged

fix(cli): add wheels deploy fetch-secrets/extract-secrets/print-secrets flat aliases (#2697)#2699
bpamiri merged 4 commits into
developfrom
claude/dreamy-cerf-2a6543

Conversation

@bpamiri
Copy link
Copy Markdown
Collaborator

@bpamiri bpamiri commented May 15, 2026

Summary

Fixes #2697wheels deploy secrets <verb> (fetch/extract/print) never reaches the deploy dispatcher because LuCLI's picocli root registers secrets as its own top-level subcommand (the local credential store: init/set/list/rm/get/provider) and intercepts the token before Module.cfc::deploy() can dispatch on it.

This is the same picocli command-shadowing pattern documented as gotcha #7 in CLAUDE.md (the server collision, tracked as #2677 and fixed in #2690).

Approach

Mirror the #2690 flat-alias pattern. Add three top-level cases to the deploy() switch:

  • wheels deploy fetch-secrets [KEY...]DeploySecretsCli.fetch
  • wheels deploy extract-secrets [KEY]DeploySecretsCli.extract
  • wheels deploy print-secretsDeploySecretsCli.print

The existing case "secrets": branch is retained unchanged for MCP and programmatic callers (same treatment as case "server":).

Verb-noun naming (vs. bare fetch/extract/print) for namespace safety — fetch and print are generic enough that future deploy subcommands could legitimately claim them.

Changes

  • cli/lucli/Module.cfc — three new case arms in deploy(), just before case "secrets":. ~20 lines including the explanatory docblock.
  • cli/lucli/tests/specs/commands/DeployCommandSpec.cfc — extends the existing #2677 dispatcher regression spec with 7 new it blocks covering all three flat aliases (positive routing, error propagation, and a regression guard for the retained secrets <verb> branch).
  • CLAUDE.md — new gotcha adding better description for hasManyCheckBox #8 alongside the existing New master - readme #7 (server collision); secrets quick-reference promotes flat aliases as canonical.
  • v4-0-1-snapshot guides — three new doc pages (fetch-secrets.mdx, extract-secrets.mdx, print-secrets.mdx), :::caution blocks on the four nested-form pages pointing to the flat aliases, plus updated deployment/secrets.mdx, deployment/hooks.mdx, deployment/migrating-from-kamal.mdx, and command-line-tools/commands/deploy/index.mdx.
  • v4-0-0 guides — left untouched (frozen).
  • CHANGELOG.md[Unreleased] § Fixed entry alongside the existing #2677 line.
  • docs/superpowers/specs/2026-05-15-deploy-secrets-flat-aliases-design.md — design spec.

Test plan

  • Static trace verification of dispatch paths (input arg array → $deployStripFlags output → opts struct → CLI method invocation). All seven dispatcher tests pass under static analysis.
  • CI: compat-matrix + CLI test bundle. Local execution skipped due to a worktree-server conflict (another worktree is currently holding the wheels server registration on this machine; the wheels start CLI doesn't expose a name override to coexist). The test logic is mechanical (toThrow(regex=...) and toBe(...) assertions on a Module.deploy() dispatcher that previously throws Unknown deploy subcommand: <verb> and now routes correctly), so CI is the authoritative gate.
  • Manual smoke: wheels deploy fetch-secrets --adapter=op --from=op://Test/T DUMMY_KEY on a workstation with op configured.

Follow-up (out of scope)

While reviewing the secrets surface I noticed two pre-existing bugs that I deliberately left alone:

  • wheels deploy secrets extract --key=<name> documents the --key= flag, but DeployArgsParser has no parser arm for it — the flag gets stripped silently and the key never makes it into opts. The dispatcher's positional-arg fallback covers the path my fix uses; the documented flag form remains broken.
  • wheels deploy secrets print --project-root=<path> is similarly documented but unparsed.

These are tiny parser additions; I'll spin them off as their own issue if no one else gets there first.

bpamiri added 2 commits May 15, 2026 08:34
Mirrors the #2690 bootstrap/exec flat-alias pattern. Picocli registers
`secrets` as a top-level command (init/set/list/rm/get/provider) and
intercepts it before the deploy() switch can dispatch. Plan: add
`fetch-secrets` / `extract-secrets` / `print-secrets` top-level
cases, retain nested `case "secrets":` for MCP / programmatic callers.
…secrets` flat aliases for #2697

LuCLI's picocli root registers `secrets` as a top-level subcommand for
its own local credential store (init/set/list/rm/get/provider), so the
nested `wheels deploy secrets <verb>` form is shortcut into LuCLI's
own secrets help before module dispatch sees it. The deploy() switch's
`case "secrets":` branch never fires through the CLI — only through
MCP/direct invocation. Same shape as #2677 (`server` collision), fixed
identically in #2690 with flat `bootstrap`/`exec` aliases.

Add top-level `fetch-secrets`, `extract-secrets`, and `print-secrets`
cases to deploy() that map to DeploySecretsCli, sidestepping the
`secrets` collision entirely. The legacy `case "secrets":` branch is
retained for Kamal parity and programmatic callers.

Updates docs (CLAUDE.md gotcha #8, v4-0-1-snapshot guides) to recommend
the flat aliases as the canonical CLI form, with the nested form marked
legacy plus a note explaining the collision. Adds three new
v4-0-1-snapshot doc pages for the flat aliases, mirroring the existing
`bootstrap.mdx`/`exec.mdx` structure from #2690. Adds dispatcher unit
specs in DeployCommandSpec.cfc exercising all three flat aliases plus
a regression guard for the retained `secrets <verb>` branch.

Spec self-review note: `--key=` (extract) and `--project-root=`
(print) flags are documented in the existing nested-form docs but
aren't actually parsed by `DeployArgsParser` — pre-existing bugs,
out of scope for this PR. Filed as follow-up.
@github-actions github-actions Bot added the docs label May 15, 2026
Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR: This PR correctly applies the flat-alias pattern from #2690 to the secrets picocli collision. The core change — three new case arms in Module.cfc::deploy() — is mechanically sound, the tests exercise the routing logic faithfully, and the documentation is thorough. No correctness, cross-engine, or security blockers. One commit message nit and a minor test coverage observation below.


Commits

6439e6440 docs(spec): design doc for ##2697 deploy secrets flat aliases

The ##2697 in the subject is a CFML escape convention that leaked into a git commit message. CFML requires ## to emit a literal # in string output, but commit messages are not CFML — GitHub sees ##2697 literally and will not auto-link it to issue #2697. The body of the same commit correctly uses #2697. The fix-commit subject (7c3391604) is clean.

# current (broken issue reference)
docs(spec): design doc for ##2697 deploy secrets flat aliases

# should be
docs(spec): design doc for #2697 deploy secrets flat aliases

No commitlint rule fires here (type, scope, length, and case are all fine), so this is a pure readability/linkability nit.


Tests

cli/lucli/tests/specs/commands/DeployCommandSpec.cfc lines 88–172

The seven new it blocks cover the three flat aliases and the legacy regression guard. The routing logic is correct on all paths I could trace:

  • fetch-secrets with an unknown adapter: $deployStripFlags removes --adapter=..., leaving positional = ["fetch-secrets", "K1"]; opts.adapter is set by $deployArgsToOptions; the dispatch reaches DeploySecretsCli.fetch, which throws UnknownAdapter before shelling. The toThrow(regex="Unknown adapter") expectation will fire. ✓
  • fetch-secrets with no keys: positional = ["fetch-secrets"]; loop body (fsi=2 to arrayLen=1) never executes; opts.keys = []; fetch() throws NoKeys. ✓
  • extract-secrets A --from=A=hello: positional = ["extract-secrets", "A"]; opts.key = "A", opts.from = "A=hello"; pure string scan in extract() returns "hello". ✓
  • print-secrets in a temp project with no .kamal/secrets: SecretResolver.all() returns an empty struct; arrayToList([], chr(10)) returns ""; toBeTypeOf("string") passes. ✓
  • Legacy secrets extract A --from=A=legacy: positional = ["secrets", "extract", "A"]; opts.from = "A=legacy" from opts; extract returns "legacy". ✓

One observation: neither fetch-secrets test verifies that multiple positional keys are all appended to opts.keys. The first test passes one key (K1) but throws on the adapter before the key is consumed; the second test passes zero keys. This is impractical to cover without a real adapter CLI, and the loop logic (for fsi = 2 to arrayLen(positional)) is identical to the exec case at line 1921 that already has production coverage. Noting it for completeness — not a blocker.

CI-only gate (PR checklist item left [ ]): The author explains a worktree-server conflict prevented local execution. The test logic is mechanical, follows the established DeployCommandSpec pattern, and CI is the nominated gate. Acceptable justification; flagging it so a human reviewer can confirm CI passes before merge.


Correctness

The positional-argument boundary in the fetch-secrets loop (fsi = 2 to arrayLen(positional)) correctly skips positional[1] ("fetch-secrets" itself) and collects all remaining positional tokens as keys — the same slice logic used by the existing exec loop at line 1921.

The extract-secrets ternary at line 1983:

opts.key = arrayLen(positional) >= 2 ? positional[2] : "";

handles the missing-key case cleanly and matches the secrets extract branch at line 2002. No off-by-one.

The secrets branch is correctly retained unchanged (lines 1989–2005), keeping the MCP / programmatic path working. The regression guard test confirms this.


Docs

Documentation changes are consistent and complete: caution blocks on all four nested-form pages, three new canonical-alias pages (fetch-secrets.mdx, extract-secrets.mdx, print-secrets.mdx), updated index.mdx <CardGrid>, deployment/secrets.mdx, hooks.mdx, and migrating-from-kamal.mdx. CLAUDE.md subcommands block and gotcha #8 both accurately describe the collision and the fix. v4-0-0 is correctly left untouched.


@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 15, 2026

Wheels Bot — Reviewer B (round 1)

A's review checks out. The ##2697 commit-message nit is a genuine, correctly-scoped finding; the test-trace claims are accurate against the diff; no false positives; no missed issues worth blocking on. A submitted as COMMENTED rather than APPROVE, which is consistent with finding only a non-blocking nit — I agree with both the finding and the soft landing.

Sycophancy

None detected. A identified a concrete issue (the commit subject escaping CFML convention leaking into git), provided a before/after fix, and explicitly called out the CI-only gate. No empty "looks good" claims.

False positives

None detected.

  • ##2697 commit nit: Accurate. Commit 6439e6440's subject contains ##2697 — a CFML escape for # that has no meaning in a git commit message and breaks GitHub issue auto-linking. The diff confirms the body of that commit correctly uses #2697, and the fix-commit subject (7c3391604) is clean. The ##2697 strings that appear in DeployCommandSpec.cfc lines 93 and 115 (inside CFML describe() string literals) are a different matter — those are correct CFML escaping and A did not flag them, which is the right call.
  • Dispatch trace claims: Verified against the Module.cfc diff. $deployStripFlags produces the flag-free positional array; $deployArgsToOptions populates opts; the three new case arms read from both correctly. The boundary arithmetic (fsi = 2 to arrayLen(positional)) matches the existing exec loop at line 1921 and is off-by-one-free.
  • extract-secrets ternary (arrayLen(positional) >= 2 ? positional[2] : ""): A called this correct and it is — matches the parallel secrets extract branch.

Missed issues

None detected.

The diff is well-scoped: three case arms in Module.cfc, seven it blocks in DeployCommandSpec.cfc, CLAUDE.md gotcha entry and subcommands table, v4-0-1-snapshot docs and legacy caution blocks. No cross-engine APIs introduced (all CFML primitives: arrayLen, arrayAppend, for/to loop, ternary). No security surface opened. Pre-existing parser bugs (--key= and --project-root= not parsed by DeployArgsParser) are correctly declared out of scope in the PR body and do not affect the new aliases. No .ai/wheels/ update needed — the deploy section lives in docs/ and CLAUDE.md, both of which are updated.

The only coverage gap A notes (multiple positional keys not verified end-to-end) is correctly assessed as non-blocking given the loop is mechanically identical to the tested exec case.

Verdict alignment

A's COMMENTED state is consistent with the findings: one non-blocking nit on a commit already in history (which can't be amended without a force-push), one minor coverage observation. No formal REQUEST_CHANGES warranted.

Convergence

Aligned. A's analysis is accurate and the findings are non-blocking. The commit message typo is already baked into history; it affects only GitHub's auto-link from that one commit, not the PR title, body, CHANGELOG entry, or any code path. The CI gate is the correct nominee for final validation given the mechanical nature of the tests. Joint recommendation: approve and merge after CI passes.

Addresses Reviewer A's coverage observation on #2699: the existing
`fetch-secrets` tests pass either one positional key (throws on
adapter check) or zero (throws on NoKeys), so the multi-key path
through the `for fsi=2 to arrayLen(positional)` loop wasn't directly
exercised.

The new `it` block passes three positional keys + an unknown adapter.
A's observation that this can't verify the slice end-to-end without a
real adapter CLI is still correct — the adapter check fires first, so
opts.keys content isn't asserted — but the loop now completes with a
non-trivial positional list and any off-by-one regression there would
manifest as an index-out-of-range, not UnknownAdapter. Mirror coverage
to the existing `exec` smoke at line 43.

No production-code change. Behavior under test is unchanged.
@bpamiri
Copy link
Copy Markdown
Collaborator Author

bpamiri commented May 15, 2026

Author response to Reviewer A & B

Both reviews agreed: approve and merge after CI passes. Three threads of feedback:

1. ##2697 in commit 6439e6440 subject (Reviewer A)

Acknowledged, not fixed. The ## is a CFML escape that has no meaning in a git commit message and breaks GitHub's auto-link from that specific commit. Reviewer B independently noted this isn't worth a force-push to fix — I agree. The PR title, body, CHANGELOG entry, and every other in-history reference uses the correct #2697 form. Affects only the auto-link from one historical commit subject; semantic content is intact.

2. Multi-key fetch-secrets coverage (Reviewer A)

Addressed in fe834a8ac. Added a "boundary smoke" it block that drives three positional keys + an unknown adapter through the dispatcher. A's caveat is preserved in the docblock: the adapter check fires before the keys check, so this doesn't verify the slice landed in opts.keys end-to-end (would need a real adapter CLI for that). What the new test does cover: the for fsi=2 to arrayLen(positional) loop completes without crashing on a non-trivial positional list — any off-by-one would surface as an index-out-of-range rather than UnknownAdapter. Mirrors the existing exec smoke pattern at line 43.

3. Visual regression CI failure

Pre-existing on develop, not introduced by this PR. The Deploy static sites run on develop after PR #2695 shows the same [blog] FAIL — 43,560 pixels differ against /. Introduced by #2694 (blog API rate-limiting), baselines weren't refreshed. This PR doesn't touch web/blog, doesn't change any other site's baseline-tracked surface, and adds three brand-new doc pages under v4-0-1-snapshot/command-line-tools/commands/deploy/ (paths the visual-regression script doesn't screenshot). The blog baseline refresh is a separate-PR concern.

Ready for merge when CI is green ignoring the pre-existing visual regression.

Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR: The new commit (fe834a8ac) directly addresses the multi-key boundary gap I flagged in round one. No production code changed; the addition is one targeted it block that exercises the for fsi=2 to arrayLen(positional) loop with a three-key positional list, consistent with the exec smoke pattern at line 43. No correctness, cross-engine, or security findings on the new diff. Two minor nits below; neither blocks merge.


Commits

fe834a8ac test(cli): boundary smoke for multi-key fetch-secrets dispatch

Type (test), scope (cli), subject, and length are all clean.

The commit message body uses #2699 correctly — single #, will auto-link. ✓

6439e6440 docs(spec): design doc for ##2697 deploy secrets flat aliases

Pre-existing nit flagged in round one: ##2697 in the subject doesn't auto-link on GitHub. This commit is not part of the new diff and fixing it would require a force-push — no action expected.


Tests

cli/lucli/tests/specs/commands/DeployCommandSpec.cfc lines 113–125 (the new it block):

mod.__arguments = ["fetch-secrets", "K1", "K2", "K3", "--adapter=nope-not-a-real-adapter"];
expect(() => mod.deploy()).toThrow(regex="Unknown adapter");

This covers the gap I noted: an off-by-one in the for fsi = 2 to arrayLen(positional) loop would surface as an index-out-of-range before the adapter check fires, not as UnknownAdapter. Three keys is sufficient to distinguish the two failure modes. The limitation I noted (can't assert opts.keys content without a real adapter CLI) is correctly documented inline and is not a blocker.


Nit

cli/lucli/tests/specs/commands/DeployCommandSpec.cfc line 121 — inline comment:

// ...see Reviewer A on PR ##2699). What it does prove...

The ##2699 is inside a // comment, which CFML does not evaluate, so it causes no compilation error. But // comments are not CFML output — the ## escape is unnecessary and slightly misleading. A single #2699 is fine here (and would also auto-link if this ever appeared in rendered output). Trivial; not worth a follow-up PR.


Trivial follow-up to Reviewer A round 2 on #2699. `##` is CFML's
escape for emitting a literal `#` in evaluated output, but `//` line
comments are not evaluated — so `##2699` inside a `//` comment is
just noise. Single `#2699` is correct (and would auto-link if the
comment ever ended up in rendered output).

No behavior change.
@bpamiri
Copy link
Copy Markdown
Collaborator Author

bpamiri commented May 15, 2026

Author response to Reviewer A round 2

The `##2699` is inside a `//` comment, which CFML does not evaluate, so it causes no compilation error. But `//` comments are not CFML output — the `##` escape is unnecessary and slightly misleading.

Fixed in c3fb9a8ed. Agreed — the test docstrings in this file got mixed because the same describe() string literals legitimately need the ## escape (per the CLAUDE.md gotcha that bit #2690's first commit), but // line comments don't, so the ## is just noise there.

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 15, 2026

Wheels Bot — Reviewer B (round 1)

A's review of the fe834a8ac boundary smoke commit is accurate and well-scoped. No sycophancy, no false positives, no missed issues. The ##2699 in a // comment nit is correct, trivial, and correctly categorized as such.

Sycophancy

None detected. A did not offer empty "LGTM" language. The verdict is COMMENTED (not APPROVE), which is consistent with two non-blocking nits rather than a clean slate. A verified the loop arithmetic explicitly and cited line numbers.

False positives

None detected.

  • "No production code changed": Accurate. The fe834a8ac diff touches only DeployCommandSpec.cfc — the three new case arms in Module.cfc and all other changes belong to prior commits already reviewed in round 1 on the old SHA.
  • Boundary smoke coverage claim: Accurate. An off-by-one in the for fsi=2 to arrayLen(positional) loop would throw a CFML index-out-of-range before reaching UnknownAdapter. Three positional keys correctly distinguishes the two failure modes.
  • ##2699 in the // comment (line 121 of DeployCommandSpec.cfc): Accurate. CFML does not evaluate expression delimiters inside // line comments, so the ## escape is unnecessary. A correctly notes it causes no compilation error and is not worth a follow-up PR. The reasoning note that #2699 "would auto-link if this ever appeared in rendered output" is technically moot (a // comment never renders), but the conclusion is correct regardless.
  • Pre-existing ##2697 in commit 6439e6440 subject: A reaffirms the prior-round finding that it cannot be fixed without a force-push. Correct, not a new finding.

Missed issues

None detected.

The diff is test-only. No cross-engine APIs introduced — for / to, arrayLen, and arrayAppend are CFML primitives consistent across Lucee, Adobe, and BoxLang. No security surface opened. No .ai/wheels/ or CLAUDE.md update needed.

Verdict alignment

COMMENTED is consistent with two trivial, non-blocking nits on a test-only commit. APPROVE would also have been defensible; COMMENTED is the more conservative landing and is not wrong.

Convergence

Aligned. A's analysis is accurate on every claim I verified against the diff. Both nits are non-blocking: one is already baked into history, the other is a harmless ## in a // comment. No changes needed before merge. Joint recommendation: approve and merge after CI passes.

Copy link
Copy Markdown
Contributor

@wheels-bot wheels-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

TL;DR: Round 3 diff is a single-character style cleanup — ##2699#2699 inside a // comment in DeployCommandSpec.cfc line 121. This is exactly the nit I flagged in round 2. No production code changed, no behavior change, no new risk surface. Approving.


Commits

c3fb9a8ed style(cli): drop unnecessary ## escape inside a // comment

Type (style), scope (cli), subject length, and case are all clean. The commit body correctly uses a single #2699 (will auto-link) and explains the reasoning accurately — ## is the CFML escape for evaluated string output; // comments are not evaluated, so the escape was pure noise.

The one remaining pre-existing nit — ##2697 in the subject of commit 6439e6440 — was acknowledged in round 1 as not worth a force-push and is unchanged. No further action expected.


All prior findings have been addressed. The overall change (three flat-alias case arms in Module.cfc::deploy(), full test coverage, complete docs) is clean.

@wheels-bot
Copy link
Copy Markdown
Contributor

wheels-bot Bot commented May 15, 2026

Wheels Bot — Reviewer B (round 1)

A's APPROVE on the c3fb9a8 commit is accurate and well-grounded. The diff is literally one character changed in a // comment — ##2699#2699 — and A's characterization matches the git show output exactly. No sycophancy, no false positives, nothing missed.

Sycophancy

None detected. A substantiated the approval: named the file and the change, confirmed it resolves the round-2 nit, verified no production code was touched, and explicitly noted the pre-existing ##2697-in-commit-subject issue remains unchanged and intentionally unaddressed.

False positives

None detected.

  • "No production code changed": Accurate — git show c3fb9a8 shows only cli/lucli/tests/specs/commands/DeployCommandSpec.cfc modified, one line.
  • Commit metadata (type/scope/subject): Accurate — style(cli): drop unnecessary ## escape inside a // comment conforms to commitlint: valid type, optional scope, subject is sentence-case, header is well under 100 chars.
  • ##2697 nit on 6439e6440: A correctly restates the prior-round finding and correctly notes no action is expected.

Missed issues

None detected. One-character test comment edit — no cross-engine surface, no security implications, no behavior change. .ai/wheels/ and CLAUDE.md updates are not needed.

Verdict alignment

APPROVED is correct. A flagged this exact character in round 2; the author fixed it in c3fb9a8. Approving a one-line style fix that resolves your own prior finding is precisely what APPROVE is for.

Convergence

Aligned. A's analysis is accurate on every claim I verified against the diff. The PR is review-clean on this SHA — all substantive findings from prior rounds have been addressed, the outstanding ##2697 commit-subject nit is correctly left as-is (not worth a force-push). Joint recommendation: merge after CI passes.

@bpamiri bpamiri merged commit 8c35d8d into develop May 15, 2026
13 of 14 checks passed
@bpamiri bpamiri deleted the claude/dreamy-cerf-2a6543 branch May 15, 2026 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wheels deploy secrets fetch invokes LuCLI global secrets command instead of Wheels deploy subcommand

1 participant