From 5b1898c80b716100cc8ebbb6abfb63fb9542398f Mon Sep 17 00:00:00 2001 From: DreamCoder08 Date: Wed, 30 Sep 2026 01:47:33 -0500 Subject: [PATCH] fix: honour indented toml headers and shim precedence on the bash path A backfill review of files that gga could not review during the Codex usage limit (now that the quota is back) found two real defects: - apply-ml4w-hooks.sh only recognised TOML table headers in column 0. TOML allows leading whitespace, so after a disabled template an indented [templates.kitty] was commented out too. Match and trim indented headers. - .bashrc and the gga config block checked whether the shim dir was on PATH, not whether it came first. A shim already behind ~/.local/bin left the real codex in front and bypassed the model pin. Drop any existing entry and prepend, without leaving an empty PATH element. Tests cover both cases and fail without the change. The PATH rule now lives in one place, lib/gga-shim-path.sh, which the installer copies to ~/.config/gga/shim-path.sh and both the gga config block and .bashrc source (a review flagged the duplicated logic). The installer reports the extra file and tests cover it. Follow-ups from the next review round: the fragment removes adjacent duplicate entries (a:a:b kept one copy), both callers require a regular readable file before sourcing, and the awk program of render_listener_hook moves to its own function so both stay screen-sized. --- DreamcoderShell/.bashrc | 4 ++- docs/configuration/gga.md | 1 + lib/gga-shim-path.sh | 22 +++++++++++++ scripts/apply-ml4w-hooks.sh | 19 +++++++---- scripts/install-gga-pin.sh | 22 ++++++++++--- tests/ml4w/apply_ml4w_hooks.bats | 14 ++++++++ tests/shell/test_gga_pin.bats | 55 ++++++++++++++++++++++++++++++-- 7 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 lib/gga-shim-path.sh diff --git a/DreamcoderShell/.bashrc b/DreamcoderShell/.bashrc index e2a17d0a..e62ae6b0 100644 --- a/DreamcoderShell/.bashrc +++ b/DreamcoderShell/.bashrc @@ -21,7 +21,9 @@ export BUN_INSTALL="${HOME}/.bun" _dc_gga_bin="${XDG_CONFIG_HOME:-${HOME}/.config}/gga/bin" if [[ -d "${_dc_gga_bin}" ]]; then export GGA_PROVIDER="codex" - [[ ":${PATH}:" == *":${_dc_gga_bin}:"* ]] || export PATH="${_dc_gga_bin}:${PATH}" + # Shared with the gga config block: the shim dir must come first, exactly once. + # shellcheck source=../lib/gga-shim-path.sh + [[ -f "${_dc_gga_bin%/bin}/shim-path.sh" && -r "${_dc_gga_bin%/bin}/shim-path.sh" ]] && source "${_dc_gga_bin%/bin}/shim-path.sh" fi unset _dc_gga_bin SHELL_DIR="${XDG_CONFIG_HOME:-${HOME}/.config}/shell" diff --git a/docs/configuration/gga.md b/docs/configuration/gga.md index 72395135..09c000c9 100644 --- a/docs/configuration/gga.md +++ b/docs/configuration/gga.md @@ -10,6 +10,7 @@ Every gga review, in every repository, runs on the OpenAI Codex provider with | File | Role | |------|------| | `~/.config/gga/bin/codex` | Shim (source: `scripts/gga-codex-shim.sh`). Injects `-m -c model_reasoning_effort=` into `codex exec` only when a `gga` process is an ancestor and no `-m`/`--model` is given. Everything else passes through to the real `codex`. | +| `~/.config/gga/shim-path.sh` | Shared fragment (source: `lib/gga-shim-path.sh`) that puts the shim dir first on `PATH`, exactly once. Sourced by the config block and by `.bashrc`, so the rule lives in one place. | | `~/.config/gga/pin.env` | `GGA_PIN_MODEL` and `GGA_PIN_EFFORT`. Created once; never overwritten. | | `~/.config/gga/config` | Marked block at the end (`# >>> dreamcoder gga pin >>>`): `PROVIDER`/`GGA_PROVIDER="codex"` and the shim dir first on `PATH`. | | `~/.config/environment.d/50-gga-pin.conf` | Same provider and `PATH` for desktop-launched programs (IDE git hooks). | diff --git a/lib/gga-shim-path.sh b/lib/gga-shim-path.sh new file mode 100644 index 00000000..484fc0b9 --- /dev/null +++ b/lib/gga-shim-path.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +# ============================================================================ +# gga-shim-path.sh — put the gga codex shim directory first on PATH, exactly once +# ============================================================================ +# Sourced, never executed (installed as ~/.config/gga/shim-path.sh by +# scripts/install-gga-pin.sh); it must not set shell options. Callers set +# _dc_gga_bin to the shim directory first: the gga config block and .bashrc. +# +# Precedence, not membership, decides which codex runs. A directory that is +# already on PATH but behind ~/.local/bin would leave the real codex in front +# and bypass the model pin, so any existing entry is dropped before prepending. +# ============================================================================ +# _dc_gga_bin is set by the caller (see the header); a guard that aborts would kill gga itself. +# shellcheck disable=SC2154 +_dc_gga_path=":${PATH}:" +# The substitution consumes both separators, so adjacent duplicates (a:a:b) need another pass. +while [[ "${_dc_gga_path}" == *":${_dc_gga_bin}:"* ]]; do + _dc_gga_path="${_dc_gga_path//":${_dc_gga_bin}:"/:}" +done +_dc_gga_path="${_dc_gga_path#:}" +export PATH="${_dc_gga_bin}${_dc_gga_path:+:${_dc_gga_path%:}}" +unset _dc_gga_path diff --git a/scripts/apply-ml4w-hooks.sh b/scripts/apply-ml4w-hooks.sh index 76e160ec..5a9ad422 100755 --- a/scripts/apply-ml4w-hooks.sh +++ b/scripts/apply-ml4w-hooks.sh @@ -110,9 +110,10 @@ ${END_MARK}" # one falls back to reading gtk-application-prefer-dark-theme at run time, # because `dreamcoder sync` renders Dark when DREAMCODER_THEME_MODE is unset. # Exits 3 when no Matugen call is found, leaving the decision to the caller. -render_listener_hook() { - DREAMCODER_DISPATCHER_Q="$(printf '%q' "${DREAMCODER_DOTS_DIR}/scripts/dreamcoder")" \ - awk -v begin="${LISTENER_BEGIN_MARK}" -v end="${LISTENER_END_MARK}" ' +# The awk program that inserts the Dreamcoder block after every Matugen call. Quoted heredoc: +# nothing in it is expanded by the shell. Reads the escaped dispatcher path from ENVIRON. +listener_awk_program() { + cat <<'AWK' function trim(s) { sub(/^[[:space:]]+/, "", s); sub(/[[:space:]]+$/, "", s); return s } BEGIN { dispatcher = ENVIRON["DREAMCODER_DISPATCHER_Q"] } trim($0) == begin { skip = 1; next } @@ -146,7 +147,12 @@ render_listener_hook() { } } END { if (!found) exit 3 } - ' "$1" +AWK +} + +render_listener_hook() { + DREAMCODER_DISPATCHER_Q="$(printf '%q' "${DREAMCODER_DOTS_DIR}/scripts/dreamcoder")" \ + awk -v begin="${LISTENER_BEGIN_MARK}" -v end="${LISTENER_END_MARK}" "$(listener_awk_program)" "$1" } hook_gtk_listener() { @@ -205,8 +211,9 @@ restart_gtk_listener() { disable_owned_templates() { awk -v owned="${DREAMCODER_MATUGEN_TEMPLATES}" -v prefix="${MATUGEN_OFF_PREFIX}" ' BEGIN { n = split(owned, names, " "); for (i = 1; i <= n; i++) want["[templates." names[i] "]"] = 1 } - /^\[/ { header = $0; sub(/[ \t]+$/, "", header); off = (header in want) } - off && $0 != "" && $0 !~ /^#/ { print prefix $0; next } + # TOML allows whitespace before a table header, so match and trim it. + /^[ \t]*\[/ { header = $0; gsub(/^[ \t]+|[ \t]+$/, "", header); off = (header in want) } + off && $0 !~ /^[ \t]*(#|$)/ { print prefix $0; next } { print } ' "$1" } diff --git a/scripts/install-gga-pin.sh b/scripts/install-gga-pin.sh index 42de6cc1..0fa2d2a3 100755 --- a/scripts/install-gga-pin.sh +++ b/scripts/install-gga-pin.sh @@ -36,6 +36,8 @@ gga_dir="${config_home}/gga" shim_dir="${gga_dir}/bin" shim_src="${script_dir}/gga-codex-shim.sh" shim_dst="${shim_dir}/codex" +path_src="${script_dir}/../lib/gga-shim-path.sh" +path_dst="${gga_dir}/shim-path.sh" pin_file="${gga_dir}/pin.env" config_file="${gga_dir}/config" env_file="${config_home}/environment.d/50-gga-pin.conf" @@ -47,10 +49,12 @@ if [[ ! "${shim_dir}" =~ ^/[A-Za-z0-9._@+/-]+$ ]]; then exit 1 fi -[[ -r "${shim_src}" ]] || { - printf 'install-gga-pin: shim source not found: %s\n' "${shim_src}" >&2 - exit 1 -} +for required in "${shim_src}" "${path_src}"; do + [[ -r "${required}" ]] || { + printf 'install-gga-pin: source not found: %s\n' "${required}" >&2 + exit 1 + } +done # Write stdin to $1 when its content differs; report what changed. write_if_changed() { @@ -79,6 +83,10 @@ install_shim() { fi } +install_shim_path() { + write_if_changed "${path_dst}" "gga shim PATH fragment" <"${path_src}" +} + install_pin_env() { [[ -e "${pin_file}" ]] && return 0 write_if_changed "${pin_file}" "gga pin model/effort" <<'EOF' @@ -98,7 +106,10 @@ PROVIDER="codex" GGA_PROVIDER="codex" # A real STATUS: FAILED still blocks; an unavailable provider (usage limit, network) does not. STRICT_MODE="false" -case ":\${PATH}:" in *":${shim_dir}:"*) ;; *) export PATH="${shim_dir}:\${PATH}" ;; esac +_dc_gga_bin="${shim_dir}" +# shellcheck source=/dev/null +[[ -f "${path_dst}" && -r "${path_dst}" ]] && . "${path_dst}" +unset _dc_gga_bin ${BLOCK_END} EOF } @@ -144,6 +155,7 @@ EOF } install_shim +install_shim_path install_pin_env install_config_block install_environment_d diff --git a/tests/ml4w/apply_ml4w_hooks.bats b/tests/ml4w/apply_ml4w_hooks.bats index d887ca7b..95188041 100644 --- a/tests/ml4w/apply_ml4w_hooks.bats +++ b/tests/ml4w/apply_ml4w_hooks.bats @@ -479,3 +479,17 @@ run_hooks_with_path() { [ "$status" -ne 0 ] [[ "$output" == *"Required library not found"* ]] } + +@test "matugen: an indented table header ends the disabled section instead of swallowing the next template" { + # TOML allows leading whitespace before a table header: an owned section followed by an + # indented [templates.kitty] must not comment kitty out. + printf '%s\n' '[config]' '' '[templates.hyprland]' "input_path = 'a'" "output_path = 'b'" '' \ + ' [templates.kitty]' " input_path = 'c'" " output_path = 'd'" '' \ + ' [templates.waybar]' " input_path = 'e'" " output_path = 'f'" >"${MATUGEN_CONFIG}" + run run_hooks + [ "$status" -eq 0 ] + ids="$(template_ids "${MATUGEN_CONFIG}")" + [[ " ${ids} " == *" kitty "* ]] + [[ " ${ids} " != *" hyprland "* ]] + [[ " ${ids} " != *" waybar "* ]] +} diff --git a/tests/shell/test_gga_pin.bats b/tests/shell/test_gga_pin.bats index 041b1db3..c41c9761 100644 --- a/tests/shell/test_gga_pin.bats +++ b/tests/shell/test_gga_pin.bats @@ -38,18 +38,19 @@ shim() { PATH="${SHIM_DIR}:${REAL_BIN}:${PATH}" "$@"; } # ── installer ──────────────────────────────────────────────────────── -@test "installer creates shim, pin.env, config block and environment.d file" { +@test "installer creates shim, PATH fragment, pin.env, config block and environment.d file" { gga_setup run installer [ "$status" -eq 0 ] [ -x "${SHIM_DIR}/codex" ] cmp -s "${SHIM_DIR}/codex" "${DREAMCODER_DOTS_DIR}/scripts/gga-codex-shim.sh" + cmp -s "${GGA_DIR}/shim-path.sh" "${DREAMCODER_DOTS_DIR}/lib/gga-shim-path.sh" grep -qx 'GGA_PIN_MODEL="gpt-6.1-sol"' "${PIN}" grep -qx 'GGA_PIN_EFFORT="medium"' "${PIN}" grep -qx '# >>> dreamcoder gga pin >>>' "${CONFIG}" grep -qx 'GGA_PROVIDER=codex' "${ENV_D}" grep -qx "PATH=${SHIM_DIR}:\${PATH}" "${ENV_D}" - [ "$(grep -c '^✓' <<<"$output")" -eq 4 ] + [ "$(grep -c '^✓' <<<"$output")" -eq 5 ] } @test "installer is idempotent: second run prints nothing and changes nothing" { @@ -101,6 +102,56 @@ shim() { PATH="${SHIM_DIR}:${REAL_BIN}:${PATH}" "$@"; } [ "$output" = "false" ] } +# Membership is not precedence: a shim dir that is already on PATH but behind the real codex +# (a parent process put it there) must be moved to the front, or the model pin is bypassed. +@test "the config block moves a shim dir that is already on PATH to the front" { + gga_setup + run installer + [ "$status" -eq 0 ] + run env -u GGA_PROVIDER PATH="${REAL_BIN}:${SHIM_DIR}:/usr/bin:/bin" bash -c \ + 'source "$1"; IFS=: read -ra p <<<"$PATH"; printf "%s\n" "${p[0]}"; printf "%s\n" "${p[@]}" | grep -cxF "$2"' _ "${CONFIG}" "${SHIM_DIR}" + [ "${lines[0]}" = "${SHIM_DIR}" ] + [ "${lines[1]}" = "1" ] +} + +@test "bashrc: a shim dir already on PATH behind the real codex is moved to the front" { + gga_setup + run installer + mkdir -p "${HOME}/.local/bin" "${SHIM_DIR}" + run env -u GGA_PROVIDER -u XDG_CONFIG_HOME HOME="${HOME}" PATH="${HOME}/.local/bin:${SHIM_DIR}:/usr/bin:/bin" \ + bash --norc -i -c 'source "$1" >/dev/null 2>&1; IFS=: read -ra p <<<"$PATH" + for i in "${!p[@]}"; do [[ "${p[i]}" == "$2" ]] && s=$i; [[ "${p[i]}" == "$3" ]] && r=$i; done + [[ -n "${s:-}" && -n "${r:-}" && "$s" -lt "$r" ]] && echo ordered' \ + _ "${DREAMCODER_DOTS_DIR}/DreamcoderShell/.bashrc" "${SHIM_DIR}" "${HOME}/.local/bin" + [[ "$output" == *ordered* ]] +} + +@test "the shared fragment leaves the shim dir exactly once even when it is repeated in a row" { + gga_setup + run installer + run env -u GGA_PROVIDER PATH="${SHIM_DIR}:${SHIM_DIR}:${REAL_BIN}:${SHIM_DIR}:/usr/bin:/bin" bash -c \ + 'source "$1"; IFS=: read -ra p <<<"$PATH"; printf "%s\n" "${p[0]}"; printf "%s\n" "${p[@]}" | grep -cxF "$2"' _ "${CONFIG}" "${SHIM_DIR}" + [ "${lines[0]}" = "${SHIM_DIR}" ] + [ "${lines[1]}" = "1" ] +} + +@test "the installer installs the shared PATH fragment and never rewrites it when current" { + gga_setup + run installer + [ -f "${GGA_DIR}/shim-path.sh" ] + cmp -s "${GGA_DIR}/shim-path.sh" "${DREAMCODER_DOTS_DIR}/lib/gga-shim-path.sh" + run installer + [ -z "$output" ] +} + +@test "bashrc: without the installed fragment the gga PATH wiring is skipped without an error" { + gga_setup + mkdir -p "${SHIM_DIR}" + run env -u XDG_CONFIG_HOME HOME="${HOME}" bash --norc -i -c \ + 'source "$1" >/dev/null 2>&1; echo "status=$?"' _ "${DREAMCODER_DOTS_DIR}/DreamcoderShell/.bashrc" + [[ "$output" == *"status=0"* ]] +} + @test "sourcing the block twice does not duplicate the shim dir on PATH" { gga_setup installer >/dev/null