refactor(dream-cli): enable set -u and add guards for conditional variables#1002
Draft
yasinBursali wants to merge 4 commits intoLight-Heart-Labs:mainfrom
Draft
refactor(dream-cli): enable set -u and add guards for conditional variables#1002yasinBursali wants to merge 4 commits intoLight-Heart-Labs:mainfrom
yasinBursali wants to merge 4 commits intoLight-Heart-Labs:mainfrom
Conversation
Promote shell option from set -e to set -eo pipefail so failures in
upstream pipeline commands propagate and surface the real exit code
(not the status of the last stage). Aligns dream-cli with the project
standard (CLAUDE.md: 'set -euo pipefail everywhere').
Scope is deliberately narrow — nounset (-u) is intentionally not added
here because it requires a full bare-variable audit.
Side-effect audit:
- Two '| head -1' sites converted to '| sed -n 1p' (SIGPIPE-safe under
pipefail; upstream producers no longer die when head closes the pipe):
* _check_version_compat: grep DREAM_VERSION .env
* preset import: tar tzf archive listing
- '|| true' sites reviewed individually; all are intentional tolerance
of known-benign failures (missing optional env keys, idempotent stops,
arithmetic increment quirks) and were left in place.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…mark' Before: 'dream chat' against a dead backend printed '[dream] Sending to ...', emitted an empty line, and exited 0. 'dream benchmark' propagated that silent failure and happily reported sub-second 'Excellent' performance while the LLM was not even running. Root cause: curl was invoked with '-s' (silent, no error-show, no fail-on-HTTP-error) and the response was parsed with 'jq -r .choices[0].message.content // .error.message // "Error: no response"', which masks every failure path behind a human-friendly string on stdout. Fix: - Add a /v1/models pre-flight probe with --fail-with-body and --max-time 3, so an unreachable llama-server fails loudly before the user sees 'Sending to ...'. - Use --show-error --fail-with-body --max-time 30 on the completion call; non-2xx HTTP responses now fail the command and the body is shown. - Parse responses with 'jq -er' so missing '.choices[0].message.content' exits non-zero; on failure we re-parse '.error.message' and surface it through 'error' (exit 1). - cmd_benchmark: capture cmd_chat output via 'if ! response=...' so a failed chat aborts the benchmark instead of reporting a false-positive duration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ssign
Three commands previously printed warnings or pseudo-success messages
while exiting 0, breaking scripted callers that rely on exit codes to
detect failure.
- cmd_disable: 'sr_resolve' returns its input unchanged for unknown
names, so 'dream disable bogus-service' used to silently stop a
non-existent container ('|| true') and print a success-shaped
message. Mirror the validation pattern already in cmd_enable: if
neither the built-in nor user-extension directory exists for the
resolved id, 'error "Unknown service: <input>"' (exit 1).
- cmd_agent logs: on a fresh install with no host-agent log file, the
logs subcommand warned and returned 0. Return 1 so 'dream agent
logs' fails fast when there is nothing to tail.
- _gpu_reassign: the NVIDIA-only guard and three prerequisite guards
(jq missing, topo lib missing, assign script missing) all exited 0
after warning, so automation could not tell whether reassignment
succeeded or was skipped. Each 'warn ...; return' becomes 'warn ...;
return 1'.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…iables
Adds nounset (-u) to the shell options on top of the existing pipefail
foundation, catching typos in variable names at runtime instead of
silently expanding to empty strings. The audit added ${VAR:-} guards
to 6 variables that are legitimately conditional (missing positional
args for preset diff, sparse associative-array lookups when comparing
preset ext/env maps, and a registry lookup for service dirs without
valid manifests). All other variables in the file are unconditionally
set before use; no guard added.
Audited by running every read-only subcommand end-to-end in a fake
INSTALL_DIR and against a live install: help, version, list, status,
status-json, config show, gpu status/topology/validate/assignment,
model current/list, mode, stt current/status, preset list/save/load/
delete/export/import/diff, agent status, doctor --json, template
list/preview, audit, chat (against down backend), benchmark,
enable/disable/purge (against both valid-service and unknown paths).
Also verified with minimal environment (env -i PATH HOME BASH
INSTALL_DIR) to catch implicit TERM/USER/LANG assumptions.
Specific bugs caught during audit (each reproducible on main tip +
pipefail foundation):
- preset diff: bare \$2/\$3 crashed immediately with no args.
- preset diff: ext1/env1 associative-array reads for keys only present
in the other preset crashed partway through comparison.
- enable <svc>: bare SERVICE_DEPENDS[\$service_id] crashed for service
directories that exist on disk but have no valid manifest (edge case
user-extensions with malformed manifest).
Platform impact:
- macOS / Linux / Windows (WSL2): identical. Portable Bash 4+ parameter
expansion. No platform branching. No variables that are set on one
OS but not another were found during the audit — all guarded reads
guard against missing registry entries or unprovided positional args,
neither of which is platform-specific.
Scope note: this PR depends on the pipefail addition from
fix/dream-cli-pipefail-exit-codes and is intended to land as a
follow-up after that merges upstream.
Contributor
Author
This was referenced Apr 23, 2026
Collaborator
|
Audit follow-up: keep draft / rebase on current The broad nounset/pipefail direction overlaps with focused fixes that have now landed, especially #1008. Please rebase and ensure |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Enable
set -u(nounset) indream-clion top of the existingset -eo pipefailfoundation. Audit every variable read; add${VAR:-}guards to six legitimately-conditional sites (missing positional args forpreset diff, sparse associative-array lookups when comparing preset ext/env maps, registry lookups for service dirs without valid manifests). All other variables in the file are unconditionally set before use; no guard added elsewhere.Why
Nounset catches typos in variable names at runtime instead of silently expanding to empty strings. The audit also surfaced three pre-existing latent bugs that
set -uwould have caught immediately:preset diff: bare$2/$3crashed with no args.preset diff:ext1[$key]/env1[$key]associative-array reads crashed partway through comparison when a key exists only in the other preset.enable <svc>: bareSERVICE_DEPENDS[$service_id]crashed for service directories that exist on disk but have no valid manifest (edge case: user-extensions with malformed manifest).How
Single commit (tip
bea96fe2; the originala7bd6cacwas rebased onto this PR's parent branchfix/dream-cli-pipefail-exit-codesafter that branch's commits were adjusted during review — PR-11's nounset audit content is unchanged, only the base commits it sits on have updated SHAs).Audited by running every read-only subcommand end-to-end in a fake
INSTALL_DIRand against a live install:help,version,list,status,status-json,config show,gpu {status|topology|validate|assignment},model {current|list},mode,stt {current|status},preset {list|save|load|delete|export|import|diff},agent status,doctor --json,template {list|preview},audit,chat(against down backend),benchmark,enable/disable/purge(against both valid-service and unknown paths). Also verified with minimal environment (env -i PATH HOME BASH INSTALL_DIR dream-cli ...) to catch implicitTERM/USER/LANGassumptions.Testing
env -i.Scope Note
Depends on the
set -eo pipefailfoundation fromfix/dream-cli-pipefail-exit-codes— this branch's base commits are that PR's three commits (rebased onto the review-adjusted tips). Intended to merge after that PR lands.Platform Impact
📋 Chain status (operator's reminder — apr-25)
This PR is the last link in the
dream-clistrict-mode chain:Before this PR can be promoted:
set -eo pipefail+ exit-code contract — the foundation-ubuilds on).This PR currently shares 6+ identical hunks with #998 (line 6 set-line, identical
cmd_chat/cmd_benchmark/cmd_disable/cmd_agent/_gpu_reassignrewrites,sed -n '1p'swaps). Merging in parallel = textual conflict.After #998 merges, the operator must:
-uwork: the extra flag at line 6, plus:-empty-string default guards incmd_preset,cmd_enable, etc.gh pr ready 1002).Kept DRAFT until then as a mechanical safeguard — cannot merge before #998.