diff --git a/AGENTS.md b/AGENTS.md index 8d638ae..352be2a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,7 +125,18 @@ networking or the image, `make e2e` green too. anywhere in there silently closes the argument and the rest runs in the *outer* shell — valid bash, wrong program, clean `bash -n` and clean shellcheck. Nothing in it may use `'`; jq programs there are written unquoted (`.defaultThinkingLevel=\$lvl`), and - `tests/unit/06_entrypoint.sh` fails the build if that ever changes. + `tests/unit/06_entrypoint.sh` fails the build if that ever changes. (It bit once while + editing a comment: one apostrophe in a word like `pi's` is enough.) + **Stdout inside the container belongs to the command that ran.** Everything the sandbox + says about itself goes to stderr — `log()` and `fail()` do, and so must every new line, + because the driver's preflight reads its answer from that stream and progress.mjs reads the + child pi event stream from it. The pi-seeding notices were on stdout once, and the steady + state (host file unchanged, so `cur = prev`) prints one on *every* pi-bridged run: it made + `ai-sandbox-autophase` refuse a working sandbox on every project. `tests/unit/06_entrypoint.sh` + now runs the seeding block with the two streams split and asserts stdout empty in each state + (cold, steady, edited-in-sandbox, unparseable, level changed), and greps the entrypoint for + any `[aisb]` line without `>&2`. Same block, second contract: the thinking level is policy, + so it is written on the *first* seed too, not only by the merge on the second run. * `split_target()` / `classify_allows()` — the mapping from what the user typed to what the firewall and the proxy enforce. * `build_argv()` — every isolation flag lives here. @@ -189,6 +200,13 @@ networking or the image, `make e2e` green too. the preflight name it, and every child starts `/dev/null || fail "setpriv is missing from the image; rebuil # the entrypoint has no CAP_DAC_OVERRIDE, so root could not write there anyway. It is # also the honest identity -- these are the user's own files. # +# Stdout belongs to the command that was asked for. Everything the sandbox says about +# itself goes to stderr, because two readers take that stream as data: ai-sandbox-autophase's +# preflight reads its own answer from it, and progress.mjs reads the child pi's JSON event +# stream from it. A notice on stdout is not cosmetic -- it is a line the reader has to +# explain. tests/unit/06_entrypoint.sh runs this block with stdout split off and fails if it +# is not empty, and checks every `[aisb]` line in the entrypoint for the redirect. +# # The two files are managed differently, on purpose. # # models.json is a catalog, so it is managed whole: replaced when it is missing or still @@ -1052,12 +1059,12 @@ if [[ -d "$POLICY_DIR/pi" ]]; then echo "[aisb] pi config: cannot read $f -- its mode or SELinux label blocks this container, and nothing inside can repair it (remove it from the host, or use --ephemeral-home)" >&2 elif [ -z "$cur" ]; then install -m 600 "$src" "$tgt" && printf %s "$want" > "$mark" - echo "[aisb] pi config: seeded $f" + echo "[aisb] pi config: seeded $f" >&2 elif [ "$cur" = "$prev" ]; then install -m 600 "$src" "$tgt" && printf %s "$want" > "$mark" - echo "[aisb] pi config: refreshed $f from the host config" + echo "[aisb] pi config: refreshed $f from the host config" >&2 else - echo "[aisb] pi config: left $f alone (changed inside the sandbox; delete it to take the host version again)" + echo "[aisb] pi config: left $f alone (changed inside the sandbox; delete it to take the host version again)" >&2 fi fi @@ -1067,8 +1074,23 @@ if [[ -d "$POLICY_DIR/pi" ]]; then tgt="$dst/$f"; wrote="$dst/.aisb-$f.wrote"; tmp="$dst/.aisb-$f.tmp" rm -f "$dst/.aisb-$f.seeded" # the whole-file marker is .wrote now if [ ! -e "$tgt" ]; then - install -m 600 "$src" "$tgt" && cp "$tgt" "$wrote" - echo "[aisb] pi config: seeded $f" + # The level is policy, so it goes in on the very first seed too. Copying the host + # file alone left a fresh sandbox home running one unit at the default of pi itself, + # with the merge on the next run quietly correcting it -- policy that waits a run is + # not policy. + if [ -n "$lvl" ]; then + first="$(jq --arg lvl "$lvl" .defaultThinkingLevel=\$lvl "$src" 2>/dev/null)" + else + first="$(jq . "$src" 2>/dev/null)" + fi + if [ -n "$first" ] && printf "%s\n" "$first" > "$tmp" && install -m 600 "$tmp" "$tgt"; then + : + else + install -m 600 "$src" "$tgt" # not JSON we could rewrite, so take it as it is + fi + rm -f "$tmp" + cp "$tgt" "$wrote" + echo "[aisb] pi config: seeded $f" >&2 elif [ ! -r "$tgt" ]; then echo "[aisb] pi config: cannot read $f -- its mode or SELinux label blocks this container, and nothing inside can repair it (remove it from the host, or use --ephemeral-home)" >&2 else @@ -1093,7 +1115,7 @@ if [[ -d "$POLICY_DIR/pi" ]]; then if [ -n "$merged" ] && printf "%s\n" "$merged" > "$tmp" \ && install -m 600 "$tmp" "$tgt" && rm -f "$tmp" && cp "$tgt" "$wrote"; then if [ -n "$lvl" ] && [ "$had" != "$lvl" ]; then - echo "[aisb] pi config: thinking -> $lvl (policy: --thinking, or host)" + echo "[aisb] pi config: thinking -> $lvl (policy: --thinking, or host)" >&2 fi fi fi diff --git a/bin/ai-sandbox-autophase b/bin/ai-sandbox-autophase index ffd0019..154018b 100755 --- a/bin/ai-sandbox-autophase +++ b/bin/ai-sandbox-autophase @@ -697,14 +697,23 @@ ap_git_plan() { # preflight: one throwaway container, before any agent tokens are spent # -------------------------------------------------------------------------- ap_preflight() { - local out errf rc=0 + local out got="" line errf rc=0 ap_log "preflight: one sandboxed shell, to prove the launcher, the image and the policy" errf="$(mktemp)" # /dev/null | cut -d" " -f3 || echo none)" "$(id -un)" "$PWD"' 2>"$errf" &2 printf ' command: %s\n' "$(ap_child_preview)" >&2 tail -n 12 "$errf" | sed 's/^/ | /' >&2 @@ -716,13 +725,22 @@ ap_preflight() { printf ' the sandbox started and answered nothing at all: a launcher that runs no command, or one\n' >&2 printf ' that prints its answer somewhere other than stdout, fails exactly like this. It has to run\n' >&2 printf ' follows the -- it was given (a function needs "$@" at the end; see --help).\n' >&2 + else + printf ' stdout carried no aisb-ok answer at all; this is what came back instead:\n' >&2 + printf '%s\n' "$out" | awk -v max=6 '{ if (++n <= max) printf " > %s\n", $0 }' >&2 fi rm -f "$errf" ap_die "Fix the launcher or the sandbox before running the pipeline: every unit would fail this way. (--no-preflight to skip the check.)" fi rm -f "$errf" - echo " $out" - if [[ "$out" == *"git=none"* ]]; then + printf ' %s\n' "$got" + if [[ "$out" != "$got" ]]; then + ap_warn "the sandbox put lines of its own on stdout before answering. That stream is data: the agent" + ap_warn "stream is parsed from it, and so is this check. Notices belong on stderr -- rebuild the" + ap_warn "image if they read [aisb], or move any echo in your launcher there. Continuing: it answered." + printf '%s\n' "$out" | awk -v max=4 '!/^aisb-ok/ { if (++n <= max) printf " > %s\n", $0 }' >&2 + fi + if [[ "$got" == *"git=none"* ]]; then ap_warn "no git inside the sandbox -- the agent cannot read its own diff. Add it to the image (--install git) or expect blind fixer runs." fi return 0 diff --git a/tests/unit/06_entrypoint.sh b/tests/unit/06_entrypoint.sh index c05656e..d36fe56 100755 --- a/tests/unit/06_entrypoint.sh +++ b/tests/unit/06_entrypoint.sh @@ -138,6 +138,66 @@ jq -n --slurpfile T "$M_IN/broken.json" --slurpfile B /dev/null --slurpfile N "$ -f "$MJQ" > /dev/null 2>&1 \ && _bad "unparseable input fails loudly for the caller to handle" || _ok "unparseable input fails loudly for the caller to handle" +t_name "the sandbox keeps its own chatter off the stream the command owns" +# stdout inside the container is data. ai-sandbox-autophase reads its preflight answer from +# it and progress.mjs reads the child pi event stream from it, so every word the sandbox says +# about itself belongs on stderr. Five notices in this very block were once on stdout, and one +# of them fires on every pi-bridged run -- which made the driver refuse a working sandbox. +# Hence a test that runs the block and separates the two streams, not one that reads it. +if grep -nE '(echo|printf).*\[aisb\]' "$EP" | grep -v '>&2' | grep -q .; then + _bad "every [aisb] line in the entrypoint is redirected" \ + "$(grep -nE '(echo|printf).*\[aisb\]' "$EP" | grep -v '>&2' | head -2 | tr '\n' ' ')" +else + _ok "every [aisb] line in the entrypoint is redirected" +fi + +# The extracted block, run as the entrypoint runs it (bash -c with the policy directory, the +# level and the merge program as $1..$3). Only this snippet sees the substituted HOME, and it +# starts no container, so podman's own HOME is untouched. +SP="$WORK/seed"; mkdir -p "$SP/pi" +printf '{ "providers": { "gateway": { "models": [ { "id": "turbo" } ] } } }\n' > "$SP/pi/models.json" +printf '{ "theme": "dark/dark" }\n' > "$SP/pi/settings.json" +SEED_OUT="$WORK/seed.out"; SEED_ERR="$WORK/seed.err" +seed_run() { # + HOME="$SP/home" bash -c "$(cat "$INNER")" aisb-seed "$SP" "${1-xhigh}" "$(cat "$MJQ")" \ + > "$SEED_OUT" 2> "$SEED_ERR" +} +seed_says() { grep -qF -- "$1" "$SEED_ERR"; } +seed_stdout() { [[ -s "$SEED_OUT" ]] && printf '%s' "$(tr '\n' ';' < "$SEED_OUT")"; } +seed_level() { jq -r '.defaultThinkingLevel // "absent"' "$SP/home/.pi/agent/settings.json" 2>/dev/null; } + +seed_run xhigh; RC=$? +assert_eq 0 "$RC" "seeding runs outside a container" +[[ -z "$(seed_stdout)" ]] && _ok "and it writes nothing on stdout (cold seed)" \ + || _bad "stdout belongs to the command (cold seed): $(seed_stdout)" +seed_says "seeded models.json" && _ok "the notice went to stderr" || _bad "the notice went to stderr" +seed_says "seeded settings.json" && _ok "including the settings one" || _bad "including the settings one" +# the level is policy, so a home we create must not run one unit at the default of pi itself +assert_eq "xhigh" "$(seed_level)" "the first seed already carries the thinking policy" + +seed_run xhigh; RC=$? +assert_eq 0 "$RC" "and on the next run" +[[ -z "$(seed_stdout)" ]] && _ok "nothing on stdout once the files exist (the steady state)" \ + || _bad "steady-state stdout is not clean: $(seed_stdout)" +seed_says "refreshed models.json from the host config" \ + && _ok "the line that broke the driver is on stderr" || _bad "the line that broke the driver is on stderr" + +sed -i 's/turbo/juggernaut/' "$SP/home/.pi/agent/models.json" +seed_run xhigh +[[ -z "$(seed_stdout)" ]] && _ok "an in-sandbox edit is reported without touching stdout" \ + || _bad "edited-file stdout: $(seed_stdout)" +seed_says "left models.json alone" && _ok "and the explanation still arrives" || _bad "and the explanation still arrives" + +printf '{oops' > "$SP/home/.pi/agent/settings.json" +seed_run xhigh +[[ -z "$(seed_stdout)" ]] && _ok "even a repair is announced off stdout" || _bad "broken-json stdout: $(seed_stdout)" +seed_says "not usable JSON" && _ok "with the warning on stderr" || _bad "with the warning on stderr" + +seed_run low +assert_eq "low" "$(seed_level)" "a changed level is applied over the merged file" +seed_says "thinking -> low" && _ok "and announced there too" || _bad "and announced there too" +[[ -z "$(seed_stdout)" ]] && _ok "with stdout still empty" || _bad "level-change stdout: $(seed_stdout)" + t_name "privilege drop is the last thing that happens" tail -5 "$EP" | grep -q 'setpriv' && _ok "exec setpriv at the end" || _bad "setpriv last: $(tail -3 "$EP" | tr '\n' ' ')" grep -q -- '--bounding-set=-all' "$EP" && _ok "bounding set cleared" || _bad "bounding set cleared" diff --git a/tests/unit/10_autophase.sh b/tests/unit/10_autophase.sh index 5a4592a..8356e4a 100644 --- a/tests/unit/10_autophase.sh +++ b/tests/unit/10_autophase.sh @@ -89,6 +89,28 @@ broken-sandbox() { return 125 } EOF +# A launcher that puts a notice of its own on stdout before the command runs -- the shape the +# sandbox entrypoint had, where the pi-config notice came first on every bridged run. The +# sandbox does answer, so the run is workable: what the driver owes the user is a warning that +# names the pollution (the same stream is parsed as data every unit), not a refusal. +cat > "$WORK/rc/noisy.bash" <<'EOF' +noisy-pi() { + local -a cmd=() + while (($#)); do case "$1" in --) shift; cmd=("$@"); break ;; *) shift ;; esac; done + printf 'LAUNCH cmd=%s\n' "${cmd[0]:-none}" >> "${AISB_AP_TRACE:-/dev/null}" + printf '[aisb] pi config: refreshed models.json from the host config\n' + "${cmd[@]}" +} +EOF +# The same talking with no answer in it. That is a broken launcher, and it still has to stop +# the run -- with the foreign stdout quoted back rather than a bare exit code. +cat > "$WORK/rc/yap.bash" <<'EOF' +yap-pi() { + local -a cmd=() + while (($#)); do case "$1" in --) shift; cmd=("$@"); break ;; *) shift ;; esac; done + printf 'hello, I am a launcher that runs nothing\n' +} +EOF # --- the harness and the git shim ------------------------------------------- cat > "$WORK/bin/pi" <<'EOF' @@ -515,6 +537,26 @@ if grep -q 'cmd=pi' "$TRACE"; then _bad "no unit runs"; else _ok "no unit runs"; T_PREFLIGHT=0 ap_run --max-units 1 assert_rc 0 "--no-preflight is available for the runs that cannot afford it" +t_name "a sandbox that talks on stdout is named, not refused" +cd "$PROJ6" || exit 1 +: > "$TRACE" +T_CMD=noisy-pi T_RC="$WORK/rc/noisy.bash" ap_run --max-units 1 +assert_rc 0 "a launcher that answers is a working sandbox, banner or not" +assert_contains "aisb-ok" "the answer is taken from wherever it is in the stream" +assert_contains "put lines of its own on stdout" "and the pollution itself is named" +assert_contains "> [aisb] pi config: refreshed" "with the offending line quoted back" +if grep -q 'cmd=pi' "$TRACE"; then _ok "the pipeline went on to run its unit"; else _bad "the pipeline went on to run its unit: $(tr '\n' ';' < "$TRACE")"; fi + +t_name "a launcher that talks instead of answering is still refused" +cd "$PROJ6" || exit 1 +: > "$TRACE" +T_CMD=yap-pi T_RC="$WORK/rc/yap.bash" ap_run --max-units 1 +assert_rc 1 "no aisb-ok answer means no run" +assert_contains "preflight failed" "and it says so" +assert_contains "no aisb-ok answer at all" "naming what the check was looking for" +assert_contains "> hello, I am a launcher that runs nothing" "quoting the stdout that came back instead" +if grep -q 'cmd=pi' "$TRACE"; then _bad "no unit runs"; else _ok "no unit runs"; fi + t_name "bad usage is refused before anything happens" cd "$PROJ6" || exit 1 ap_run --not-a-flag