fix(msb): kit environment[] never reached agent sessions — persist and replay it - #401
fix(msb): kit environment[] never reached agent sessions — persist and replay it#401basiliskus wants to merge 5 commits into
Conversation
… sessions On msb, a hybrid/v1 kit's environment[] entries were threaded only onto the kit's own provisioning commands and never reached the agent session or acq exec / acq shell, so agent-runtime config (OPENCODE_CONFIG-style vars — the use case the vocabulary was added for) silently no-op'd. sbx has no such gap because it sets kit env at the sandbox level. Fix, mirroring the adapter's existing marker-file pattern (/var/lib/acq/agent, /var/lib/acq/ssh-auth-sock): - At kit apply, append the validated NAME=value entries to the root-owned guest marker /var/lib/acq/kit-env (entries passed as argv to a fixed sh -c body — kit bytes are never interpolated into shell syntax). - On every session path (run/attach/shell), read the marker back and thread each entry as `msb exec -e NAME=value`, re-validating names and keeping the last value for a duplicate (later kits override earlier ones). Also amends ADR-0011's msb mapping, which described the command-only scoping, and deduplicates the env-collect loop in _acq_msb_run_commands into _acq_msb_collect_kit_env_into. Verified live on msb 0.6.15: a kit-declared var now reaches acq exec (previously empty), with the pinned kits' entries appending correctly. Fixes #400
…set -e Follow-up reported on #400: with a marker file absent, the in-guest `cat` exits 1 inside the reader's command substitution and — since acq runs under `set -euo pipefail` — the whole session verb dies with rc 1 and no output. Verified live on msb 0.6.15 for both the new kit-env marker (any sandbox created before the feature) and the pre-existing ssh-auth-sock marker (any sandbox without ssh-agent forwarding; its trailing `| tr` does not mask the failure — pipefail takes the failing stage's status). The workspace/agent marker reads in attach/shell share the same shape, with documented fallbacks that were unreachable in practice. Guard all four reads (`|| true` before the pipe / `|| _kvs=""`), and make the test stubs faithful: an UNSET STUB_RECORDED_* now models an absent marker by exiting 1 like real cat, so unguarded readers fail the suite. New regression test runs exec/attach/shell under `set -euo pipefail` with every marker absent.
…update note Restores ADR-0011's original msb env-mapping wording (command-only scoping) and records the new persist-and-replay behavior as a dated Update blockquote instead — the repo's established ADR amendment convention — so the decision record keeps what was originally decided.
… note Comment-only: the set -e / absent-marker rationale was repeated at five sites; keep the full explanation at _acq_msb_ssh_auth_sock_for and point the other readers at it. No logic change.
mogul
left a comment
mogul
left a comment
There was a problem hiding this comment.
Code Review Report
Reviewed PR #401 with an additional sub-agent review pass. The implementation is generally well covered by regression tests and the offline suite is passing. I found one medium-severity issue to resolve or explicitly accept before merge.
Finding
Medium: persisted kit environment can retain stale values after kit updates or re-applies
Files:
acq.backends/msb.sh:1477-1481acq.backends/msb.sh:3299-3317acq.backends/msb.sh:4336-4347
The new MSB kit environment replay appends current environment[] entries to /var/lib/acq/kit-env on every apply:
printf "%s\n" "$@" >> /var/lib/acq/kit-envReplay later deduplicates duplicate names with “last value wins,” which handles changed values, but it does not remove variables that were present in an earlier kit version and later removed. Since acq_backend_ensure_kits_applied re-applies kits during heal/update flows, a removed runtime config value can remain active in future acq exec, acq run, and acq shell sessions indefinitely.
Impact: removed runtime config can keep influencing agent sessions after the kit no longer declares it. This matters for values like OPENCODE_CONFIG, host selectors, feature toggles, or other backend behavior controls.
Suggested fix: rebuild /var/lib/acq/kit-env from the current effective kit set during provision/heal, or store per-kit env markers and synthesize the effective runtime environment from currently applied kits. Please add a regression test that starts with an existing marker containing a value, reapplies/heals with a kit set that no longer declares it, and verifies sessions no longer receive that value.
Follow-Up / Optional Feature
environment[] values appear to be treated as single-line static strings today, and the current marker format is line-oriented. If multiline YAML scalar env values are intended to be supported, this PR should either implement an encoded marker format or explicitly reject multiline values.
If multiline env values are out of scope for this fix, I suggest tracking that as a follow-up issue so the current behavior is documented and intentional.
Nit
The PR title contains an em dash. CI accepts it, so this is not a blocker. If the project wants strict ASCII in titles, consider replacing it before merge.
Verification
Automated checks I ran:
- PASS:
git diff --check origin/main...HEAD - PASS:
bash -n acq acq.backends/msb.sh scripts/test-acq-lib.sh - PASS:
printf '%s\n' acq acq.backends/*.sh scripts/test-acq-lib.sh scripts/test-acq-bats test/bats/*.bats test/bats/helper.bash | xargs -r -n8 shellcheck --severity=warning - PASS:
./scripts/test-acq-bats test/bats/78-msb-kit-env.bats - PASS:
./scripts/test-acq-bats— 374 tests passing - PASS:
npx --prefix .github/linters markdownlint-cli2 docs/adr/0011-msb-backend-and-neutral-kits.md - PASS: GitHub PR checks are green: Conventional Commit Title, Link Check, Markdown Lint, acq offline suite under bash 3.2, pre-commit
- BLOCKED:
gitleaks detect --no-git -vbecausegitleaksis not installed locally - PASS: diff-based secret-pattern fallback found only benign references to
token,secret-store.sh, andACQ_SECRET_STORE_DIR; no secret values observed
Sub-agent review independently confirmed the stale persisted environment finding.
Why I'm asking for this to be fixed: I am dreading the experience of trying to support/debug inscrutable problems people report due to stale state. |
2 participants
Fixes #400
Problem
On the msb backend, a hybrid/v1 kit's
environmentblock was applied only to the kit's own provisioning commands — it never reached the agent session oracq exec/acq shell. Any kit usingenvironmentfor agent-runtime config (OPENCODE_CONFIG-style vars — the use case ADR-0011 added the vocabulary for) silently no-op'd on msb, while the same kit worked on sbx (which sets kit env at the sandbox level).Reproduced live on msb 0.6.15: a minimal kit with
environment: {PROBE_VAR: hello}provisioned cleanly, butacq exec <sandbox> -- printenv PROBE_VARcame back empty from every session path.Fix
Mirrors the adapter's existing marker-file pattern (
/var/lib/acq/agent,/var/lib/acq/ssh-auth-sock):NAME=valueentries to a root-owned guest marker,/var/lib/acq/kit-env. Entries are passed as argv to a fixedsh -cbody — kit bytes are never interpolated into shell syntax (SI-10).acq exec,acq shell): read the marker back and thread each entry asmsb exec -e NAME=value. Names are re-validated against the kit_spec_env charset (tampered-marker defense) and the last value wins for a duplicate name, so a later kit overrides an earlier one — including mid-lifeacq kit applyre-applies./var/lib/acqmarker file absent, the in-guestcatexits 1 inside the reader's command substitution and acq'sset -euo pipefailkilled the whole session verb (rc 1, no output). Verified live for the new kit-env marker (sandbox created before this feature) and the pre-existing ssh-auth-sock marker (no ssh-agent forwarding — its trailing| trdoes not mask the failure; pipefail takes the failing stage's status). All four marker reads (kit-env, ssh-auth-sock, workspace, agent) are now failure-guarded.Also amends ADR-0011's msb mapping (it documented the command-only scoping, which contradicted the block's own motivating use case) and deduplicates the env-collect loop in
_acq_msb_run_commandsinto_acq_msb_collect_kit_env_into.Testing
test/bats/78-msb-kit-env.bats(6 tests, written failing-first): persist at apply, no marker withoutenvironment[], replay on exec, replay on attach/shell, absent-marker crash guard underset -euo pipefail, tampered-name drop + last-wins dedupe. The stubs now model an absent marker faithfully (exit 1 like realcat), so unguarded readers fail the suite.--severity=warning, repo.shellcheckrc) and pinned markdownlint clean.acq exec(previously empty); the pinned kits' entries append correctly and all arrive in sessions.