Skip to content

fix(cli): suggest no longer aborts with empty output under strict set -e#70

Merged
Chosen9115 merged 1 commit into
mainfrom
fix/suggest-set-e-arithmetic-abort
Jun 12, 2026
Merged

fix(cli): suggest no longer aborts with empty output under strict set -e#70
Chosen9115 merged 1 commit into
mainfrom
fix/suggest-set-e-arithmetic-abort

Conversation

@Chosen9115

Copy link
Copy Markdown
Owner

What & why

A user reported that on their (Linux) box the CLI test suite exits non-zero at the local suggest section, and reproduced it directly:

opensop suggest "qualify sales lead by email" --local --json
# exits 1, empty output

Root cause. The threshold-parse loop in both local_suggest and cmd_suggest used (( i++ )). A standalone (( i++ )) returns exit status 1 when i is 0 — the post-increment evaluates to the old value 0, and ((…)) reports a zero result as "false". Under set -euo pipefail, on bashes that abort on arithmetic-evaluating-to-zero (common on Linux; Apple's bash 3.2 happens not to), this killed suggest on the very first loop iteration, before any output — hence "exit 1, empty stdout".

search was unaffected because local_search has no such loop. That asymmetry is exactly what the reporter saw: a no-match search passed, then suggest failed in the same run.

The fix replaces both (( i++ )) with the assignment form i=$((i + 1)), which always returns exit status 0.

Changes

  • cli/bin/opensop(( i++ ))i=$((i + 1)) in local_suggest (the reported --local path) and cmd_suggest (the remote path; identical latent bug from the mirrored implementation).
  • cli/test/test.sh — source-level guard forbidding standalone (( var++ )) / (( var-- )). A behavioral test passes on a non-aborting bash and gives false confidence; this guard is portable and catches the whole footgun class.
  • cli/CHANGELOG.md[Unreleased] → Fixed entry.

Test plan

  • bash -n cli/bin/opensop — clean.
  • bash cli/test/test.shALL PASS (exit 0), including the new guard and all suggest --local cases.
  • Reproduced the abort under an emulated strict-set -e bash (treating the arithmetic command's rc=1 as fatal) and confirmed the assignment form survives where (( i++ )) aborted.

Self-rating

  • Correctness 9 · simplicity 9 · test coverage 8 (portable source-guard + existing behavioral coverage) · naming 9 · spec fidelity 10 (no contract change) · public-repo hygiene 10 (no PII/secrets/internal identifiers).

🤖 Generated with Claude Code

The threshold-parse loop in both `local_suggest` and `cmd_suggest` used
`(( i++ ))`. A standalone `(( i++ ))` returns exit status 1 when `i` is 0
(post-increment yields the old value 0, which `((…))` reports as false).
Under `set -euo pipefail` on bashes that abort on arithmetic-evaluating-to-
zero (common on Linux; Apple's bash 3.2 does not), this killed `suggest` on
the first loop iteration before any output:

    opensop suggest "qualify sales lead by email" --local --json
    # exit 1, empty stdout

`search` was unaffected because it has no such loop — which is exactly why a
no-match `search` passed while `suggest` failed in the same test run.

Replace both occurrences with the assignment form `i=$((i + 1))`, which
always returns 0. Add a source-level guard to test/test.sh forbidding
standalone `(( var++ ))` / `(( var-- ))` so the footgun cannot reappear on a
bash where it doesn't bite locally.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Chosen9115 Chosen9115 merged commit 42a07ed into main Jun 12, 2026
1 check passed
@Chosen9115 Chosen9115 deleted the fix/suggest-set-e-arithmetic-abort branch June 12, 2026 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant