fix(cli): SecretResolver Left(str,0) crash and secret parsing corruption

[bpamiri]

Summary

Fixes the two deploy-secretresolver findings from the 2026-06-09 internal framework review: a Left(str, 0) crash path (Cross-Engine Invariant 8) in the deploy secrets CLI/resolver, and a SecretResolver parsing design that silently dropped or corrupted secrets. The resolver no longer subtracts a baseline environment snapshot; it scans candidate keys from the secrets file itself, reads each declared key back from the sourced bash as KEY<US>VALUE<RS> records, and throws typed errors (SecretResolver.BashUnavailable, SecretResolver.ResolutionFailed) instead of silently yielding zero secrets.

Findings addressed

• DEP-8 — Left(str, 0) crash on keyless = lines @ cli/lucli/services/deploy/cli/DeploySecretsCli.cfc:68 — extract() now requires eq > 1 so a line starting with = cannot reach left(line, 0), which crashes Lucee 7. The two equivalent sites in cli/lucli/services/deploy/lib/SecretResolver.cfc are removed entirely by the parser rewrite; the new $parseRecords() guards with sep > 1.
• DEP-9 — dropped and corrupted secrets @ cli/lucli/services/deploy/lib/SecretResolver.cfc:71-160 ($resolveFile / $candidateKeys / $parseRecords / $runBash):
  • Keys that also exist in the parent environment (user export, CI var) now resolve to the file's value instead of being dropped — registry login no longer proceeds with an empty password.
  • Multi-line secrets (TLS certs, SSH keys) survive intact via US/RS record delimiters instead of newline-split env diffing; base64 continuation lines are filtered by a bash ${!k+x} set-check so they are no longer misparsed as keys. (NUL records were the first choice, but Lucee's chr(0) yields an empty string, so RS chr(30) is used.)
  • Bash failures surface as SecretResolver.BashUnavailable (unlaunchable bash) or SecretResolver.ResolutionFailed (non-zero exit, with stderr) instead of returning an empty struct.
  • arrayContainsNoCase() (avoided elsewhere in the deploy tree for engine availability) removed; candidates are deduped case-sensitively, matching bash's case-sensitive resolution.

Candidate keys are regex-restricted to [A-Za-z_][A-Za-z0-9_]* before interpolation into the bash script (no command injection); the file path remains single-quote-escaped.

Findings verified already-fixed

None — staleRefs is empty for this package. Both develop baselines (the eq < 1 / eq > 0 sites in SecretResolver.cfc and the DeploySecretsCli.cfc:68 site) were confirmed present pre-fix.

Source

Internal multi-agent framework review 2026-06-09, wave 2, package deploy-secretresolver.

Tests

• cli/lucli/tests/specs/deploy/lib/SecretResolverSpec.cfc — four new cases: parent-env-overridden key resolves to file value; multi-line cert value survives intact; base64 =-padded continuation line not misparsed as a key; export-prefixed declarations resolve.
• cli/lucli/tests/specs/deploy/cli/DeploySecretsCliSpec.cfc — one new case: extract() ignores a keyless line starting with = (the Left(str, 0) guard).
• Four of the five specs fail against the pre-fix code; the export-form spec was already green pre-fix (the old set -a/source/env flow handled export form) and is kept as regression coverage for the new candidate scanner.
• Local verification: the record protocol was exercised on macOS bash 3.2 and the candidate-scan/record-parse logic run red→green on the bundled Lucee (LuCLI 0.3.17) in script mode — the pre-fix code crashes with parameter 2 of the function left can not be 0. These are CLI specs (not core-framework specs), so the worktree docker single-bundle recipe doesn't apply; the full CLI suite runs in CI (test-cli), which is the real gate.

Cross-engine notes

• Invariant 8 (Left(str, 0) crashes Lucee 7) is the subject of DEP-8; all new code guards length before left() (eq > 1, sep > 1). No new unguarded Left(str, N) sites.
• RS chr(30) / US chr(31) record delimiters chosen over NUL because Lucee's chr(0) yields an empty string.
• No reserved-scope parameter names, no inline closures as constructor named args, no attributeCollection usage, no arrayContainsNoCase (engine availability).
• Specs follow sibling style in cli/lucli/tests/specs/deploy/.

Changelog

Entry deliberately omitted; consolidated at campaign end.

🤖 Generated with Claude Code

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This PR correctly fixes two real bugs in the deploy secrets subsystem — the Left(str,0) Lucee 7 crash (Cross-Engine Invariant 8) and the baseline-subtraction design that silently dropped keys shared with the parent environment. The rewritten resolver is architecturally sounder. One correctness issue needs attention before merge: the sequential stdout → stderr stream drain in the new $runBash can deadlock when bash produces enough stderr to fill the OS pipe buffer. Error-path tests for the two new typed exceptions are also missing.

Correctness

Latent deadlock in $runBash — stdout drained before stderr

cli/lucli/services/deploy/lib/SecretResolver.cfc lines 158–160:

var out = $readStream(proc.getInputStream());   // blocks until EOF
var err = $readStream(proc.getErrorStream());   // not started yet
var exitCode = proc.waitFor();

$readStream uses Scanner.useDelimiter("\A"); scanner.next(), which reads the entire stream in one blocking call. If the bash subprocess generates more stderr than the OS pipe buffer (65 536 bytes on Linux, same on macOS) — possible when a secret-manager CLI such as op prints a verbose error on a failed $(op read …) substitution — the process blocks trying to write to stderr while Java blocks waiting for stdout to reach EOF. Neither side can advance: deadlock.

The previous code sidestepped this by setting pb.redirectErrorStream(true), merging the two streams. The new design is architecturally better (separate exit code + stderr for ResolutionFailed), but it must drain both streams concurrently or redirect stderr before starting the process.

Recommended fix — redirect stderr to a temp file before pb.start(), then read it after waitFor():

private struct function $runBash(required string cmd) {
    var errFile = createObject("java", "java.io.File")
        .init(getTempDirectory() & "wheels-secret-err-" & createUUID() & ".tmp");
    var proc = "";
    try {
        var pb = createObject("java", "java.lang.ProcessBuilder").init(["bash", "-c", arguments.cmd]);
        pb.redirectError(errFile);
        proc = pb.start();
    } catch (any e) {
        throw(type="SecretResolver.BashUnavailable", ...);
    }
    var out = $readStream(proc.getInputStream());
    var exitCode = proc.waitFor();
    var err = errFile.exists() ? fileRead(errFile.getPath(), "UTF-8") : "";
    errFile.delete();
    return {exitCode: exitCode, out: out, err: err};
}

This preserves separate stderr capture and eliminates the deadlock path.

Tests

Two new typed exceptions — SecretResolver.BashUnavailable and SecretResolver.ResolutionFailed — are introduced with no test coverage.

• BashUnavailable: exercisable via a subclass-stub that overrides $runBash to throw, or by asserting the typed exception propagates correctly.
• ResolutionFailed: directly testable by writing a secrets file that contains a failing subshell, e.g. BROKEN=$(exit 1), and asserting the exception type. This is the path that fires on real credential-manager failures — the most important error path to cover.

Without at least a ResolutionFailed test, a regression (e.g. accidentally dropping the result.exitCode != 0 branch in a future refactor) would go undetected.

Positive observations

• DEP-8 fix (DeploySecretsCli.cfc:70, $parseRecords sep > 1 guard): both sites correctly guard Left(str, N) with N > 0. The sep <= 1 skip in $parseRecords is tight and clearly annotated with the invariant reference.
• Candidate-key injection (SecretResolver.cfc:86-90): restricting via reFind("^[ \t]*(export[ \t]+)?([A-Za-z_][A-Za-z0-9_]*]=", ...) before interpolating into the bash loop body is correct — validated identifiers cannot carry shell metacharacters.
• chr(30) / chr(31) delimiter choice: rationale (Lucee chr(0) yields empty string) is documented and sound.
• ${!__wheels_key+x} set-check: correctly filters false candidates (base64 continuation lines) without dropping empty-value keys — the has("PRESENT") test would catch a regression here.
• arrayContainsNoCase removal: correct; case-sensitive arrayFind matches bash's case-sensitive variable namespace.
• Commit: fix(cli): SecretResolver Left(str,0) crash and secret parsing corruption — valid conventional-commit header (70 chars), DCO sign-off present, body explains the why clearly.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is technically sound. Both findings are real, the verdict is consistent with the evidence, and no sycophancy or false positives are present. Aligned on request-changes.

Sycophancy

None detected. A gave CHANGES_REQUESTED with evidence-based, specific findings — no unearned LGTM.

False positives

None detected.

• Deadlock claim (new $runBash, stdout before stderr drain): the diff confirms the exact pattern A described. $readStream uses Scanner.useDelimiter("\A"); scanner.next() — a single blocking read to EOF — and stdout is fully drained before stderr reading begins. If bash fills the stderr OS pipe buffer (~64 KB) before writing EOF on stdout, the subprocess stalls on the stderr write while the JVM stalls on the stdout read. Classic deadlock. A's diagnosis and the pb.redirectError(errFile) fix are both correct.
• Missing error-path tests: the diff adds four SecretResolverSpec cases and one DeploySecretsCliSpec case, none of which exercise the BashUnavailable or ResolutionFailed throw paths. A's claim is accurate.

The regex A quotes ([A-Za-z0-9_]*]) has a stray ] not present in the actual code ([A-Za-z0-9_]*), but this is a transcription slip in A's review prose — the underlying injection-safety assessment is correct.

Missed issues

None material. One minor point A did not raise: the suggested temp-file fix should wrap errFile.delete() in a try/finally block to guarantee cleanup when proc.waitFor() throws an InterruptedException. Not a blocker, but worth noting when applying the fix.

Verdict alignment

CHANGES_REQUESTED is consistent with the findings: one latent deadlock on a code path that runs on every secrets resolution, and a gap in error-path test coverage for the two new typed exceptions. Both are actionable.

Convergence

Aligned. The two required changes before merge are: (1) fix $runBash to avoid the sequential stdout-then-stderr drain deadlock, and (2) add at least one test each for SecretResolver.BashUnavailable (e.g. bash-not-found stub) and SecretResolver.ResolutionFailed (e.g. a secrets file whose subshell exits non-zero). A's suggested pb.redirectError(errFile) approach is sound; the fix should also guard errFile.delete() in a finally block.

[wheels-bot]

Wheels Bot — Address Review held for human review

The consensus findings touch a sensitive area (cli/lucli/services/deploy/**) and the bot's safety net requires a human in the loop before any code change. The PR's reviewer-feedback exchange is preserved above for context.

For the human implementer, the converged-on changes are:

1. Fix the $runBash stdout-before-stderr deadlock in cli/lucli/services/deploy/lib/SecretResolver.cfc. Redirect stderr to a temp file (pb.redirectError(errFile)) before pb.start(), then read it after proc.waitFor(). Wrap errFile.delete() in a try/finally block so cleanup survives an InterruptedException from waitFor().
2. Add error-path tests for the two new typed exceptions: at least one SecretResolver.BashUnavailable case (bash-not-found stub) and one SecretResolver.ResolutionFailed case (secrets file containing a failing subshell, e.g. BROKEN=$(exit 1)).

[bpamiri]

Addressed the review in f765455:

Deadlock in `` — fixed with the recommended pb.redirectError(File) temp-file sink, read after `waitFor()`; cleanup is in a `finally` block per Reviewer B's note so the file is deleted even when `waitFor()` or the throw paths interrupt. Verified red/green locally on the bundled Lucee: the old sequential drain blocks past 30s on a 70 KB-stderr subprocess; the new shape completes in milliseconds with stdout intact and no `wheels-secret-err*` files left in the temp dir.

Error-path tests — three new specs in SecretResolverSpec.cfc:

• ResolutionFailed on a failing subshell (BROKEN=$(exit 1), the suggested vector)
• ResolutionFailed when the failing command is mid-file
• BashUnavailable via a new opts.bashCmd seam pointing at a nonexistent binary, so the real ProcessBuilder.start() throw site is exercised rather than a stubbed override

One finding surfaced while implementing the suggested ResolutionFailed vector: it didn't fire against the script as merged-candidate — without errexit, bash continues past a failing $(cmd) substitution and exports the key with an empty value (script exit 0), i.e. the silent-empty-credential mode this PR set out to eliminate. The resolution script is now set -ae, so any failing command while sourcing (a not-signed-in op read, a locked bw, …) aborts with a non-zero exit and surfaces as ResolutionFailed with stderr in the detail. Verified on macOS bash 3.2 (single-line, mid-file, and success paths), and all ten pre-existing behaviors re-verified green under errexit.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: The second commit (f765455c9) closes both open items from the round-1 review: the stdout-before-stderr deadlock is fixed by redirecting bash's stderr to a temp file (a file sink never fills, so bash always runs to completion), and the missing error-path tests are now present — BashUnavailable via the new bashCmd opts seam, ResolutionFailed via a $(exit 1) subshell, and a mid-file failure to verify that set -ae (added in this commit) catches non-terminal failures. All correctness findings from round 1 remain addressed. Verdict: approve.

Correctness

Deadlock resolved correctly (SecretResolver.cfc, new $runBash): pb.redirectError(new File(errPath)) replaces the piped getErrorStream(). A file sink has no OS pipe-buffer limit, so the subprocess can always flush stderr without blocking on a full pipe while the JVM blocks reading stdout. Stdout is still read in-memory via $readStream — correct, since the record stream is bounded by the secrets file size. The temp file is deleted in the outer finally block even when the BashUnavailable throw path or waitFor() interrupts the happy path.

set -ae closes the silent-empty-value gap ($resolveFile, line 97): Without -e, BROKEN=$(exit 1) in a sourced secrets file sets BROKEN to the empty string and lets bash exit 0 — the exact silent-credential-leak this PR set out to prevent. Adding errexit causes bash to abort with a non-zero exit on any failing command substitution, which $runBash captures and $resolveFile surfaces as SecretResolver.ResolutionFailed with the stderr detail. The commit message documents macOS bash 3.2 verification, the common macOS CI host.

errPath outside the try/finally: getTempFile() is called before the outer try block, so a failure there (disk-full, permissions) propagates without cleanup. This is intentional and correct — if the file was never created, there is nothing to delete.

Minor: process handle not destroyed on $readStream exception: If scanner.next() throws (JVM/OOM edge case), proc.destroy() is never called. The bash script is a short-lived for-loop so it terminates on its own, but worth noting. Not a blocker — consistent with the rest of the deploy tree and essentially unreachable in practice.

Tests

All three new specs in this commit (SecretResolverSpec.cfc):

• "throws BashUnavailable when bash cannot be launched" — passes a nonexistent path as opts.bashCmd, exercising the real ProcessBuilder throw site rather than a stub.
• "throws ResolutionFailed when a $(cmd) substitution fails" — writes BROKEN=$(exit 1) and asserts the typed exception; directly validates the set -ae change.
• "throws ResolutionFailed when the failing command is mid-file" — BROKEN=$(exit 1)\nGOOD=ok confirms that a non-terminal failure aborts sourcing rather than continuing to the succeeding GOOD line.

Combined with the four specs from the first commit (parent-env override, multi-line cert, export-prefix, =-padded continuation lines), all significant code paths are now exercised.

Commits

Both commits conform to commitlint.config.js: type fix, scope cli, headers within 100 chars, sentence-case. Both carry Signed-off-by trailers matching the commit author email. Commit bodies explain the why (pipe-buffer deadlock mechanics, set -e and bash 3.2 behaviour) clearly.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's approval is well-founded. Both requirements from the prior round converged on SHA 09a2c57 — deadlock fix and error-path test coverage — are correctly verified as addressed. One small framing inaccuracy and one minor omission noted below, neither blocking.

Sycophancy

None detected. A's approval is backed by line-level reasoning for every change: the stderr-to-file redirect mechanics, why file sinks avoid the pipe-buffer deadlock, why set -ae closes the silent-empty-credential gap, and how each of the three new specs exercises a distinct throw path. No unearned LGTM.

One structural note: this PR rewrites the credential-resolution path inside the deploy subsystem — code that, when broken, silently authenticates with empty passwords. The prior-round B comment requested changes; A is now approving. Per the sycophancy checklist for deploy/security subsystems, a human sign-off should precede merge regardless of bot verdict.

False positives

One minor framing issue:

• A says pb.redirectError(new File(errPath)) replaces "the piped getErrorStream()". The pre-PR develop baseline used redirectErrorStream(true) (stderr merged into stdout, not a separate pipe). The first commit introduced the separate stderr pipe that A is describing; the full PR diff (develop -> HEAD) does not show getErrorStream. A is accurately describing the first-commit -> second-commit delta, but a reader looking at the overall PR diff would not find getErrorStream in the old code. Not a false positive in terms of conclusion — the new code is unambiguously better — but the framing is anchored to the inter-commit change rather than the net change from develop.

Missed issues

One low-severity item A did not raise:

• proc.waitFor() has no timeout. If a command in the secrets file blocks waiting for input (e.g. op read prompting for a master password, a locked vault CLI), the CFML request thread hangs indefinitely. set -e only catches non-zero exits — it cannot abort a blocking but not-yet-failed subprocess. The ProcessBuilder API offers waitFor(long, TimeUnit) which would let $runBash surface a ResolutionFailed after a reasonable deadline rather than freezing the deploy thread. Low severity in practice (the secrets file is dev-authored and the typical tools exit non-zero quickly when unauthenticated), but worth a follow-up issue.

The proc.destroy() gap A flagged (no destroy on $readStream exception) is correctly dismissed — bash exits independently when its stdout pipe closes, and the exception path is essentially unreachable under normal JVM operation.

Verdict alignment

APPROVED is consistent with A's findings. The prior round's required changes are addressed: pb.redirectError(new File(errPath)) + finally { fileDelete(errPath) } fixes the deadlock and cleanup, and the three new specs cover BashUnavailable, ResolutionFailed on a terminal failure, and ResolutionFailed on a mid-file failure. No unaddressed gaps remain.

Convergence

Aligned. A correctly verified that both required changes from round 1 are present and correct. My one missed-issue finding (waitFor timeout) is low severity and appropriate for a follow-up issue, not a blocker. The framing inaccuracy about getErrorStream is cosmetic and does not change the technical conclusion. Joint recommendation: approve, with the note that a human reviewer should sign off on this deploy-subsystem credential-handling rewrite before merge.

[bpamiri]

Noted on Reviewer B's round-2 follow-up: proc.waitFor() with no deadline means an interactively-blocking secrets command (e.g. op read prompting for a master password) would hang the resolution thread rather than fail. Agreed it's out of scope here — waitFor(long, TimeUnit) + destroyForcibly() surfacing as ResolutionFailed after a deadline is the obvious shape — and it applies to the whole deploy subprocess surface, so it deserves its own issue rather than a rider on this PR. Human sign-off before merge per B's note is with @bpamiri.