diff --git a/docs/PRD.md b/docs/PRD.md index 454f66cb..c8fe7a74 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -176,6 +176,38 @@ roster-derived part of the generated launch projection and prove it reaches the 3. `AC-FGI-03`: Focused launcher and generated-environment tests, repository quality gates, independent review, and the required RED/green/R7 evidence are recorded before push. +### Framework shell assertion portability (#1098) + +#### Problem and objective + +The blocking framework-shell chain can report that a pane command omitted `/usr/bin/env -i` even when +`-i` matched successfully. A short-circuiting `grep -q` under `set -o pipefail` may close its pipe after +the match and cause an upstream producer to exit with SIGPIPE, turning a valid semantic result into a +nonzero aggregate pipeline. The objective is to inspect the captured NUL-delimited argv directly and +make failures carry the observed records needed for diagnosis. + +#### Normative requirements + +1. `FSP-REQ-01`: The pane-boundary test SHALL validate an adjacent `/usr/bin/env`, `-i` argv pair from + the authoritative NUL-delimited tmux capture without a short-circuit pipeline whose upstream status + can override a successful match. +2. `FSP-REQ-02`: Missing, reversed, or non-adjacent boundary tokens SHALL fail, while valid boundaries + SHALL remain valid regardless of trailing argv size, pipe capacity, process scheduling, or host/CI + utility implementation. +3. `FSP-REQ-03`: A failed boundary check SHALL print stable indexed, shell-escaped observed argv records + before exiting nonzero; the fixture SHALL continue to contain generated non-secret launch data only. +4. `FSP-REQ-04`: Verification SHALL include RED-first large-payload evidence, negative token-order + controls, the complete focused launcher suite, canonical Woodpecker CI, and independent review. + +#### Acceptance criteria + +1. `AC-FSP-01`: A large captured argv with adjacent `/usr/bin/env`, `-i` passes even when the former + `grep -q` pipeline returns nonzero from an upstream SIGPIPE. +2. `AC-FSP-02`: Missing executable, missing flag, and detached/reversed flag fixtures return nonzero and + emit the indexed observed argv. +3. `AC-FSP-03`: The focused suite passes on the development host and CI image, and the merged-main + Woodpecker pipeline is terminal green before #1098 closes. + --- ## Exact Cross-Harness Fleet Communications Contract (#766) diff --git a/docs/scratchpads/1098-framework-shell-portability.md b/docs/scratchpads/1098-framework-shell-portability.md new file mode 100644 index 00000000..285a6926 --- /dev/null +++ b/docs/scratchpads/1098-framework-shell-portability.md @@ -0,0 +1,97 @@ +# #1098 — Framework shell portability / red main + +## Objective + +Restore terminal-green `main` by making the `test-start-agent-session.sh` clean-environment assertion semantic and portable without removing either newly enumerated framework-shell suite. + +## Scope + +- Tracking issue: `mosaicstack/stack#1098` +- Branch: `fix/framework-shell-portability` +- Base: `origin/main` at `4fa2768962702d53e16e8b67ee6ad52ebcb0910e` +- Primary file: `packages/mosaic/framework/tools/fleet/test-start-agent-session.sh` +- Requirements source: `docs/PRD.md` § Framework shell assertion portability (#1098) +- Out of scope: deployed files under `~/.config/mosaic`, pnpm-store cleanup, checkout deletion, and changes to the launcher’s `/usr/bin/env -i` behavior. + +## Acceptance criteria + +1. The test inspects the captured NUL-delimited tmux argv semantically and accepts an adjacent `/usr/bin/env`, `-i` pair regardless of trailing payload size or pipe scheduling. +2. Missing `/usr/bin/env`, missing `-i`, and non-adjacent `-i` remain failures. +3. Failure output includes the observed argv records with stable indexes and shell escaping; it exposes no credentials because this fixture supplies only generated non-secret launch data. +4. The focused suite passes on the dev host and in the repository CI image; the blocking PR/main pipeline returns terminal green. +5. Independent review passes; PR is squash-merged and #1098 is closed only after merged-main CI is terminal green. + +## Budget + +- ASSUMPTION: 30K-token working budget; rationale: one shell-test defect plus full PR/CI lifecycle. +- Auto-reduction: focused shell and package gates first; rely on canonical Woodpecker for the full monorepo suite rather than duplicating a dependency install under constrained `/home`. +- Disk baseline before clone/build: `/home` 7.1G free (99% used), `/tmp` 2.4G free (92% used). + +## Investigation + +### First-hand CI evidence + +- Public log: `GET https://ci.mosaicstack.dev/api/repos/47/logs/2269/53041` +- Decoded 1,436 entries (11 null `data` entries treated as empty log rows), 190,756 bytes. +- Failure: `FAIL: pane command did not clear its environment` immediately after the expected pane-PID warning. +- BusyBox primitives, complete assertion pipeline, real CI image, stale/current image digests, Turbo cache masking, gateway failure, and heartbeat-sidecar concurrent writing were independently excluded. + +### Root cause + +The assertion ends in: + +```bash +printf '%s\n' "$pane_args" | tail -n +"$after_pane_env" | grep -qxF -- '-i' +``` + +The script has `set -o pipefail`. `grep -q` exits as soon as it finds the valid `-i` record. Upstream `tail`/`printf` can then receive SIGPIPE, making the aggregate pipeline nonzero even though grep returned 0 and the semantic property is true. This depends on payload size, pipe capacity, and scheduling, explaining a local/image pass with a CI failure. + +Discriminating stress control with `/usr/bin/env` followed immediately by `-i`: + +- 8,192-byte trailing payload: `printf=0 tail=0 grep=0`, aggregate 0. +- 16,384-byte trailing payload: `printf=0 tail=141 grep=0`, aggregate 141. +- 32,768+ bytes: `printf=141 tail=141 grep=0`, aggregate 141. +- A full-reading `grep -xF` control remained 0 for every payload. + +This is a third branch omitted by the earlier present-vs-corrupted split: the pair can be present and intact while `pipefail` reports an upstream SIGPIPE. + +## TDD plan + +1. RED: preserve the one-off stress reproducer above and add an automated large-argv semantic regression that fails under the current pipeline implementation. +2. GREEN: parse the authoritative NUL-delimited capture into a Bash array and search for an adjacent `/usr/bin/env`, `-i` pair without a short-circuit pipeline. +3. Add negative controls for missing, detached, and reversed tokens. +4. On failure, print indexed `%q` argv records before returning nonzero. +5. Run focused suite, mutation controls, shell syntax/format checks, then repository baseline gates feasible without dependency installation. +6. Independent review, queue guard, push, PR, CI, coordinator merge authorization, squash merge, merged-main CI, issue close. + +## Progress + +- [x] Checkout created and based on `origin/main` `4fa27689`. +- [x] CI log decoded directly. +- [x] Root-cause stress control reproduced semantic match + aggregate pipeline failure. +- [x] RED evidence: intact `/usr/bin/env`, `-i` fixture produced component statuses `0/141/0` and aggregate 141 under the former `grep -q` pipeline; full-reading semantic control stayed 0. +- [x] GREEN implementation: direct NUL-argv adjacency parser, indexed diagnostics, and full-reading scalar predicates replace all load-bearing early-exit pipelines in this test. +- [x] Baseline/situational tests: + - focused launcher suite: PASS on GNU host and cached Alpine CI image; + - paired `test-fleet-units.sh`: PASS; + - enumeration guard: PASS (`population=53`, `enumerated=36`, `excluded=18`), 14/14 mutation needles; + - `bash -n`, ShellCheck, `git diff --check`: PASS; + - static denominator after change: zero load-bearing `grep -q`/`head`/`-m1` pipeline candidates in `test-start-agent-session.sh`; + - delete-the-subject mutation removing production `-i`: RED with 78 indexed argv records, byte count, and explicit boundary failure. +- [x] Independent review: + - first Codex review: request changes — negative fixtures did not each assert diagnostics; + - remediation: centralized predicate + diagnostic wrapper and exercised all four negative fixtures; + - second Codex review: APPROVE, 0 blockers/should-fix/suggestions; + - Codex security review: risk none, 0 findings. +- [ ] PR CI, formal fleet review, merge, merged-main CI, issue closure. + +## Documentation disposition + +- Updated canonical `docs/PRD.md` with FSP requirements and acceptance criteria. +- This is an internal test/reliability change with no API, user workflow, deployment, navigation, or publishing-surface change; no user/admin/API/sitemap update is required. +- `docs/TASKS.md` remains unchanged because the project contract makes it orchestrator-only. + +## Risks + +- The CI failure did not print its captured argv, so the exact CI payload is unavailable. The stress control proves the assertion is non-portable and can emit the exact false verdict; branch CI is the canonical confirmation that replacing it resolves pipeline 2269’s failure class. +- Printing fixture argv is safe only while this test’s projection remains non-secret. The diagnostic must stay scoped to the test capture and shell-escaped. diff --git a/packages/mosaic/framework/tools/fleet/test-start-agent-session.sh b/packages/mosaic/framework/tools/fleet/test-start-agent-session.sh index 6775f13e..378ad234 100755 --- a/packages/mosaic/framework/tools/fleet/test-start-agent-session.sh +++ b/packages/mosaic/framework/tools/fleet/test-start-agent-session.sh @@ -14,6 +14,82 @@ fail() { exit 1 } +pane_command_clears_environment() { + local calls_file="$1" + local -a argv=() + local index + mapfile -d '' -t argv < "$calls_file" + for ((index = 0; index + 1 < ${#argv[@]}; index++)); do + if [ "${argv[$index]}" = /usr/bin/env ] && [ "${argv[$((index + 1))]}" = -i ]; then + return 0 + fi + done + return 1 +} + +print_pane_argv() { + local calls_file="$1" + local -a argv=() + local bytes index + mapfile -d '' -t argv < "$calls_file" + bytes=$(wc -c < "$calls_file") + printf 'observed pane argv: records=%s bytes=%s\n' "${#argv[@]}" "$bytes" >&2 + for ((index = 0; index < ${#argv[@]}; index++)); do + printf ' [%03d] %q\n' "$index" "${argv[$index]}" >&2 + done +} + +check_pane_environment_boundary() { + local calls_file="$1" + if pane_command_clears_environment "$calls_file"; then + return 0 + fi + print_pane_argv "$calls_file" + return 1 +} + +contains_literal() { + grep -F -- "$2" <<< "$1" >/dev/null +} + +contains_line() { + grep -xF -- "$2" <<< "$1" >/dev/null +} + +# Portability regression: inspect the authoritative NUL-delimited argv instead +# of piping a newline reconstruction through `grep -q` under pipefail. The old +# pipeline could report failure after a successful match when an upstream +# producer received SIGPIPE. A large trailing argument keeps that failure class +# covered without making stream size part of the semantic contract. +PORTABILITY_CALLS="$ROOT/portability-calls" +printf -v PORTABILITY_PADDING '%*s' 32768 '' +PORTABILITY_PADDING=${PORTABILITY_PADDING// /x} +printf '%s\0' /usr/bin/env -i "$PORTABILITY_PADDING" > "$PORTABILITY_CALLS" +pane_command_clears_environment "$PORTABILITY_CALLS" || \ + fail "valid large pane argv was rejected by the environment-boundary assertion" + +assert_pane_boundary_rejected() { + local case_name="$1" + local expected_records="$2" + local diagnostic + if diagnostic=$(check_pane_environment_boundary "$PORTABILITY_CALLS" 2>&1); then + fail "pane boundary accepted invalid $case_name fixture" + fi + contains_literal "$diagnostic" "records=$expected_records bytes=" || \ + fail "pane argv diagnostic omitted counts for $case_name fixture" + contains_literal "$diagnostic" '[000]' || \ + fail "pane argv diagnostic omitted indexed arguments for $case_name fixture" +} + +printf '%s\0' tmux -i > "$PORTABILITY_CALLS" +assert_pane_boundary_rejected missing-env 2 +printf '%s\0' /usr/bin/env HOME=/untrusted > "$PORTABILITY_CALLS" +assert_pane_boundary_rejected missing-i 2 +printf '%s\0' /usr/bin/env HOME=/untrusted -i > "$PORTABILITY_CALLS" +assert_pane_boundary_rejected non-adjacent-i 3 +printf '%s\0' -i /usr/bin/env > "$PORTABILITY_CALLS" +assert_pane_boundary_rejected reversed-boundary 2 + cat > "$FAKE_BIN/tmux" <<'SHIM' #!/usr/bin/env bash set -euo pipefail @@ -115,19 +191,19 @@ AGENT_VALID="coder0" write_generated "$HOME_VALID" "$AGENT_VALID" run_start "$HOME_VALID" "$AGENT_VALID" valid_args=$(tr '\0' '\n' < "$TMUX_CALLS") -echo "$valid_args" | grep -qF new-session || fail "valid generated projection did not reach tmux" -echo "$valid_args" | grep -qF 'mosaic' || fail "fixed mosaic launcher command missing" -echo "$valid_args" | grep -qF 'yolo' || fail "fixed yolo launcher command missing" -echo "$valid_args" | grep -qF 'pi' || fail "roster runtime missing" -if echo "$valid_args" | grep -qF 'bash -c'; then +contains_literal "$valid_args" new-session || fail "valid generated projection did not reach tmux" +contains_literal "$valid_args" mosaic || fail "fixed mosaic launcher command missing" +contains_literal "$valid_args" yolo || fail "fixed yolo launcher command missing" +contains_literal "$valid_args" pi || fail "roster runtime missing" +if contains_literal "$valid_args" 'bash -c'; then fail "launcher constructed a shell command payload" fi # The pane must start through an absolute clean-environment boundary. Its # runtime command remains an argv vector, but no holder/session environment # control variable can pass through the pane command. -echo "$valid_args" | grep -qxF '/usr/bin/env' || fail "pane does not use absolute env" -echo "$valid_args" | grep -qxF -- '-i' || fail "pane environment is not cleared" +check_pane_environment_boundary "$TMUX_CALLS" || \ + fail "pane command did not use an adjacent /usr/bin/env -i boundary" # Git identity is generated authority, not an optional or independently mutable # local value. Each invalid form must fail before fake tmux receives a call. @@ -156,7 +232,7 @@ assert_git_identity_rejected() { fail "Git identity case $case_name was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before Git identity $case_name rejection" - echo "$output" | grep -qF "code=$expected_code" || \ + contains_literal "$output" "code=$expected_code" || \ fail "Git identity $case_name diagnostic omitted code $expected_code" } @@ -176,7 +252,7 @@ if output=$(run_start "$HOME_UNSAFE_PARENT" coder-parent 2>&1); then fail "generated file under a world-writable parent was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before unsafe parent rejection" -echo "$output" | grep -qF 'code=unsafe-permissions' || fail "unsafe parent diagnostic missing" +contains_literal "$output" 'code=unsafe-permissions' || fail "unsafe parent diagnostic missing" : > "$TMUX_CALLS" HOME_SYMLINK_PARENT="$ROOT/symlink-parent" @@ -187,7 +263,7 @@ if output=$(run_start "$HOME_SYMLINK_PARENT" coder-symlink-parent 2>&1); then fail "generated file under a symlinked parent was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before symlinked parent rejection" -echo "$output" | grep -qF 'code=unsafe-directory' || fail "symlinked parent diagnostic missing" +contains_literal "$output" 'code=unsafe-directory' || fail "symlinked parent diagnostic missing" # Every managed ancestor is a boundary: MOSAIC_HOME, fleet, and agents. A # symlink or group/world-writable ancestor must fail before environment parsing, @@ -225,8 +301,8 @@ assert_managed_ancestor_rejected() { fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before $hazard $ancestor rejection" [ ! -e "$home/work" ] || fail "workdir was created before $hazard $ancestor rejection" - echo "$output" | grep -qF "code=unsafe-" || fail "managed ancestor diagnostic missing" - if echo "$output" | grep -qF 'key=MOSAIC_AGENT_COMMAND'; then + contains_literal "$output" 'code=unsafe-' || fail "managed ancestor diagnostic missing" + if contains_literal "$output" 'key=MOSAIC_AGENT_COMMAND'; then fail "environment parsing ran before $hazard $ancestor rejection" fi } @@ -247,9 +323,9 @@ if output=$(run_start "$HOME_SHADOW" coder1 2>&1); then fail "generated-key shadow was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before generated-key shadow rejection" -echo "$output" | grep -qF 'key=MOSAIC_AGENT_RUNTIME' || fail "shadow diagnostic omitted key" -echo "$output" | grep -qF 'sha256=' || fail "shadow diagnostic omitted hash" -if echo "$output" | grep -qF 'codex'; then +contains_literal "$output" 'key=MOSAIC_AGENT_RUNTIME' || fail "shadow diagnostic omitted key" +contains_literal "$output" 'sha256=' || fail "shadow diagnostic omitted hash" +if contains_literal "$output" codex; then fail "shadow diagnostic leaked value" fi @@ -265,9 +341,9 @@ if output=$(run_start "$HOME_COMMAND" coder2 2>&1); then fail "arbitrary command override was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before command rejection" -echo "$output" | grep -qF 'key=MOSAIC_AGENT_COMMAND' || fail "command diagnostic omitted key" -echo "$output" | grep -qF 'sha256=' || fail "command diagnostic omitted hash" -if echo "$output" | grep -qF "$COMMAND_VALUE"; then +contains_literal "$output" 'key=MOSAIC_AGENT_COMMAND' || fail "command diagnostic omitted key" +contains_literal "$output" 'sha256=' || fail "command diagnostic omitted hash" +if contains_literal "$output" "$COMMAND_VALUE"; then fail "command diagnostic leaked command value" fi @@ -281,7 +357,7 @@ if output=$(run_start "$HOME_PERMS" coder3 2>&1); then fail "world-readable local input was accepted" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before permissions rejection" -echo "$output" | grep -qF 'code=unsafe-permissions' || fail "permission diagnostic missing" +contains_literal "$output" 'code=unsafe-permissions' || fail "permission diagnostic missing" # A unit/holder-like clean bootstrap must yield a pane with trusted HOME and # computed PATH only. The pane command itself must not carry loader, shell @@ -311,19 +387,17 @@ PATH="$PANE_STALE_PATH" \ MOSAIC_TEST_EXECUTE_PANE=1 \ "$START" coder-pane-boundary pane_args=$(tr '\0' '\n' < "$TMUX_CALLS") -echo "$pane_args" | grep -qxF "HOME=$PANE_TRUSTED_HOME" || \ +contains_line "$pane_args" "HOME=$PANE_TRUSTED_HOME" || \ fail "pane did not restore trusted HOME" -echo "$pane_args" | grep -qF "HOME=$PANE_STALE_HOME" && \ +contains_literal "$pane_args" "HOME=$PANE_STALE_HOME" && \ fail "pane inherited stale HOME" -echo "$pane_args" | grep -qF "$PANE_STALE_PATH" && fail "pane inherited stale PATH" +contains_literal "$pane_args" "$PANE_STALE_PATH" && fail "pane inherited stale PATH" for blocked in LD_PRELOAD= BASH_ENV= MOSAIC_UNTRUSTED_SENTINEL=; do - echo "$pane_args" | grep -qF "$blocked" && fail "pane inherited $blocked" + contains_literal "$pane_args" "$blocked" && fail "pane inherited $blocked" done -after_pane_env=$(printf '%s\n' "$pane_args" | grep -n -m1 -F '/usr/bin/env' | cut -d: -f1) -[ -n "$after_pane_env" ] || fail "pane command did not use absolute env" -printf '%s\n' "$pane_args" | tail -n +"$after_pane_env" | grep -qxF -- '-i' || \ - fail "pane command did not clear its environment" +check_pane_environment_boundary "$TMUX_CALLS" || \ + fail "pane command did not use an adjacent /usr/bin/env -i boundary" pane_environment=$(tr '\0' '\n' < "$HOME_PANE_BOUNDARY/fleet/pane-environment") # Exercise the repository launcher at $START, not the independently installed # host copy. Set-compare every declared generated projection entry with the @@ -337,11 +411,11 @@ if [ -n "$missing_or_changed_generated_environment" ]; then missing_or_changed_keys=$(printf '%s\n' "$missing_or_changed_generated_environment" | cut -d= -f1 | paste -sd, -) fail "runtime pane omitted or changed generated environment keys: $missing_or_changed_keys" fi -echo "$pane_environment" | grep -qxF "HOME=$PANE_TRUSTED_HOME" || \ +contains_line "$pane_environment" "HOME=$PANE_TRUSTED_HOME" || \ fail "runtime pane did not receive trusted HOME" -echo "$pane_environment" | grep -qF "$PANE_STALE_PATH" && fail "runtime pane received stale PATH" +contains_literal "$pane_environment" "$PANE_STALE_PATH" && fail "runtime pane received stale PATH" for blocked in LD_PRELOAD= BASH_ENV= MOSAIC_UNTRUSTED_SENTINEL=; do - echo "$pane_environment" | grep -qF "$blocked" && fail "runtime pane received $blocked" + contains_literal "$pane_environment" "$blocked" && fail "runtime pane received $blocked" done write_interaction_generated() { @@ -442,7 +516,7 @@ if output=$(run_interaction "$HOME_INTERACTION_MALFORMED" interaction-malformed fail "interaction wrapper accepted malformed generated data" fi [ ! -s "$TMUX_CALLS" ] || fail "tmux ran before interaction strict-parser rejection" -echo "$output" | grep -qF 'code=unknown-key' || fail "interaction did not use shared strict parser first" +contains_literal "$output" 'code=unknown-key' || fail "interaction did not use shared strict parser first" # A syntactically valid but policy-incompatible projection reaches the pinned # interaction policy check only after strict parsing and never starts tmux. @@ -455,9 +529,9 @@ if output=$(run_interaction "$HOME_INTERACTION_POLICY" interaction-policy 2>&1); fail "interaction wrapper accepted a policy-incompatible projection" fi interaction_policy_args=$(tr '\0' '\n' < "$TMUX_CALLS") -echo "$interaction_policy_args" | grep -qF 'new-session' && \ +contains_literal "$interaction_policy_args" new-session && \ fail "interaction pinned-policy rejection created a tmux session" -echo "$output" | grep -qF 'operator interaction service requires runtime pi' || \ +contains_literal "$output" 'operator interaction service requires runtime pi' || \ fail "interaction pinned-policy check did not follow strict parsing" # Exact stop derives the socket exclusively from the validated generated @@ -470,10 +544,10 @@ HOME="$HOME_STOP" PATH="$FAKE_BIN:$PATH" MOSAIC_TEST_TMUX_CALLS="$TMUX_CALLS" \ MOSAIC_TEST_FLEET_OWNER=123e4567-e89b-12d3-a456-426614174000 \ MOSAIC_HOME="$HOME_STOP" MOSAIC_TMUX_SOCKET=ambient-socket "$START" --stop coder-stop stop_args=$(tr '\0' '\n' < "$TMUX_CALLS") -echo "$stop_args" | grep -qxF 'mosaic-test' || fail "exact stop did not use the validated generated socket" -echo "$stop_args" | grep -qxF 'kill-session' || fail "exact stop did not request session termination" -echo "$stop_args" | grep -qxF '=coder-stop' || fail "exact stop did not exact-match the generated agent name" -if echo "$stop_args" | grep -qF 'ambient-socket'; then +contains_line "$stop_args" mosaic-test || fail "exact stop did not use the validated generated socket" +contains_line "$stop_args" kill-session || fail "exact stop did not request session termination" +contains_line "$stop_args" '=coder-stop' || fail "exact stop did not exact-match the generated agent name" +if contains_literal "$stop_args" ambient-socket; then fail "exact stop trusted an ambient socket" fi