fix(agent-server): rebuild the execution env per spawn so a removed .env key stops applying (#1999)

[dolho]

Summary

.env and the environment executions actually receive were two independent channels. The credential endpoints mirrored every submitted key into the long-lived agent-server process's os.environ; all five spawn sites passed env={**os.environ, ...}; the file was never read back into that process. One code path, one direction, present keys only — no delete phase.

So a key removed from .env by any non-mirroring write path (SSH, docker exec, or an agent editing its own .env) kept reaching every subsequently-spawned execution until container restart, and was invisible to /proc/<pid>/environ (an exec-time snapshot) and to docker exec (the container baseline, not the mutated process env). Credential revocation silently failed, and every inspection route confirmed the wrong answer.

The fix

docker/base-image/agent_server/services/execution_env.py — the execution environment becomes a pure function of three inspectable inputs, rebuilt per spawn instead of accumulated in process state:

env = build_execution_env({EXECUTION_TAG_NAME: execution_id})
#   INITIAL_ENV        container baseline captured at import — what `docker exec` shows
#   .env               parsed fresh at every spawn — authoritative for credentials
#   RUNTIME_OVERRIDES  values that are deliberately not .env credentials (#1089)
#   extra              last, so EXECUTION_TAG_NAME (#407) can never be displaced

A key that is in neither the baseline nor .env now has no path into a spawned process, so the class cannot recur.

Two deliberate divergences from the issue's sketch

Overrides are applied AFTER the file, not before. /api/credentials/reload-token rotates the subscription token without writing .env (by design — "the subscription token is not a .env credential"). With file-last precedence, a stale CLAUDE_CODE_OAUTH_TOKEN that ever reaches .env — a hand edit, a credential export that captured it — would beat the rotation and re-open #1089 from the other side. An explicit hot-reload is the most recent and most specific signal; it wins. None means force-unset, which a plain dict merge cannot express and remove_api_key requires.

The process mirror stays — deleting it would break the in-process readers the issue names (error classifier's ANTHROPIC_API_KEY/CLAUDE_CODE_OAUTH_TOKEN probes, AGENT_RUNTIME, GOOGLE_API_KEY, and the sanitizer's redaction set). It gains the missing delete phase: a key this process mirrored that the new .env no longer contains is restored to its container-baseline value, or popped when it has none. Only keys the mirror itself wrote are eligible, so a baseline-only key is never deleted.

One behaviour narrowing, on purpose

.env may no longer set PATH, LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, PYTHONHOME, PYTHONPATH, PYTHONSTARTUP, BASH_ENV, ENV, IFS. These carry no credentials — they redirect what the child executes or loads. .env is agent-writable and is now read at every spawn, a wider trigger than the old backend-only mirror, so a prompt-injected workspace could otherwise repoint the runtime binary. Ignored with a warning, never fatal.

Diagnosability

GET /api/credentials/status gains env_drift: {key, in_file, in_process_env, equal} per key — names only, never values. The issue took hours to root-cause because every available inspection route read a different channel than the enforcement path; this is the route that compares them.

Acceptance criteria

AC	Where
Removed key stops reaching executions (any write path)	build_execution_env parses .env per spawn · test_key_deleted_out_of_band_does_not_reach_the_next_execution
.env authoritative for credential values	file layer applied over the baseline · test_env_file_is_authoritative_over_the_baseline
/reload-token still rotates for the next execution (#1089)	set_runtime_override, applied after the file · 3 precedence tests incl. the stale-token-in-file case
EXECUTION_TAG_NAME still reaches every process (#407)	extra applied last · test_execution_tag_cannot_be_displaced_by_the_env_file
AGENT_RUNTIME / ANTHROPIC_API_KEY / GOOGLE_API_KEY still resolve in-process	mirror retained; baseline restored on delete · test_a_baseline_key_is_restored_not_deleted
/inject mirror removes keys absent from the new .env	sync_process_env delete phase · test_reinjecting_a_cleaned_env_clears_the_process_mirror
Regression test for the reported scenario	TestRemovedKeyStopsReachingExecutions
Uniform across Claude Code, Codex, Gemini	all five spawn sites wired · static guard test_every_spawn_site_builds_its_env_through_the_helper

Test plan

• tests/unit/test_1999_execution_env.py — 36 tests: the issue's reproduction, baseline restore, precedence (incl. both feat: credential rotation via hot-reload, not container recreate #1089 no-regression directions), tolerant .env parsing, unreadable/oversized files degrading to the baseline rather than raising on the spawn path, protected keys, the name-only drift report, and a static guard against a sixth spawn site reintroducing {**os.environ}
• Mutation-verified — each of (a) reverting one spawn site, (b) removing the delete phase, (c) applying overrides before the file fails exactly its own tests (1, 2, 3 failures respectively), never silently passing
• 871 adjacent tests green (-k "credential or agent_server or 1089 or subscription or codex or gemini or claude_code or 407 or orphan")
• .env parser matches the writer's own escaping — .strip('"') eats a legitimately trailing quote, so it is a one-matched-pair unquote instead

Not included

The issue's amplifiers are noted but out of scope: the boot re-push (inject_assigned_credentials) and the fleet-wide PAT propagation still rewrite .env from the backend's stored archive. That is the intended source of truth and the result is now visible in the file, so it is no longer a ghost — but it does mean a hand-edit to .env is still overwritten on next boot. #1967 is open on that same propagation path.

Closes #1999

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[dolho]

Rebased onto dev — it had gone conflicting after #1901 and others landed. Only tests/registry.json and docs/memory/learnings.md conflicted, both append-only on each side; unioned with ours last and the registry re-serialized from parsed JSON (never line-spliced, so the separating comma can't be lost).

Verified wider than the conflicted files this time, which is the lesson from #1976 earlier today: git auto-merged two ExecutionOrigin class definitions there without reporting a conflict, and my post-rebase test filter was narrower than the blast radius, so 129 tests passed over a broken file. So: swept every changed .py for duplicate top-level definitions (clean), confirmed the registry diff has zero deletions, and ran 1224 tests across 1999 / ent127 / credential / agent_server / execution / 1089 / codex / gemini.

Unblocking this also unblocks #2023 — the .env quote-escaping asymmetry can only land on top of the parser this PR introduces.

[vybe]

The core mechanism is right and the argument for it is good — build_execution_env rebuilding from dict(INITIAL_ENV) → .env → overrides → extra genuinely gives the deletion property, all five spawn sites are converted, and sync_process_env's delete phase correctly gates on _MIRRORED_KEYS so baseline-only keys are restored rather than popped. Overrides-after-file for #1089 is the right ordering.

Two things block.

1. env_drift names credential variables on the one credential endpoint deliberately left on the looser gate — and this repo documents why that was safe.

agent_server/routers/credentials.py adds env_drift (one entry per .env key, by name) to GET /api/credentials/status. Its backend proxy is get_authorized_agent_by_name with no reject_agent_principal, while every sibling (/credential-requirements, /inject, /export, /import) is owner + human-only.

I verified the rationale is written out in src/backend/routers/credentials.py right above the sibling route:

"What it would disclose is a targeting map, not a status light: it names STRIPE_SECRET_KEY per agent and says which are populated … A prompt-injected agent gets it with one curl."
"/credentials/status gets away with the read gate because it returns a COUNT and names zero variables."

That last sentence is exactly what this PR falsifies. Reachable by any user the agent is merely shared with, and by any agent-scoped MCP key — get_authorized_agent_by_name resolves it to the owner carrying the owner's role, and the MCP tool get_credential_status does a full JSON.stringify pass-through, so on a default admin-owned install that is the whole fleet. Arguably wider than the ent#127 checklist it is now looser than: that names declared requirements, env_drift names what is actually in .env, including undeclared variables.

Please move the drift report behind get_owned_agent_by_name + reject_agent_principal (own route, or fold into /credential-requirements) and update that comment either way. A diagnostic added by a security fix shouldn't widen the surface the fix is about.

2. /update now delivers quote-bearing values corrupted.

Pre-PR, /update mirrored the raw submitted value into os.environ and the child inherited it. Post-PR it round-trips through the writer's escaping and back through a parser that strips but does not unescape:

raw 'pa"ss'    -> file 'K="pa\"ss"'  -> delivered 'pa\"ss'   CORRUPTED
raw "'quoted'" -> ...                  -> delivered 'quoted'    quotes eaten
raw 'tail"'    -> ...                  -> delivered 'tail\'     CORRUPTED

The "byte-faithful, don't move the ent#127 predicate under a security fix" reasoning is sound about the predicate, but not about delivered values, and /update was the one path that never had this loss. It is a silent auth failure of exactly the class #1999 is about. Either fix writer + parser + _env_pairs together here, or state the exposure and land it as an immediate follow-up. (Note #2030 moves the writer/reader pair — worth coordinating rather than landing these independently.)

Non-blocking, but worth addressing:

• The wiring is untested. The static guard covers the 5 spawn sites; nothing covers the 4 new router call sites. Deleting the two set_runtime_override(...) lines the AC table cites as "what keeps #1089 working" leaves the suite green.
• The static guard walks one tree (agent_server/services/ only). routers/brain_orb.py already spawns a subprocess, and a future routers/ spawn site with {**os.environ} wouldn't be caught — the #1965 lesson ("a guard that walks only one of the two trees is not a guard") one directory over.
• Deleting test_decode_divergence_is_deliberate hides a behaviour change: the agent server went from raising on a non-UTF-8 .env to silently substituting U+FFFD into a credential value.
• PROTECTED_KEYS omits the Node equivalents — NODE_OPTIONS=--require … is LD_PRELOAD for Node, and both runtimes are Node processes. Same for GIT_SSH_COMMAND/GIT_CONFIG_*.
• New torn-read window: .env is written non-atomically and now read on the spawn path from a worker thread. Fail-safe (degrades to baseline), but tmp + os.replace closes it — the codebase already uses that idiom for sync-state.json.
• Stale PR body: the test-plan bullet describing a "one-matched-pair unquote" is the opposite of what ships, and the test count is 40, not 36.

Also needs a rebase — conflicts on learnings.md and tests/registry.json only.

[vybe]

Re-validated today as part of a merge sweep. Holding — head unchanged since my review; the only new commit is a Merge origin/dev.

Both blocking items confirmed still present:

1. env_drift on the loose gate. "env_drift": env_drift_report(env_file) is still wired into GET /api/credentials/status, whose backend proxy is get_authorized_agent_by_name with no reject_agent_principal. That endpoint's own in-repo comment — "/credentials/status gets away with the read gate because it returns a COUNT and names zero variables" — is exactly what this falsifies. Worth noting fix(security): admin gates reject agent-scoped keys #1890 landed today, so require_admin/assert_admin now reject agent principals fleet-wide; that does not cover this route, which is on the per-agent access gate, so the finding is unchanged.
2. /update delivers quote-bearing values corrupted — still round-trips through a writer that escapes and a parser that strips without unescaping.

Core mechanism is still right and I want it in; it's the drift-report gate and the delivery corruption that block.

[dolho]

@vybe — both blocking items addressed.

1. env_drift moved off the loose gate. You're right that this falsified the sentence the repo has written down ("/credentials/status gets away with the read gate because it returns a COUNT and names zero variables"), and that it's reachable by a merely-shared user and by any agent-scoped MCP key resolving to the owner's role — with get_credential_status doing a JSON.stringify pass-through, so the whole fleet on a default admin-owned install.

• new GET /api/agents/{name}/credentials/env-drift, behind get_owned_agent_by_name + reject_agent_principal — the same gate as /credential-requirements, /inject, /export, /import;
• /credentials/status strips the field, so the existing rationale comment is true again, and it now records where the drift went and why. A diagnostic added by a security fix shouldn't widen the surface the fix is about — agreed.

2. /update value corruption — pinned as a strict xfail rather than left as a paragraph.

Fixing the encoding here would move the ent#127 predicate under a security fix — the exact thing this PR's own reasoning argues against — and collide head-on with #2030, which does it properly and is stacked on this branch. So it's now a strict=True xfail naming the exposure, and I verified the follow-up actually closes it by composing the two locally:

this branch alone:      41 passed, 1 xfailed        <- corruption pinned, visible
merged with #2030:      [XPASS(strict)] ...         <- flips loud; drop the marker there

That way the loss can't be forgotten between the two merges, and whoever lands #2030 is told to retire it.

Non-blocking items, done:

• PROTECTED_KEYS gained the Node and git equivalents — NODE_OPTIONS/NODE_PATH/NODE_REPL_EXTERNAL_MODULE (every runtime here is a Node process, so --require is LD_PRELOAD under another name) and GIT_SSH_COMMAND/GIT_SSH/GIT_EXTERNAL_DIFF/GIT_PAGER/GIT_EDITOR/GIT_CONFIG_*, which git executes on the agent's next fetch or push.
• The static guard walks both trees now (services/ and routers/) — routers/brain_orb.py already spawns a subprocess, so this was the bug(security): agent server still parses author-controlled YAML with bare safe_load — outside the ent#314 sweep #1965 lesson one directory over.
• The router wiring is tested. Deleting both set_runtime_override(...) calls the AC table cites left the suite green; an AST test now pins both keys and the force-unset (value=None) form.

Not done, deliberately — each is a behaviour change I'd rather not fold into a security fix: restoring test_decode_divergence_is_deliberate (the U+FFFD substitution is a real change and belongs with the parser work in #2030), the atomic .env write (tmp + os.replace), and the stale PR-body bullets — I'll refresh the body separately.

Merged latest dev; the sequencing you asked for (#2010 → #2030 → #2024) is unchanged and the xfail now enforces the first hop.

[vybe]

Re-reviewed. Both blocking items addressed the way I'd hoped.

1. env_drift moved to its own owner-gated route — GET /credentials/env-drift behind get_owned_agent_by_name + reject_agent_principal, and /credentials/status strips the field so the repo's own written rationale ("gets away with the read gate because it returns a COUNT and names zero variables") is true again. A diagnostic added by a security fix widening the surface that fix is about was the whole objection; this closes it.

2. /update corruption pinned as xfail(strict=True) rather than fixed here — correct call. Folding the ent#127 predicate under a security fix is the thing this PR's own reasoning argues against, and it would collide with #2030. The marker makes the loss impossible to forget between merges.

PROTECTED_KEYS gaining the Node and git equivalents is the right generalization — NODE_OPTIONS --require is LD_PRELOAD under another name, and GIT_SSH_COMMAND executes on the next fetch. The static guard walking routers/ as well as services/ is the #1965 lesson applied one directory over.

Merging first in the chain. #2030 lands next and retires the xfail; #2014 after that, per its own merge-order note.