From 744023cfc29f1170803d4c27633fd006c03ced62 Mon Sep 17 00:00:00 2001 From: Basilio Bogado <541149+basiliskus@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:13:12 +0000 Subject: [PATCH 1/4] fix(msb): thread the merged kit env onto kit lifecycle commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A kit's lifecycle commands ran with only that kit's own environment[], so a `background: true` startup daemon launched agents that inherited none of the other kits' configuration. With a kit that sets OPENCODE_CONFIG (agent instructions plus a default-deny permission layer), daemon-launched agents ran without it while the injected credentials were still present — a silent loss of a policy control. Interactive sessions were already whole-guest (they replay the persisted /var/lib/acq/kit-env marker). Kit environment[] is now guest-wide for install/initFiles/startup too: every lifecycle command runs with the merged env of the full effective kit set, with the same last-value-wins resolution sessions use. The merged set is computed in a pre-pass over the whole kit set in the full-set paths (provision, heal), so it does not depend on kit application order — a per-kit marker read would have worked only when the env-declaring kit happened to be applied first. The mid-life `acq kit apply` path has no full set and falls back to the sandbox's already-persisted merged value; with neither available it degrades to the kit's own env. The non-interactive git guards stay scoped per kit: a guard var contributed by another kit is filtered out of a command's env and acq's own guard value used instead, so one kit setting GIT_TERMINAL_PROMPT cannot disable the guards for a different kit's commands (a prompting kit command blocks provision forever). The staged create-time --script-path body gets the same merged env for consistency. That path is inert today (a bare registration is not auto-run at boot), so it fixes no live symptom; it keeps the two paths from drifting so the ADR-0017 re-verification trap cannot resurface this bug. Known limitation, documented in the ADR: a merged var can now name a path a later kit has not staged yet, turning "no config" into "possibly missing config file" for that window. That is the startup-races-its-prerequisites class tracked as GSA-TTS/agentic-coding-quickstart#506, not an env-scoping problem. Refs: GSA-TTS/agentic-coding-quickstart#515 --- acq.backends/msb.sh | 435 +++++++++++++----- docs/BACKEND_GUIDE.md | 8 +- docs/adr/0011-msb-backend-and-neutral-kits.md | 15 + ...-msb-create-time-startup-script-staging.md | 21 + ...nv-is-guest-wide-for-lifecycle-commands.md | 155 +++++++ test/bats/73-msb-kits-startup.bats | 68 +++ test/bats/78-msb-kit-env.bats | 297 +++++++++++- 7 files changed, 892 insertions(+), 107 deletions(-) create mode 100644 docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md diff --git a/acq.backends/msb.sh b/acq.backends/msb.sh index 97ba7503..7f2c5fac 100644 --- a/acq.backends/msb.sh +++ b/acq.backends/msb.sh @@ -1526,7 +1526,7 @@ EOF # also substitutes the GitHub token on the wire for the API and HTTPS # git-transport hosts bound above, so direct HTTPS clone/push is eligible too. _acq_msb_apply_kit_dir() { - local name="$1" kitdir="$2" + local name="$1" kitdir="$2" mergedn="${3:-}" local spec="${kitdir}/spec.yaml" if [ ! -f "$spec" ]; then echo "acq(msb): kit spec not found: $spec" >&2 @@ -1597,11 +1597,29 @@ EOF # 3) Run commands[]. Reassemble each argv record and exec it as the given uid. # install → run once (idempotent, marker-gated); initFiles/startup → every # apply. msb has no create-time-only hook, so install collapses to a - # marker-gated exec (design §3 lifecycle table). The kit's environment[] - # entries are threaded onto every command as `msb exec -e NAME=value` (msb's - # native per-exec env flag), so the kit's declared guest env is present when - # its lifecycle commands run. - _acq_msb_run_commands "$name" "$spec" + # marker-gated exec (design §3 lifecycle table). The MERGED kit env (every + # effective kit's environment[]) is threaded onto every command as + # `msb exec -e NAME=value` (msb's native per-exec env flag): kit env is + # guest-wide config, so a command — especially a `background: true` daemon + # that launches agents — must see the other kits' vars too (ADR-0033). + # + # Where the merged set comes from: + # - FULL-SET callers (provision, heal) computed it in a pre-pass over every + # kit BEFORE this per-kit loop and pass its array name in, so the result + # does not depend on the order the kits are applied in. + # - The MID-LIFE single-kit path (`acq kit apply NAME KITREF`) has no full + # set, so it recovers the sandbox's already-persisted merged value from + # the marker (step 2 just appended THIS kit's entries, and the read + # applies last-value-wins, so the new kit still overrides — matching the + # additive semantics of a mid-life add). + # - Neither available (marker absent/unwritable) → this kit's own env, the + # pre-ADR-0033 behavior. Fail-soft, never empty. + local _merged_env=() _merged_envn="$mergedn" + if [ -z "$_merged_envn" ]; then + _acq_msb_persisted_kit_env_into _merged_env "$name" + [ "${#_merged_env[@]}" -gt 0 ] && _merged_envn=_merged_env + fi + _acq_msb_run_commands "$name" "$spec" "$_merged_envn" } # _acq_msb_reset_kit_env NAME — remove the persisted kit-env marker so a @@ -1725,16 +1743,35 @@ _acq_msb_copy_file_verified() { } # Parse and execute a kit spec's commands[] against sandbox NAME. +# +# Args: NAME SPEC [MERGEDENV_ARRVAR] +# MERGEDENV_ARRVAR names an array of already-merged NAME=value tokens covering +# the FULL effective kit set (see _acq_msb_merge_kit_env_into). Omitted, the env +# falls back to this spec's own environment[] — used only where no full set +# exists and no merged value could be recovered (see _acq_msb_apply_kit_dir). _acq_msb_run_commands() { - local name="$1" spec="$2" - - # Collect the kit's environment[] entries as `NAME=value` argv-ready tokens, - # threaded onto every command's `msb exec` as `-e NAME=value`. kit_spec_env - # already validates each NAME (^[A-Za-z_][A-Za-z0-9_]*$) and drops unsafe ones, - # so these are safe to pass as exec env. Values are passed as a single argv - # element (never re-split by a shell). + local name="$1" spec="$2" mergedn="${3:-}" + + # The guest env threaded onto every command's `msb exec` as `-e NAME=value`. + # kit_spec_env already validates each NAME (^[A-Za-z_][A-Za-z0-9_]*$) and drops + # unsafe ones, and the marker replay re-validates, so these are safe to pass as + # exec env. Values are passed as a single argv element (never re-split by a + # shell). Kit env is guest-wide config, so a command sees the MERGED set — a + # `background: true` daemon that launches agents must not be limited to its own + # kit's vars (ADR-0033). local _kit_env=() - _acq_msb_collect_kit_env_into _kit_env "$spec" + if [ -n "$mergedn" ]; then + eval "_kit_env=(\${${mergedn}[@]+\"\${${mergedn}[@]}\"})" + else + _acq_msb_collect_kit_env_into _kit_env "$spec" + fi + + # The git-guard decision stays PER KIT: only the guard vars THIS spec declares + # suppress the adapter's guards, so another kit's GIT_TERMINAL_PROMPT in the + # merged set cannot disable them here (see _acq_msb_env_tokens_with_guards_into). + local _own_env=() _ownguards + _acq_msb_collect_kit_env_into _own_env "$spec" + _ownguards=$(_acq_msb_own_guard_names _own_env) # Buffer the parsed command stream FIRST, then execute. The exec step calls # `msb exec`, which would consume this loop's stdin if we iterated the heredoc @@ -1762,7 +1799,7 @@ EOF ;; "__END__") reading=0 - _acq_msb_exec_command "$name" "$phase" "$user" "$background" \ + _acq_msb_exec_command "$name" "$phase" "$user" "$background" "$_ownguards" \ ${_kit_env[@]+"${_kit_env[@]}"} -- ${argv[@]+"${argv[@]}"} ;; *) @@ -1809,13 +1846,69 @@ _acq_msb_split_env_argv() { eval "$_argvarr=(\"\$@\")" } -# _acq_msb_exec_flags_into UFLAG_ARRVAR EFLAG_ARRVAR USER KITENV_ARRVAR — build -# the `msb exec` -u/-e flag arrays for one kit command. Maps the sbx uid contract -# (1000/agent) onto our by-name agent user, threads the kit's declared env as -# `-e NAME=value`, and injects git non-interactive guards unless the kit set -# GIT_TERMINAL_PROMPT itself. Arrays passed/returned by name (bash 3.2 compat). +# The three non-interactive git guard vars the adapter injects onto kit lifecycle +# commands (see _acq_msb_env_tokens_with_guards_into). +ACQ_MSB_GIT_GUARD_NAMES="GIT_TERMINAL_PROMPT GIT_ASKPASS SSH_ASKPASS" + +# _acq_msb_own_guard_names ENVARR — echo the space-joined subset of +# ACQ_MSB_GIT_GUARD_NAMES that the array named ENVARR (ONE kit spec's own +# environment[]) declares. The result is a word list drawn from a FIXED +# adapter-owned name set, never kit bytes, so it is safe to pass as a single +# positional argument through the exec/staging call chain. +_acq_msb_own_guard_names() { + local _envarrn="$1" _names="" _g _ev + eval "set -- \${${_envarrn}[@]+\"\${${_envarrn}[@]}\"}" + for _ev in "$@"; do + for _g in $ACQ_MSB_GIT_GUARD_NAMES; do + case "$_ev" in + "$_g"=*) case " $_names " in *" $_g "*) : ;; *) _names="${_names:+$_names }$_g" ;; esac ;; + esac + done + done + printf '%s' "$_names" +} + +# _acq_msb_env_tokens_with_guards_into ARRVAR ENVARR OWNGUARDS — store the +# NAME=value tokens one kit command should run with into the array named ARRVAR: +# the MERGED kit env (ENVARR, every effective kit's environment[]) plus the +# non-interactive git guards. +# +# GUARD SCOPING (do not widen): the guard decision stays PER KIT. OWNGUARDS is +# the guard-var subset THIS kit declared (_acq_msb_own_guard_names). A guard var +# coming from ANOTHER kit is filtered out of the merged set and the adapter's own +# guard value is used instead, so one kit setting GIT_TERMINAL_PROMPT can never +# disable the guards for a different kit's commands (nor produce two conflicting +# values for one name). A kit that declares a guard var itself still overrides it +# for its OWN commands, exactly as before. +_acq_msb_env_tokens_with_guards_into() { + local _arrn="$1" _envarrn="$2" _ownguards="$3" + eval "$_arrn=()" + local _ev _g _keep + eval "set -- \${${_envarrn}[@]+\"\${${_envarrn}[@]}\"}" + for _ev in "$@"; do + _keep=1 + for _g in $ACQ_MSB_GIT_GUARD_NAMES; do + case "$_ev" in + "$_g"=*) case " $_ownguards " in *" $_g "*) : ;; *) _keep=0 ;; esac ;; + esac + done + [ "$_keep" -eq 1 ] && eval "$_arrn+=(\"\$_ev\")" + done + case " $_ownguards " in + *" GIT_TERMINAL_PROMPT "*) return 0 ;; + esac + eval "$_arrn+=(\"GIT_TERMINAL_PROMPT=0\" \"GIT_ASKPASS=/bin/false\" \"SSH_ASKPASS=/bin/false\")" +} + +# _acq_msb_exec_flags_into UFLAG_ARRVAR EFLAG_ARRVAR USER KITENV_ARRVAR OWNGUARDS +# — build the `msb exec` -u/-e flag arrays for one kit command. Maps the sbx uid +# contract (1000/agent) onto our by-name agent user, threads the MERGED kit env +# (KITENV_ARRVAR) as `-e NAME=value`, and injects git non-interactive guards +# unless THIS kit declared them (OWNGUARDS, from _acq_msb_own_guard_names — the +# guard decision must stay per kit; see _acq_msb_env_tokens_with_guards_into). +# Arrays passed/returned by name (bash 3.2 compat). _acq_msb_exec_flags_into() { - local _uflag="$1" _eflag="$2" _user="$3" _envarr="$4" + local _uflag="$1" _eflag="$2" _user="$3" _envarr="$4" _ownguards="${5:-}" eval "$_uflag=()" eval "$_eflag=()" @@ -1845,13 +1938,9 @@ _acq_msb_exec_flags_into() { ;; esac - # Append the kit's declared guest env as additional `-e NAME=value` flags. - local _ev - eval "set -- \${${_envarr}[@]+\"\${${_envarr}[@]}\"}" - for _ev in "$@"; do - eval "$_eflag+=(-e \"\$_ev\")" - done - + # Append the guest env as additional `-e NAME=value` flags: the MERGED kit env + # (every effective kit's environment[]) plus the non-interactive git guards. + # # NON-INTERACTIVE ENFORCEMENT. Kit lifecycle commands run before the agent # attaches, with no terminal, so they MUST NOT try to read from a TTY. Docker's # kit-reference states startup commands are non-interactive; a kit that prompts @@ -1860,14 +1949,16 @@ _acq_msb_exec_flags_into() { # kit hung here). Defense-in-depth alongside the kit's own prompt-suppression: # - stdin from /dev/null for every kit exec, so nothing can read the TTY. # - GIT_TERMINAL_PROMPT=0 (+ GIT_ASKPASS/SSH_ASKPASS=false) so git fails fast - # instead of prompting. These are injected only if the kit did not already - # set GIT_TERMINAL_PROMPT (kits may override intentionally). - local _kit_env_str - eval "_kit_env_str=\" \${${_envarr}[*]-} \"" - case "$_kit_env_str" in - *" GIT_TERMINAL_PROMPT="*) : ;; - *) eval "$_eflag+=(-e \"GIT_TERMINAL_PROMPT=0\" -e \"GIT_ASKPASS=/bin/false\" -e \"SSH_ASKPASS=/bin/false\")" ;; - esac + # instead of prompting. Injected unless THIS kit set GIT_TERMINAL_PROMPT + # (kits may override intentionally) — the guard decision is per kit even + # though the env is merged, so another kit cannot disable these guards. See + # _acq_msb_env_tokens_with_guards_into. + local _envtok=() + _acq_msb_env_tokens_with_guards_into _envtok "$_envarr" "$_ownguards" + local _ev + for _ev in ${_envtok[@]+"${_envtok[@]}"}; do + eval "$_eflag+=(-e \"\$_ev\")" + done } # _acq_msb_exec_install NAME USER UFLAG_ARRVAR EFLAG_ARRVAR -- ARGV... — run an @@ -1936,8 +2027,8 @@ _acq_msb_exec_run() { } _acq_msb_exec_command() { - local name="$1" phase="$2" user="$3" background="$4" - shift 4 + local name="$1" phase="$2" user="$3" background="$4" ownguards="$5" + shift 5 # Split leading NAME=value env tokens (up to the `--` sentinel) from argv. local _kit_env=() _argv=() @@ -1945,9 +2036,10 @@ _acq_msb_exec_command() { [ "${#_argv[@]}" -gt 0 ] || return 0 - # Build the `msb exec` -u/-e flag arrays (uid mapping, kit env, git guards). + # Build the `msb exec` -u/-e flag arrays (uid mapping, merged kit env, and the + # git guards — decided from THIS kit's own guard vars, not the merged set). local uflag=() eflag=() - _acq_msb_exec_flags_into uflag eflag "$user" _kit_env + _acq_msb_exec_flags_into uflag eflag "$user" _kit_env "$ownguards" if [ "$phase" = "install" ]; then _acq_msb_exec_install "$name" "$user" uflag eflag -- "${_argv[@]}" @@ -2000,34 +2092,30 @@ _acq_msb_sq() { printf "'%s'" "${_s//\'/$_q}" } -# _acq_msb_startup_env_prefix_into PREFIX_ARRVAR USER KITENV_ARRVAR — build the -# in-guest `env NAME=value …` prefix tokens for one startup command: HOME for the -# agent user, the kit env vars, and the git non-interactive guards (unless the -# kit set GIT_TERMINAL_PROMPT). Mirrors _acq_msb_exec_flags_into's -e set, but as -# `env` argv rather than `msb exec -e`. Tokens are RAW here (NAME=value); the -# caller single-quote-escapes each before writing it to the script. +# _acq_msb_startup_env_prefix_into PREFIX_ARRVAR USER KITENV_ARRVAR OWNGUARDS — +# build the in-guest `env NAME=value …` prefix tokens for one startup command: +# HOME for the agent user, the MERGED kit env vars (KITENV_ARRVAR), and the git +# non-interactive guards (unless THIS kit declared them — OWNGUARDS keeps that +# decision per kit, exactly as the exec path does). Mirrors +# _acq_msb_exec_flags_into's -e set, but as `env` argv rather than `msb exec -e`. +# Tokens are RAW here (NAME=value); the caller single-quote-escapes each before +# writing it to the script. _acq_msb_startup_env_prefix_into() { - local _prefixn="$1" _user="$2" _envarrn="$3" + local _prefixn="$1" _user="$2" _envarrn="$3" _ownguards="${4:-}" eval "$_prefixn=()" case "$_user" in 1000|agent) eval "$_prefixn+=(\"HOME=/home/agent\")" ;; esac - # Kit-declared env vars (already NAME-validated by kit_spec_env). - local _ev - eval "set -- \${${_envarrn}[@]+\"\${${_envarrn}[@]}\"}" - for _ev in "$@"; do + # Kit-declared env vars (already NAME-validated by kit_spec_env) plus the + # non-interactive git guards, with the same per-kit guard scoping the exec path + # uses (see _acq_msb_env_tokens_with_guards_into). + local _envtok=() _ev + _acq_msb_env_tokens_with_guards_into _envtok "$_envarrn" "$_ownguards" + for _ev in ${_envtok[@]+"${_envtok[@]}"}; do eval "$_prefixn+=(\"\$_ev\")" done - - # Non-interactive git guards, unless the kit set GIT_TERMINAL_PROMPT itself. - local _kit_env_str - eval "_kit_env_str=\" \${${_envarrn}[*]-} \"" - case "$_kit_env_str" in - *" GIT_TERMINAL_PROMPT="*) : ;; - *) eval "$_prefixn+=(\"GIT_TERMINAL_PROMPT=0\" \"GIT_ASKPASS=/bin/false\" \"SSH_ASKPASS=/bin/false\")" ;; - esac } # _acq_msb_startup_emit_command USER BACKGROUND PREFIX_ARRVAR ARGV_ARRVAR — echo @@ -2120,16 +2208,78 @@ $(kit_spec_env "$_spec") EOF } +# _acq_msb_dedupe_env_into ARRVAR TOKEN... — store the NAME=value TOKENs in the +# array named ARRVAR with duplicates collapsed: the LAST value for a name wins, +# the FIRST appearance fixes the order. Same resolution rule the session replay +# applies to the persisted marker, so a command's env and a session's env agree +# on which kit's value survives. Malformed tokens (no `=`, empty name) are +# dropped. Tokens are read from argv and written back as whole array elements, +# so a value containing spaces or metacharacters is never re-split (SI-10). +_acq_msb_dedupe_env_into() { + local _arrn="$1" + shift + eval "$_arrn=()" + local _line + while IFS= read -r _line; do + [ -n "$_line" ] || continue + eval "$_arrn+=(\"\$_line\")" + done <\n" by name; $'\n' is a literal newline (ANSI-C). @@ -2176,14 +2326,16 @@ EOF [ "$_emitted" -eq 1 ] } -# _acq_msb_generate_startup_script SPEC OUTFILE — write the guest startup script -# for one kit spec's STARTUP-phase commands into OUTFILE. Returns 0 and writes a -# non-empty script iff the kit HAS at least one startup command; returns 1 (and -# writes nothing) when there are none, so the caller registers no empty script. +# _acq_msb_generate_startup_script SPEC OUTFILE [MERGEDENV_ARRVAR] — write the +# guest startup script for one kit spec's STARTUP-phase commands into OUTFILE. +# MERGEDENV_ARRVAR, when given, is the merged whole-kit-set env the body should +# carry (ADR-0033). Returns 0 and writes a non-empty script iff the kit HAS at +# least one startup command; returns 1 (and writes nothing) when there are none, +# so the caller registers no empty script. _acq_msb_generate_startup_script() { - local _spec="$1" _out="$2" _body="" + local _spec="$1" _out="$2" _mergedn="${3:-}" _body="" - _acq_msb_startup_body_into _body "$_spec" || return 1 + _acq_msb_startup_body_into _body "$_spec" "$_mergedn" || return 1 # Emit the file: a self-contained /bin/sh with its own shebang (so --script-path # reads a complete verbatim body; no --shell shebang derivation dependency). @@ -2198,7 +2350,8 @@ _acq_msb_generate_startup_script() { return 0 } -# _acq_msb_stage_startup_script SPEC ARRVAR — generate the startup script for one +# _acq_msb_stage_startup_script SPEC ARRVAR [MERGEDENV_ARRVAR] — generate the +# startup script for one # kit SPEC and, if non-empty, append a `--script-path acq-startup:` # entry into the create-flags array named ARRVAR. The host file is created under # ACQ_MSB_STARTUP_STAGE_DIR (a private 0700 dir under the acq state tree, so kit @@ -2237,7 +2390,7 @@ _acq_msb_generate_startup_script() { # Script at Boot") for the re-verification procedure and remediation. ACQ_MSB_STARTUP_SCRIPT_NAME="acq-startup" _acq_msb_stage_startup_script() { - local _spec="$1" _arrn="$2" + local _spec="$1" _arrn="$2" _mergedn="${3:-}" # One staged script per sandbox provision. If we already staged one, skip # (increment-1 scope; see NOTE above). @@ -2261,7 +2414,7 @@ _acq_msb_stage_startup_script() { } chmod 600 "$_file" 2>/dev/null || true - if _acq_msb_generate_startup_script "$_spec" "$_file"; then + if _acq_msb_generate_startup_script "$_spec" "$_file" "$_mergedn"; then # --script-path names a HOST file native msb reads, so pass the host form # (the shell keeps the POSIX _file for cleanup). See ADR-0029. local _hostfile="$_file" @@ -2764,15 +2917,35 @@ acq_backend_provision() { # declare the same path. _volrecs="${_volrecs} $(kit_spec_volumes "$spec")" + done - # Startup-phase commands → a create-time `--script-path acq-startup:` - # (ADR-0017). The script is REGISTERED at create; a bare registration is - # runtime-neutral (staged on the guest PATH, not auto-run at start). Restart - # durability is delivered by the acq `start`/`restart` verb re-running startup - # via the exec heal. install + mid-life apply stay exec-based (see the DESIGN - # NOTE and _acq_msb_apply_kit_dir). Only the first kit with startup commands - # stakes the fixed script name (see _acq_msb_stage_startup_script). - _acq_msb_stage_startup_script "$spec" create_flags + # MERGED KIT ENV (ADR-0033), computed ONCE per provision now that every kit is + # fetched: the union of all kits' environment[] with last-kit-wins. Threaded + # onto each kit's lifecycle commands (the apply loop below) and into the staged + # startup-script body, so a kit's `background: true` daemon sees the whole + # guest env, not only its own kit's vars. Computed here, BEFORE any kit is + # applied, so the result is independent of kit application order (the per-kit + # marker append happens inside the apply loop). + local _merged_kit_env=() _pspecs=() _kd + for _kd in ${kitdirs[@]+"${kitdirs[@]}"}; do + _pspecs+=("${_kd}/spec.yaml") + done + _acq_msb_merge_kit_env_into _merged_kit_env ${_pspecs[@]+"${_pspecs[@]}"} + + # Startup-phase commands → a create-time `--script-path acq-startup:` + # (ADR-0017). The script is REGISTERED at create; a bare registration is + # runtime-neutral (staged on the guest PATH, not auto-run at start). Restart + # durability is delivered by the acq `start`/`restart` verb re-running startup + # via the exec heal. install + mid-life apply stay exec-based (see the DESIGN + # NOTE and _acq_msb_apply_kit_dir). Only the first kit with startup commands + # stakes the fixed script name (see _acq_msb_stage_startup_script). + # + # Staged AFTER the fetch loop (not inside it) so the body can carry the merged + # kit env, which is only knowable once every kit has been fetched. Iterating + # kitdirs in the same order preserves the first-kit-stakes rule. + for _kd in ${kitdirs[@]+"${kitdirs[@]}"}; do + [ -f "${_kd}/spec.yaml" ] || continue + _acq_msb_stage_startup_script "${_kd}/spec.yaml" create_flags _merged_kit_env done # Volumes (ADR-0023) → create-time storage flags, from the union of every @@ -3309,12 +3482,18 @@ EOF # sandbox half-configured with no diagnostic. Warn and continue — the kits are # individually non-fatal (the playbook kit already self-heals on next start), # matching acq_backend_ensure_kits_applied's best-effort heal loop. + # + # Each kit's lifecycle commands run with the MERGED kit env computed in the + # pre-pass above (ADR-0033), so a kit's `background: true` daemon sees the whole + # guest env rather than only its own kit's vars. It has to be a PRE-pass: the + # marker is appended inside this loop, so reading it per kit would make the + # result depend on kit order (a daemon kit applied first would see nothing). local kd acq_spin_start "Applying configuration kits" _acq_msb_reset_kit_env "$name" for kd in "${kitdirs[@]}"; do acq_debug "msb provision: applying kit dir $kd ($name)" - if _acq_msb_apply_kit_dir "$name" "$kd"; then + if _acq_msb_apply_kit_dir "$name" "$kd" _merged_kit_env; then acq_debug "msb provision: applied kit dir $kd ($name)" else echo "acq(msb): warning: kit did not fully apply: $kd" >&2 @@ -3964,18 +4143,17 @@ EOF ' /dev/null 2>&1 || true } -# _acq_msb_kit_env_flags_into ARRVAR NAME — build the `-e NAME=value` flag array -# for the kit environment[] entries persisted at /var/lib/acq/kit-env by -# _acq_msb_apply_kit_dir, so every session path (run/attach/shell) sees the env -# the kits declared for agent runtime (see ADR-0011). Empty array when no kit -# declared environment[]. Array passed by name (bash 3.2 compat). +# _acq_msb_persisted_kit_env_into ARRVAR NAME — read the kit environment[] entries +# persisted at /var/lib/acq/kit-env by _acq_msb_apply_kit_dir into the array named +# ARRVAR as NAME=value tokens (see ADR-0011). Empty array when the marker is +# absent or no kit declared environment[]. Array passed by name (bash 3.2 compat). # # The marker is root-owned but its content is kit-derived guest data: re-validate # each NAME (same ^[A-Za-z_][A-Za-z0-9_]*$ charset kit_spec_env enforces) so a # tampered line cannot smuggle an option-shaped or quote-bearing token, and keep # the LAST value for a duplicate name (kits append in application order, so a # later kit overrides an earlier one). -_acq_msb_kit_env_flags_into() { +_acq_msb_persisted_kit_env_into() { local _arrn="$1" _name="$2" eval "$_arrn=()" # Failure-guarded: an absent marker (pre-kit-env sandbox, or no kit declared @@ -3988,7 +4166,7 @@ _acq_msb_kit_env_flags_into() { local _line while IFS= read -r _line; do [ -n "$_line" ] || continue - eval "$_arrn+=(-e \"\$_line\")" + eval "$_arrn+=(\"\$_line\")" done <&2 echo "acq(msb): this apply skips them. Recreate the sandbox (acq rm && acq run) to mount them." >&2 fi + # No merged-env array is passed here on purpose: a mid-life apply knows only + # THIS kit, so there is no full set to pre-merge. _acq_msb_apply_kit_dir falls + # back to the sandbox's already-persisted merged value (ADR-0033), which is the + # correct whole-guest view for an additive add. _acq_msb_apply_kit_dir "$name" "$kitdir" } +# _acq_msb_heal_kit_set NAME BUILTIN_COUNT KITREF... — the heal's kit loop: fetch +# every ref, merge their environment[] in a PRE-PASS, reset the marker, then apply +# each kit with that merged env. Returns 1 if any of the first BUILTIN_COUNT refs +# failed to fetch or apply (the caller's provenance verdict), 0 otherwise. +# +# Two loops, not one: the merged env must be known BEFORE the first kit is applied +# (ADR-0033). The per-kit marker append happens inside the apply loop, so reading +# the marker per kit would make a daemon kit's env depend on whether the +# env-declaring kit happened to be applied before it. _builtin mirrors _dirs so a +# built-in's apply failure is still attributable once the loops are separated. +_acq_msb_heal_kit_set() { + local name="$1" builtin_count="$2" + shift 2 + local kitref kitdir i=0 ok=1 + local _dirs=() _builtin=() + for kitref in "$@"; do + if kitdir=$(_acq_msb_fetch_kit "$kitref"); then + _dirs+=("$kitdir") + if [ "$i" -lt "$builtin_count" ]; then _builtin+=(1); else _builtin+=(0); fi + else + echo "acq(msb): warning: could not fetch kit for healing: $kitref" >&2 + # A built-in kit that can't even be fetched means we cannot claim the + # bundle is current. Extra-kit fetch failures don't affect the verdict. + [ "$i" -lt "$builtin_count" ] && ok=0 + fi + i=$((i + 1)) + done + local _merged=() _specs=() + for kitdir in ${_dirs[@]+"${_dirs[@]}"}; do + _specs+=("${kitdir}/spec.yaml") + done + _acq_msb_merge_kit_env_into _merged ${_specs[@]+"${_specs[@]}"} + _acq_msb_reset_kit_env "$name" + local _ki + for _ki in ${_dirs[@]+"${!_dirs[@]}"}; do + if ! _acq_msb_apply_kit_dir "$name" "${_dirs[$_ki]}" _merged; then + [ "${_builtin[$_ki]}" -eq 1 ] && ok=0 + fi + done + [ "$ok" -eq 1 ] +} + # --------------------------------------------------------------------------- # acq_backend_ensure_kits_applied — best-effort heal of a sandbox missing kits # --------------------------------------------------------------------------- @@ -5163,23 +5402,9 @@ acq_backend_ensure_kits_applied() { if [ "${#ACQ_CLI_KITS[@]}" -gt 0 ]; then kits+=("${ACQ_CLI_KITS[@]}") fi - local kitref kitdir i=0 ok=1 + local ok=1 acq_spin_start "Refreshing configuration kits" - _acq_msb_reset_kit_env "$name" - for kitref in "${kits[@]}"; do - kitdir=$(_acq_msb_fetch_kit "$kitref") || { - echo "acq(msb): warning: could not fetch kit for healing: $kitref" >&2 - # A built-in kit that can't even be fetched means we cannot claim the - # bundle is current. Extra-kit fetch failures don't affect the verdict. - [ "$i" -lt "$builtin_count" ] && ok=0 - i=$((i + 1)) - continue - } - if ! _acq_msb_apply_kit_dir "$name" "$kitdir"; then - [ "$i" -lt "$builtin_count" ] && ok=0 - fi - i=$((i + 1)) - done + _acq_msb_heal_kit_set "$name" "$builtin_count" "${kits[@]}" || ok=0 acq_spin_stop "Refreshing configuration kits" # Record host-side bundle provenance ONLY when every built-in kit applied. # msb re-applies all built-in kits idempotently, so on full diff --git a/docs/BACKEND_GUIDE.md b/docs/BACKEND_GUIDE.md index 3e9c65f4..7cc211d4 100644 --- a/docs/BACKEND_GUIDE.md +++ b/docs/BACKEND_GUIDE.md @@ -1035,7 +1035,13 @@ The neutral vocabulary is: `caps.network.allow`, `files[]`, `commands[]`, `OPENCODE_TUI_CONFIG`, `GITLAB_HOST`). Names must be POSIX identifiers (`^[A-Za-z_][A-Za-z0-9_]*$`; an invalid name is dropped with a warning and reported by `acq kit validate`); values are plain strings. It maps to sbx-v2 -`environment.variables` (synthesized) and to `msb exec -e NAME=value` (per-exec). +`environment.variables` (synthesized) and, on msb, to `msb exec -e NAME=value`. +On both backends these variables are **guest-wide**, not per kit: msb threads the +**merged** set from every applied kit onto every kit's lifecycle commands and +replays the same set on `acq exec`/`acq shell`/attach, so a daemon started by one +kit's `background: true` startup command still sees another kit's config +(last-value-wins for a duplicate name — see +[ADR-0033](adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md)). **Secrets do NOT go here** — use the credential/secret path (`acq secret …`); the kit spec never carries a secret value. diff --git a/docs/adr/0011-msb-backend-and-neutral-kits.md b/docs/adr/0011-msb-backend-and-neutral-kits.md index c7406092..5b4b0b0f 100644 --- a/docs/adr/0011-msb-backend-and-neutral-kits.md +++ b/docs/adr/0011-msb-backend-and-neutral-kits.md @@ -412,6 +412,21 @@ strings). The translate layer: > sbx's sandbox-level env semantics. Names are re-validated on replay (tampered > marker defense) and the last value wins for a duplicate name (kits append in > application order, so a later kit overrides an earlier one). +> +> **Update (2026-09-29):** The per-kit scoping of the *command* env above is also +> gone. Widening only the session paths left a gap wherever a kit command +> outlives itself: a `background: true` startup daemon launches agents, and those +> agents inherited only the daemon kit's vars, so another kit's agent +> configuration (including a permission layer) was silently absent. Kit +> `environment[]` is now **guest-wide for kit lifecycle commands too** — every +> `install`/`initFiles`/`startup` command runs with the merged env of the full +> effective kit set, last-value-wins, the same resolution sessions replay. The +> merged set is computed in a **pre-pass over the whole kit set** (so it does not +> depend on kit application order), with the persisted marker as the fallback for +> the mid-life single-kit `acq kit apply` path. The adapter's non-interactive git +> guards stay scoped **per kit**: one kit setting `GIT_TERMINAL_PROMPT` must not +> disable them for another kit's commands. See +> [ADR-0033](0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md). Deliberately minimal (YAGNI): **static string values only** — no interpolation, no references to `files[]`-staged paths (a kit needing a computed value uses a diff --git a/docs/adr/0017-msb-create-time-startup-script-staging.md b/docs/adr/0017-msb-create-time-startup-script-staging.md index da687532..3b97908b 100644 --- a/docs/adr/0017-msb-create-time-startup-script-staging.md +++ b/docs/adr/0017-msb-create-time-startup-script-staging.md @@ -128,6 +128,27 @@ re-passed on the same invocation stays authoritative. This is consistent with th above: the verb now restores the *full* kit set, not just the built-ins. See `docs/KNOWN_FAILURE_MODES.md` §33. +## Update (2026-09-29): the staged body carries the merged, guest-wide kit env + +Kit `environment[]` became guest-wide for **all** kit lifecycle commands +([ADR-0033](0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md)): each +command runs with the merged env of the full effective kit set rather than only +its own kit's vars. The generated `--script-path` body now emits the same merged +`env NAME=value …` prefix, so the staged path and the exec path cannot drift. + +Two notes specific to this ADR: + +- The staged body is still **inert at boot** (a bare `--script-path` registration + stages the script on the guest PATH; microsandbox does not auto-run it), so this + change fixes no live symptom here. It exists so the re-verification trap above — + a future msb that *does* auto-run a registered script — cannot resurface the + wrong-env bug in a path nobody re-checked. +- The staging call moved **out of** the kit fetch loop in + `acq_backend_provision` into a second loop over the same kit dirs, because the + merged env is only knowable once every kit has been fetched. The + first-kit-with-startup-commands-stakes-the-script-name rule is unchanged + (same list, same order). + --- # ADR-0017: Stage msb kit startup commands as a create-time script for restart durability (original) diff --git a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md new file mode 100644 index 00000000..94a8f108 --- /dev/null +++ b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md @@ -0,0 +1,155 @@ +--- +title: "msb kit environment[] is guest-wide for kit lifecycle commands" +status: accepted +date: 2026-09-29 +decision_makers: ["Bret Mogilefsky"] +category: architecture +nist_controls: ["AC-3", "AC-6", "CM-2", "CM-6", "CM-7", "SA-8", "SA-15", "SI-10"] +impact_level: low +ato_relevance: no +risk_treatment: mitigate +supersedes: [] +--- + +# ADR-0033: msb kit `environment[]` is guest-wide for kit lifecycle commands + +## Context and Problem Statement + +On the msb backend, a kit's lifecycle commands (`install`, `initFiles`, +`startup`) ran with only **that kit's own** `environment[]`, threaded as +`msb exec -e NAME=value`. Interactive sessions were already whole-guest: acq +replays the merged marker on `run`/`attach`/`shell` (ADR-0011's 2026-08-26 +update). Commands started by a kit's lifecycle phase were not. + +That split is invisible until a kit's command **outlives the command** and +launches other processes. A daemon kit starts its supervisor from a +`background: true` startup command, so every agent that daemon later spawns +inherits only the daemon kit's env. A separate kit that sets `OPENCODE_CONFIG` +(agent instructions plus a default-deny permission layer) is silently missing +from those agents, while the injected credentials are still present — a quiet +loss of a policy/permission control, not a visible failure. Terminal sessions +look correct, so a smoke test passes. + +The hard part is **order independence**. The persisted marker +(`/var/lib/acq/kit-env`) is appended per kit *inside the same loop that runs that +kit's commands*, so "read the merged marker when running commands" is correct +only when the env-declaring kit happens to be applied before the daemon kit, and +silently wrong in the reverse order. + +## Decision Drivers + +- **A kit must not be able to silently drop another kit's policy config** from + the environment of agents it launches (AC-3, AC-6, CM-6). +- **Order independence** — the effective env must not depend on the order kits + are applied in, which is an implementation detail of the kit list. +- **One kit must not weaken another kit's hardening** — the adapter's + non-interactive git guards are per-kit safety, not shared config (CM-7). +- **Same resolution rule as sessions** — last-value-wins, so a command and a + session agree on which kit's value survives. +- **No new guest dependency** — kits must not each have to source a marker file + themselves; the adapter owns the threading. +- **Fail soft, never empty** — an unreadable marker degrades to the previous + per-kit behavior rather than stripping a kit's own env. + +## Considered Options + +1. **Pre-pass over the full kit set in the full-set paths; marker fallback for + the mid-life single-kit path.** Chosen. +2. **Read the persisted marker inside `_acq_msb_run_commands` for every path.** + Rejected: order-dependent by construction — the marker is written in the same + per-kit loop, so a daemon kit applied before the env-declaring kit still sees + nothing. It would fix the reported case only for one kit ordering. +3. **Have each kit source `/var/lib/acq/kit-env` in its own commands.** Rejected: + pushes an adapter concern into every kit, is opt-in (so a kit that forgets is + silently broken), and couples kit content to a guest path acq owns. +4. **Do nothing; document that kit env is session-only.** Rejected: the + vocabulary exists for agent-runtime config (ADR-0011), and the failure mode is + a silent loss of permission/policy configuration. + +## Decision Outcome + +**Chosen: Option 1.** Kit `environment[]` is treated as **guest-wide +configuration**, so every kit lifecycle command runs with the merged env of the +full effective kit set, using the same last-value-wins order sessions replay. + +Where the merged set comes from, per path: + +| Path | Source of the merged env | +|------|--------------------------| +| `acq_backend_provision` (create) | pre-pass over every fetched kit spec, computed **before** the first kit is applied | +| `acq_backend_ensure_kits_applied` (heal / `acq start`, `restart`, re-attach) | same pre-pass; kit refs are resolved in one loop, merged, then applied in a second loop | +| `acq_backend_apply_kit` (`acq kit apply NAME KITREF`) | no full set exists — falls back to the sandbox's already-persisted merged marker value | +| neither available (marker absent or unwritable) | this kit's own `environment[]` — the pre-existing behavior | + +The mid-life fallback is correct for an additive add: the new kit's own entries +are appended to the marker *before* its commands run, and the read applies +last-value-wins, so the new kit still overrides an older value. + +**Guard scoping stays per kit (load-bearing).** The adapter injects +`GIT_TERMINAL_PROMPT=0` + `GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit +commands unless *that* kit declared `GIT_TERMINAL_PROMPT` itself. Deciding this +from the *merged* set would let one kit's opt-out disable the guards for every +other kit's commands — and a prompting kit command with no credential blocks +provision forever (observed with the playbook kit). So the merged set is widened +for **threading** only: a guard var contributed by a *different* kit is filtered +out of a command's env and the adapter's own guard value is used instead. A kit +that declares a guard var still overrides it for its own commands. + +The staged create-time `--script-path` body (ADR-0017) gets the same treatment, +for consistency. **That path is inert today** — a bare `--script-path` +registration is staged on the guest PATH and is not auto-run at boot — so this +part fixes no live symptom; it keeps the two paths from drifting so the ADR-0017 +version-bump re-verification trap (a future msb that *does* auto-run the script) +cannot resurface this bug. + +### Positive Consequences + +- A daemon kit's launched agents see every kit's declared config, so a policy/ + permission layer contributed by another kit is no longer silently absent. +- The effective env is identical in both kit application orders. +- Commands and sessions now agree on the env and on duplicate resolution. +- Kits need no knowledge of `/var/lib/acq/kit-env`. + +### Negative Consequences + +- **Broader blast radius per var.** A var declared by one kit now reaches every + kit's commands. This is the intended semantics (it matches sbx, where + `environment.variables` is sandbox-level), but a kit author can no longer + assume their command's env is theirs alone. +- **Staging-order coupling at create.** The `--script-path` staging moved out of + the kit fetch loop into a second loop so the merged env is known first. The + first-kit-stakes-the-script-name rule is preserved by iterating the same list + in the same order. +- **Not addressed: a merged var can name a not-yet-staged path.** Kit *N*'s + commands can now receive e.g. `OPENCODE_CONFIG` pointing at a file kit *N+k* + has not copied in yet. Before this change the daemon saw **no** var; now it can + see a var pointing at a **missing** file, which converts "no team config" into + "possibly broken team config" for the window between the daemon starting and + the declaring kit's files landing. This is the same class as the sbx + startup/attach race (a startup command racing the state it depends on, tracked + separately — see Links) and is deliberately out of scope here: fixing it needs + an ordering/barrier decision (stage every kit's `files[]` before any kit's + commands, or a readiness barrier before background startup), not an env-scoping + one. Kits whose commands read a path from a var should already tolerate its + absence. +- The guard-scoping rule is a real subtlety in the adapter: merged for threading, + per-kit for the guard decision. It is asserted by a regression test so a future + simplification cannot quietly collapse the two. + +## Links + +- ADR-0011 (msb backend and neutral hybrid/v1 kits) — defines the `environment` + vocabulary and the session-replay marker this widens to lifecycle commands. +- ADR-0014 (neutral port-publish and background-command vocabulary) — defines the + `background: true` startup commands whose daemons made the scoping gap visible. +- ADR-0017 (msb create-time startup-script staging) — the staged body updated for + consistency here, including its inertness caveat and re-verification trap. +- `acq.backends/msb.sh` — `_acq_msb_merge_kit_env_into` (the pre-pass), + `_acq_msb_env_tokens_with_guards_into` (merged threading, per-kit guards), + `_acq_msb_persisted_kit_env_into` (the mid-life fallback and session replay). +- `test/bats/78-msb-kit-env.bats`, `test/bats/73-msb-kits-startup.bats` — the + offline regression coverage (both kit orders, all three phases, the mid-life + and heal paths, duplicate resolution, and the guard-scoping rule). +- Reported as GSA-TTS/agentic-coding-quickstart#515. The related + startup-races-its-own-prerequisites class noted under Negative Consequences is + tracked as GSA-TTS/agentic-coding-quickstart#506. diff --git a/test/bats/73-msb-kits-startup.bats b/test/bats/73-msb-kits-startup.bats index 6cc39853..5d3e98b5 100644 --- a/test/bats/73-msb-kits-startup.bats +++ b/test/bats/73-msb-kits-startup.bats @@ -475,3 +475,71 @@ SPEC assert_regex "$body" 'echo NAMED_USER_MARKER' refute_regex "$body" 'runuser -u agent' } + +@test "0017 + ADR-0033: the staged startup body carries the MERGED kit env, guards per kit" { + # The staged --script-path body must scope env the same way the exec path does: + # merged across the whole kit set, but the git-guard decision kept per kit. This + # path is INERT today (a bare --script-path registration is staged on the guest + # PATH, not auto-run at boot — see ADR-0017), so fixing it does not fix the + # reported daemon symptom; it keeps the two paths from drifting, so the ADR-0017 + # re-verification trap (a future msb that DOES auto-run the script) cannot turn + # into a live instance of the same bug. + : > "$CALLS" + run bash -c ' + export ACQ_SECRET_STORE_DIR="'"$STUBDIR"'/mergestage-secrets" + export ACQ_MSB_KEEP_STARTUP_STAGE=1 ACQ_MSB_STARTUP_STAGE_DIR="'"$STUBDIR"'/mergestage-stage" + . "'"$REPO_ROOT"'/acq.backends/secret-store.sh" + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + # The kit with the startup command is fetched FIRST (it stakes the single + # script name), and the kit declaring the env comes LATER — the order that + # fails without a pre-pass over the full kit set. + sk="'"$STUBDIR"'/stagestartupkit"; mkdir -p "$sk" + cat >"$sk/spec.yaml" <<'"'"'SPEC'"'"' +schemaVersion: "hybrid/v1" +kind: mixin +name: stage-startup-kit +displayName: Stage Startup Kit +description: owns the staged startup command and opts out of the git guards +environment: + GIT_TERMINAL_PROMPT: "1" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo STAGED_MERGED_MARKER +SPEC + ck="'"$STUBDIR"'/stageconfigkit"; mkdir -p "$ck" + cat >"$ck/spec.yaml" <<'"'"'SPEC'"'"' +schemaVersion: "hybrid/v1" +kind: mixin +name: stage-config-kit +displayName: Stage Config Kit +description: declares agent-runtime config a later-merged body must carry +environment: + OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc +SPEC + nk="'"$STUBDIR"'/stagenokit"; mkdir -p "$nk" + printf "schemaVersion: \"hybrid/v1\"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n" > "$nk/spec.yaml" + ZSCALER_KIT=k1; USAI_KIT=k2; PLAYBOOK_KIT=k3; GITSSHSIGN_KIT=k4 + _acq_msb_fetch_kit() { + case "$1" in k1) printf "%s\n" "$sk" ;; k2) printf "%s\n" "$ck" ;; *) printf "%s\n" "$nk" ;; esac + } + acq_backend_provision stagemergebox shell /tmp >/dev/null 2>&1 + ' + local body; body=$(_staged_body mergestage-stage) + assert_regex "$body" 'echo STAGED_MERGED_MARKER' + # The other kit's var reaches this kit's staged command. + assert_regex "$body" "'OPENCODE_CONFIG=/home/agent/.config/opencode/team.jsonc'" + # This kit opted out of the guards, so its own value stands and acq's does not + # appear alongside it. + assert_regex "$body" "'GIT_TERMINAL_PROMPT=1'" + refute_regex "$body" 'GIT_TERMINAL_PROMPT=0' + # Still a valid, busybox-safe /bin/sh body. + refute_regex "$body" 'env --' + local f; f=$(find "$STUBDIR/mergestage-stage" -type f 2>/dev/null | head -n1) + run sh -n "$f" + assert_success +} diff --git a/test/bats/78-msb-kit-env.bats b/test/bats/78-msb-kit-env.bats index d02282da..e0a366d6 100644 --- a/test/bats/78-msb-kit-env.bats +++ b/test/bats/78-msb-kit-env.bats @@ -70,7 +70,11 @@ SPEC _acq_msb_apply_kit_dir envbox "$nk" ' assert_success - refute_regex "$(cat "$CALLS")" '/var/lib/acq/kit-env' + # Asserts the WRITE specifically (`>> /var/lib/acq/kit-env`), not any mention of + # the path: a kit with no environment[] of its own still READS the merged marker + # so its commands inherit the other kits' env (see ADR-0033) — that read is + # expected here, an append is not. + refute_regex "$(cat "$CALLS")" '>> /var/lib/acq/kit-env' } @test "msb kit env: acq exec replays persisted entries as -e flags; none when marker empty" { @@ -256,3 +260,294 @@ GITLAB_HOST=gitlab.override.gov" assert_regex "$log" '-e GITLAB_HOST=gitlab\.override\.gov' refute_regex "$log" 'GITLAB_HOST=gitlab\.example\.gov' } + +# --- merged kit env on kit lifecycle commands (ADR-0033) ------------------- +# +# Kit env is guest-wide guest configuration, but on msb a kit's lifecycle +# commands used to run with only THAT kit's environment[]. The reported symptom: +# a `background: true` startup daemon (the Paseo kit's `paseo daemon`) launches +# agents, and those agents inherited only the daemon kit's vars — a team kit's +# OPENCODE_CONFIG (team instructions + a default-deny permission layer) was +# silently absent while the secrets were still present. The fix threads the +# MERGED env of the full effective kit set onto every lifecycle command. + +# _env_merge_provision KITDIRS... — provision `mergebox` with the given local kit +# dirs as the full built-in kit set, in the order given (so the same two kits can +# be applied in either order). Echoes nothing; assertions read $CALLS. +_env_merge_provision() { + : > "$CALLS" + local a="$1" b="$2" + run bash -c ' + a="$1"; b="$2" + export ACQ_SECRET_STORE_DIR="'"$STUBDIR"'/merge-secrets" + export ACQ_MSB_STARTUP_STAGE_DIR="'"$STUBDIR"'/merge-stage" + . "'"$REPO_ROOT"'/acq.backends/secret-store.sh" + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + ZSCALER_KIT=k1; USAI_KIT=k2; PLAYBOOK_KIT=k3; GITSSHSIGN_KIT=k4 + nk="'"$STUBDIR"'/mergenokit"; mkdir -p "$nk" + printf "schemaVersion: \"hybrid/v1\"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n" > "$nk/spec.yaml" + _acq_msb_fetch_kit() { + case "$1" in k1) printf "%s\n" "$a" ;; k2) printf "%s\n" "$b" ;; *) printf "%s\n" "$nk" ;; esac + } + acq_backend_provision mergebox shell /tmp >/dev/null 2>&1 + ' _ "$a" "$b" +} + +# Write a kit dir that declares OPENCODE_CONFIG and has no commands. +_env_kit_config() { + local d="$1"; mkdir -p "$d" + cat > "$d/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: team-kit +displayName: Team Kit +description: declares agent-runtime config other kits' commands must see +environment: + OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc +SPEC +} + +# Write a kit dir whose commands span all three lifecycle phases, the startup one +# detached (the reported daemon shape). +_env_kit_daemon() { + local d="$1"; mkdir -p "$d" + cat > "$d/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: daemon-kit +displayName: Daemon Kit +description: install + initFiles + a background startup daemon +environment: + PASEO_OWN_VAR: own +commands: + - phase: install + user: "0" + command: + - sh + - -c + - echo INSTALL_CMD + - phase: initFiles + user: "0" + command: + - sh + - -c + - echo INITFILES_CMD + - phase: startup + user: "0" + background: true + command: + - paseo-daemon +SPEC +} + +@test "msb kit env (ADR-0033): merged env reaches a background startup daemon and every phase" { + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" + local log; log=$(cat "$CALLS") + # The detached daemon exec carries the OTHER kit's var, not just its own. + local bg; bg=$(printf '%s\n' "$log" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e PASEO_OWN_VAR=own' + # install and initFiles too — the same guest-wide env contract. + local inst; inst=$(printf '%s\n' "$log" | grep 'echo INSTALL_CMD' | head -n1) + assert_regex "$inst" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + local init; init=$(printf '%s\n' "$log" | grep 'echo INITFILES_CMD' | head -n1) + assert_regex "$init" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' +} + +@test "msb kit env (ADR-0033): the merged env is the same in BOTH kit application orders" { + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + # config kit applied FIRST (the order a per-kit marker read would also pass). + _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" + local bg1; bg1=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg1" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + # daemon kit applied FIRST — the order that silently fails without a pre-pass: + # the marker does not yet hold the config kit's entry when the daemon launches. + _env_merge_provision "$STUBDIR/daemonkit" "$STUBDIR/cfgkit" + local bg2; bg2=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg2" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' +} + +@test "msb kit env (ADR-0033): a duplicate name resolves last-kit-wins on commands" { + local k1="$STUBDIR/dupkit1" k2="$STUBDIR/dupkit2" + mkdir -p "$k1" "$k2" + cat > "$k1/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: dup-one +displayName: Dup One +description: sets GITLAB_HOST first and owns the command +environment: + GITLAB_HOST: gitlab.first.gov +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo DUP_CMD +SPEC + cat > "$k2/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: dup-two +displayName: Dup Two +description: overrides GITLAB_HOST from a later kit +environment: + GITLAB_HOST: gitlab.second.gov +SPEC + _env_merge_provision "$k1" "$k2" + local cmd; cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo DUP_CMD' | head -n1) + # The command belongs to the EARLIER kit, yet must see the LATER kit's value — + # the same resolution the session replay applies, and exactly once (never both). + assert_regex "$cmd" '\-e GITLAB_HOST=gitlab\.second\.gov' + refute_regex "$cmd" 'gitlab\.first\.gov' +} + +# Guard test, not a bug reproduction: this passes BEFORE the merged-env change too +# (each kit then saw only its own env, so the question could not arise). It exists +# because widening the threaded env makes the failure reachable — if the merged set +# were also used to DECIDE the guards, one kit setting GIT_TERMINAL_PROMPT would +# disable GIT_TERMINAL_PROMPT=0/GIT_ASKPASS/SSH_ASKPASS for every other kit's +# commands, and a prompting kit command would block provision forever. +@test "msb kit env (ADR-0033): one kit's GIT_TERMINAL_PROMPT does not strip another kit's guards" { + local gk="$STUBDIR/gitpromptkit" pk="$STUBDIR/plainkit" + mkdir -p "$gk" "$pk" + # A kit that deliberately opts out of the non-interactive git guards. + cat > "$gk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: prompt-kit +displayName: Prompt Kit +description: opts itself out of the git guards +environment: + GIT_TERMINAL_PROMPT: "1" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo PROMPT_KIT_CMD +SPEC + # A different kit that did NOT opt out: its commands must keep the guards, or a + # prompting command would block provision forever. + cat > "$pk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: plain-kit +displayName: Plain Kit +description: relies on the adapter's git guards +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo PLAIN_KIT_CMD +SPEC + _env_merge_provision "$gk" "$pk" + local log; log=$(cat "$CALLS") + local plain; plain=$(printf '%s\n' "$log" | grep 'echo PLAIN_KIT_CMD' | head -n1) + assert_regex "$plain" '\-e GIT_TERMINAL_PROMPT=0' + assert_regex "$plain" '\-e GIT_ASKPASS=/bin/false' + assert_regex "$plain" '\-e SSH_ASKPASS=/bin/false' + refute_regex "$plain" 'GIT_TERMINAL_PROMPT=1' + # The opting-out kit still gets its own value, and only that value. + local prompt; prompt=$(printf '%s\n' "$log" | grep 'echo PROMPT_KIT_CMD' | head -n1) + assert_regex "$prompt" '\-e GIT_TERMINAL_PROMPT=1' + refute_regex "$prompt" 'GIT_TERMINAL_PROMPT=0' +} + +@test "msb kit env (ADR-0033): mid-life 'kit apply' recovers the merged env from the marker" { + # A mid-life single-kit apply has NO full kit set to pre-merge, so it must fall + # back to the env the sandbox already persisted — otherwise adding a daemon kit + # to a live sandbox reintroduces exactly the reported bug for that kit. + printf 'OPENCODE_CONFIG=/home/agent/.config/opencode/team.jsonc\n' > "$STUBDIR/.kit_env" + local mk="$STUBDIR/midlifekit"; mkdir -p "$mk" + cat > "$mk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: midlife-kit +displayName: Midlife Kit +description: added to a live sandbox; its own env plus what is already persisted +environment: + MIDLIFE_OWN: yes +commands: + - phase: startup + user: "0" + background: true + command: + - midlife-daemon +SPEC + : > "$CALLS" + ( export ACQ_SECRET_STORE_DIR="$STUBDIR/midlife-secrets" + . "${REPO_ROOT}/acq.backends/secret-store.sh" + . "${REPO_ROOT}/acq.backends/kit-translate.sh" + . "${REPO_ROOT}/acq.backends/msb.sh" + acq_backend_apply_kit midlifebox "$mk" >/dev/null 2>&1 ) + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e MIDLIFE_OWN=yes' +} + +@test "msb kit env (ADR-0033): the heal path also merges, daemon kit applied first" { + # The heal (acq start/restart, re-attach) is the OTHER full-set path, and it is + # the one that restarts a kit's daemon on resume. Same pre-pass requirement: the + # daemon kit is healed FIRST here, so a per-kit marker read would give it an env + # missing the config kit's var. + printf 'healmergebox\n' > "$STUBDIR/.msb_sandbox_list" + printf 'healmergebox\n' > "$STUBDIR/.msb_running_list" + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + : > "$CALLS" + ( export ACQ_SECRET_STORE_DIR="$STUBDIR/healmerge-secrets" + export ACQ_MSB_STARTUP_STAGE_DIR="$STUBDIR/healmerge-stage" + . "${REPO_ROOT}/acq.backends/secret-store.sh" + . "${REPO_ROOT}/acq.backends/kit-translate.sh" + . "${REPO_ROOT}/acq.backends/msb.sh" + # shellcheck disable=SC2034 # read by the sourced acq_backend_ensure_kits_applied + ACQ_CLI_KITS=() + nk="$STUBDIR/healmergenokit"; mkdir -p "$nk" + printf 'schemaVersion: "hybrid/v1"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n' > "$nk/spec.yaml" + # Resolve by CALL ORDER rather than by kit ref, so the built-in kit refs stay + # untouched: the heal fetches them in list order, so call 1 is the daemon kit + # and call 2 the config kit — the daemon applied FIRST. + printf 0 > "$STUBDIR/.healmerge_n" + _acq_msb_fetch_kit() { + local n; n=$(cat "$STUBDIR/.healmerge_n") + printf '%s' "$((n + 1))" > "$STUBDIR/.healmerge_n" + case "$n" in + 0) printf '%s\n' "$STUBDIR/daemonkit" ;; + 1) printf '%s\n' "$STUBDIR/cfgkit" ;; + *) printf '%s\n' "$nk" ;; + esac + } + acq_backend_ensure_kits_applied healmergebox >/dev/null 2>&1 ) + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e PASEO_OWN_VAR=own' +} + +@test "msb kit env (ADR-0033): the merge re-validates names and never re-splits a value" { + # The merge helper is a second place kit-derived NAME=value tokens flow through, + # so it re-applies the kit_spec_env charset itself (defense in depth), and each + # token stays ONE array element so a value with spaces or metacharacters is + # never re-split into a second variable (SI-10). + run bash -c ' + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + m=() + _acq_msb_dedupe_env_into m "BAD-NAME=x" "=noname" "noequals" \ + "OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED" "OK=1" + printf "TOKEN[%s]\n" ${m[@]+"${m[@]}"} + ' + assert_success + refute_output --partial 'BAD-NAME' + refute_output --partial 'noequals' + assert_output --partial 'TOKEN[OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED]' + assert_output --partial 'TOKEN[OK=1]' +} From eb9284b319580d6a9c2a8226da12be7503cd0d7c Mon Sep 17 00:00:00 2001 From: Basilio Bogado <541149+basiliskus@users.noreply.github.com> Date: Tue, 29 Sep 2026 22:51:48 +0000 Subject: [PATCH 2/4] fix(msb): track git-guard ownership by value, not just by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a regression against base in the merged-env change: guard ownership was recorded as a set of NAMES, but _acq_msb_merge_kit_env_into has already collapsed a duplicate name to the LAST kit's value before the filter runs. When the owning kit had declared a guard name, the filter kept the merged token without knowing whose value it now held. With kit A declaring GIT_TERMINAL_PROMPT=0 and a later kit B declaring =1, kit A's own command ran with kit B's opt-out and with no GIT_ASKPASS/SSH_ASKPASS at all — where before the merged-env change it correctly got its own 0. One kit could make another kit's command prompt, and a prompting kit command blocks provision forever. Ownership is now the owning kit's guard TOKENS (NAME=value), carried by array name (a guard value may contain spaces and must never be re-split): _acq_msb_own_guard_tokens_into replaces _acq_msb_own_guard_names, and _acq_msb_env_tokens_with_guards_into strips EVERY guard-name token from the merged set regardless of contributor, re-adds this kit's own guard tokens, then appends acq's guard values unless this kit set GIT_TERMINAL_PROMPT itself. That closes both collision shapes — another kit declaring a guard name this kit does not, and another kit declaring the same name with a different value — while a kit's own intentional override still stands for its own commands. Applied to both the exec path and the staged --script-path body, which shared the defect. Also split the ADR-0033 lifecycle-command regressions out of 78-msb-kit-env.bats (which had grown to 553 lines) into a new 79-msb-merged-kit-env.bats. 78 keeps the marker/session-replay coverage it was written for; nothing pre-existing moved or was renumbered. Refs: GSA-TTS/agentic-coding-quickstart#515 --- acq.backends/msb.sh | 141 +++--- docs/adr/0011-msb-backend-and-neutral-kits.md | 7 +- ...nv-is-guest-wide-for-lifecycle-commands.md | 35 +- test/bats/73-msb-kits-startup.bats | 59 +++ test/bats/78-msb-kit-env.bats | 291 ------------ test/bats/79-msb-merged-kit-env.bats | 413 ++++++++++++++++++ 6 files changed, 589 insertions(+), 357 deletions(-) create mode 100644 test/bats/79-msb-merged-kit-env.bats diff --git a/acq.backends/msb.sh b/acq.backends/msb.sh index 7f2c5fac..9853aa31 100644 --- a/acq.backends/msb.sh +++ b/acq.backends/msb.sh @@ -1766,12 +1766,14 @@ _acq_msb_run_commands() { _acq_msb_collect_kit_env_into _kit_env "$spec" fi - # The git-guard decision stays PER KIT: only the guard vars THIS spec declares - # suppress the adapter's guards, so another kit's GIT_TERMINAL_PROMPT in the - # merged set cannot disable them here (see _acq_msb_env_tokens_with_guards_into). - local _own_env=() _ownguards + # The git guards stay PER KIT: a command may only ever receive THIS spec's own + # guard tokens, so neither another kit declaring a guard name this kit does not, + # nor another kit declaring the same name with a different value (which the merge + # resolves in the later kit's favor), can change or disable the guards here. See + # _acq_msb_env_tokens_with_guards_into. + local _own_env=() _own_guard_tok=() _acq_msb_collect_kit_env_into _own_env "$spec" - _ownguards=$(_acq_msb_own_guard_names _own_env) + _acq_msb_own_guard_tokens_into _own_guard_tok _own_env # Buffer the parsed command stream FIRST, then execute. The exec step calls # `msb exec`, which would consume this loop's stdin if we iterated the heredoc @@ -1799,7 +1801,7 @@ EOF ;; "__END__") reading=0 - _acq_msb_exec_command "$name" "$phase" "$user" "$background" "$_ownguards" \ + _acq_msb_exec_command "$name" "$phase" "$user" "$background" _own_guard_tok \ ${_kit_env[@]+"${_kit_env[@]}"} -- ${argv[@]+"${argv[@]}"} ;; *) @@ -1849,66 +1851,90 @@ _acq_msb_split_env_argv() { # The three non-interactive git guard vars the adapter injects onto kit lifecycle # commands (see _acq_msb_env_tokens_with_guards_into). ACQ_MSB_GIT_GUARD_NAMES="GIT_TERMINAL_PROMPT GIT_ASKPASS SSH_ASKPASS" - -# _acq_msb_own_guard_names ENVARR — echo the space-joined subset of -# ACQ_MSB_GIT_GUARD_NAMES that the array named ENVARR (ONE kit spec's own -# environment[]) declares. The result is a word list drawn from a FIXED -# adapter-owned name set, never kit bytes, so it is safe to pass as a single -# positional argument through the exec/staging call chain. -_acq_msb_own_guard_names() { - local _envarrn="$1" _names="" _g _ev +# A permanently-empty array used as the default "this kit declared no guard vars" +# argument, so the by-name guard-token parameter is always a real array even when a +# caller omits it (set -u safe). +_acq_msb_no_guards=() + +# _acq_msb_own_guard_tokens_into ARRVAR ENVARR — store, in the array named ARRVAR, +# the subset of the array named ENVARR (ONE kit spec's own environment[]) whose +# NAME is in ACQ_MSB_GIT_GUARD_NAMES, as whole `NAME=value` tokens. +# +# TOKENS, NOT NAMES — this is load-bearing. The guard decision must be answerable +# as "is this token still MY kit's value?", not merely "did my kit declare this +# name?". The merged env has already collapsed a duplicate name to the LAST kit's +# value, so a name-only record cannot tell a kit's own guard value from another +# kit's: kit A declaring GIT_TERMINAL_PROMPT=0 and kit B declaring =1 would leave +# kit A's own commands running with kit B's opt-out. Keeping the value here lets +# _acq_msb_env_tokens_with_guards_into restore the owning kit's own token. +_acq_msb_own_guard_tokens_into() { + local _arrn="$1" _envarrn="$2" _g _ev + eval "$_arrn=()" eval "set -- \${${_envarrn}[@]+\"\${${_envarrn}[@]}\"}" for _ev in "$@"; do for _g in $ACQ_MSB_GIT_GUARD_NAMES; do case "$_ev" in - "$_g"=*) case " $_names " in *" $_g "*) : ;; *) _names="${_names:+$_names }$_g" ;; esac ;; + "$_g"=*) eval "$_arrn+=(\"\$_ev\")" ;; esac done done - printf '%s' "$_names" } -# _acq_msb_env_tokens_with_guards_into ARRVAR ENVARR OWNGUARDS — store the +# _acq_msb_env_tokens_with_guards_into ARRVAR ENVARR OWNGUARDARR — store the # NAME=value tokens one kit command should run with into the array named ARRVAR: # the MERGED kit env (ENVARR, every effective kit's environment[]) plus the # non-interactive git guards. # -# GUARD SCOPING (do not widen): the guard decision stays PER KIT. OWNGUARDS is -# the guard-var subset THIS kit declared (_acq_msb_own_guard_names). A guard var -# coming from ANOTHER kit is filtered out of the merged set and the adapter's own -# guard value is used instead, so one kit setting GIT_TERMINAL_PROMPT can never -# disable the guards for a different kit's commands (nor produce two conflicting -# values for one name). A kit that declares a guard var itself still overrides it -# for its OWN commands, exactly as before. +# GUARD SCOPING (do not widen): a command may only ever receive ITS OWN kit's +# value for a guard name. OWNGUARDARR names an array of THIS kit's own guard +# tokens (_acq_msb_own_guard_tokens_into). Every guard-name token is stripped from +# the merged set regardless of which kit contributed it, then this kit's own guard +# tokens are re-added, then the adapter's guard values are appended unless this +# kit set GIT_TERMINAL_PROMPT itself. So no other kit can change — or disable — +# the guards for this kit's commands, whether by declaring a guard name this kit +# does not (name collision) or by declaring the same name with a different value +# (value collision, which the merge would otherwise resolve in the other kit's +# favor). A kit that declares a guard var still overrides it for its OWN commands. _acq_msb_env_tokens_with_guards_into() { - local _arrn="$1" _envarrn="$2" _ownguards="$3" + local _arrn="$1" _envarrn="$2" _ownarrn="$3" eval "$_arrn=()" - local _ev _g _keep + local _own=() _ev _g _keep + eval "_own=(\${${_ownarrn}[@]+\"\${${_ownarrn}[@]}\"})" + + # 1) The merged env with EVERY guard-name token removed (any kit's value). eval "set -- \${${_envarrn}[@]+\"\${${_envarrn}[@]}\"}" for _ev in "$@"; do _keep=1 for _g in $ACQ_MSB_GIT_GUARD_NAMES; do - case "$_ev" in - "$_g"=*) case " $_ownguards " in *" $_g "*) : ;; *) _keep=0 ;; esac ;; - esac + case "$_ev" in "$_g"=*) _keep=0 ;; esac done [ "$_keep" -eq 1 ] && eval "$_arrn+=(\"\$_ev\")" done - case " $_ownguards " in - *" GIT_TERMINAL_PROMPT "*) return 0 ;; - esac + + # 2) Re-add only THIS kit's own guard tokens (its intentional override). + for _ev in ${_own[@]+"${_own[@]}"}; do + eval "$_arrn+=(\"\$_ev\")" + done + + # 3) acq's guards, unless this kit set GIT_TERMINAL_PROMPT itself. Appended + # last, so where this kit declared one of the other two the adapter's value + # still wins — the flag order the single-kit path has always emitted. + for _ev in ${_own[@]+"${_own[@]}"}; do + case "$_ev" in GIT_TERMINAL_PROMPT=*) return 0 ;; esac + done eval "$_arrn+=(\"GIT_TERMINAL_PROMPT=0\" \"GIT_ASKPASS=/bin/false\" \"SSH_ASKPASS=/bin/false\")" } -# _acq_msb_exec_flags_into UFLAG_ARRVAR EFLAG_ARRVAR USER KITENV_ARRVAR OWNGUARDS -# — build the `msb exec` -u/-e flag arrays for one kit command. Maps the sbx uid -# contract (1000/agent) onto our by-name agent user, threads the MERGED kit env -# (KITENV_ARRVAR) as `-e NAME=value`, and injects git non-interactive guards -# unless THIS kit declared them (OWNGUARDS, from _acq_msb_own_guard_names — the -# guard decision must stay per kit; see _acq_msb_env_tokens_with_guards_into). +# _acq_msb_exec_flags_into UFLAG_ARRVAR EFLAG_ARRVAR USER KITENV_ARRVAR +# OWNGUARD_ARRVAR — build the `msb exec` -u/-e flag arrays for one kit command. +# Maps the sbx uid contract (1000/agent) onto our by-name agent user, threads the +# MERGED kit env (KITENV_ARRVAR) as `-e NAME=value`, and injects git +# non-interactive guards unless THIS kit declared them (OWNGUARD_ARRVAR holds this +# kit's own guard TOKENS, from _acq_msb_own_guard_tokens_into — the guard value +# must stay this kit's; see _acq_msb_env_tokens_with_guards_into). # Arrays passed/returned by name (bash 3.2 compat). _acq_msb_exec_flags_into() { - local _uflag="$1" _eflag="$2" _user="$3" _envarr="$4" _ownguards="${5:-}" + local _uflag="$1" _eflag="$2" _user="$3" _envarr="$4" _ownguards="${5:-_acq_msb_no_guards}" eval "$_uflag=()" eval "$_eflag=()" @@ -1950,8 +1976,8 @@ _acq_msb_exec_flags_into() { # - stdin from /dev/null for every kit exec, so nothing can read the TTY. # - GIT_TERMINAL_PROMPT=0 (+ GIT_ASKPASS/SSH_ASKPASS=false) so git fails fast # instead of prompting. Injected unless THIS kit set GIT_TERMINAL_PROMPT - # (kits may override intentionally) — the guard decision is per kit even - # though the env is merged, so another kit cannot disable these guards. See + # (kits may override intentionally). The env is merged but a guard value is + # never taken from another kit — not by name and not by value. See # _acq_msb_env_tokens_with_guards_into. local _envtok=() _acq_msb_env_tokens_with_guards_into _envtok "$_envarr" "$_ownguards" @@ -2026,6 +2052,10 @@ _acq_msb_exec_run() { fi } +# Args: NAME PHASE USER BACKGROUND OWNGUARD_ARRVAR [NAME=value ...] -- ARGV... +# OWNGUARD_ARRVAR is the NAME of an array holding this kit's own git-guard tokens +# (see _acq_msb_own_guard_tokens_into); it is passed by name, not by value, because +# a guard VALUE can contain spaces and must never be re-split. _acq_msb_exec_command() { local name="$1" phase="$2" user="$3" background="$4" ownguards="$5" shift 5 @@ -2037,7 +2067,7 @@ _acq_msb_exec_command() { [ "${#_argv[@]}" -gt 0 ] || return 0 # Build the `msb exec` -u/-e flag arrays (uid mapping, merged kit env, and the - # git guards — decided from THIS kit's own guard vars, not the merged set). + # git guards — taken from THIS kit's own guard tokens, not the merged set). local uflag=() eflag=() _acq_msb_exec_flags_into uflag eflag "$user" _kit_env "$ownguards" @@ -2092,16 +2122,16 @@ _acq_msb_sq() { printf "'%s'" "${_s//\'/$_q}" } -# _acq_msb_startup_env_prefix_into PREFIX_ARRVAR USER KITENV_ARRVAR OWNGUARDS — -# build the in-guest `env NAME=value …` prefix tokens for one startup command: -# HOME for the agent user, the MERGED kit env vars (KITENV_ARRVAR), and the git -# non-interactive guards (unless THIS kit declared them — OWNGUARDS keeps that -# decision per kit, exactly as the exec path does). Mirrors +# _acq_msb_startup_env_prefix_into PREFIX_ARRVAR USER KITENV_ARRVAR +# OWNGUARD_ARRVAR — build the in-guest `env NAME=value …` prefix tokens for one +# startup command: HOME for the agent user, the MERGED kit env vars +# (KITENV_ARRVAR), and the git non-interactive guards taken from THIS kit's own +# guard tokens (OWNGUARD_ARRVAR), exactly as the exec path does. Mirrors # _acq_msb_exec_flags_into's -e set, but as `env` argv rather than `msb exec -e`. # Tokens are RAW here (NAME=value); the caller single-quote-escapes each before # writing it to the script. _acq_msb_startup_env_prefix_into() { - local _prefixn="$1" _user="$2" _envarrn="$3" _ownguards="${4:-}" + local _prefixn="$1" _user="$2" _envarrn="$3" _ownguards="${4:-_acq_msb_no_guards}" eval "$_prefixn=()" case "$_user" in @@ -2109,8 +2139,8 @@ _acq_msb_startup_env_prefix_into() { esac # Kit-declared env vars (already NAME-validated by kit_spec_env) plus the - # non-interactive git guards, with the same per-kit guard scoping the exec path - # uses (see _acq_msb_env_tokens_with_guards_into). + # non-interactive git guards, with the same per-kit guard ownership the exec path + # applies (see _acq_msb_env_tokens_with_guards_into). local _envtok=() _ev _acq_msb_env_tokens_with_guards_into _envtok "$_envarrn" "$_ownguards" for _ev in ${_envtok[@]+"${_envtok[@]}"}; do @@ -2270,16 +2300,17 @@ _acq_msb_startup_body_into() { local _bodyn="$1" _spec="$2" _mergedn="${3:-}" # The env the emitted commands run with: the MERGED kit env when the caller has - # the full kit set (ADR-0033), else this spec's own environment[]. The guard - # decision stays per kit either way (_ownguards below). - local _kit_env=() _own_env=() _ownguards + # the full kit set (ADR-0033), else this spec's own environment[]. Either way the + # git guards come from THIS spec's own guard tokens (_own_guard_tok), never from + # another kit's value in the merged set. + local _kit_env=() _own_env=() _own_guard_tok=() _acq_msb_collect_kit_env_into _own_env "$_spec" if [ -n "$_mergedn" ]; then eval "_kit_env=(\${${_mergedn}[@]+\"\${${_mergedn}[@]}\"})" else _kit_env=(${_own_env[@]+"${_own_env[@]}"}) fi - _ownguards=$(_acq_msb_own_guard_names _own_env) + _acq_msb_own_guard_tokens_into _own_guard_tok _own_env # Buffer the command stream first (kit_spec_commands runs its own subshell). local _lines=() line @@ -2306,7 +2337,7 @@ EOF reading=0 if [ "$phase" = "startup" ] && [ "${#argv[@]}" -gt 0 ]; then local _prefix=() _cmdline - _acq_msb_startup_env_prefix_into _prefix "$user" _kit_env "$_ownguards" + _acq_msb_startup_env_prefix_into _prefix "$user" _kit_env _own_guard_tok _cmdline=$(_acq_msb_startup_emit_command "$user" "$background" _prefix argv) if [ -n "$_cmdline" ]; then # Append "\n" by name; $'\n' is a literal newline (ANSI-C). diff --git a/docs/adr/0011-msb-backend-and-neutral-kits.md b/docs/adr/0011-msb-backend-and-neutral-kits.md index 5b4b0b0f..49aef629 100644 --- a/docs/adr/0011-msb-backend-and-neutral-kits.md +++ b/docs/adr/0011-msb-backend-and-neutral-kits.md @@ -424,8 +424,11 @@ strings). The translate layer: > merged set is computed in a **pre-pass over the whole kit set** (so it does not > depend on kit application order), with the persisted marker as the fallback for > the mid-life single-kit `acq kit apply` path. The adapter's non-interactive git -> guards stay scoped **per kit**: one kit setting `GIT_TERMINAL_PROMPT` must not -> disable them for another kit's commands. See +> guards stay **owned by the kit whose command is running**: a command receives +> only its own kit's value for a guard name, so another kit can neither disable +> them (by declaring a guard name this kit does not) nor change them (by declaring +> the same name with a different value, which the merge would otherwise resolve in +> the later kit's favor). See > [ADR-0033](0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md). Deliberately minimal (YAGNI): **static string values only** — no interpolation, diff --git a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md index 94a8f108..eacdb621 100644 --- a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md +++ b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md @@ -85,15 +85,29 @@ The mid-life fallback is correct for an additive add: the new kit's own entries are appended to the marker *before* its commands run, and the read applies last-value-wins, so the new kit still overrides an older value. -**Guard scoping stays per kit (load-bearing).** The adapter injects -`GIT_TERMINAL_PROMPT=0` + `GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit +**Guard ownership stays per kit, tracked by value (load-bearing).** The adapter +injects `GIT_TERMINAL_PROMPT=0` + `GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit commands unless *that* kit declared `GIT_TERMINAL_PROMPT` itself. Deciding this from the *merged* set would let one kit's opt-out disable the guards for every other kit's commands — and a prompting kit command with no credential blocks provision forever (observed with the playbook kit). So the merged set is widened -for **threading** only: a guard var contributed by a *different* kit is filtered -out of a command's env and the adapter's own guard value is used instead. A kit -that declares a guard var still overrides it for its own commands. +for **threading** only: a command receives **only its own kit's value** for a +guard name. + +The ownership record is the owning kit's guard **tokens** (`NAME=value`), not just +the names it declared. A name-only record is insufficient, because the merge has +already collapsed a duplicate name to the *last* kit's value before the filter +runs — so it cannot tell a kit's own guard value from another kit's. That gap was +a real regression against the pre-change behavior: with kit A declaring +`GIT_TERMINAL_PROMPT=0` and a later kit B declaring `=1`, kit A's own command ran +with kit B's opt-out *and* with no `GIT_ASKPASS`/`SSH_ASKPASS` at all, where +before it had correctly got its own `0`. The implementation therefore strips +**every** guard-name token from the merged set regardless of contributor, re-adds +this kit's own guard tokens, then appends acq's guard values unless this kit set +`GIT_TERMINAL_PROMPT` itself. Both collision shapes — another kit declaring a +guard name this kit does not (name collision) and another kit declaring the same +name with a different value (value collision) — are closed, while a kit that +declares a guard var still overrides it for its own commands. The staged create-time `--script-path` body (ADR-0017) gets the same treatment, for consistency. **That path is inert today** — a bare `--script-path` @@ -145,11 +159,14 @@ cannot resurface this bug. - ADR-0017 (msb create-time startup-script staging) — the staged body updated for consistency here, including its inertness caveat and re-verification trap. - `acq.backends/msb.sh` — `_acq_msb_merge_kit_env_into` (the pre-pass), - `_acq_msb_env_tokens_with_guards_into` (merged threading, per-kit guards), + `_acq_msb_own_guard_tokens_into` + `_acq_msb_env_tokens_with_guards_into` + (merged threading, per-kit guard ownership by value), `_acq_msb_persisted_kit_env_into` (the mid-life fallback and session replay). -- `test/bats/78-msb-kit-env.bats`, `test/bats/73-msb-kits-startup.bats` — the - offline regression coverage (both kit orders, all three phases, the mid-life - and heal paths, duplicate resolution, and the guard-scoping rule). +- `test/bats/79-msb-merged-kit-env.bats` — the lifecycle-command regression + coverage (both kit orders, all three phases, the mid-life and heal paths, + duplicate resolution, and both guard-collision shapes). + `test/bats/78-msb-kit-env.bats` keeps the marker/session-replay coverage; + `test/bats/73-msb-kits-startup.bats` covers the staged-body side. - Reported as GSA-TTS/agentic-coding-quickstart#515. The related startup-races-its-own-prerequisites class noted under Negative Consequences is tracked as GSA-TTS/agentic-coding-quickstart#506. diff --git a/test/bats/73-msb-kits-startup.bats b/test/bats/73-msb-kits-startup.bats index 5d3e98b5..fce33281 100644 --- a/test/bats/73-msb-kits-startup.bats +++ b/test/bats/73-msb-kits-startup.bats @@ -543,3 +543,62 @@ SPEC run sh -n "$f" assert_success } + +@test "0017 + ADR-0033: a staged body keeps its OWN kit's guard value, not a later kit's" { + # Value-collision on the staged path: the kit that stakes the script declares + # GIT_TERMINAL_PROMPT=0 (the safe value); a later-merged kit declares =1. The + # body must carry the staking kit's own value. Same defect class as the exec + # path — the merge collapses a duplicate name to the LAST kit's value, so guard + # ownership has to be tracked by value, not by name. + run bash -c ' + export ACQ_SECRET_STORE_DIR="'"$STUBDIR"'/gvstage-secrets" + export ACQ_MSB_KEEP_STARTUP_STAGE=1 ACQ_MSB_STARTUP_STAGE_DIR="'"$STUBDIR"'/gvstage-stage" + . "'"$REPO_ROOT"'/acq.backends/secret-store.sh" + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + sk="'"$STUBDIR"'/gvstagekit"; mkdir -p "$sk" + cat >"$sk/spec.yaml" <<'"'"'SPEC'"'"' +schemaVersion: "hybrid/v1" +kind: mixin +name: gv-stage-kit +displayName: GV Stage Kit +description: stakes the staged script and declares the safe guard value +environment: + GIT_TERMINAL_PROMPT: "0" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo GV_STAGED_MARKER +SPEC + bk="'"$STUBDIR"'/gvoptoutkit"; mkdir -p "$bk" + cat >"$bk/spec.yaml" <<'"'"'SPEC'"'"' +schemaVersion: "hybrid/v1" +kind: mixin +name: gv-optout-kit +displayName: GV OptOut Kit +description: a later kit opting ITSELF out, plus a shared non-guard var +environment: + GIT_TERMINAL_PROMPT: "1" + TEAM_VAR: shared +SPEC + nk="'"$STUBDIR"'/gvnokit"; mkdir -p "$nk" + printf "schemaVersion: \"hybrid/v1\"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n" > "$nk/spec.yaml" + ZSCALER_KIT=k1; USAI_KIT=k2; PLAYBOOK_KIT=k3; GITSSHSIGN_KIT=k4 + _acq_msb_fetch_kit() { + case "$1" in k1) printf "%s\n" "$sk" ;; k2) printf "%s\n" "$bk" ;; *) printf "%s\n" "$nk" ;; esac + } + acq_backend_provision gvstagebox shell /tmp >/dev/null 2>&1 + ' + local body; body=$(_staged_body gvstage-stage) + assert_regex "$body" 'echo GV_STAGED_MARKER' + assert_regex "$body" "'GIT_TERMINAL_PROMPT=0'" + refute_regex "$body" 'GIT_TERMINAL_PROMPT=1' + # The other kit's NON-guard var still reaches the body (merge still applies). + assert_regex "$body" "'TEAM_VAR=shared'" + local f; f=$(find "$STUBDIR/gvstage-stage" -type f 2>/dev/null | head -n1) + run sh -n "$f" + assert_success +} diff --git a/test/bats/78-msb-kit-env.bats b/test/bats/78-msb-kit-env.bats index e0a366d6..dd48882b 100644 --- a/test/bats/78-msb-kit-env.bats +++ b/test/bats/78-msb-kit-env.bats @@ -260,294 +260,3 @@ GITLAB_HOST=gitlab.override.gov" assert_regex "$log" '-e GITLAB_HOST=gitlab\.override\.gov' refute_regex "$log" 'GITLAB_HOST=gitlab\.example\.gov' } - -# --- merged kit env on kit lifecycle commands (ADR-0033) ------------------- -# -# Kit env is guest-wide guest configuration, but on msb a kit's lifecycle -# commands used to run with only THAT kit's environment[]. The reported symptom: -# a `background: true` startup daemon (the Paseo kit's `paseo daemon`) launches -# agents, and those agents inherited only the daemon kit's vars — a team kit's -# OPENCODE_CONFIG (team instructions + a default-deny permission layer) was -# silently absent while the secrets were still present. The fix threads the -# MERGED env of the full effective kit set onto every lifecycle command. - -# _env_merge_provision KITDIRS... — provision `mergebox` with the given local kit -# dirs as the full built-in kit set, in the order given (so the same two kits can -# be applied in either order). Echoes nothing; assertions read $CALLS. -_env_merge_provision() { - : > "$CALLS" - local a="$1" b="$2" - run bash -c ' - a="$1"; b="$2" - export ACQ_SECRET_STORE_DIR="'"$STUBDIR"'/merge-secrets" - export ACQ_MSB_STARTUP_STAGE_DIR="'"$STUBDIR"'/merge-stage" - . "'"$REPO_ROOT"'/acq.backends/secret-store.sh" - . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" - . "'"$REPO_ROOT"'/acq.backends/msb.sh" - ZSCALER_KIT=k1; USAI_KIT=k2; PLAYBOOK_KIT=k3; GITSSHSIGN_KIT=k4 - nk="'"$STUBDIR"'/mergenokit"; mkdir -p "$nk" - printf "schemaVersion: \"hybrid/v1\"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n" > "$nk/spec.yaml" - _acq_msb_fetch_kit() { - case "$1" in k1) printf "%s\n" "$a" ;; k2) printf "%s\n" "$b" ;; *) printf "%s\n" "$nk" ;; esac - } - acq_backend_provision mergebox shell /tmp >/dev/null 2>&1 - ' _ "$a" "$b" -} - -# Write a kit dir that declares OPENCODE_CONFIG and has no commands. -_env_kit_config() { - local d="$1"; mkdir -p "$d" - cat > "$d/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: team-kit -displayName: Team Kit -description: declares agent-runtime config other kits' commands must see -environment: - OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc -SPEC -} - -# Write a kit dir whose commands span all three lifecycle phases, the startup one -# detached (the reported daemon shape). -_env_kit_daemon() { - local d="$1"; mkdir -p "$d" - cat > "$d/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: daemon-kit -displayName: Daemon Kit -description: install + initFiles + a background startup daemon -environment: - PASEO_OWN_VAR: own -commands: - - phase: install - user: "0" - command: - - sh - - -c - - echo INSTALL_CMD - - phase: initFiles - user: "0" - command: - - sh - - -c - - echo INITFILES_CMD - - phase: startup - user: "0" - background: true - command: - - paseo-daemon -SPEC -} - -@test "msb kit env (ADR-0033): merged env reaches a background startup daemon and every phase" { - _env_kit_config "$STUBDIR/cfgkit" - _env_kit_daemon "$STUBDIR/daemonkit" - _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" - local log; log=$(cat "$CALLS") - # The detached daemon exec carries the OTHER kit's var, not just its own. - local bg; bg=$(printf '%s\n' "$log" | grep 'nohup' | head -n1) - assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' - assert_regex "$bg" '\-e PASEO_OWN_VAR=own' - # install and initFiles too — the same guest-wide env contract. - local inst; inst=$(printf '%s\n' "$log" | grep 'echo INSTALL_CMD' | head -n1) - assert_regex "$inst" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' - local init; init=$(printf '%s\n' "$log" | grep 'echo INITFILES_CMD' | head -n1) - assert_regex "$init" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' -} - -@test "msb kit env (ADR-0033): the merged env is the same in BOTH kit application orders" { - _env_kit_config "$STUBDIR/cfgkit" - _env_kit_daemon "$STUBDIR/daemonkit" - # config kit applied FIRST (the order a per-kit marker read would also pass). - _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" - local bg1; bg1=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) - assert_regex "$bg1" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' - # daemon kit applied FIRST — the order that silently fails without a pre-pass: - # the marker does not yet hold the config kit's entry when the daemon launches. - _env_merge_provision "$STUBDIR/daemonkit" "$STUBDIR/cfgkit" - local bg2; bg2=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) - assert_regex "$bg2" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' -} - -@test "msb kit env (ADR-0033): a duplicate name resolves last-kit-wins on commands" { - local k1="$STUBDIR/dupkit1" k2="$STUBDIR/dupkit2" - mkdir -p "$k1" "$k2" - cat > "$k1/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: dup-one -displayName: Dup One -description: sets GITLAB_HOST first and owns the command -environment: - GITLAB_HOST: gitlab.first.gov -commands: - - phase: startup - user: "0" - command: - - sh - - -c - - echo DUP_CMD -SPEC - cat > "$k2/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: dup-two -displayName: Dup Two -description: overrides GITLAB_HOST from a later kit -environment: - GITLAB_HOST: gitlab.second.gov -SPEC - _env_merge_provision "$k1" "$k2" - local cmd; cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo DUP_CMD' | head -n1) - # The command belongs to the EARLIER kit, yet must see the LATER kit's value — - # the same resolution the session replay applies, and exactly once (never both). - assert_regex "$cmd" '\-e GITLAB_HOST=gitlab\.second\.gov' - refute_regex "$cmd" 'gitlab\.first\.gov' -} - -# Guard test, not a bug reproduction: this passes BEFORE the merged-env change too -# (each kit then saw only its own env, so the question could not arise). It exists -# because widening the threaded env makes the failure reachable — if the merged set -# were also used to DECIDE the guards, one kit setting GIT_TERMINAL_PROMPT would -# disable GIT_TERMINAL_PROMPT=0/GIT_ASKPASS/SSH_ASKPASS for every other kit's -# commands, and a prompting kit command would block provision forever. -@test "msb kit env (ADR-0033): one kit's GIT_TERMINAL_PROMPT does not strip another kit's guards" { - local gk="$STUBDIR/gitpromptkit" pk="$STUBDIR/plainkit" - mkdir -p "$gk" "$pk" - # A kit that deliberately opts out of the non-interactive git guards. - cat > "$gk/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: prompt-kit -displayName: Prompt Kit -description: opts itself out of the git guards -environment: - GIT_TERMINAL_PROMPT: "1" -commands: - - phase: startup - user: "0" - command: - - sh - - -c - - echo PROMPT_KIT_CMD -SPEC - # A different kit that did NOT opt out: its commands must keep the guards, or a - # prompting command would block provision forever. - cat > "$pk/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: plain-kit -displayName: Plain Kit -description: relies on the adapter's git guards -commands: - - phase: startup - user: "0" - command: - - sh - - -c - - echo PLAIN_KIT_CMD -SPEC - _env_merge_provision "$gk" "$pk" - local log; log=$(cat "$CALLS") - local plain; plain=$(printf '%s\n' "$log" | grep 'echo PLAIN_KIT_CMD' | head -n1) - assert_regex "$plain" '\-e GIT_TERMINAL_PROMPT=0' - assert_regex "$plain" '\-e GIT_ASKPASS=/bin/false' - assert_regex "$plain" '\-e SSH_ASKPASS=/bin/false' - refute_regex "$plain" 'GIT_TERMINAL_PROMPT=1' - # The opting-out kit still gets its own value, and only that value. - local prompt; prompt=$(printf '%s\n' "$log" | grep 'echo PROMPT_KIT_CMD' | head -n1) - assert_regex "$prompt" '\-e GIT_TERMINAL_PROMPT=1' - refute_regex "$prompt" 'GIT_TERMINAL_PROMPT=0' -} - -@test "msb kit env (ADR-0033): mid-life 'kit apply' recovers the merged env from the marker" { - # A mid-life single-kit apply has NO full kit set to pre-merge, so it must fall - # back to the env the sandbox already persisted — otherwise adding a daemon kit - # to a live sandbox reintroduces exactly the reported bug for that kit. - printf 'OPENCODE_CONFIG=/home/agent/.config/opencode/team.jsonc\n' > "$STUBDIR/.kit_env" - local mk="$STUBDIR/midlifekit"; mkdir -p "$mk" - cat > "$mk/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: midlife-kit -displayName: Midlife Kit -description: added to a live sandbox; its own env plus what is already persisted -environment: - MIDLIFE_OWN: yes -commands: - - phase: startup - user: "0" - background: true - command: - - midlife-daemon -SPEC - : > "$CALLS" - ( export ACQ_SECRET_STORE_DIR="$STUBDIR/midlife-secrets" - . "${REPO_ROOT}/acq.backends/secret-store.sh" - . "${REPO_ROOT}/acq.backends/kit-translate.sh" - . "${REPO_ROOT}/acq.backends/msb.sh" - acq_backend_apply_kit midlifebox "$mk" >/dev/null 2>&1 ) - local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) - assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' - assert_regex "$bg" '\-e MIDLIFE_OWN=yes' -} - -@test "msb kit env (ADR-0033): the heal path also merges, daemon kit applied first" { - # The heal (acq start/restart, re-attach) is the OTHER full-set path, and it is - # the one that restarts a kit's daemon on resume. Same pre-pass requirement: the - # daemon kit is healed FIRST here, so a per-kit marker read would give it an env - # missing the config kit's var. - printf 'healmergebox\n' > "$STUBDIR/.msb_sandbox_list" - printf 'healmergebox\n' > "$STUBDIR/.msb_running_list" - _env_kit_config "$STUBDIR/cfgkit" - _env_kit_daemon "$STUBDIR/daemonkit" - : > "$CALLS" - ( export ACQ_SECRET_STORE_DIR="$STUBDIR/healmerge-secrets" - export ACQ_MSB_STARTUP_STAGE_DIR="$STUBDIR/healmerge-stage" - . "${REPO_ROOT}/acq.backends/secret-store.sh" - . "${REPO_ROOT}/acq.backends/kit-translate.sh" - . "${REPO_ROOT}/acq.backends/msb.sh" - # shellcheck disable=SC2034 # read by the sourced acq_backend_ensure_kits_applied - ACQ_CLI_KITS=() - nk="$STUBDIR/healmergenokit"; mkdir -p "$nk" - printf 'schemaVersion: "hybrid/v1"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n' > "$nk/spec.yaml" - # Resolve by CALL ORDER rather than by kit ref, so the built-in kit refs stay - # untouched: the heal fetches them in list order, so call 1 is the daemon kit - # and call 2 the config kit — the daemon applied FIRST. - printf 0 > "$STUBDIR/.healmerge_n" - _acq_msb_fetch_kit() { - local n; n=$(cat "$STUBDIR/.healmerge_n") - printf '%s' "$((n + 1))" > "$STUBDIR/.healmerge_n" - case "$n" in - 0) printf '%s\n' "$STUBDIR/daemonkit" ;; - 1) printf '%s\n' "$STUBDIR/cfgkit" ;; - *) printf '%s\n' "$nk" ;; - esac - } - acq_backend_ensure_kits_applied healmergebox >/dev/null 2>&1 ) - local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) - assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' - assert_regex "$bg" '\-e PASEO_OWN_VAR=own' -} - -@test "msb kit env (ADR-0033): the merge re-validates names and never re-splits a value" { - # The merge helper is a second place kit-derived NAME=value tokens flow through, - # so it re-applies the kit_spec_env charset itself (defense in depth), and each - # token stays ONE array element so a value with spaces or metacharacters is - # never re-split into a second variable (SI-10). - run bash -c ' - . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" - . "'"$REPO_ROOT"'/acq.backends/msb.sh" - m=() - _acq_msb_dedupe_env_into m "BAD-NAME=x" "=noname" "noequals" \ - "OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED" "OK=1" - printf "TOKEN[%s]\n" ${m[@]+"${m[@]}"} - ' - assert_success - refute_output --partial 'BAD-NAME' - refute_output --partial 'noequals' - assert_output --partial 'TOKEN[OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED]' - assert_output --partial 'TOKEN[OK=1]' -} diff --git a/test/bats/79-msb-merged-kit-env.bats b/test/bats/79-msb-merged-kit-env.bats new file mode 100644 index 00000000..2190c191 --- /dev/null +++ b/test/bats/79-msb-merged-kit-env.bats @@ -0,0 +1,413 @@ +#!/usr/bin/env bats +# +# 79-msb-merged-kit-env.bats — msb kit environment[] is guest-wide for kit +# lifecycle commands (ADR-0033). +# +# Companion to 78-msb-kit-env.bats, which covers the environment[] marker and its +# SESSION replay. This file covers the LIFECYCLE-COMMAND side: the merged +# whole-kit-set env threaded onto install/initFiles/startup, its order +# independence, duplicate resolution, the mid-life and heal paths, and the +# per-kit ownership of the non-interactive git guards. +# +# shellcheck shell=bats + +setup() { acq_setup_stubs; load_acq; } +teardown() { acq_teardown_stubs; } + +load 'helper' + +# Kit env is guest-wide guest configuration, but on msb a kit's lifecycle +# commands used to run with only THAT kit's environment[]. The reported symptom: +# a `background: true` startup daemon (the Paseo kit's `paseo daemon`) launches +# agents, and those agents inherited only the daemon kit's vars — a team kit's +# OPENCODE_CONFIG (team instructions + a default-deny permission layer) was +# silently absent while the secrets were still present. The fix threads the +# MERGED env of the full effective kit set onto every lifecycle command. + +# _env_merge_provision KITDIRS... — provision `mergebox` with the given local kit +# dirs as the full built-in kit set, in the order given (so the same two kits can +# be applied in either order). Echoes nothing; assertions read $CALLS. +_env_merge_provision() { + : > "$CALLS" + local a="$1" b="$2" + run bash -c ' + a="$1"; b="$2" + export ACQ_SECRET_STORE_DIR="'"$STUBDIR"'/merge-secrets" + export ACQ_MSB_STARTUP_STAGE_DIR="'"$STUBDIR"'/merge-stage" + . "'"$REPO_ROOT"'/acq.backends/secret-store.sh" + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + ZSCALER_KIT=k1; USAI_KIT=k2; PLAYBOOK_KIT=k3; GITSSHSIGN_KIT=k4 + nk="'"$STUBDIR"'/mergenokit"; mkdir -p "$nk" + printf "schemaVersion: \"hybrid/v1\"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n" > "$nk/spec.yaml" + _acq_msb_fetch_kit() { + case "$1" in k1) printf "%s\n" "$a" ;; k2) printf "%s\n" "$b" ;; *) printf "%s\n" "$nk" ;; esac + } + acq_backend_provision mergebox shell /tmp >/dev/null 2>&1 + ' _ "$a" "$b" +} + +# Write a kit dir that declares OPENCODE_CONFIG and has no commands. +_env_kit_config() { + local d="$1"; mkdir -p "$d" + cat > "$d/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: team-kit +displayName: Team Kit +description: declares agent-runtime config other kits' commands must see +environment: + OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc +SPEC +} + +# Write a kit dir whose commands span all three lifecycle phases, the startup one +# detached (the reported daemon shape). +_env_kit_daemon() { + local d="$1"; mkdir -p "$d" + cat > "$d/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: daemon-kit +displayName: Daemon Kit +description: install + initFiles + a background startup daemon +environment: + PASEO_OWN_VAR: own +commands: + - phase: install + user: "0" + command: + - sh + - -c + - echo INSTALL_CMD + - phase: initFiles + user: "0" + command: + - sh + - -c + - echo INITFILES_CMD + - phase: startup + user: "0" + background: true + command: + - paseo-daemon +SPEC +} + +@test "ADR-0033: merged env reaches a background startup daemon and every phase" { + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" + local log; log=$(cat "$CALLS") + # The detached daemon exec carries the OTHER kit's var, not just its own. + local bg; bg=$(printf '%s\n' "$log" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e PASEO_OWN_VAR=own' + # install and initFiles too — the same guest-wide env contract. + local inst; inst=$(printf '%s\n' "$log" | grep 'echo INSTALL_CMD' | head -n1) + assert_regex "$inst" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + local init; init=$(printf '%s\n' "$log" | grep 'echo INITFILES_CMD' | head -n1) + assert_regex "$init" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' +} + +@test "ADR-0033: the merged env is the same in BOTH kit application orders" { + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + # config kit applied FIRST (the order a per-kit marker read would also pass). + _env_merge_provision "$STUBDIR/cfgkit" "$STUBDIR/daemonkit" + local bg1; bg1=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg1" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + # daemon kit applied FIRST — the order that silently fails without a pre-pass: + # the marker does not yet hold the config kit's entry when the daemon launches. + _env_merge_provision "$STUBDIR/daemonkit" "$STUBDIR/cfgkit" + local bg2; bg2=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg2" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' +} + +@test "ADR-0033: a duplicate name resolves last-kit-wins on commands" { + local k1="$STUBDIR/dupkit1" k2="$STUBDIR/dupkit2" + mkdir -p "$k1" "$k2" + cat > "$k1/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: dup-one +displayName: Dup One +description: sets GITLAB_HOST first and owns the command +environment: + GITLAB_HOST: gitlab.first.gov +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo DUP_CMD +SPEC + cat > "$k2/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: dup-two +displayName: Dup Two +description: overrides GITLAB_HOST from a later kit +environment: + GITLAB_HOST: gitlab.second.gov +SPEC + _env_merge_provision "$k1" "$k2" + local cmd; cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo DUP_CMD' | head -n1) + # The command belongs to the EARLIER kit, yet must see the LATER kit's value — + # the same resolution the session replay applies, and exactly once (never both). + assert_regex "$cmd" '\-e GITLAB_HOST=gitlab\.second\.gov' + refute_regex "$cmd" 'gitlab\.first\.gov' +} + +# --- the git guards belong to the kit whose command is running ------------ +# +# Widening the threaded env makes a whole failure class reachable: if the MERGED +# set were also used to decide the guards, one kit could disable +# GIT_TERMINAL_PROMPT=0/GIT_ASKPASS/SSH_ASKPASS for every other kit's commands, +# and a prompting kit command blocks provision forever (the adapter's own comment +# records the playbook kit hanging there). The next three tests pin the two ways +# a kit could reach another kit's guards — NAME collision and VALUE collision — +# plus the case that must keep working: a kit's own intentional override. + +# NAME collision. Passes at base too (each kit then saw only its own env, so the +# question could not arise); it guards the widening rather than reproducing a bug. +@test "ADR-0033: one kit's GIT_TERMINAL_PROMPT does not strip another kit's guards" { + local gk="$STUBDIR/gitpromptkit" pk="$STUBDIR/plainkit" + mkdir -p "$gk" "$pk" + # A kit that deliberately opts out of the non-interactive git guards. + cat > "$gk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: prompt-kit +displayName: Prompt Kit +description: opts itself out of the git guards +environment: + GIT_TERMINAL_PROMPT: "1" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo PROMPT_KIT_CMD +SPEC + # A different kit that did NOT opt out: its commands must keep the guards, or a + # prompting command would block provision forever. + cat > "$pk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: plain-kit +displayName: Plain Kit +description: relies on the adapter's git guards +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo PLAIN_KIT_CMD +SPEC + _env_merge_provision "$gk" "$pk" + local log; log=$(cat "$CALLS") + local plain; plain=$(printf '%s\n' "$log" | grep 'echo PLAIN_KIT_CMD' | head -n1) + assert_regex "$plain" '\-e GIT_TERMINAL_PROMPT=0' + assert_regex "$plain" '\-e GIT_ASKPASS=/bin/false' + assert_regex "$plain" '\-e SSH_ASKPASS=/bin/false' + refute_regex "$plain" 'GIT_TERMINAL_PROMPT=1' + # The opting-out kit still gets its own value, and only that value. + local prompt; prompt=$(printf '%s\n' "$log" | grep 'echo PROMPT_KIT_CMD' | head -n1) + assert_regex "$prompt" '\-e GIT_TERMINAL_PROMPT=1' + refute_regex "$prompt" 'GIT_TERMINAL_PROMPT=0' +} + +@test "ADR-0033: mid-life 'kit apply' recovers the merged env from the marker" { + # A mid-life single-kit apply has NO full kit set to pre-merge, so it must fall + # back to the env the sandbox already persisted — otherwise adding a daemon kit + # to a live sandbox reintroduces exactly the reported bug for that kit. + printf 'OPENCODE_CONFIG=/home/agent/.config/opencode/team.jsonc\n' > "$STUBDIR/.kit_env" + local mk="$STUBDIR/midlifekit"; mkdir -p "$mk" + cat > "$mk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: midlife-kit +displayName: Midlife Kit +description: added to a live sandbox; its own env plus what is already persisted +environment: + MIDLIFE_OWN: yes +commands: + - phase: startup + user: "0" + background: true + command: + - midlife-daemon +SPEC + : > "$CALLS" + ( export ACQ_SECRET_STORE_DIR="$STUBDIR/midlife-secrets" + . "${REPO_ROOT}/acq.backends/secret-store.sh" + . "${REPO_ROOT}/acq.backends/kit-translate.sh" + . "${REPO_ROOT}/acq.backends/msb.sh" + acq_backend_apply_kit midlifebox "$mk" >/dev/null 2>&1 ) + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e MIDLIFE_OWN=yes' +} + +@test "ADR-0033: the heal path also merges, daemon kit applied first" { + # The heal (acq start/restart, re-attach) is the OTHER full-set path, and it is + # the one that restarts a kit's daemon on resume. Same pre-pass requirement: the + # daemon kit is healed FIRST here, so a per-kit marker read would give it an env + # missing the config kit's var. + printf 'healmergebox\n' > "$STUBDIR/.msb_sandbox_list" + printf 'healmergebox\n' > "$STUBDIR/.msb_running_list" + _env_kit_config "$STUBDIR/cfgkit" + _env_kit_daemon "$STUBDIR/daemonkit" + : > "$CALLS" + ( export ACQ_SECRET_STORE_DIR="$STUBDIR/healmerge-secrets" + export ACQ_MSB_STARTUP_STAGE_DIR="$STUBDIR/healmerge-stage" + . "${REPO_ROOT}/acq.backends/secret-store.sh" + . "${REPO_ROOT}/acq.backends/kit-translate.sh" + . "${REPO_ROOT}/acq.backends/msb.sh" + # shellcheck disable=SC2034 # read by the sourced acq_backend_ensure_kits_applied + ACQ_CLI_KITS=() + nk="$STUBDIR/healmergenokit"; mkdir -p "$nk" + printf 'schemaVersion: "hybrid/v1"\nkind: mixin\nname: x\ndisplayName: X\ndescription: x\n' > "$nk/spec.yaml" + # Resolve by CALL ORDER rather than by kit ref, so the built-in kit refs stay + # untouched: the heal fetches them in list order, so call 1 is the daemon kit + # and call 2 the config kit — the daemon applied FIRST. + printf 0 > "$STUBDIR/.healmerge_n" + _acq_msb_fetch_kit() { + local n; n=$(cat "$STUBDIR/.healmerge_n") + printf '%s' "$((n + 1))" > "$STUBDIR/.healmerge_n" + case "$n" in + 0) printf '%s\n' "$STUBDIR/daemonkit" ;; + 1) printf '%s\n' "$STUBDIR/cfgkit" ;; + *) printf '%s\n' "$nk" ;; + esac + } + acq_backend_ensure_kits_applied healmergebox >/dev/null 2>&1 ) + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + assert_regex "$bg" '\-e PASEO_OWN_VAR=own' +} + +@test "ADR-0033: the merge re-validates names and never re-splits a value" { + # The merge helper is a second place kit-derived NAME=value tokens flow through, + # so it re-applies the kit_spec_env charset itself (defense in depth), and each + # token stays ONE array element so a value with spaces or metacharacters is + # never re-split into a second variable (SI-10). + run bash -c ' + . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" + . "'"$REPO_ROOT"'/acq.backends/msb.sh" + m=() + _acq_msb_dedupe_env_into m "BAD-NAME=x" "=noname" "noequals" \ + "OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED" "OK=1" + printf "TOKEN[%s]\n" ${m[@]+"${m[@]}"} + ' + assert_success + refute_output --partial 'BAD-NAME' + refute_output --partial 'noequals' + assert_output --partial 'TOKEN[OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED]' + assert_output --partial 'TOKEN[OK=1]' +} + + +# VALUE collision — the case a name-only ownership record cannot catch. The merge +# collapses a duplicate name to the LAST kit's value, so an earlier kit that +# declared GIT_TERMINAL_PROMPT=0 saw kit B's =1 on its own commands, with no +# GIT_ASKPASS/SSH_ASKPASS added either (the "owner declared it, leave it alone" +# branch). Reproduces a real regression: at base that command got its own 0. +@test "ADR-0033: two kits declaring the SAME guard name with different values" { + # Kit A owns the command and declares the safe value; kit B, merged later, + # declares the opt-out. Kit A's command must keep kit A's value. + local ka="$STUBDIR/guardvalkitA" kb="$STUBDIR/guardvalkitB" + mkdir -p "$ka" "$kb" + cat > "$ka/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: guardval-a +displayName: GuardVal A +description: owns the command and declares the safe guard value +environment: + GIT_TERMINAL_PROMPT: "0" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo GUARDVAL_A_CMD +SPEC + cat > "$kb/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: guardval-b +displayName: GuardVal B +description: a later kit opting ITSELF out of the guards +environment: + GIT_TERMINAL_PROMPT: "1" + TEAM_VAR: shared +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo GUARDVAL_B_CMD +SPEC + _env_merge_provision "$ka" "$kb" + local log; log=$(cat "$CALLS") + # Kit A's command: its own value, never kit B's. + local a; a=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_A_CMD' | head -n1) + assert_regex "$a" '\-e GIT_TERMINAL_PROMPT=0' + refute_regex "$a" 'GIT_TERMINAL_PROMPT=1' + # Kit B's command: its own opt-out value, never kit A's. + local b; b=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_B_CMD' | head -n1) + assert_regex "$b" '\-e GIT_TERMINAL_PROMPT=1' + refute_regex "$b" 'GIT_TERMINAL_PROMPT=0' + # Both still receive the other kit's NON-guard merged vars. + assert_regex "$a" '\-e TEAM_VAR=shared' + assert_regex "$b" '\-e TEAM_VAR=shared' +} + +@test "ADR-0033: a kit's own guard override is not clobbered by the merged set" { + # The mirror of the finding: fixing value-collision must not stop a kit from + # overriding the guards for ITS OWN commands. Its own value appears once, with + # no conflicting duplicate, and acq's GIT_TERMINAL_PROMPT=0 is not added. + local ok="$STUBDIR/ownoverridekit" ck="$STUBDIR/ownotherkit" + mkdir -p "$ok" "$ck" + cat > "$ok/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: own-override +displayName: Own Override +description: deliberately opts itself out of the git guards +environment: + GIT_TERMINAL_PROMPT: "1" +commands: + - phase: startup + user: "0" + command: + - sh + - -c + - echo OWN_OVERRIDE_CMD +SPEC + cat > "$ck/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: own-other +displayName: Own Other +description: contributes only a non-guard var +environment: + OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc +SPEC + _env_merge_provision "$ok" "$ck" + local cmd n + cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo OWN_OVERRIDE_CMD' | head -n1) + assert_regex "$cmd" '\-e GIT_TERMINAL_PROMPT=1' + refute_regex "$cmd" 'GIT_TERMINAL_PROMPT=0' + n=$(printf '%s\n' "$cmd" | tr ' ' '\n' | grep -c '^GIT_TERMINAL_PROMPT=') + assert_equal "$n" "1" + # And it still receives the other kit's merged non-guard var. + assert_regex "$cmd" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' +} From 58b83de81efb23fec3fc63c3c3a7d6d98fcae5e5 Mon Sep 17 00:00:00 2001 From: Basilio Bogado <541149+basiliskus@users.noreply.github.com> Date: Wed, 30 Sep 2026 00:10:27 +0000 Subject: [PATCH 3/4] docs(msb): trim duplicated rationale from ADR-0033 and the adapter comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment, doc and test trim only — no behavior change. Verified: with comments and blank lines removed, acq.backends/msb.sh is byte-identical to the previous commit, and the six guard/merge shapes (own-0-vs-later-1, own override =1, owns-no-guard, owns-GIT_ASKPASS-only, no env, plain merged) emit identical -e flags on both the exec and staged-script paths. ADR-0033: 172 -> 113 lines. Kept the problem statement, the order-independence trap, the per-path source table, guard-ownership-by-token with one worked example, the inert-`--script-path` caveat, and the deferred startup-races-its-prerequisites gap. Cut the implementation walkthrough (it duplicated ADR-0011, ADR-0017 and BACKEND_GUIDE), the Positive Consequences list (restated the decision), and compressed Drivers and Options. msb.sh: the merged-env rationale is now stated once, at _acq_msb_merge_kit_env_into (why it must be a pre-pass), and the guard-ownership rule once, at _acq_msb_env_tokens_with_guards_into; the other five sites point at those. Kept every comment recording a verified external fact or a past failure — the ADR-0017 staged-script inertness note, the prompting-kit-command hang, the loop-stdin-consumption and eval-by-name-collision notes. Comment delta vs base is now 138 added lines (was 183); code delta is unchanged at 161. Tests: folded "a kit's own guard override is not clobbered" into the value-collision test, keeping both its assertions (GIT_TERMINAL_PROMPT appears exactly once; the other kit's non-guard var still arrives). Dropped the helper-pinned _acq_msb_dedupe_env_into unit test and re-asserted its observable behavior through the public provision path instead: a value containing spaces and shell metacharacters crosses the merge as one -e token. Refs: GSA-TTS/agentic-coding-quickstart#515 --- acq.backends/msb.sh | 208 ++++++----------- ...nv-is-guest-wide-for-lifecycle-commands.md | 221 +++++++----------- test/bats/79-msb-merged-kit-env.bats | 107 +++------ 3 files changed, 190 insertions(+), 346 deletions(-) diff --git a/acq.backends/msb.sh b/acq.backends/msb.sh index 9853aa31..91f93134 100644 --- a/acq.backends/msb.sh +++ b/acq.backends/msb.sh @@ -1597,23 +1597,14 @@ EOF # 3) Run commands[]. Reassemble each argv record and exec it as the given uid. # install → run once (idempotent, marker-gated); initFiles/startup → every # apply. msb has no create-time-only hook, so install collapses to a - # marker-gated exec (design §3 lifecycle table). The MERGED kit env (every - # effective kit's environment[]) is threaded onto every command as - # `msb exec -e NAME=value` (msb's native per-exec env flag): kit env is - # guest-wide config, so a command — especially a `background: true` daemon - # that launches agents — must see the other kits' vars too (ADR-0033). + # marker-gated exec (design §3 lifecycle table). # - # Where the merged set comes from: - # - FULL-SET callers (provision, heal) computed it in a pre-pass over every - # kit BEFORE this per-kit loop and pass its array name in, so the result - # does not depend on the order the kits are applied in. - # - The MID-LIFE single-kit path (`acq kit apply NAME KITREF`) has no full - # set, so it recovers the sandbox's already-persisted merged value from - # the marker (step 2 just appended THIS kit's entries, and the read - # applies last-value-wins, so the new kit still overrides — matching the - # additive semantics of a mid-life add). - # - Neither available (marker absent/unwritable) → this kit's own env, the - # pre-ADR-0033 behavior. Fail-soft, never empty. + # Commands run with the MERGED env of the whole kit set, not just this kit's + # (ADR-0033). Full-set callers pass its array name; the mid-life single-kit + # path (`acq kit apply`) has none, so it recovers the sandbox's persisted + # merged value — step 2 appended THIS kit's entries first and the read is + # last-value-wins, so a mid-life add still overrides. An unreadable marker + # degrades to this kit's own env: fail-soft, never empty. local _merged_env=() _merged_envn="$mergedn" if [ -z "$_merged_envn" ]; then _acq_msb_persisted_kit_env_into _merged_env "$name" @@ -1745,20 +1736,15 @@ _acq_msb_copy_file_verified() { # Parse and execute a kit spec's commands[] against sandbox NAME. # # Args: NAME SPEC [MERGEDENV_ARRVAR] -# MERGEDENV_ARRVAR names an array of already-merged NAME=value tokens covering -# the FULL effective kit set (see _acq_msb_merge_kit_env_into). Omitted, the env -# falls back to this spec's own environment[] — used only where no full set -# exists and no merged value could be recovered (see _acq_msb_apply_kit_dir). +# MERGEDENV_ARRVAR names an array of already-merged NAME=value tokens covering the +# FULL effective kit set (_acq_msb_merge_kit_env_into). Omitted, the env falls back +# to this spec's own environment[] — see _acq_msb_apply_kit_dir step 3. _acq_msb_run_commands() { local name="$1" spec="$2" mergedn="${3:-}" # The guest env threaded onto every command's `msb exec` as `-e NAME=value`. - # kit_spec_env already validates each NAME (^[A-Za-z_][A-Za-z0-9_]*$) and drops - # unsafe ones, and the marker replay re-validates, so these are safe to pass as - # exec env. Values are passed as a single argv element (never re-split by a - # shell). Kit env is guest-wide config, so a command sees the MERGED set — a - # `background: true` daemon that launches agents must not be limited to its own - # kit's vars (ADR-0033). + # Names are already validated by kit_spec_env and re-validated on marker replay; + # each value is one argv element, never re-split by a shell. local _kit_env=() if [ -n "$mergedn" ]; then eval "_kit_env=(\${${mergedn}[@]+\"\${${mergedn}[@]}\"})" @@ -1766,10 +1752,7 @@ _acq_msb_run_commands() { _acq_msb_collect_kit_env_into _kit_env "$spec" fi - # The git guards stay PER KIT: a command may only ever receive THIS spec's own - # guard tokens, so neither another kit declaring a guard name this kit does not, - # nor another kit declaring the same name with a different value (which the merge - # resolves in the later kit's favor), can change or disable the guards here. See + # The git guards are owned per kit even though the env is merged — see # _acq_msb_env_tokens_with_guards_into. local _own_env=() _own_guard_tok=() _acq_msb_collect_kit_env_into _own_env "$spec" @@ -1857,16 +1840,9 @@ ACQ_MSB_GIT_GUARD_NAMES="GIT_TERMINAL_PROMPT GIT_ASKPASS SSH_ASKPASS" _acq_msb_no_guards=() # _acq_msb_own_guard_tokens_into ARRVAR ENVARR — store, in the array named ARRVAR, -# the subset of the array named ENVARR (ONE kit spec's own environment[]) whose -# NAME is in ACQ_MSB_GIT_GUARD_NAMES, as whole `NAME=value` tokens. -# -# TOKENS, NOT NAMES — this is load-bearing. The guard decision must be answerable -# as "is this token still MY kit's value?", not merely "did my kit declare this -# name?". The merged env has already collapsed a duplicate name to the LAST kit's -# value, so a name-only record cannot tell a kit's own guard value from another -# kit's: kit A declaring GIT_TERMINAL_PROMPT=0 and kit B declaring =1 would leave -# kit A's own commands running with kit B's opt-out. Keeping the value here lets -# _acq_msb_env_tokens_with_guards_into restore the owning kit's own token. +# the guard-named entries of ENVARR (ONE kit spec's own environment[]) as whole +# `NAME=value` tokens. Tokens, not names: see the ownership rule at +# _acq_msb_env_tokens_with_guards_into. _acq_msb_own_guard_tokens_into() { local _arrn="$1" _envarrn="$2" _g _ev eval "$_arrn=()" @@ -1880,21 +1856,22 @@ _acq_msb_own_guard_tokens_into() { done } -# _acq_msb_env_tokens_with_guards_into ARRVAR ENVARR OWNGUARDARR — store the -# NAME=value tokens one kit command should run with into the array named ARRVAR: -# the MERGED kit env (ENVARR, every effective kit's environment[]) plus the -# non-interactive git guards. +# _acq_msb_env_tokens_with_guards_into ARRVAR ENVARR OWNGUARDARR — store into the +# array named ARRVAR the NAME=value tokens one kit command runs with: the MERGED +# kit env (ENVARR) plus the non-interactive git guards. # -# GUARD SCOPING (do not widen): a command may only ever receive ITS OWN kit's -# value for a guard name. OWNGUARDARR names an array of THIS kit's own guard -# tokens (_acq_msb_own_guard_tokens_into). Every guard-name token is stripped from -# the merged set regardless of which kit contributed it, then this kit's own guard -# tokens are re-added, then the adapter's guard values are appended unless this -# kit set GIT_TERMINAL_PROMPT itself. So no other kit can change — or disable — -# the guards for this kit's commands, whether by declaring a guard name this kit -# does not (name collision) or by declaring the same name with a different value -# (value collision, which the merge would otherwise resolve in the other kit's -# favor). A kit that declares a guard var still overrides it for its OWN commands. +# GUARD OWNERSHIP (do not widen). A command may only ever receive ITS OWN kit's +# value for a guard name. OWNGUARDARR names THIS kit's own guard tokens. Tracking +# ownership by NAME is not enough: the merge has already collapsed a duplicate +# name to the LAST kit's value, so kit A declaring GIT_TERMINAL_PROMPT=0 and a +# later kit B declaring =1 would leave A's own commands running with B's opt-out, +# and with no ASKPASS guards either. Hence: strip every guard-name token from the +# merged set whoever contributed it, re-add this kit's own, then append acq's +# values unless this kit set GIT_TERMINAL_PROMPT. Closes both collision shapes +# (another kit declaring a guard name this kit does not; another kit declaring the +# same name with a different value) while a kit's own override still stands for +# its own commands. Shared by the exec and staged-script paths, so they cannot +# drift. See ADR-0033. _acq_msb_env_tokens_with_guards_into() { local _arrn="$1" _envarrn="$2" _ownarrn="$3" eval "$_arrn=()" @@ -1927,12 +1904,10 @@ _acq_msb_env_tokens_with_guards_into() { # _acq_msb_exec_flags_into UFLAG_ARRVAR EFLAG_ARRVAR USER KITENV_ARRVAR # OWNGUARD_ARRVAR — build the `msb exec` -u/-e flag arrays for one kit command. -# Maps the sbx uid contract (1000/agent) onto our by-name agent user, threads the -# MERGED kit env (KITENV_ARRVAR) as `-e NAME=value`, and injects git -# non-interactive guards unless THIS kit declared them (OWNGUARD_ARRVAR holds this -# kit's own guard TOKENS, from _acq_msb_own_guard_tokens_into — the guard value -# must stay this kit's; see _acq_msb_env_tokens_with_guards_into). -# Arrays passed/returned by name (bash 3.2 compat). +# Maps the sbx uid contract (1000/agent) onto our by-name agent user and threads +# the env as `-e NAME=value` (merged kit env + git guards, per +# _acq_msb_env_tokens_with_guards_into, which owns the guard rule). OWNGUARD_ARRVAR +# names this kit's own guard tokens. Arrays by name (bash 3.2 compat). _acq_msb_exec_flags_into() { local _uflag="$1" _eflag="$2" _user="$3" _envarr="$4" _ownguards="${5:-_acq_msb_no_guards}" eval "$_uflag=()" @@ -1964,21 +1939,12 @@ _acq_msb_exec_flags_into() { ;; esac - # Append the guest env as additional `-e NAME=value` flags: the MERGED kit env - # (every effective kit's environment[]) plus the non-interactive git guards. - # # NON-INTERACTIVE ENFORCEMENT. Kit lifecycle commands run before the agent - # attaches, with no terminal, so they MUST NOT try to read from a TTY. Docker's - # kit-reference states startup commands are non-interactive; a kit that prompts - # (e.g. `git clone` of a private repo with no credential -> "Username for - # 'https://github.com':") would BLOCK provision forever (observed: the playbook - # kit hung here). Defense-in-depth alongside the kit's own prompt-suppression: - # - stdin from /dev/null for every kit exec, so nothing can read the TTY. - # - GIT_TERMINAL_PROMPT=0 (+ GIT_ASKPASS/SSH_ASKPASS=false) so git fails fast - # instead of prompting. Injected unless THIS kit set GIT_TERMINAL_PROMPT - # (kits may override intentionally). The env is merged but a guard value is - # never taken from another kit — not by name and not by value. See - # _acq_msb_env_tokens_with_guards_into. + # attaches, with no terminal, so they MUST NOT read from a TTY: a kit that + # prompts (e.g. `git clone` of a private repo with no credential -> "Username + # for 'https://github.com':") would BLOCK provision forever — observed, the + # playbook kit hung here. Two defenses: stdin from /dev/null on every kit exec + # (at the call sites), and the GIT_TERMINAL_PROMPT/ASKPASS guards appended below. local _envtok=() _acq_msb_env_tokens_with_guards_into _envtok "$_envarr" "$_ownguards" local _ev @@ -2066,8 +2032,6 @@ _acq_msb_exec_command() { [ "${#_argv[@]}" -gt 0 ] || return 0 - # Build the `msb exec` -u/-e flag arrays (uid mapping, merged kit env, and the - # git guards — taken from THIS kit's own guard tokens, not the merged set). local uflag=() eflag=() _acq_msb_exec_flags_into uflag eflag "$user" _kit_env "$ownguards" @@ -2124,10 +2088,8 @@ _acq_msb_sq() { # _acq_msb_startup_env_prefix_into PREFIX_ARRVAR USER KITENV_ARRVAR # OWNGUARD_ARRVAR — build the in-guest `env NAME=value …` prefix tokens for one -# startup command: HOME for the agent user, the MERGED kit env vars -# (KITENV_ARRVAR), and the git non-interactive guards taken from THIS kit's own -# guard tokens (OWNGUARD_ARRVAR), exactly as the exec path does. Mirrors -# _acq_msb_exec_flags_into's -e set, but as `env` argv rather than `msb exec -e`. +# startup command: HOME for the agent user plus the same env set +# _acq_msb_exec_flags_into threads, as `env` argv rather than `msb exec -e`. # Tokens are RAW here (NAME=value); the caller single-quote-escapes each before # writing it to the script. _acq_msb_startup_env_prefix_into() { @@ -2138,9 +2100,6 @@ _acq_msb_startup_env_prefix_into() { 1000|agent) eval "$_prefixn+=(\"HOME=/home/agent\")" ;; esac - # Kit-declared env vars (already NAME-validated by kit_spec_env) plus the - # non-interactive git guards, with the same per-kit guard ownership the exec path - # applies (see _acq_msb_env_tokens_with_guards_into). local _envtok=() _ev _acq_msb_env_tokens_with_guards_into _envtok "$_envarrn" "$_ownguards" for _ev in ${_envtok[@]+"${_envtok[@]}"}; do @@ -2239,12 +2198,11 @@ EOF } # _acq_msb_dedupe_env_into ARRVAR TOKEN... — store the NAME=value TOKENs in the -# array named ARRVAR with duplicates collapsed: the LAST value for a name wins, -# the FIRST appearance fixes the order. Same resolution rule the session replay -# applies to the persisted marker, so a command's env and a session's env agree -# on which kit's value survives. Malformed tokens (no `=`, empty name) are -# dropped. Tokens are read from argv and written back as whole array elements, -# so a value containing spaces or metacharacters is never re-split (SI-10). +# array named ARRVAR with duplicates collapsed: LAST value for a name wins, FIRST +# appearance fixes the order — the same rule the session replay applies to the +# marker, so a command's env and a session's env agree on which kit's value +# survives. Malformed tokens are dropped; each token stays one array element, so a +# value with spaces or metacharacters is never re-split (SI-10). _acq_msb_dedupe_env_into() { local _arrn="$1" shift @@ -2270,14 +2228,19 @@ $([ "$#" -eq 0 ] || printf '%s\n' "$@" | awk ' EOF } -# _acq_msb_merge_kit_env_into ARRVAR SPEC... — collect the environment[] entries -# of EVERY given kit spec into the array named ARRVAR as merged NAME=value -# tokens (last kit wins for a duplicate name, in spec order). This is the -# whole-kit-set view a kit's lifecycle commands need: kit env is guest-wide -# configuration, so a daemon a kit starts must see the other kits' vars too, not -# only its own (see ADR-0033). Callers that HAVE the full set (provision, heal) -# build it here in a pre-pass, so the result is independent of the order the kits -# are then applied in. +# _acq_msb_merge_kit_env_into ARRVAR SPEC... — collect the environment[] entries of +# EVERY given kit spec into the array named ARRVAR as merged NAME=value tokens +# (last kit wins for a duplicate name, in spec order). Kit env is guest-wide +# configuration, so a kit's lifecycle commands — especially a `background: true` +# daemon that launches agents — must see the other kits' vars too, not only their +# own (ADR-0033). +# +# WHY A PRE-PASS, not a per-kit marker read: full-set callers (provision, heal) +# MUST call this over the whole kit set BEFORE applying the first kit. Each kit's +# entries are appended to the marker inside the apply loop, so reading the marker +# per kit would make a daemon kit's env depend on whether the env-declaring kit +# happened to be applied first — correct in one kit order, silently wrong in the +# other. Computing the merge up front removes the ordering dependence entirely. _acq_msb_merge_kit_env_into() { local _arrn="$1" shift @@ -2299,10 +2262,8 @@ _acq_msb_merge_kit_env_into() { _acq_msb_startup_body_into() { local _bodyn="$1" _spec="$2" _mergedn="${3:-}" - # The env the emitted commands run with: the MERGED kit env when the caller has - # the full kit set (ADR-0033), else this spec's own environment[]. Either way the - # git guards come from THIS spec's own guard tokens (_own_guard_tok), never from - # another kit's value in the merged set. + # Same env contract as the exec path: the merged kit env when the caller has the + # full kit set, else this spec's own; guards always from this spec's own tokens. local _kit_env=() _own_env=() _own_guard_tok=() _acq_msb_collect_kit_env_into _own_env "$_spec" if [ -n "$_mergedn" ]; then @@ -2950,13 +2911,9 @@ acq_backend_provision() { $(kit_spec_volumes "$spec")" done - # MERGED KIT ENV (ADR-0033), computed ONCE per provision now that every kit is - # fetched: the union of all kits' environment[] with last-kit-wins. Threaded - # onto each kit's lifecycle commands (the apply loop below) and into the staged - # startup-script body, so a kit's `background: true` daemon sees the whole - # guest env, not only its own kit's vars. Computed here, BEFORE any kit is - # applied, so the result is independent of kit application order (the per-kit - # marker append happens inside the apply loop). + # Merged kit env, computed ONCE now that every kit is fetched and BEFORE any kit + # is applied — see _acq_msb_merge_kit_env_into for why it must be a pre-pass. + # Consumed by the apply loop below and by the staged startup-script body. local _merged_kit_env=() _pspecs=() _kd for _kd in ${kitdirs[@]+"${kitdirs[@]}"}; do _pspecs+=("${_kd}/spec.yaml") @@ -2971,9 +2928,8 @@ $(kit_spec_volumes "$spec")" # NOTE and _acq_msb_apply_kit_dir). Only the first kit with startup commands # stakes the fixed script name (see _acq_msb_stage_startup_script). # - # Staged AFTER the fetch loop (not inside it) so the body can carry the merged - # kit env, which is only knowable once every kit has been fetched. Iterating - # kitdirs in the same order preserves the first-kit-stakes rule. + # Runs AFTER the fetch loop, not inside it, so the body can carry the merged kit + # env; the same list in the same order keeps the first-kit-stakes rule. for _kd in ${kitdirs[@]+"${kitdirs[@]}"}; do [ -f "${_kd}/spec.yaml" ] || continue _acq_msb_stage_startup_script "${_kd}/spec.yaml" create_flags _merged_kit_env @@ -3512,13 +3468,8 @@ EOF # abort provision under `set -e` and leave an already-created, agent-installed # sandbox half-configured with no diagnostic. Warn and continue — the kits are # individually non-fatal (the playbook kit already self-heals on next start), - # matching acq_backend_ensure_kits_applied's best-effort heal loop. - # - # Each kit's lifecycle commands run with the MERGED kit env computed in the - # pre-pass above (ADR-0033), so a kit's `background: true` daemon sees the whole - # guest env rather than only its own kit's vars. It has to be a PRE-pass: the - # marker is appended inside this loop, so reading it per kit would make the - # result depend on kit order (a daemon kit applied first would see nothing). + # matching acq_backend_ensure_kits_applied's best-effort heal loop. Each kit's + # commands get the merged env from the pre-pass above. local kd acq_spin_start "Applying configuration kits" _acq_msb_reset_kit_env "$name" @@ -5316,23 +5267,18 @@ acq_backend_apply_kit() { echo "acq(msb): note: this kit declares volumes:, which apply at CREATE time only —" >&2 echo "acq(msb): this apply skips them. Recreate the sandbox (acq rm && acq run) to mount them." >&2 fi - # No merged-env array is passed here on purpose: a mid-life apply knows only - # THIS kit, so there is no full set to pre-merge. _acq_msb_apply_kit_dir falls - # back to the sandbox's already-persisted merged value (ADR-0033), which is the - # correct whole-guest view for an additive add. + # No merged-env array on purpose: a mid-life apply knows only THIS kit, so + # _acq_msb_apply_kit_dir uses its persisted-marker fallback (see its step 3). _acq_msb_apply_kit_dir "$name" "$kitdir" } # _acq_msb_heal_kit_set NAME BUILTIN_COUNT KITREF... — the heal's kit loop: fetch -# every ref, merge their environment[] in a PRE-PASS, reset the marker, then apply -# each kit with that merged env. Returns 1 if any of the first BUILTIN_COUNT refs -# failed to fetch or apply (the caller's provenance verdict), 0 otherwise. -# -# Two loops, not one: the merged env must be known BEFORE the first kit is applied -# (ADR-0033). The per-kit marker append happens inside the apply loop, so reading -# the marker per kit would make a daemon kit's env depend on whether the -# env-declaring kit happened to be applied before it. _builtin mirrors _dirs so a -# built-in's apply failure is still attributable once the loops are separated. +# every ref, merge their environment[] in a pre-pass (see +# _acq_msb_merge_kit_env_into for why fetch and apply must be separate loops), +# reset the marker, then apply each kit with that merged env. Returns 1 if any of +# the first BUILTIN_COUNT refs failed to fetch or apply (the caller's provenance +# verdict), 0 otherwise. _builtin mirrors _dirs so a built-in's apply failure stays +# attributable across the two loops. _acq_msb_heal_kit_set() { local name="$1" builtin_count="$2" shift 2 diff --git a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md index eacdb621..1a35e324 100644 --- a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md +++ b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md @@ -15,158 +15,99 @@ supersedes: [] ## Context and Problem Statement -On the msb backend, a kit's lifecycle commands (`install`, `initFiles`, -`startup`) ran with only **that kit's own** `environment[]`, threaded as -`msb exec -e NAME=value`. Interactive sessions were already whole-guest: acq -replays the merged marker on `run`/`attach`/`shell` (ADR-0011's 2026-08-26 -update). Commands started by a kit's lifecycle phase were not. - -That split is invisible until a kit's command **outlives the command** and -launches other processes. A daemon kit starts its supervisor from a -`background: true` startup command, so every agent that daemon later spawns -inherits only the daemon kit's env. A separate kit that sets `OPENCODE_CONFIG` -(agent instructions plus a default-deny permission layer) is silently missing -from those agents, while the injected credentials are still present — a quiet -loss of a policy/permission control, not a visible failure. Terminal sessions -look correct, so a smoke test passes. - -The hard part is **order independence**. The persisted marker -(`/var/lib/acq/kit-env`) is appended per kit *inside the same loop that runs that -kit's commands*, so "read the merged marker when running commands" is correct -only when the env-declaring kit happens to be applied before the daemon kit, and -silently wrong in the reverse order. +On msb, a kit's lifecycle commands (`install`, `initFiles`, `startup`) ran with only +**that kit's own** `environment[]`. Sessions were already whole-guest — acq replays +the merged `/var/lib/acq/kit-env` marker on `run`/`attach`/`shell` (ADR-0011's +2026-08-26 update) — but commands were not. + +That split is invisible until a kit's command outlives itself. A daemon kit starts its +supervisor from a `background: true` startup command, so every agent that daemon spawns +inherits only the daemon kit's env: another kit's `OPENCODE_CONFIG` (agent instructions +plus a default-deny permission layer) was silently absent from those agents while the +injected credentials were still present — a quiet loss of a policy control, and +sessions still looked correct, so smoke tests passed. + +**The order-independence trap.** The marker is appended per kit *inside the same loop +that runs that kit's commands*, so "read the merged marker when running commands" is +correct only when the env-declaring kit happens to be applied first, and silently +wrong in the reverse order. Any fix must establish the merged set before the first +kit is applied. ## Decision Drivers -- **A kit must not be able to silently drop another kit's policy config** from - the environment of agents it launches (AC-3, AC-6, CM-6). -- **Order independence** — the effective env must not depend on the order kits - are applied in, which is an implementation detail of the kit list. -- **One kit must not weaken another kit's hardening** — the adapter's - non-interactive git guards are per-kit safety, not shared config (CM-7). -- **Same resolution rule as sessions** — last-value-wins, so a command and a - session agree on which kit's value survives. -- **No new guest dependency** — kits must not each have to source a marker file - themselves; the adapter owns the threading. -- **Fail soft, never empty** — an unreadable marker degrades to the previous - per-kit behavior rather than stripping a kit's own env. +- A kit must not silently drop another kit's policy config from the env of agents it + launches, and the result must not depend on kit application order (AC-3, AC-6, CM-6). +- One kit must not weaken another kit's hardening: the non-interactive git guards are + per-kit safety, not shared config (CM-7). +- Kits must not each have to source a marker file; the adapter owns the threading. ## Considered Options -1. **Pre-pass over the full kit set in the full-set paths; marker fallback for - the mid-life single-kit path.** Chosen. -2. **Read the persisted marker inside `_acq_msb_run_commands` for every path.** - Rejected: order-dependent by construction — the marker is written in the same - per-kit loop, so a daemon kit applied before the env-declaring kit still sees - nothing. It would fix the reported case only for one kit ordering. -3. **Have each kit source `/var/lib/acq/kit-env` in its own commands.** Rejected: - pushes an adapter concern into every kit, is opt-in (so a kit that forgets is - silently broken), and couples kit content to a guest path acq owns. -4. **Do nothing; document that kit env is session-only.** Rejected: the - vocabulary exists for agent-runtime config (ADR-0011), and the failure mode is - a silent loss of permission/policy configuration. +**Pre-pass over the full kit set — chosen.** A **per-kit marker read** is +order-dependent by construction (see the trap above), fixing the reported case in one +kit order only. Having each kit source the marker itself is opt-in — a kit that +forgets is silently broken — and pushes an adapter concern into every kit. ## Decision Outcome -**Chosen: Option 1.** Kit `environment[]` is treated as **guest-wide -configuration**, so every kit lifecycle command runs with the merged env of the -full effective kit set, using the same last-value-wins order sessions replay. - -Where the merged set comes from, per path: +Kit `environment[]` is **guest-wide configuration**: every kit lifecycle command runs +with the merged env of the full effective kit set, last-value-wins — the same +resolution sessions replay. | Path | Source of the merged env | |------|--------------------------| -| `acq_backend_provision` (create) | pre-pass over every fetched kit spec, computed **before** the first kit is applied | -| `acq_backend_ensure_kits_applied` (heal / `acq start`, `restart`, re-attach) | same pre-pass; kit refs are resolved in one loop, merged, then applied in a second loop | -| `acq_backend_apply_kit` (`acq kit apply NAME KITREF`) | no full set exists — falls back to the sandbox's already-persisted merged marker value | -| neither available (marker absent or unwritable) | this kit's own `environment[]` — the pre-existing behavior | - -The mid-life fallback is correct for an additive add: the new kit's own entries -are appended to the marker *before* its commands run, and the read applies -last-value-wins, so the new kit still overrides an older value. - -**Guard ownership stays per kit, tracked by value (load-bearing).** The adapter -injects `GIT_TERMINAL_PROMPT=0` + `GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit -commands unless *that* kit declared `GIT_TERMINAL_PROMPT` itself. Deciding this -from the *merged* set would let one kit's opt-out disable the guards for every -other kit's commands — and a prompting kit command with no credential blocks -provision forever (observed with the playbook kit). So the merged set is widened -for **threading** only: a command receives **only its own kit's value** for a -guard name. - -The ownership record is the owning kit's guard **tokens** (`NAME=value`), not just -the names it declared. A name-only record is insufficient, because the merge has -already collapsed a duplicate name to the *last* kit's value before the filter -runs — so it cannot tell a kit's own guard value from another kit's. That gap was -a real regression against the pre-change behavior: with kit A declaring -`GIT_TERMINAL_PROMPT=0` and a later kit B declaring `=1`, kit A's own command ran -with kit B's opt-out *and* with no `GIT_ASKPASS`/`SSH_ASKPASS` at all, where -before it had correctly got its own `0`. The implementation therefore strips -**every** guard-name token from the merged set regardless of contributor, re-adds -this kit's own guard tokens, then appends acq's guard values unless this kit set -`GIT_TERMINAL_PROMPT` itself. Both collision shapes — another kit declaring a -guard name this kit does not (name collision) and another kit declaring the same -name with a different value (value collision) — are closed, while a kit that -declares a guard var still overrides it for its own commands. - -The staged create-time `--script-path` body (ADR-0017) gets the same treatment, -for consistency. **That path is inert today** — a bare `--script-path` -registration is staged on the guest PATH and is not auto-run at boot — so this -part fixes no live symptom; it keeps the two paths from drifting so the ADR-0017 -version-bump re-verification trap (a future msb that *does* auto-run the script) -cannot resurface this bug. - -### Positive Consequences - -- A daemon kit's launched agents see every kit's declared config, so a policy/ - permission layer contributed by another kit is no longer silently absent. -- The effective env is identical in both kit application orders. -- Commands and sessions now agree on the env and on duplicate resolution. -- Kits need no knowledge of `/var/lib/acq/kit-env`. - -### Negative Consequences - -- **Broader blast radius per var.** A var declared by one kit now reaches every - kit's commands. This is the intended semantics (it matches sbx, where - `environment.variables` is sandbox-level), but a kit author can no longer - assume their command's env is theirs alone. -- **Staging-order coupling at create.** The `--script-path` staging moved out of - the kit fetch loop into a second loop so the merged env is known first. The - first-kit-stakes-the-script-name rule is preserved by iterating the same list - in the same order. -- **Not addressed: a merged var can name a not-yet-staged path.** Kit *N*'s - commands can now receive e.g. `OPENCODE_CONFIG` pointing at a file kit *N+k* - has not copied in yet. Before this change the daemon saw **no** var; now it can - see a var pointing at a **missing** file, which converts "no team config" into - "possibly broken team config" for the window between the daemon starting and - the declaring kit's files landing. This is the same class as the sbx - startup/attach race (a startup command racing the state it depends on, tracked - separately — see Links) and is deliberately out of scope here: fixing it needs - an ordering/barrier decision (stage every kit's `files[]` before any kit's - commands, or a readiness barrier before background startup), not an env-scoping - one. Kits whose commands read a path from a var should already tolerate its - absence. -- The guard-scoping rule is a real subtlety in the adapter: merged for threading, - per-kit for the guard decision. It is asserted by a regression test so a future - simplification cannot quietly collapse the two. +| `acq_backend_provision` (create) | pre-pass over every fetched kit spec, before the first apply | +| `acq_backend_ensure_kits_applied` (heal / `start`, `restart`, re-attach) | same pre-pass; refs resolved in one loop, merged, applied in a second | +| `acq_backend_apply_kit` (`acq kit apply`) | no full set exists — the sandbox's already-persisted merged marker value | +| neither available (marker absent/unwritable) | this kit's own `environment[]` — the pre-existing behavior | + +The mid-life fallback suits an additive add: the new kit's entries reach the marker +*before* its commands run, and the read is last-value-wins, so it still overrides. + +**Guard ownership is per kit, tracked by value.** acq injects `GIT_TERMINAL_PROMPT=0` + +`GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit commands unless *that* kit declared +`GIT_TERMINAL_PROMPT` itself; a prompting kit command with no credential blocks +provision forever (observed with the playbook kit). So a command may only ever receive +**its own kit's value** for a guard name — and the ownership record must be that kit's +guard **tokens** (`NAME=value`), not the names it declared, because the merge has +already collapsed a duplicate name to the *last* kit's value: + +> Kit A declares `GIT_TERMINAL_PROMPT=0` and owns a command; a later kit B declares +> `=1`. Under name-only ownership A's own command ran with B's opt-out *and* no +> `GIT_ASKPASS`/`SSH_ASKPASS` at all — worse than before the change, where A correctly +> got its own `0`. + +So: strip every guard-name token from the merged set whoever contributed it, re-add this +kit's own, then append acq's values unless this kit set `GIT_TERMINAL_PROMPT`. Both +collision shapes (name and value) are closed; a kit's own override still stands for its +own commands. The staged `--script-path` body shares the same helper so the paths cannot +drift — **that path is inert today** (ADR-0017), so it fixes no live symptom; it exists +so ADR-0017's re-verification trap cannot resurface this bug unnoticed. + +## Consequences + +- **Broader blast radius per var.** A var one kit declares now reaches every kit's + commands — intended (it matches sbx, where `environment.variables` is sandbox-level), + but a kit author can no longer assume a command's env is theirs alone. +- **Staging-order coupling at create.** `--script-path` staging moved into a second + loop over the same kit dirs, since the merged env is only knowable once every kit is + fetched; same list, same order, so the first-kit-stakes-the-name rule holds. +- **Knowingly left undone: a merged var can name a not-yet-staged path.** Kit *N*'s + commands can now receive e.g. `OPENCODE_CONFIG` pointing at a file kit *N+k* has not + copied in yet, turning "no team config" into "possibly broken team config" until the + declaring kit's files land. That is the startup-races-its-own-prerequisites class, not + an env-scoping one: fixing it needs an ordering/barrier decision (stage all `files[]` + before any commands, or a readiness barrier before background startup). ## Links -- ADR-0011 (msb backend and neutral hybrid/v1 kits) — defines the `environment` - vocabulary and the session-replay marker this widens to lifecycle commands. -- ADR-0014 (neutral port-publish and background-command vocabulary) — defines the - `background: true` startup commands whose daemons made the scoping gap visible. -- ADR-0017 (msb create-time startup-script staging) — the staged body updated for - consistency here, including its inertness caveat and re-verification trap. -- `acq.backends/msb.sh` — `_acq_msb_merge_kit_env_into` (the pre-pass), - `_acq_msb_own_guard_tokens_into` + `_acq_msb_env_tokens_with_guards_into` - (merged threading, per-kit guard ownership by value), - `_acq_msb_persisted_kit_env_into` (the mid-life fallback and session replay). -- `test/bats/79-msb-merged-kit-env.bats` — the lifecycle-command regression - coverage (both kit orders, all three phases, the mid-life and heal paths, - duplicate resolution, and both guard-collision shapes). - `test/bats/78-msb-kit-env.bats` keeps the marker/session-replay coverage; - `test/bats/73-msb-kits-startup.bats` covers the staged-body side. -- Reported as GSA-TTS/agentic-coding-quickstart#515. The related - startup-races-its-own-prerequisites class noted under Negative Consequences is - tracked as GSA-TTS/agentic-coding-quickstart#506. +- ADR-0011 (the `environment` vocabulary and session-replay marker this widens), + ADR-0014 (`background: true`, whose daemons made the gap visible), ADR-0017 (the + staged-script path and its inertness caveat). +- `acq.backends/msb.sh`: `_acq_msb_merge_kit_env_into` (pre-pass), + `_acq_msb_own_guard_tokens_into` + `_acq_msb_env_tokens_with_guards_into` (guard + ownership), `_acq_msb_persisted_kit_env_into` (mid-life fallback, session replay). +- Tests: `test/bats/79-msb-merged-kit-env.bats` (lifecycle commands), + `78-msb-kit-env.bats` (marker/session replay), `73-msb-kits-startup.bats` (staged body). +- Reported as GSA-TTS/agentic-coding-quickstart#515; the deferred + startup-races-its-prerequisites class is GSA-TTS/agentic-coding-quickstart#506. diff --git a/test/bats/79-msb-merged-kit-env.bats b/test/bats/79-msb-merged-kit-env.bats index 2190c191..5923a907 100644 --- a/test/bats/79-msb-merged-kit-env.bats +++ b/test/bats/79-msb-merged-kit-env.bats @@ -148,9 +148,10 @@ schemaVersion: "hybrid/v1" kind: mixin name: dup-two displayName: Dup Two -description: overrides GITLAB_HOST from a later kit +description: overrides GITLAB_HOST from a later kit, and carries an awkward value environment: GITLAB_HOST: gitlab.second.gov + OPENCODE_CONFIG: /home/agent/a b.jsonc; touch /tmp/MERGE_PWNED SPEC _env_merge_provision "$k1" "$k2" local cmd; cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo DUP_CMD' | head -n1) @@ -158,6 +159,11 @@ SPEC # the same resolution the session replay applies, and exactly once (never both). assert_regex "$cmd" '\-e GITLAB_HOST=gitlab\.second\.gov' refute_regex "$cmd" 'gitlab\.first\.gov' + # A value with spaces and metacharacters crosses the merge as ONE token: it is + # threaded as a single `-e` argv element, never re-split into a second variable + # and never interpreted as shell syntax (SI-10). + assert_regex "$cmd" '\-e OPENCODE_CONFIG=/home/agent/a b\.jsonc; touch /tmp/MERGE_PWNED' + refute_regex "$cmd" '\-e touch' } # --- the git guards belong to the kit whose command is running ------------ @@ -291,35 +297,19 @@ SPEC assert_regex "$bg" '\-e PASEO_OWN_VAR=own' } -@test "ADR-0033: the merge re-validates names and never re-splits a value" { - # The merge helper is a second place kit-derived NAME=value tokens flow through, - # so it re-applies the kit_spec_env charset itself (defense in depth), and each - # token stays ONE array element so a value with spaces or metacharacters is - # never re-split into a second variable (SI-10). - run bash -c ' - . "'"$REPO_ROOT"'/acq.backends/kit-translate.sh" - . "'"$REPO_ROOT"'/acq.backends/msb.sh" - m=() - _acq_msb_dedupe_env_into m "BAD-NAME=x" "=noname" "noequals" \ - "OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED" "OK=1" - printf "TOKEN[%s]\n" ${m[@]+"${m[@]}"} - ' - assert_success - refute_output --partial 'BAD-NAME' - refute_output --partial 'noequals' - assert_output --partial 'TOKEN[OPENCODE_CONFIG=/home/agent/a b.jsonc; touch /tmp/PWNED]' - assert_output --partial 'TOKEN[OK=1]' -} - # VALUE collision — the case a name-only ownership record cannot catch. The merge # collapses a duplicate name to the LAST kit's value, so an earlier kit that # declared GIT_TERMINAL_PROMPT=0 saw kit B's =1 on its own commands, with no # GIT_ASKPASS/SSH_ASKPASS added either (the "owner declared it, leave it alone" # branch). Reproduces a real regression: at base that command got its own 0. +# +# The two kits also cover the mirror requirement in one pass: kit B is a kit that +# deliberately opts ITSELF out, so its own override must survive intact (its value, +# exactly once, no acq-injected 0 alongside it) and both kits must still receive +# the other's NON-guard merged vars — proving the hole was closed by scoping the +# guards, not by disabling the merge. @test "ADR-0033: two kits declaring the SAME guard name with different values" { - # Kit A owns the command and declares the safe value; kit B, merged later, - # declares the opt-out. Kit A's command must keep kit A's value. local ka="$STUBDIR/guardvalkitA" kb="$STUBDIR/guardvalkitB" mkdir -p "$ka" "$kb" cat > "$ka/spec.yaml" <<'SPEC' @@ -327,9 +317,10 @@ schemaVersion: "hybrid/v1" kind: mixin name: guardval-a displayName: GuardVal A -description: owns the command and declares the safe guard value +description: owns a command and declares the safe guard value environment: GIT_TERMINAL_PROMPT: "0" + A_VAR: from-a commands: - phase: startup user: "0" @@ -346,7 +337,7 @@ displayName: GuardVal B description: a later kit opting ITSELF out of the guards environment: GIT_TERMINAL_PROMPT: "1" - TEAM_VAR: shared + B_VAR: from-b commands: - phase: startup user: "0" @@ -356,58 +347,24 @@ commands: - echo GUARDVAL_B_CMD SPEC _env_merge_provision "$ka" "$kb" - local log; log=$(cat "$CALLS") - # Kit A's command: its own value, never kit B's. - local a; a=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_A_CMD' | head -n1) + local log a b na nb + log=$(cat "$CALLS") + # Kit A's command: its own value, never kit B's, and the ASKPASS guards absent + # because A declared GIT_TERMINAL_PROMPT itself. + a=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_A_CMD' | head -n1) assert_regex "$a" '\-e GIT_TERMINAL_PROMPT=0' refute_regex "$a" 'GIT_TERMINAL_PROMPT=1' - # Kit B's command: its own opt-out value, never kit A's. - local b; b=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_B_CMD' | head -n1) + na=$(printf '%s\n' "$a" | tr ' ' '\n' | grep -c '^GIT_TERMINAL_PROMPT=') + assert_equal "$na" "1" + # Kit B's command: its own opt-out value stands, exactly once, with no + # acq-injected 0 — a kit's intentional override is not clobbered. + b=$(printf '%s\n' "$log" | grep 'echo GUARDVAL_B_CMD' | head -n1) assert_regex "$b" '\-e GIT_TERMINAL_PROMPT=1' refute_regex "$b" 'GIT_TERMINAL_PROMPT=0' - # Both still receive the other kit's NON-guard merged vars. - assert_regex "$a" '\-e TEAM_VAR=shared' - assert_regex "$b" '\-e TEAM_VAR=shared' -} - -@test "ADR-0033: a kit's own guard override is not clobbered by the merged set" { - # The mirror of the finding: fixing value-collision must not stop a kit from - # overriding the guards for ITS OWN commands. Its own value appears once, with - # no conflicting duplicate, and acq's GIT_TERMINAL_PROMPT=0 is not added. - local ok="$STUBDIR/ownoverridekit" ck="$STUBDIR/ownotherkit" - mkdir -p "$ok" "$ck" - cat > "$ok/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: own-override -displayName: Own Override -description: deliberately opts itself out of the git guards -environment: - GIT_TERMINAL_PROMPT: "1" -commands: - - phase: startup - user: "0" - command: - - sh - - -c - - echo OWN_OVERRIDE_CMD -SPEC - cat > "$ck/spec.yaml" <<'SPEC' -schemaVersion: "hybrid/v1" -kind: mixin -name: own-other -displayName: Own Other -description: contributes only a non-guard var -environment: - OPENCODE_CONFIG: /home/agent/.config/opencode/team.jsonc -SPEC - _env_merge_provision "$ok" "$ck" - local cmd n - cmd=$(printf '%s\n' "$(cat "$CALLS")" | grep 'echo OWN_OVERRIDE_CMD' | head -n1) - assert_regex "$cmd" '\-e GIT_TERMINAL_PROMPT=1' - refute_regex "$cmd" 'GIT_TERMINAL_PROMPT=0' - n=$(printf '%s\n' "$cmd" | tr ' ' '\n' | grep -c '^GIT_TERMINAL_PROMPT=') - assert_equal "$n" "1" - # And it still receives the other kit's merged non-guard var. - assert_regex "$cmd" '\-e OPENCODE_CONFIG=/home/agent/\.config/opencode/team\.jsonc' + nb=$(printf '%s\n' "$b" | tr ' ' '\n' | grep -c '^GIT_TERMINAL_PROMPT=') + assert_equal "$nb" "1" + # Both still receive the OTHER kit's non-guard merged vars: the guards were + # scoped, the merge was not disabled. + assert_regex "$a" '\-e B_VAR=from-b' + assert_regex "$b" '\-e A_VAR=from-a' } From 0b1de5ef766244bf04d33e607a6f948936778136 Mon Sep 17 00:00:00 2001 From: Basilio Bogado <541149+basiliskus@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:08:32 -0700 Subject: [PATCH 4/4] fix(msb): layer a kit's own env last and skip shortcut kits in the merge The mid-life `acq kit apply` path used the persisted kit-env marker whenever it was non-empty. The append of the kit's own entries only warns on failure, so a stale marker silently dropped the kit's own environment[] from its commands. Layer the kit's own entries after the persisted value instead. Kits with a backend_shortcuts.msb entry skip the generic apply, so their environment[] is never persisted for sessions; leave them out of the merged env so lifecycle commands and sessions agree on the guest env. Refs: GSA-TTS/agentic-coding-quickstart#519 --- acq.backends/msb.sh | 17 ++++-- ...nv-is-guest-wide-for-lifecycle-commands.md | 12 ++-- test/bats/79-msb-merged-kit-env.bats | 61 +++++++++++++++++++ 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/acq.backends/msb.sh b/acq.backends/msb.sh index 91f93134..31998dbd 100644 --- a/acq.backends/msb.sh +++ b/acq.backends/msb.sh @@ -1602,13 +1602,16 @@ EOF # Commands run with the MERGED env of the whole kit set, not just this kit's # (ADR-0033). Full-set callers pass its array name; the mid-life single-kit # path (`acq kit apply`) has none, so it recovers the sandbox's persisted - # merged value — step 2 appended THIS kit's entries first and the read is - # last-value-wins, so a mid-life add still overrides. An unreadable marker - # degrades to this kit's own env: fail-soft, never empty. + # merged value with THIS kit's entries layered last (last-value-wins). The + # re-layering matters: step 2's append only warns on failure, so the marker + # can be stale — without it the kit's own env would be silently dropped. local _merged_env=() _merged_envn="$mergedn" if [ -z "$_merged_envn" ]; then - _acq_msb_persisted_kit_env_into _merged_env "$name" - [ "${#_merged_env[@]}" -gt 0 ] && _merged_envn=_merged_env + local _persisted_env=() + _acq_msb_persisted_kit_env_into _persisted_env "$name" + _acq_msb_dedupe_env_into _merged_env \ + ${_persisted_env[@]+"${_persisted_env[@]}"} ${_kit_env[@]+"${_kit_env[@]}"} + _merged_envn=_merged_env fi _acq_msb_run_commands "$name" "$spec" "$_merged_envn" } @@ -2249,6 +2252,10 @@ _acq_msb_merge_kit_env_into() { local _mkall=() _mkspec for _mkspec in "$@"; do [ -f "$_mkspec" ] || continue + # A backend-shortcut kit skips the generic apply, so its env is never + # persisted for sessions; merging it here would make commands and sessions + # disagree on the guest env. + kit_spec_has_shortcut "$_mkspec" msb && continue _acq_msb_collect_kit_env_into _mkall "$_mkspec" done _acq_msb_dedupe_env_into "$_arrn" ${_mkall[@]+"${_mkall[@]}"} diff --git a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md index 1a35e324..0f812d24 100644 --- a/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md +++ b/docs/adr/0033-msb-kit-env-is-guest-wide-for-lifecycle-commands.md @@ -58,11 +58,15 @@ resolution sessions replay. |------|--------------------------| | `acq_backend_provision` (create) | pre-pass over every fetched kit spec, before the first apply | | `acq_backend_ensure_kits_applied` (heal / `start`, `restart`, re-attach) | same pre-pass; refs resolved in one loop, merged, applied in a second | -| `acq_backend_apply_kit` (`acq kit apply`) | no full set exists — the sandbox's already-persisted merged marker value | -| neither available (marker absent/unwritable) | this kit's own `environment[]` — the pre-existing behavior | +| `acq_backend_apply_kit` (`acq kit apply`) | no full set exists: the sandbox's persisted marker value, with this kit's own `environment[]` layered last | -The mid-life fallback suits an additive add: the new kit's entries reach the marker -*before* its commands run, and the read is last-value-wins, so it still overrides. +The mid-life path suits an additive add: layering the kit's own entries last means it +still overrides, and a stale or absent marker (the append in the same apply is only +best-effort) can never drop the kit's own env. + +A kit with a `backend_shortcuts.msb` entry is left out of the merge: msb applies it +natively and skips the generic apply, so its `environment[]` never reaches the marker, +and merging it into commands would make them disagree with sessions. **Guard ownership is per kit, tracked by value.** acq injects `GIT_TERMINAL_PROMPT=0` + `GIT_ASKPASS`/`SSH_ASKPASS=/bin/false` onto kit commands unless *that* kit declared diff --git a/test/bats/79-msb-merged-kit-env.bats b/test/bats/79-msb-merged-kit-env.bats index 5923a907..fa404c4d 100644 --- a/test/bats/79-msb-merged-kit-env.bats +++ b/test/bats/79-msb-merged-kit-env.bats @@ -368,3 +368,64 @@ SPEC assert_regex "$a" '\-e B_VAR=from-b' assert_regex "$b" '\-e A_VAR=from-a' } + +@test "ADR-0033: mid-life 'kit apply' keeps its own env when the marker append is lost" { + # The persisted read can return a STALE non-empty marker that lacks this kit's + # entries (step 2's append failed: it only warns). The kit's commands must still + # carry its own environment[], winning over a stale value for the same name, + # while keeping the other persisted entries. + export STUB_RECORDED_KIT_ENV='OPENCODE_CONFIG=/stale.jsonc +OTHER_KIT_VAR=kept' + local mk="$STUBDIR/lostappendkit"; mkdir -p "$mk" + cat > "$mk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: lostappend-kit +displayName: Lost Append Kit +description: added to a live sandbox whose marker append does not land +environment: + OPENCODE_CONFIG: /fresh.jsonc + LOST_OWN: yes +commands: + - phase: startup + user: "0" + background: true + command: + - lostappend-daemon +SPEC + : > "$CALLS" + ( export ACQ_SECRET_STORE_DIR="$STUBDIR/lostappend-secrets" + . "${REPO_ROOT}/acq.backends/secret-store.sh" + . "${REPO_ROOT}/acq.backends/kit-translate.sh" + . "${REPO_ROOT}/acq.backends/msb.sh" + acq_backend_apply_kit lostappendbox "$mk" >/dev/null 2>&1 ) + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e LOST_OWN=yes' + assert_regex "$bg" '\-e OPENCODE_CONFIG=/fresh\.jsonc' + refute_regex "$bg" 'stale\.jsonc' + assert_regex "$bg" '\-e OTHER_KIT_VAR=kept' +} + +@test "ADR-0033: a backend-shortcut kit's environment[] is not merged into other kits" { + # A kit with a backend_shortcuts.msb entry skips the generic apply path, so its + # environment[] is never persisted for sessions. Merging it into other kits' + # commands would make commands and sessions disagree on the guest env. + local sk="$STUBDIR/shortcutkit"; mkdir -p "$sk" + cat > "$sk/spec.yaml" <<'SPEC' +schemaVersion: "hybrid/v1" +kind: mixin +name: shortcut-kit +displayName: Shortcut Kit +description: handled natively by msb; declares env that must stay unmerged +environment: + SHORTCUT_VAR: leaked +backend_shortcuts: + msb: + trust_host_cas: true +SPEC + _env_kit_daemon "$STUBDIR/daemonkit" + _env_merge_provision "$sk" "$STUBDIR/daemonkit" + local bg; bg=$(printf '%s\n' "$(cat "$CALLS")" | grep 'nohup' | head -n1) + assert_regex "$bg" '\-e PASEO_OWN_VAR=own' + refute_regex "$bg" 'SHORTCUT_VAR' +}