fix: [nomad-step-0] S0.1-fix — bin/disinto swallows --backend=nomad as repo_url positional (#835) #839

Merged

dev-bot merged 1 commit from fix/issue-835 into main

2026-04-16 09:31:24 +00:00

dev-bot commented

2026-04-16 09:19:49 +00:00

Collaborator

Fixes #835

Changes

Fixes #835 ## Changes

dev-bot added 1 commit 2026-04-16 09:19:50 +00:00

fix: [nomad-step-0] S0.1-fix — bin/disinto swallows --backend=nomad as repo_url positional (#835 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/push/nomad-validate Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/nomad-validate Pipeline was successful

Details

ci/woodpecker/pr/smoke-init Pipeline was successful

Details

72ed1f112d

Why: disinto_init() consumed $1 as repo_url before the argparse loop ran,
so `disinto init --backend=nomad --empty` had --backend=nomad swallowed
into repo_url, backend stayed at its "docker" default, and the --empty
validation then produced the nonsense "--empty is only valid with
--backend=nomad" error — flagged during S0.1 end-to-end verification on
a fresh LXC. nomad backend takes no positional anyway; the LXC already
has the repo cloned by the operator.

Change: only consume $1 as repo_url if it doesn't start with "--", then
defer the "repo URL required" check to after argparse (so the docker
path still errors with a helpful message on a missing positional, not
"Unknown option: --backend=docker").

Verified acceptance criteria:
  1. init --backend=nomad --empty             → dispatches to nomad
  2. init --backend=nomad --empty --dry-run   → 9-step plan, exit 0
  3. init <repo-url>                          → docker path unchanged
  4. init                                     → "repo URL required"
  5. init --backend=docker                    → "repo URL required"
                                                (not "Unknown option")
  6. shellcheck clean

Tests: 4 new regression cases in tests/disinto-init-nomad.bats covering
flag-first nomad invocation (both --flag=value and --flag value forms),
no-args docker default, and --backend=docker missing-positional error
path. Full suite: 10/10 pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

review-bot commented

2026-04-16 09:30:19 +00:00

Collaborator

AI Review

Summary

The bug was a classic eager-consume-before-parse problem: disinto_init grabbed $1 as repo_url unconditionally, so disinto init --backend=nomad --empty --dry-run set repo_url="--backend=nomad", left backend at the docker default, then hit the --empty guard with the misleading error.

The fix is exactly right: check [[ "$1" != --* ]] before consuming the positional, then defer the "repo URL required" guard to after argparse where the backend is known.

Checklist

Logic: Correct. [[ "$1" != --* ]] is bash glob pattern matching — works reliably under [[ ]]. The deferred check at line 753 runs after backend is set, so docker-without-URL errors cleanly and nomad-without-URL is silently fine (as intended).
Regression: disinto init (no args) still errors — $# -gt 0 is false so repo_url stays empty, docker default triggers the check. ✓
Error message: --backend=docker without a positional now produces "repo URL required" instead of the misleading "Unknown option: --backend=docker". ✓
Tests: All four new tests target real failure modes. Assertions match the actual output strings in _disinto_init_nomad (verified against the source). Existing tests with placeholder/repo positional continue to cover the docker path.
Scope: Two files, precisely scoped to the bug. No out-of-scope changes.

Verdict

APPROVE — Fix is minimal and correct: defers repo_url consumption until after argparse; tests cover the exact bug, regression cases, and improved error messaging

Reviewed at 72ed1f1 | AGENTS.md

## AI Review  ### Summary The bug was a classic eager-consume-before-parse problem: `disinto_init` grabbed `$1` as `repo_url` unconditionally, so `disinto init --backend=nomad --empty --dry-run` set `repo_url="--backend=nomad"`, left `backend` at the docker default, then hit the `--empty` guard with the misleading error. The fix is exactly right: check `[[ "$1" != --* ]]` before consuming the positional, then defer the "repo URL required" guard to after argparse where the backend is known. ### Checklist - **Logic**: Correct. `[[ "$1" != --* ]]` is bash glob pattern matching — works reliably under `[[ ]]`. The deferred check at line 753 runs after `backend` is set, so docker-without-URL errors cleanly and nomad-without-URL is silently fine (as intended). - **Regression**: `disinto init` (no args) still errors — `$# -gt 0` is false so `repo_url` stays empty, docker default triggers the check. ✓ - **Error message**: `--backend=docker` without a positional now produces "repo URL required" instead of the misleading "Unknown option: --backend=docker". ✓ - **Tests**: All four new tests target real failure modes. Assertions match the actual output strings in `_disinto_init_nomad` (verified against the source). Existing tests with `placeholder/repo` positional continue to cover the docker path. - **Scope**: Two files, precisely scoped to the bug. No out-of-scope changes. ### Verdict **APPROVE** — Fix is minimal and correct: defers repo_url consumption until after argparse; tests cover the exact bug, regression cases, and improved error messaging --- *Reviewed at `72ed1f1` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 09:30:20 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Fix is minimal and correct: defers repo_url consumption until after argparse; tests cover the exact bug, regression cases, and improved error messaging

AI Review: **APPROVE** — Fix is minimal and correct: defers repo_url consumption until after argparse; tests cover the exact bug, regression cases, and improved error messaging