fix(cli): return "unknown" from $gitShortSha() when git exits non-zero#2689
Conversation
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This fix correctly resolves issue #2671 by checking the git process exit code before returning its output as a version string, and adds solid regression tests. The change is safe to merge with a couple of minor nits noted below.
Correctness
Bonus fix in DeployMainCli.cfc: The original code called proc.waitFor() before creating the BufferedReader and reading any output:
// Original DeployMainCli.cfc (pre-fix)
var proc = pb.start();
proc.waitFor(); // <-- blocks waiting for exit BEFORE draining the stream
var reader = createObject("java", "java.io.BufferedReader")...The fix moves waitFor() to after the read loop — the correct ordering. On a process with small output this was never going to deadlock, but it was a latent bug. The PR description doesn't mention this; it's worth calling out as an incidental correctness improvement.
DeployBuildCli.cfc did not have the ordering bug (its waitFor() was already after reading); it only needed the exit-code check. Both copies are now correct and consistent.
Tests
Tests are well-structured: try/finally cleanup, UUID-suffixed temp directories to avoid collisions, and targeting both CLI classes. One minor redundancy:
cli/lucli/tests/specs/deploy/cli/DeployMainCliSpec.cfc, lines 366–367:
expect(sha).toBe("unknown");
expect(findNoCase("fatal", sha)).toBe(0); // always true if sha == "unknown"
expect(findNoCase("not a git repository", sha)).toBe(0); // sameIf expect(...).toBe("unknown") throws on failure (standard in WheelsTest BDD), the two findNoCase assertions are unreachable in the failure case. They read as documentation of the original bug rather than live assertions. They're harmless, but a comment explaining they're intentional smoke-assertions would clarify intent. Not blocking.
Docs
No CHANGELOG.md entry. The [Unreleased] → ### Fixed section in this repo tracks user-visible bug fixes, and the other recent entries in that section are at the same level of impact as this one. A one-liner belongs there:
- `$gitShortSha()` in the deploy CLI no longer leaks git's `"fatal: not a git repository..."` stderr text as the version label when run outside a git repository; it now returns `"unknown"` (#2671)Conventions
$gitShortSha() is changed from private to public in both files. The $ prefix signals "internal helper" and private was the stronger enforcement. The change is justified here (these are standalone CLI service objects, not mixin CFCs where $integrateComponents() behaviour is the concern), and test-driven exposure of a $-prefixed method via public is an established pattern in this codebase. Acceptable.
Commits
fix(cli): return "unknown" from $gitShortSha() when git exits non-zero — valid type, valid scope, ≤ 100 chars, not ALL-CAPS. Passes commitlint.
Reviewer A nits from PR #2689: - Add CHANGELOG entry under [Unreleased] → Fixed - Annotate the belt-and-braces no-leak assertions in DeployMainCliSpec so the intent (documenting the original bug shape) is explicit
Wheels Bot -- Reviewer B (round 1)A's review is accurate and well-evidenced. One missed issue noted below, but it is minor enough that it does not change the overall verdict. Converging on approve. SycophancyNone detected. A's "safe to merge" conclusion is backed by concrete correctness analysis, test inspection, and commitlint verification. False positivesNone detected.
Missed issues
Verdict alignmentA submitted a COMMENTED review (no hard block) and characterized all nits as non-blocking. Given that the fix is correct, the tests are solid, and both nits (redundant assertions, missing changelog entry) are optional improvements, A's functional verdict of approve-with-optional-cleanup is consistent with the findings. ConvergenceAligned. A's substantive analysis is correct, A's verdict is appropriate, and the one item A missed (issue number in the code comment) is a style nit that does not affect correctness or safety. The PR is review-clean for this SHA. |
Reviewer B (PR #2689 round 1) caught a CLAUDE.md style nit: code comments shouldn't reference the originating task/issue since that belongs in the PR description and rots as the codebase evolves. The WHY clause stays.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: Both nits from the previous Reviewer A pass have been addressed — the CHANGELOG.md entry is present and the belt-and-braces test comment now documents intent. The fix itself remains correct and the regression tests are solid. One low-priority commit hygiene note about the follow-up commit's missing DCO trailer; otherwise this is ready.
Commits
8e1abcb5b — authored by Claude <noreply@anthropic.com> — lacks a Signed-off-by: trailer:
docs(cli): note #2671 fix in changelog + clarify regression-test intent
Reviewer A nits from PR #2689:
- Add CHANGELOG entry under [Unreleased] → Fixed
- Annotate the belt-and-braces no-leak assertions in DeployMainCliSpec so
the intent (documenting the original bug shape) is explicit
Per the project's DCO policy (.claude/commands/_shared-rails.md), every bot-authored commit must carry a Signed-off-by: trailer matching the commit author's identity. The DCO GitHub App check is not showing as a required status check in the current CI run, so this may not block merging in practice — but it is a policy violation worth resolving. The simplest fix is to amend the commit to add:
Signed-off-by: Claude <noreply@anthropic.com>
or, if that identity is not a registered GitHub user, have a human committer add a Co-authored-by + their own sign-off and re-push.
The first commit 91a7ad7df (the substantive fix, authored by the human contributor) is not affected — it carries the human author's identity and was already assessed as commitlint-clean in the prior review.
Docs
CHANGELOG entry added as requested, CHANGELOG.md line ~31:
- `$gitShortSha()` in the deploy CLI no longer leaks git's `fatal: not a git repository...` stderr text as the version label when `wheels deploy` is run outside a git repository. Both copies (`DeployMainCli.cfc` and `DeployBuildCli.cfc`) now check the git process exit code and return `"unknown"` on non-zero, matching the existing `catch` fallback. The `DeployMainCli` copy also fixes a latent stream-drain ordering bug where `proc.waitFor()` was called before reading stdout (#2671)Content is accurate and the format is consistent with adjacent entries. No issues.
Tests
DeployMainCliSpec.cfc line ~366 — the clarifying comment is now present:
// Belt-and-braces: explicit no-leak assertions documenting the original bug shape.
expect(findNoCase("fatal", sha)).toBe(0);
expect(findNoCase("not a git repository", sha)).toBe(0);This addresses the prior review's readability nit. No further test issues.
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new commit (0bad272d5) makes a trivially correct style adjustment -- stripping the issue-number trailer from the two $gitShortSha() comments in line with CLAUDE.md's guidance against issue references in code. The substantive fix and tests from the prior two commits remain sound. The only outstanding item is the missing DCO Signed-off-by trailer on both bot-authored commits, which the previous pass already flagged and remains unresolved.
Commits
The new commit adds nothing to address the DCO gap raised in the prior Reviewer A pass. Two commits by Claude <noreply@anthropic.com> are now without a Signed-off-by trailer:
8e1abcb5b--docs(cli): note #2671 fix in changelog + clarify regression-test intent0bad272d5--style(cli): drop issue-number trailer from $gitShortSha() comments
Neither carries:
Signed-off-by: Claude <noreply@anthropic.com>
The fix needed before merge is to amend each commit to append the trailer, or have a human co-committer add their own Signed-off-by. The DCO GitHub App is a required status check -- if it enforces on the Claude identity, these commits will block the merge.
The new commit itself (0bad272d5) is style-clean and commitlint-compliant: type style, scope cli, subject <= 100 chars, not ALL-CAPS.
Outside a git repo, `git rev-parse --short HEAD` writes
"fatal: not a git repository..." to stderr and exits non-zero. Both
copies of $gitShortSha() in the deploy CLI merged stderr into stdout
via redirectErrorStream(true) and ignored the exit code, so the error
text was returned as the version string and rendered in deploy output
("Deployed app version fatal: not a git repository... to 1 host(s)").
Capture the exit code from proc.waitFor() and return "unknown" on
non-zero — matching the existing catch-block fallback. redirectErrorStream
is kept so stderr is still drained (no console leak) but its contents are
discarded. Also accept an optional workingDir so the regression tests can
exercise the non-git path without depending on the JVM CWD.
Fixes #2671
Signed-off-by: Claude <noreply@anthropic.com>
Reviewer A nits from PR #2689: - Add CHANGELOG entry under [Unreleased] → Fixed - Annotate the belt-and-braces no-leak assertions in DeployMainCliSpec so the intent (documenting the original bug shape) is explicit Signed-off-by: Claude <noreply@anthropic.com>
Reviewer B (PR #2689 round 1) caught a CLAUDE.md style nit: code comments shouldn't reference the originating task/issue since that belongs in the PR description and rots as the codebase evolves. The WHY clause stays. Signed-off-by: Claude <noreply@anthropic.com>
bpamiri
force-pushed
the
claude/resolve-issue-2671-OceBD
branch
from
0bad272 to
46edbe1
Compare
Wheels Bot — Reviewer B (round 1)A's review is largely accurate: the CHANGELOG and test-comment verifications are correct, and the DCO finding on SycophancyNone detected. A's "ready" characterization is backed by explicit verification of the CHANGELOG content and the added clarifying comment — not a blanket LGTM. False positivesNone detected.
Missed issues
Verdict alignmentA submitted a COMMENTED review and characterized the DCO gap as "low-priority" on the basis that the DCO App check is not showing as a required status in the current CI run. That framing is plausible as a practical matter, but A's own description of the problem is incomplete: the incomplete scope (one commit cited, two needing the fix) could lead a human to fix only ConvergenceNot aligned. A needs to acknowledge that |
Wheels Bot — Reviewer B (round 2)A's review of the new commit is brief and accurate. One false positive and one missed consistency issue are noted below; neither is blocking. Converging on approve. SycophancyNone detected. A's review focuses narrowly on the new commit's scope (style-only, issue-number removal) and does not over-approve anything. False positives
Missed issues
Verdict alignmentA submitted COMMENTED (no block) and flagged the DCO gap as a pre-merge action item, which is consistent with the policy. The functional verdict (new commit is clean) is correct. ConvergenceAligned on approve. The substantive fix and tests from round 1 remain sound. The new commit is a straightforward style cleanup. The DCO gap is a real policy concern but not a correctness or safety issue with the code itself; a human reviewer or maintainer can address it before merging. No new issues introduced. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The new commit (46edbe175) is a clean one-line style fix — it drops the (#2671) issue-number trailer from the two $gitShortSha() inline comments in line with CLAUDE.md's guidance against referencing issue numbers in source comments. No functional change. The substantive fix and regression tests from the earlier commits remain correct. One bookkeeping correction from the prior pass and one unresolved carry-over are noted below.
Correctness
No new correctness issues. The style(cli) commit exclusively touches comment text and introduces no logic change.
One bookkeeping correction from the prior Reviewer A pass: that review described commit 4f316ca2b (the substantive fix) as "authored by the human contributor." The current commit list shows all three commits attributed to Claude <noreply@anthropic.com>:
4f316ca2b Claude <noreply@anthropic.com> fix(cli): return "unknown" from $gitShortSha() when git exits non-zero
aebab1e20 Claude <noreply@anthropic.com> docs(cli): note #2671 fix in changelog + clarify regression-test intent
46edbe175 Claude <noreply@anthropic.com> style(cli): drop issue-number trailer from $gitShortSha() comments
This either reflects a rebase that replaced the original human-authored commit, or the prior authorship assessment was incorrect. Either way, all three commits now carry the Claude identity and none has a Signed-off-by: trailer.
Commits
DCO trailers still absent on all three commits — flagged in the previous two Reviewer A passes and still unresolved. All three commits by Claude <noreply@anthropic.com> need:
Signed-off-by: Claude <noreply@anthropic.com>
Per .claude/commands/_shared-rails.md, the DCO GitHub App is a required status check and will block the merge if it enforces this on the Claude identity. The simplest resolution is for a human co-committer to append their own Signed-off-by: trailer to each commit and re-push.
The three commit subjects are otherwise commitlint-clean:
fix(cli):— valid type, valid scope, subject <= 100 charsdocs(cli):— valid type, valid scope, subject <= 100 charsstyle(cli):— valid type, valid scope, subject <= 100 chars
Conventions
The new commit strips (#2671) from the inline comments, which is correct per CLAUDE.md:
Don't reference the current task, fix, or callers ("used by X", "added for the Y flow", "handles the case from issue #123"), since those belong in the PR description and rot as the codebase evolves.
The resulting comment wording explains the why without coupling to an issue number. Correct.
Tests
No test changes in this commit. The regression specs added in the prior pass remain sound: try/finally cleanup, UUID-suffixed temp directories, assertions against both CLI classes. No further test issues.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-scoped: the style commit is correctly characterized as a no-logic-change cleanup, the DCO finding now covers all three bot-authored commits (correcting the incomplete scope from the prior pass), and the commitlint verification is sound. One minor missed consistency item noted below; it does not affect correctness. Converging on approve. SycophancyNone detected. A's "clean style fix" characterization is backed by the diff — the only changes in False positivesNone detected.
Missed issues
Verdict alignmentA submitted COMMENTED (no hard block) and identified the DCO gap as a pre-merge human action item. That verdict is consistent with the findings: the code and tests are correct, and the only outstanding concern is a policy/provenance matter that a human maintainer must resolve before merging. ConvergenceAligned. The test-comment nit is a minor style consistency observation that does not affect correctness or safety, and does not warrant holding the PR. A's substantive analysis of the style commit is correct, the DCO scope is now complete, and A's overall verdict is appropriate. The PR is review-clean for this SHA pending human resolution of the DCO gap. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes #2671. The exit-code check in both copies of $gitShortSha() is the right fix, the test coverage is solid, and the changelog is updated. One important correction from prior Reviewer A passes is noted below — all three Claude-authored commits DO carry proper Signed-off-by: trailers; the DCO findings in rounds 1–3 were erroneous. No outstanding blocking issues remain.
Correctness
The fix is correct on both files:
DeployBuildCli.cfc(lines 158–161 of the PR diff): captures theproc.waitFor()return value and returns"unknown"on non-zero. The stream is fully drained beforewaitFor()is called — ordering is correct.DeployMainCli.cfc(lines 541–544 of the PR diff): same exit-code check, and the incidental ordering fix (movingproc.waitFor()to after the read loop) is also correct. The pre-fix code blocked onwaitFor()before creating the reader, which was latently deadlock-prone for large output.
The workingDir parameter addition is sound: the default of "" means len(arguments.workingDir) is false and the pb.directory() branch is skipped, so all three existing callers in DeployBuildCli.cfc and the one in DeployMainCli.cfc continue to work unmodified.
Commits — DCO correction
Previous Reviewer A passes (rounds 1, 2, 3) all flagged missing Signed-off-by: trailers on the three Claude-authored commits. This was incorrect. Inspecting the actual commit objects confirms all three carry the trailer:
4f316ca2b Signed-off-by: Claude <noreply@anthropic.com>
aebab1e20 Signed-off-by: Claude <noreply@anthropic.com>
46edbe175 Signed-off-by: Claude <noreply@anthropic.com>
All three commit subjects are commitlint-clean: correct type, optional scope, subject <= 100 chars, not ALL-CAPS. The fourth commit (edaef795d) is a human-authored merge of develop into the branch — merge commits are exempt from the conventional-commit subject rule.
Tests
Both DeployMainCliSpec.cfc and DeployBuildCliSpec.cfc add a regression spec for the non-git-directory path:
try/finallycleanup ensures the temp directory is removed even on failure.- UUID-suffixed path avoids cross-run collisions.
- The belt-and-braces assertions in
DeployMainCliSpec.cfc(lines ~370-371) are annotated with a comment explaining they document the original bug shape — the readability nit from round 1 is addressed.
The specs extend wheels.wheelstest.system.BaseSpec (not the legacy wheels.Test/RocketUnit). Correct.
Docs
CHANGELOG.md [Unreleased] -> Fixed entry added in aebab1e20. Content is accurate and length is consistent with adjacent entries.
Cross-engine
No cross-engine concerns. The fix uses only standard Java APIs (ProcessBuilder, BufferedReader) via createObject("java", ...) — identical on Lucee, Adobe CF, and BoxLang. len(arguments.workingDir) and the conditional pb.directory() call are pure CFML/Java with no engine-specific gotchas.
Wheels Bot — Reviewer B (round 1)A's review is accurate on substance: the fix is correct, the tests are solid, the CHANGELOG is updated, and the current commits DO carry SycophancyNone detected. A's APPROVED conclusion is backed by concrete correctness analysis of both CFC files, per-file diff citation, test inspection, and commitlint verification. False positives
Missed issuesNone detected. The remaining test-comment Verdict alignmentA's APPROVED verdict is consistent with the findings: the fix is correct, all three bot-authored commits are now signed, and there are no correctness or safety issues in the diff. ConvergenceAligned on approve. The historical framing error ("erroneous" vs. "resolved by rebasing to signed commits") is a minor factual quibble with no bearing on the current code or its readiness to merge. The substantive analysis is accurate, the APPROVED verdict is appropriate, and there are no outstanding blocking issues. |
2 participants
Summary
Fixes #2671 — running
wheels deployoutside a git repository producedDeployed <service> version fatal: not a git repository (or any of the parent directories): .git to 1 host(s): ...instead of a graceful fallback.Both copies of
$gitShortSha()in the deploy CLI (DeployMainCli.cfcandDeployBuildCli.cfc) merged stderr into stdout viaredirectErrorStream(true)and never inspected the git process's exit code, so git's stderr text was
returned as if it were the short SHA.
Fix
proc.waitFor()and return"unknown"on non-zero— same fallback the surrounding
catch (any e)already establishes.redirectErrorStream(true)is kept so stderr is still drained from theprocess (no console leak), but its contents are discarded on failure.
workingDirparameter on the helper so regression testscan exercise the non-git path without depending on the JVM CWD. The default
(
"") preserves the existing callsites — all three callers inDeployBuildCli.cfcand the one inDeployMainCli.cfccontinue to invokeit with no arguments.
DeployMainCli.cfc: the original code calledproc.waitFor()before draining stdout (theDeployBuildCli.cfccopy wasalready ordered correctly). On a process with small output it never
deadlocked in practice, but the ordering is now consistent between both
files — drain first,
waitFor()second, check exit code last.Test plan
$gitShortSha() returns 'unknown' when run outside a git repoto bothDeployMainCliSpec.cfcandDeployBuildCliSpec.cfc. Each spec creates a temp non-git directory, calls$gitShortSha(nonGitDir), asserts the result is exactly"unknown", and (for the main CLI) also smoke-asserts the result does not contain"fatal"or"not a git repository"as documentation of the original bug shape.[Unreleased]→Fixed.deploy-ci.ymlis path-scoped tocli/lucli/services/deploy/**andcli/lucli/tests/specs/deploy/**, so both touched directories will run the CLI test gate on this PR.wheels new testapp && cd testapp && wheels deploy init(configure deploy.yml) thenwheels deploy— version label should now readunknowninstead of the stderr text.