Fix/many problems identified by ai by simonmysun · Pull Request #49 · simonmysun/ell

simonmysun · 2026-07-26T07:25:48Z

No description provided.

The streaming readers spawned a subprocess for every chunk/line, which adds up to hundreds or thousands of process creations for a long completion: - openai: `echo "x${line}" | grep` + `echo "${line}" | cut -c 6-` per data chunk -> use bash builtins `[[ ${line} == "data: {"* ]]` and `${line#data: }`. - gemini: `echo "${line}" | tr -d '\r'` per line -> use `${line//$'\r'/}`. Behaviour is unchanged (json_parse tolerates the leading space the old `cut` left, and the new prefix strip removes it cleanly); the existing streaming end-to-end fixtures in tests/parse_output.sh continue to pass.

Several files ran `export funcname` to "export" a shell function, but that exports an empty variable of that name; exporting a function requires `export -f`. It only appeared to work because every file is sourced into the main shell, and it would break any code relying on a child process inheriting the function (as the plugin/hook pipelines increasingly do). Fix the six sites to use `export -f`: helpers/logging.sh, helpers/piping.sh, helpers/load_config.sh, and llm_backends/{ell_echo,openai,gemini}/generate_completion.sh. This matches the already-correct exports in json.sh, resolve_paths.sh, render_template.sh and http.sh, and makes the functions genuinely available to subprocesses.

resolve_template interpolated the template name into a path without validation, so `-t ../../secret` could load a .json outside the template directories. Reject names that contain "/" or are "."/".."/empty so a name is always a single path segment. Also normalise ELL_TEMPLATE_PATH so a trailing slash is optional ("-T dir" previously failed silently, only "-T dir/" worked). list_plugin_hooks parsed `ls` output, which breaks on plugin paths containing spaces or newlines. Iterate the glob directly under nullglob (restored to its prior state afterwards) so such paths are handled correctly and a non-matching glob expands to nothing. Extend helpers/resolve_paths.test.sh (now 20 cases) with template-name traversal rejection, ELL_TEMPLATE_PATH trailing-slash handling, a plugin path containing a space, and nullglob restoration.

Every log line forked `date` and `basename "${0}"`, two subprocesses per line, which is wasteful on hot paths with debug logging enabled. Compute the program name once at source time (${0##*/}) and format the timestamp with the bash printf %()T builtin instead of forking date, via a shared _ell_log_prefix helper. Also validate ELL_LOG_LEVEL: it is compared with `-ge`, so a non-integer value (e.g. from `-l abc`) made every logging call error with "integer expression expected". Fall back to the default level when it is not a plain non-negative integer. Extend helpers/logging.test.sh with a timestamp-format assertion and cases proving an invalid ELL_LOG_LEVEL falls back cleanly without errors.

plugins/syntax_highlight/50_post_llm.sh is the largest file in the repo (a 351-line streaming markdown state machine) and had no tests, so a regression could silently corrupt or drop model output. Add plugins/syntax_highlight/50_post_llm.test.sh. It overrides the STYLE_* hooks with plain-text markers instead of ANSI codes and asserts that each construct (heading, ordered/unordered list, bold, italic, strikethrough, inline code, link) is wrapped in the right marker while its visible text is preserved, that plain text is not decorated, that markdown inside a fenced code block is not interpreted, that a backslash-escaped asterisk stays literal, and that TO_TTY=false is a byte-for-byte passthrough.

Every value-taking option (-l -m -T -t -f -o --api-style --api-key --api-url -c -O) ran `shift 2` without checking that a value was actually given. A trailing option such as `ell -m` left only one argument, so `shift 2` failed and the `while [ ${#} -gt 0 ]` loop spun forever on the same argument. Add _require_arg() and call it before each `shift 2`; a missing value now exits with EX_USAGE (64) and a clear message instead of hanging. Extend helpers/parse_arguments.test.sh with a missing-argument case for each of the eleven options (all wrapped in `timeout` so a regression to the infinite loop fails loudly) plus a check that an option with a value before the prompt still parses.

The interactive read loop had no EOF handling: at end of input `read` returned non-zero but the loop kept going, spinning forever and emitting empty completions (only Ctrl-C escaped it). Capture read's status and break out on EOF, printing a newline so the terminal is left tidy. A final unterminated line delivered together with EOF is still processed rather than dropped. Add tests/interactive.sh (registered in tests/entry.sh) covering immediate EOF, a single prompt, multiple prompts, and an unterminated final line. Every ell invocation is wrapped in `timeout` so a regression to the infinite loop fails (exit 124) instead of hanging the suite. The tests set ELL_TMP_SHELL_LOG to an existing file to bypass record-mode re-entry and drive the read loop directly -- the same path the inner ell runs under `script`.

The stdout-to-file redirection guard compared `"${ELL_RECORD}" != "xtrue"` and `"${ELL_INTERACTIVE}" != "xtrue"` with the 'x' prefix on only one side. ELL_RECORD/ELL_INTERACTIVE hold "true"/"false", never "xtrue", so both conditions were always true and stdout was redirected to ELL_OUTPUT_FILE even in record/interactive mode, where it must stay on the terminal for the session. Add the 'x' prefix to both variables so the redirect only happens in plain mode. Add tests/output_redirect.sh (registered in tests/entry.sh): plain mode -o writes to the file with empty stdout, no -o writes to stdout, and interactive mode + -o still emits on stdout. Verified the interactive assertion fails when the fix is reverted.

The pure-bash JSON parser had two hot spots when handling LLM responses: - _json_parse_string appended one character per loop iteration, which is O(n^2) for long string values (a 5 KB text value took ~0.4s). Copy the whole run of ordinary characters up to the next '"' or '\' in one operation via ${rest%%[\"\\]*}; the same value now parses in ~0.02s. - _json_key was called via command substitution ($(_json_key ...)) on every value and every lookup, forking a subshell each time. Parsing 349 small objects spent ~2.7s (mostly sys time in forks). Have it set a global _JSON_KEY instead; the same workload now takes ~0.27s. Net effect on the backends: openai streaming ~4.9s -> ~1.1s. Escape, \uXXXX and surrogate-pair handling are unchanged (the fast path stops at '\' and '"'). Add json.test.sh cases for a long plain string, a mixed escape/unicode/surrogate string, and a leading-escape string.

The -r/--record handler compared `"${ELL_RECORD}" = "xtrue"`, but ELL_RECORD holds "true"/"false" and never "xtrue", so the "Record mode already enabled" guard was dead code that never fired. Add the missing 'x' prefix so a duplicate -r is actually rejected. Also `export` ELL_RECORD like the other flags (e.g. -i) so record mode is visible to child processes, notably the session spawned via `script`. Add parse_arguments.test.sh cases: a single -r sets and exports ELL_RECORD, and a second -r when it is already enabled exits non-zero.

The help text documented `-o, --output` but the handler only matched `-o|--output-file`, so `--output` fell through to the prompt handler and was silently treated as part of the prompt. Accept -o, --output and --output-file (keeping --output-file for backward compatibility) and list all three in the usage text. Add parse_arguments.test.sh cases asserting each spelling sets ELL_OUTPUT_FILE.

Record mode re-execs ell via `script -c "ell -i"`, a bare command that only resolves when ell is installed on PATH. Running in place (./ell.sh) or from an uninstalled checkout failed with "ell: command not found". Resolve BASE_DIR to an absolute path (dirname "${0}" can be "." when run as ./ell.sh, which would not resolve in script's shell/cwd) and re-exec "${BASE_DIR}/ell" using a printf %q shell-quoted command string, so it works regardless of PATH and handles paths containing spaces. Add tests/record_launcher.sh: it runs record+interactive from a checkout in a spaced directory with an ell-free PATH and asserts the inner ell ran with no "command not found". It skips where script(1) is unavailable (e.g. the alpine CI containers). Verified it fails when reverted to a bare `ell`.

fix(security): do not log the API key value at debug level The --api-key handler logged the secret verbatim at debug level ("setting ELL_API_KEY to <key>"), so enabling debug logging wrote the key to stderr/logs that may be shared or captured. Log only that the key was set, with the value redacted; the key is still exported as before. Add parse_arguments.test.sh cases asserting the value never appears in the debug log while ELL_API_KEY is still set.

The backends passed the API key as a `curl --header "Authorization: ..."` argument, so any local user could read it via `ps` or /proc/<pid>/cmdline for the lifetime of the request. Add ELL_CURL_AUTH_HEADER support to ell_curl: when set, the header is written to a 0600 temp file and passed with `curl --config`, so the secret never appears in argv; the temp file is removed right after the call. The openai and gemini backends now set ELL_CURL_AUTH_HEADER instead of passing the auth header on the command line. Add http.test.sh cases (via a curl stub that also captures the --config file) asserting the key is absent from argv, delivered through the config file, and that the temp file is cleaned up.

ELL_API_URL was used verbatim with no scheme check, so an http:// endpoint (misconfiguration or an attacker-controlled URL) would send the API key in cleartext. Add _ell_url_is_secure and, in ell_curl, refuse to send an auth header to a non-secure URL: https:// and file:// are allowed, http:// only for loopback hosts (localhost/127.0.0.1/::1). A plaintext remote URL with a credential is rejected with a clear fatal message unless the user opts in with ELL_ALLOW_INSECURE_URL=true (which logs a warning). URLs without a credential are unaffected. Add http.test.sh cases: the secure/insecure classification, that a credential over remote http is refused without invoking curl, that the opt-out allows it, and that a credential-less plaintext URL still works.

ell.sh has several `logging_fatal; exit 1` branches that were never asserted, so a regression turning a fatal condition into a silent success (or a hang) would go unnoticed. Add tests/error_paths.sh (registered in tests/entry.sh) driving the real ell entrypoint for the three fatal conditions -- template not found, input file not found, and empty rendered payload -- asserting each exits non-zero with its specific error message. All invocations are wrapped in timeout so a hang fails loudly.

plugins/paginator/90_pre_output.sh had no tests. Its terminal pagination path needs a real pty, but its other critical guarantee is unit-testable: as a pre_output hook, when stdout is not a terminal (TO_TTY=false) it must pass the model output through byte-for-byte, never swallowing or rewriting content. Add plugins/paginator/90_pre_output.test.sh asserting that passthrough for plain, multi-line, ANSI, whitespace/tab, unicode and empty input is exact (cmp-based byte comparison for the mixed case).

The `ell` launcher resolves its own directory (following symlinks) and execs ${ELL_DIR}/ell.sh, which is how the documented install works (symlinking ell into ~/.local/bin). This resolution had no direct test. Add tests/launcher.sh (registered in tests/entry.sh) invoking the launcher by absolute path, via an absolute symlink, via a relative symlink from its own directory, and through a symlink chain, asserting each time that the real ell.sh ran.

Copilot

Pull request overview

This PR is a broad hardening pass across ell.sh, its helper library, and the CI/test suite. It focuses on closing shell-injection and path-traversal vectors, making backend HTTP handling more robust, and adding extensive automated coverage (unit + end-to-end) to prevent regressions.

Changes:

Reworks templating, backend dispatch, config loading, and argument parsing to remove eval-driven execution paths and add stricter input validation/trust checks.
Introduces a shared ell_curl wrapper plus backend common helpers, and refactors OpenAI/Gemini backends for correctness and performance.
Adds substantial new unit + end-to-end tests, expands documentation, and tightens CI gates (ShellCheck warnings now blocking; expanded test matrix).

Reviewed changes

Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/terminal_size.sh	E2E test for terminal size fallback behavior when `stty size` is unavailable.
tests/record_pty.sh	E2E PTY-driven record-mode test using a Python PTY helper.
tests/record_launcher.sh	E2E test ensuring record mode re-execs the correct `ell` path even off-PATH and with spaces.
tests/pty_run.py	New generic PTY runner helper for driving interactive/record flows.
tests/parse_output.sh	Adds fixture-based exit-status assertions for truncated vs normal streaming responses.
tests/output_redirect.sh	E2E coverage for `-o/--output-file` behavior across plain vs interactive modes.
tests/openai-stream-truncated.json	New OpenAI streaming fixture for truncated (`length`) finish reason.
tests/launcher.sh	E2E launcher/symlink resolution tests for invoking `ell` via symlink chains.
tests/interactive.sh	E2E tests for interactive EOF handling and entry-hint messaging.
tests/hooks.sh	E2E tests validating hook discovery and stage ordering across the pipeline.
tests/gemini-stream-truncated.json	New Gemini streaming fixture for truncated (`MAX_TOKENS`) finish reason.
tests/gemini-stream-braces.json	Gemini streaming fixture to guard brace/quote handling in streaming parser.
tests/error_paths.sh	E2E tests for fatal/error exits and actionable messages.
tests/entry.sh	Improves harness prerequisites, output capture, skip tallying, and explicit E2E registrations.
tests/bash_version.sh	Tests for bash version gating behavior and message correctness.
tests/assert.sh	Enhances assertion library with `ell_timeout`, `assert_matches`, and `assert_exits`.
tests/assert_lib.test.sh	Meta-tests for the assertion library helpers.
README.md	Updates docs links/sections and simplifies testing/contributing overview.
plugins/syntax_highlight/50_post_llm.test.sh	Adds unit tests for the syntax highlighting plugin behavior.
plugins/syntax_highlight/50_post_llm.sh	Small state-machine adjustments in syntax highlighter list handling.
plugins/redaction/50_post_input.test.sh	Updates tests to run the `.disabled` redaction script directly.
plugins/redaction/50_post_input.sh.disabled	Adds early passthrough/exit when `sed` is unavailable.
plugins/paginator/90_pre_output.test.sh	Adds passthrough tests for paginator when `TO_TTY=false`.
llm_backends/openai/generate_completion.sh	Refactors to use shared HTTP/helpers; improves streaming parsing and stop-reason validation.
llm_backends/generate_completion.test.sh	Adds dispatcher security tests for `ELL_API_STYLE` validation/traversal prevention.
llm_backends/generate_completion.sh	New secure dispatcher with strict `ELL_API_STYLE` validation and existence checks.
llm_backends/gemini/generate_completion.sh	Refactors streaming to linear-time boundary tracking and shared non-streaming/reporting helpers.
llm_backends/ell_echo/generate_completion.sh	Switches to exporting function via `export -f`.
helpers/resolve_paths.test.sh	Adds tests for hook discovery ordering/dedup/disabled hooks and template resolution validation.
helpers/resolve_paths.sh	Hardens template name validation, fixes trailing-slash behavior, and makes hook discovery robust to spaces/disabled hooks.
helpers/render_template.test.sh	Adds tests for safe allowlist-only substitution and JSON escaping behavior.
helpers/render_template.sh	Introduces safe template rendering (no shell eval) plus JSON escaping helpers.
helpers/piping.sh	Switches to exporting function via `export -f`.
helpers/parse_arguments.test.sh	Adds tests for injection prevention, missing-arg handling, secret logging, output flags, and record guard behavior.
helpers/parse_arguments.sh	Adds missing-arg guard, prevents `--api-key` leakage in logs, hardens `-O`, and supports `--output`.
helpers/logging.test.sh	Adds tests for timestamps, invalid log level handling, and prefix rules by verbosity.
helpers/logging.sh	Unifies default log level, avoids per-line forks, adds fast timestamping and numeric validation.
helpers/load_config.test.sh	Adds tests for config trust checks, precedence, export behavior, and explicit config erroring.
helpers/load_config.sh	Adds trust-gated config sourcing (ownership/permissions) to mitigate hostile config execution.
helpers/json.test.sh	Adds long/mixed-string tests to guard the new fast-path string parser behavior.
helpers/json.sh	Optimizes string parsing and avoids subshells for key mapping/lookup.
helpers/http.test.sh	Adds extensive tests for curl wrapper options, auth-header secrecy, URL security checks, and error strings.
helpers/http.sh	New shared curl wrapper with timeouts, HTTP failure handling, secret-off-argv auth, and URL credential safety policy.
helpers/backend_common.sh	New shared backend helpers for non-streaming, stream-end reporting, and pipeline status handling.
ell.sh	Major orchestration hardening: absolute BASE_DIR, API preflight, safe rendering, hook ordering, record-mode re-exec, and EOF-safe interactive loop.
docs/Templates.md	Documents expanded/safe template variable substitution and escaping behavior.
docs/Risk_Consideration.md	Expands risk documentation around backends, executable surfaces, credentials, and record mode.
docs/Plugins.md	Fixes links and documents `.disabled` hook behavior and built-in plugins.
docs/Configuration.md	Updates logging semantics, CLI flags, and clarifies `-O` behavior with safe templating.
docs/Backends.md	New backend overview and contract documentation.
docs/Architecture.md	New architecture and pipeline documentation (hooks, record mode, config precedence).
CONTRIBUTING.md	New contributor guide with portability/security/performance conventions and testing guidance.
CHANGELOG.md	New changelog describing the hardening pass and user-visible behavior changes.
.github/workflows/ci.yml	Makes ShellCheck warnings blocking, expands test jobs (mawk/macOS/Windows informational), and adds dependencies between jobs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The dispatcher lives in llm_backends/, so dirname(BASH_SOURCE) already points at the backends directory. The previous fallback appended a second /llm_backends, yielding a non-existent .../llm_backends/llm_backends path and breaking direct sourcing (tests). Split the two cases: use ${BASE_DIR}/llm_backends only when BASE_DIR is set, otherwise use the file's own directory. Add a regression test that exercises the fallback with BASE_DIR unset.

There are no early returns between creating the temp auth --config file and the curl call, so a RETURN trap for cleanup is unnecessary. Remove it and delete the file inline after capturing curl's exit status. This keeps the captured status unambiguous without relying on shell trap semantics (functrace, $? preservation across RETURN traps, nested-function returns), and drops the SC2064 disable. Add a test asserting curl's non-zero exit status propagates unchanged through the auth-header path where the cleanup now happens.

_ell_url_is_secure stripped the port with ${host%%:*}, which truncates a bracketed IPv6 authority like [::1]:8080 (and even [::1] with no port) at the first ':' inside the brackets, yielding '['. That never matched the '[::1]' allowlist entry, so an auth header sent to a local IPv6 loopback over http:// was wrongly refused as insecure. Trim to the closing ']' before dropping a trailing ':port' for bracketed authorities; keep the first-':' split for host:port. Non-loopback IPv6 hosts remain correctly classified as insecure over plaintext. Add tests for [::1] with and without a port and for a remote bracketed IPv6 host.

Two macOS CI failures, both from GNU-only tool behaviour: syntax_highlight: it extracted the last char of the buffer with "awk -F '' '{print $NF}'". Empty FS is a GNU awk extension; BSD/macOS awk errors with 'field separator FS is empty' and prints nothing, so the buffer was cleared while dirty=true kept reprocessing, spinning forever (the 6-hour CI timeout). Replace with the bash-native ${buffer: -1}, which is byte-identical on gawk and needs no external awk. This restores the ${buffer: -1} form that #32 had swapped for awk; it is safe because the project requires bash >= 4.1 and already uses substring expansion throughout (helpers/json.sh, render_template.sh, load_config.sh). redaction: the patterns used \b word boundaries, a GNU sed extension. BSD/macOS sed treats \b as a literal backspace, so nothing matched and 15 redaction assertions failed. Probe sed once for \b vs the BSD [[:<:]]/[[:>:]] boundary tokens and build the patterns with whichever the running sed supports, falling back to no anchoring otherwise. Also switch the base64 rule to a '|' delimiter so the '/' in its class needs no escaping. Verified: full suite green on GNU tools; redaction and the detection logic verified against a simulated BSD sed; shellcheck -S warning clean.

The #32/#33 effort tried to keep the code close to POSIX sh, swapping bash builtins for external-command workarounds. That goal is abandoned: ell hard-requires bash >= 4.1 (enforced in ell.sh, tested in bash_version.sh) and the codebase already relies on arrays, substring expansion and [[ ]] throughout. Those workarounds are now pure downgrades -- slower (subshells in hot loops) and, in one case already fixed, a BSD portability regression. Revert the remaining ones: - syntax_highlight: replace 10 'echo "x$buffer" | grep -E -q' tests (a fork storm of echo+grep, several per streamed character) with bash's in-process [[ =~ ]]. Regexes containing a backtick or an unbalanced '(' are held in RE_* variables so the shell parser does not misread them inline. Output verified byte-identical to the prior version across 25 markdown inputs. - paginator: same echo|grep -> [[ =~ ]]/glob conversion; drop the per-character "$(printf 'x\e')" subshell in favour of $'\e'; and echo -ne -> printf for the cursor-position query. - piping: 'echo "$pipes" | cut -c 4-' -> ${pipes:3} (no fork). - parse_arguments: 'read -a <<EOF ... EOF' here-doc -> '<<<' here-string. Full suite green (BSD-relevant plugins included); shellcheck -S warning clean across the repo.

Remove the note about the 'x' prefix needing to be on both sides. The comparison is unchanged; the idiom will be addressed later in one pass.

On Windows bash ($OSTYPE msys/cygwin) a file:// URL built from a POSIX path like /c/Users/... or /tmp/... is not readable by the native curl, which expects file:///C:/.... This made every file:// backend request (and the parse_output fixture tests) fail with curl exit 37. ell_curl now normalizes a file:// URL via cygpath (-m, forward-slash Windows path) before invoking curl, preserving any trailing #fragment ell appends. It is a strict no-op off Windows and for non-file:// URLs, so real API calls are unaffected. Verified: Linux output unchanged; the rewrite verified against a simulated msys OSTYPE + cygpath.

Git Bash over NTFS cannot create group/world-writable files or (by default) real symlinks, so some tests could never pass there and the informational Windows CI run was noisy. Rather than assert the impossible, the affected tests now probe the environment at runtime and SKIP when the capability is absent: - load_config: probe whether chmod can set group/other write bits; if not, SKIP the 5 permission-dependent checks (reject group/world writable, untrusted-not-sourced, refusal logging). - http: probe the same; SKIP the 'temp auth config is mode 0600' check. - launcher: probe whether 'ln -s' makes a real symlink; if not, SKIP the 3 symlink-invocation checks (the absolute-path check still runs). These are genuine platform security/capability limitations, not bugs, so they are documented rather than worked around: README gains an 'Environment differences and limitations' note and docs/Configuration.md gains a 'Windows' section spelling out that the config-trust check and the 0600 credential file are not enforceable under Git Bash, recommending WSL/MSYS2. CI: add a Windows/MSYS2 job (msys2/setup-msys2) that runs the suite in a fuller POSIX environment where these features work, so they are actually exercised on Windows; keep the Git Bash job informational and update its comment. Linux/macOS behavior is unchanged (all capabilities present, no SKIPs).

Copilot

Pull request overview

Copilot reviewed 56 out of 56 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

ell.sh:131

These mapfile calls use process substitution (< <(...)), which is rejected by bash 4.1 when running under set -o posix (the test harness runs that way per CONTRIBUTING). This can make ell fail with a syntax error on the oldest supported bash. Use a here-string + command substitution instead.

generate_completion() {
  local pre_llm_hooks post_llm_hooks backend_status;
  mapfile -t pre_llm_hooks < <(list_plugin_hooks _pre_llm.sh);
  logging_debug "Pre LLM hooks: ${pre_llm_hooks[*]}";
  mapfile -t post_llm_hooks < <(list_plugin_hooks _post_llm.sh);
  logging_debug "Post LLM hooks: ${post_llm_hooks[*]}";

The four hook-discovery mapfile calls used process substitution (mapfile -t X < <(list_plugin_hooks ...)). Under 'set -o posix' on bash 4.1 -- the oldest version ell supports and the mode the test harness runs in (CONTRIBUTING.md) -- '< <(...)' is a parse error, so ell.sh would fail to even parse there. Verified in a bash:4.1 container: previously a 'syntax error near unexpected token' before any line ran; now it parses. Switch to here-strings ('mapfile -t X <<< "$(...)"'), the form CONTRIBUTING recommends. An empty result yields a single empty element, which piping() already treats as 'no pipes' via its [ "${1}" = '' ] guard, so behavior is unchanged.

Two correctness fixes in the backend HTTP path: - helpers/http.sh: the auth header written to curl's --config file only escaped double quotes, but inside a curl config double-quoted string a backslash is also an escape introducer (\\, \", \t, \n, \r, \v; a backslash before any other letter is dropped). A header value like 'a\tb' was thus read back as 'a<TAB>b', mangling the credential. Escape backslashes first, then quotes (quotes-first would double-escape the added backslashes). Adds a test asserting the escaped config contents; verified it fails against the old quote-only escaping. - helpers/backend_common.sh: on a curl/transport failure the non-streaming path returned a bare 1, discarding curl's real exit code -- unlike the streaming path (ell_backend_check_pipestatus) and the contract in docs/Backends.md ('the curl exit code -- a transport failure, propagated as-is'). Return ${curl_status} instead. Verified end to end: a failing file:// request now exits 37, not 1.

Record mode invoked `script -q -f -c "CMD" LOGFILE`, which is the GNU/util-linux syntax. BSD/macOS script has neither -f nor -c and errors with 'script: illegal option -- f', so record mode failed on macOS (the inner ell never ran; record_launcher/record_pty tests failed). GNU and BSD script take incompatible command syntax: GNU: script -q -c "CMD-STRING" LOGFILE (command run via $SHELL -c) BSD: script -q LOGFILE CMD [args...] (command execvp'd as argv) Detect the flavour once via `script --version | grep util-linux` (side-effect-free; spawns no PTY, cannot hang on non-TTY stdin), then build the call accordingly: on GNU collapse the argv into one %q-quoted -c string; on BSD pass the command as separate positional words after the logfile. The command to capture is now held as an argv array so the BSD branch gets correct word boundaries. Dropped the GNU-only -f (flush), which record mode does not need. Verified: GNU path unchanged (record_launcher/record_pty pass locally); BSD path verified against a simulated macOS `script` (correct flavour detection, inner ell located and run, no 'illegal option'). Full suite green; shellcheck (CI invocation) clean; ell.sh parses under bash 4.1 + posix.

Two portability fixes surfaced by the MSYS2 CI run: - Byte-exact comparisons used cmp(1) (diffutils), absent in a minimal MSYS2, so all 26 render_to_text cases and the paginator byte-identical case reported false FAILs ('cmp: command not found') even though the actual output matched. Replace cmp with a pure-bash byte comparison (cat + a sentinel 'x' to preserve trailing newlines): add assert_files_equal to tests/assert.sh (used by paginator) and an inline bytes_equal in render_to_text.test.sh (which does not source assert.sh). Verified the render_to_text suite still passes with cmp absent from PATH. - record_launcher builds an isolated PATH by symlinking tools into a temp bin. On MSYS2 'ln -s' can fail and a copied/symlinked bash.exe cannot start (needs msys-2.0.dll from its original dir: 'cannot open shared object file'). Make the tool install best-effort (ln, else cp) and add a runnability probe: if 'bash -c' cannot run under the isolated PATH, SKIP rather than emit a false failure. Linux behavior unchanged: full suite green, no new skips.

Add diffutils (cmp) and python to the MSYS2 test job. Though the suite no longer requires cmp, installing diffutils lets the byte-exact tests run via their normal path, and python lets the real-PTY record test (record_pty) actually execute instead of skipping. coreutils already provides timeout; util-linux provides script.

Revert adding diffutils: the suite no longer depends on cmp (the byte-exact tests use a pure-bash comparison), so installing it is pointless and would mask a regression -- if a cmp dependency crept back in, diffutils would let it pass in CI while breaking on a minimal MSYS2. Omitting it keeps the 'no cmp dependency' guarantee under test. python is kept for the real-PTY record test.

Copilot

Pull request overview

Copilot reviewed 57 out of 57 changed files in this pull request and generated 3 comments.

piping() joins each stage with ' | ' and runs the result with bash -c. Stages may be command fragments with their own args (e.g. 'tr a-z A-Z'), so they cannot be blindly quoted -- but a plugin-hook stage is a bare executable path that list_plugin_hooks explicitly allows to contain spaces. Unquoted, bash -c word-split such a path ('/a b/hook.sh' -> '/a', 'b/hook.sh') and failed with 'No such file or directory', so hooks in spaced paths never ran. Shell-quote (printf %q) only the stages that are themselves an existing executable file, leaving command fragments (tr/sort/... and PATH lookups, which are not files by that literal name) untouched. Adds regression tests for a spaced hook path alone and mixed with a command fragment; verified both fail against the previous implementation.

Both backends unconditionally exported ELL_CURL_AUTH_HEADER, so an unset key produced 'Authorization: Bearer ' / 'x-goog-api-key: ' with an empty value. That (a) sends a bogus empty-credential header and (b) trips ell_curl's 'refuse credential over http://' guard (it treats any non-empty header as a credential), even though ell.sh's preflight explicitly permits running with no key and states the request will be sent WITHOUT an auth header. Only set/export the header when ELL_API_KEY is non-empty. Verified end to end: with an empty key over an http:// URL the request is no longer refused (previously 'Refusing to send credentials'; now it proceeds and fails only on the real transport error).

Copilot

Pull request overview

Copilot reviewed 58 out of 58 changed files in this pull request and generated 2 comments.

…s_secure The bracketed-IPv6 branch used host="${host%%]*}]" -- trim past the first ']' then re-append a literal ']'. That is functionally correct (all IPv6 cases were and are covered by tests: [::1], [::1]:8080, [2001:db8::1]:443), so it is not broken, but the trim-then-re-append trick is hard to read and easy to misjudge. Replace it with an explicit two-step trim that keeps the brackets: strip the leading '[', take the text up to the first ']', and rebuild "[inner]" (dropping any :port). Behavior is unchanged; http.test.sh IPv6 cases still pass.

The interactive loop emitted ELL_PS1 with `echo -ne`, whose -n/-e handling and escape interpretation vary across shells and echo implementations (e.g. under shopt xpg_echo or set -o posix -ne can be printed literally). ELL_PS1 already contains real escape bytes (built via printf when defaulted), so no -e interpretation is needed. Use printf '%s', matching how ELL_PS2 is already printed. Verified byte-identical output to the old echo -ne for the default prompt; interactive/output_redirect tests pass.

simonmysun added 30 commits July 24, 2026 04:13

fix(security): eliminate command injection in -O option parsing

fbd5f68

fix(security): refuse to source untrusted config files

d3fafdc

test: cover load_config precedence, export and env-override contract

4277986

fix(security): validate ELL_API_STYLE before sourcing a backend

16e0d79

fix(security): render templates without shell evaluation

711c638

fix(plugins): ship redaction disabled by default via .disabled suffix

4feeb29

fix(security): redact SHELL_CONTEXT and centralize JSON escaping

d3350b0

fix(openai): detect truncated streaming completions

b16f750

feat(backends): add timeouts and HTTP error handling to curl calls

c7ca42a

fix: restore bash 4.1 compatibility

afbc690

docs: complete template variables, fix links and bash version text

e4ecc92

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread llm_backends/generate_completion.sh

Comment thread helpers/http.sh Outdated

Comment thread helpers/http.sh Outdated

simonmysun added 8 commits July 26, 2026 18:37

docs: drop the x-prefix explanation comment on the -r guard

70747b2

Remove the note about the 'x' prefix needing to be on both sides. The comparison is unchanged; the idiom will be addressed later in one pass.

simonmysun requested a review from Copilot July 26, 2026 17:51

Copilot started reviewing on behalf of simonmysun July 26, 2026 17:51 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread ell.sh

Comment thread helpers/http.sh Outdated

Comment thread helpers/backend_common.sh

simonmysun added 6 commits July 26, 2026 20:08

simonmysun requested a review from Copilot July 26, 2026 18:45

Copilot started reviewing on behalf of simonmysun July 26, 2026 18:46 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread helpers/piping.sh Outdated

Comment thread llm_backends/openai/generate_completion.sh

Comment thread llm_backends/gemini/generate_completion.sh

simonmysun added 2 commits July 26, 2026 21:01

simonmysun requested a review from Copilot July 26, 2026 19:02

Copilot started reviewing on behalf of simonmysun July 26, 2026 19:03 View session

Copilot AI reviewed Jul 26, 2026

View reviewed changes

Comment thread helpers/http.sh

Comment thread ell.sh

simonmysun added 2 commits July 26, 2026 21:10

simonmysun merged commit b96d5c5 into main Jul 26, 2026
7 checks passed

simonmysun mentioned this pull request Jul 26, 2026

release: v0.2.0 #50

Merged

Navigation Menu

Fix/many problems identified by ai - #49

Fix/many problems identified by ai#49
simonmysun merged 77 commits into
mainfrom
fix/many-problems-identified-by-ai

simonmysun commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

simonmysun commented Jul 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants