From 6682357fbb3973443c2001deb71674c838725afa Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 04:17:39 +0000 Subject: [PATCH 1/5] =?UTF-8?q?docs(prd):=20add=20PRD=200030=20=E2=80=94?= =?UTF-8?q?=20deduplicate=20egress=20token=20resolution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracts the forward_host_credentials / CODEX_HOST_CREDENTIAL_TOKEN_REF resolution block, currently copy-pasted in both docker and smolmachines launch files, into a single shared function in bot_bottle/egress.py. Closes #118. Found via #117 hotspot review. --- ...030-deduplicate-egress-token-resolution.md | 202 ++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 docs/prds/0030-deduplicate-egress-token-resolution.md diff --git a/docs/prds/0030-deduplicate-egress-token-resolution.md b/docs/prds/0030-deduplicate-egress-token-resolution.md new file mode 100644 index 0000000..d81adb7 --- /dev/null +++ b/docs/prds/0030-deduplicate-egress-token-resolution.md @@ -0,0 +1,202 @@ +# PRD 0030: Deduplicate egress token resolution across backends + +- **Status:** Draft +- **Author:** didericis-claude +- **Created:** 2026-06-02 +- **Issue:** #118 + +## Summary + +Extract the duplicated egress token resolution block — which calls +`egress_resolve_token_values` and then slots in the Codex host +credential — into a single shared function in `bot_bottle/egress.py`, +and have both backends call it. + +## Problem + +The same logic block appears verbatim in two places: + +- `bot_bottle/backend/docker/launch.py` (~line 183): inline inside + `launch()`, building `token_values` before `compose_up`. +- `bot_bottle/backend/smolmachines/launch.py` (~line 422): the private + `_resolve_token_env` helper, called before `_bundle.start_bundle`. + +Both blocks: +1. Short-circuit to `{}` when there are no egress routes. +2. Call `egress_resolve_token_values(token_env_map, host_env)` to + resolve ordinary manifest-declared token refs. +3. Check `agent_provider.forward_host_credentials` and, when true, + call `codex_host_access_token` and slot the result into any + `token_env` whose `token_ref` is `CODEX_HOST_CREDENTIAL_TOKEN_REF`. + +The duplication means any change to step 3 — new provider credential +type, token refresh logic, a new sentinel ref — must be applied twice. +PRD 0029, which introduced `forward_host_credentials`, already had to +wire both backends; the next change will too. This is a near-certain +future sync bug. + +## Goals / Success Criteria + +- The `forward_host_credentials` resolution logic exists in exactly one + place in the codebase. +- Both `docker/launch.py` and `smolmachines/launch.py` call that + shared function. +- No behaviour change for either backend. +- The shared function is unit-testable without a plan or backend + dependency. + +## Non-goals + +- Changes to token resolution semantics. This is a pure refactor. +- Adding support for any new credential type or provider. +- Consolidating any other backend differences beyond this one block. + +## Design + +### New function: `egress_resolve_token_values_with_provider` + +Add to `bot_bottle/egress.py`: + +```python +def egress_resolve_token_values_with_provider( + token_env_map: dict[str, str], + forward_host_credentials: bool, + host_env: dict[str, str], +) -> dict[str, str]: + """Resolve all egress token env-var values from the host environ. + + Combines the manifest-declared token refs (via + `egress_resolve_token_values`) with the optional Codex host + credential slot (when `forward_host_credentials` is True). Returns + an empty dict when `token_env_map` is empty. + + Pure function: `host_env` is passed in so callers can pass a sealed + mapping in tests without touching `os.environ`. The Codex access + token is read lazily only when the flag is set and a matching slot + exists, so callers that don't use `forward_host_credentials` pay no + cost.""" + if not token_env_map: + return {} + token_values = egress_resolve_token_values(token_env_map, host_env) + if forward_host_credentials: + from .codex_auth import codex_host_access_token + access_token = codex_host_access_token(host_env) + for token_env, token_ref in token_env_map.items(): + if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: + token_values[token_env] = access_token + return token_values +``` + +The import of `codex_auth` is deferred inside the branch to avoid a +circular import — `egress.py` is imported by `pipelock.py` which +`codex_auth.py` does not import, but `codex_auth.py` imports from +`egress.py` transitively via `agent_provider`. The lazy import sidesteps +the cycle without restructuring the module graph. + +### Backend call sites + +**`docker/launch.py`** — replace the inline block: + +```python +# before +token_values: dict[str, str] = {} +if plan.egress_plan.routes: + token_values = egress_resolve_token_values( + plan.egress_plan.token_env_map, dict(os.environ), + ) + if plan.spec.manifest.bottle_for( + plan.spec.agent_name, + ).agent_provider.forward_host_credentials: + access_token = codex_host_access_token(dict(os.environ)) + for token_env, token_ref in plan.egress_plan.token_env_map.items(): + if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: + token_values[token_env] = access_token + +# after +bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) +token_values = egress_resolve_token_values_with_provider( + plan.egress_plan.token_env_map, + bottle.agent_provider.forward_host_credentials, + dict(os.environ), +) +``` + +**`smolmachines/launch.py`** — replace `_resolve_token_env`: + +```python +# before +def _resolve_token_env( + plan: SmolmachinesBottlePlan, host_env: object +) -> dict[str, str]: + ep = plan.egress_plan + if not ep.routes: + return {} + env = dict(host_env) + token_values = egress_resolve_token_values(ep.token_env_map, env) + if plan.spec.manifest.bottle_for( + plan.spec.agent_name, + ).agent_provider.forward_host_credentials: + access_token = codex_host_access_token(env) + for token_env, token_ref in ep.token_env_map.items(): + if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: + token_values[token_env] = access_token + return token_values + +# after +def _resolve_token_env( + plan: SmolmachinesBottlePlan, host_env: dict[str, str], +) -> dict[str, str]: + bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) + return egress_resolve_token_values_with_provider( + plan.egress_plan.token_env_map, + bottle.agent_provider.forward_host_credentials, + host_env, + ) +``` + +Note: the `host_env: object` annotation on the smolmachines version is +also corrected to `dict[str, str]` — the annotation was wrong; the +function body immediately called `dict(host_env)` treating it as a +mapping. + +### Imports + +- `docker/launch.py`: drop `codex_host_access_token` and + `CODEX_HOST_CREDENTIAL_TOKEN_REF` imports; add + `egress_resolve_token_values_with_provider`. +- `smolmachines/launch.py`: same drops; add + `egress_resolve_token_values_with_provider`. +- `egress.py`: add `egress_resolve_token_values_with_provider` to + `__all__`. + +## Implementation chunks + +1. **PRD (this commit).** Sets the design. +2. **Shared function.** Add `egress_resolve_token_values_with_provider` + to `bot_bottle/egress.py` with unit tests covering: no routes → + empty dict, routes with no `forward_host_credentials` → manifest + tokens only, routes with `forward_host_credentials` + matching ref → + Codex token slotted in, routes with `forward_host_credentials` + no + matching ref → manifest tokens only (Codex call still made but no + slot to fill). +3. **Docker backend.** Replace the inline block in `docker/launch.py`. +4. **Smolmachines backend.** Replace `_resolve_token_env` in + `smolmachines/launch.py`; fix the `host_env: object` annotation. +5. **Cleanup.** Remove now-unused imports from both backends. + +## Testing strategy + +- **Unit (must):** the four cases above in a new test in + `tests/unit/test_egress.py`, using a sealed `host_env` dict so tests + don't touch `os.environ` or the filesystem. +- **No integration changes needed:** both backends' existing integration + tests exercise the full launch path; a behaviour regression would + surface as a token-injection failure in those tests. + +## References + +- Issue #118: Deduplicate egress token resolution across backends. +- Issue #117: Complexity hotspots in launch, egress, and auth paths + (source of the finding). +- PRD 0029: Provider auth credentials through egress (introduced the + duplicated block). -- 2.52.0 From 75f0f9d90783c2a61e1ec595e697edbf83cb13ff Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 04:22:43 +0000 Subject: [PATCH 2/5] refactor(egress): deduplicate token resolution across backends (PRD 0030) Extract egress_resolve_token_values_with_provider into bot_bottle/egress.py. Both docker and smolmachines launch paths now call the shared function instead of duplicating the forward_host_credentials / CODEX_HOST_CREDENTIAL_TOKEN_REF resolution block. Also fixes the host_env: object annotation on smolmachines._resolve_token_env to the correct dict[str, str]. Closes #118. --- bot_bottle/backend/docker/launch.py | 24 +++------ bot_bottle/backend/smolmachines/launch.py | 27 ++++------ bot_bottle/egress.py | 27 ++++++++++ tests/unit/test_egress.py | 61 +++++++++++++++++++++++ 4 files changed, 104 insertions(+), 35 deletions(-) diff --git a/bot_bottle/backend/docker/launch.py b/bot_bottle/backend/docker/launch.py index e811cbe..a1a6f5e 100644 --- a/bot_bottle/backend/docker/launch.py +++ b/bot_bottle/backend/docker/launch.py @@ -42,11 +42,7 @@ from contextlib import ExitStack, contextmanager from pathlib import Path from typing import Callable, Generator -from ...codex_auth import codex_host_access_token -from ...egress import ( - CODEX_HOST_CREDENTIAL_TOKEN_REF, - egress_resolve_token_values, -) +from ...egress import egress_resolve_token_values_with_provider from ...log import info from . import network as network_mod from . import util as docker_mod @@ -180,18 +176,12 @@ def launch( # Step 7: compose up. Token values + the OAuth placeholder # flow through subprocess env; the compose file holds only # bare names for the secret-carrying entries. - token_values: dict[str, str] = {} - if plan.egress_plan.routes: - token_values = egress_resolve_token_values( - plan.egress_plan.token_env_map, dict(os.environ), - ) - if plan.spec.manifest.bottle_for( - plan.spec.agent_name, - ).agent_provider.forward_host_credentials: - access_token = codex_host_access_token(dict(os.environ)) - for token_env, token_ref in plan.egress_plan.token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token + bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) + token_values = egress_resolve_token_values_with_provider( + plan.egress_plan.token_env_map, + bottle.agent_provider.forward_host_credentials, + dict(os.environ), + ) compose_env: dict[str, str] = { **os.environ, **plan.forwarded_env, diff --git a/bot_bottle/backend/smolmachines/launch.py b/bot_bottle/backend/smolmachines/launch.py index da73dae..629eb2f 100644 --- a/bot_bottle/backend/smolmachines/launch.py +++ b/bot_bottle/backend/smolmachines/launch.py @@ -26,11 +26,9 @@ from contextlib import ExitStack, contextmanager from pathlib import Path from typing import Callable, Generator -from ...codex_auth import codex_host_access_token from ...egress import ( - CODEX_HOST_CREDENTIAL_TOKEN_REF, EGRESS_ROUTES_IN_CONTAINER, - egress_resolve_token_values, + egress_resolve_token_values_with_provider, ) from ...pipelock import ( PIPELOCK_CA_CERT_IN_CONTAINER, @@ -146,7 +144,7 @@ def launch( # spec's ports_to_publish list expands depending on which # daemons the agent needs to reach from the smolvm guest. bundle_spec = _bundle_launch_spec(plan, network, loopback_ip) - token_env = _resolve_token_env(plan, os.environ) + token_env = _resolve_token_env(plan, dict(os.environ)) _bundle.ensure_bundle_image(bundle_spec.image) _bundle.start_bundle(bundle_spec, env={**os.environ, **token_env}) stack.callback(_bundle.stop_bundle, plan.slug) @@ -420,24 +418,17 @@ def _bundle_launch_spec( def _resolve_token_env( - plan: SmolmachinesBottlePlan, host_env: object + plan: SmolmachinesBottlePlan, host_env: dict[str, str], ) -> dict[str, str]: """Resolve the egress token env-var values from the host's environ so they reach the bundle's process env via docker's `-e NAME` inheritance. Empty when no routes declare auth.""" - ep = plan.egress_plan - if not ep.routes: - return {} - env = dict(host_env) - token_values = egress_resolve_token_values(ep.token_env_map, env) - if plan.spec.manifest.bottle_for( - plan.spec.agent_name, - ).agent_provider.forward_host_credentials: - access_token = codex_host_access_token(env) - for token_env, token_ref in ep.token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token - return token_values + bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) + return egress_resolve_token_values_with_provider( + plan.egress_plan.token_env_map, + bottle.agent_provider.forward_host_credentials, + host_env, + ) def _ensure_smolmachine(image_ref: str, *, dockerfile: str = "") -> Path: diff --git a/bot_bottle/egress.py b/bot_bottle/egress.py index 000d5e1..19988d4 100644 --- a/bot_bottle/egress.py +++ b/bot_bottle/egress.py @@ -29,6 +29,7 @@ from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING +from .codex_auth import codex_host_access_token from .log import die if TYPE_CHECKING: @@ -360,6 +361,31 @@ def egress_resolve_token_values( return out +def egress_resolve_token_values_with_provider( + token_env_map: dict[str, str], + forward_host_credentials: bool, + host_env: dict[str, str], +) -> dict[str, str]: + """Resolve all egress token env-var values, including the optional + Codex host credential slot. + + Combines `egress_resolve_token_values` (manifest-declared token refs) + with the `forward_host_credentials` path (Codex ChatGPT bearer). + Returns an empty dict when `token_env_map` is empty. + + Pure function: `host_env` is passed in so tests can use a sealed + mapping without touching `os.environ`.""" + if not token_env_map: + return {} + token_values = egress_resolve_token_values(token_env_map, host_env) + if forward_host_credentials: + access_token = codex_host_access_token(host_env) + for token_env, token_ref in token_env_map.items(): + if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: + token_values[token_env] = access_token + return token_values + + class Egress(ABC): """The per-bottle egress proxy. Encapsulates the host-side prepare (route lift + routes.yaml render + token-env-map derivation); the @@ -403,6 +429,7 @@ __all__ = [ "egress_manifest_routes", "egress_render_routes", "egress_resolve_token_values", + "egress_resolve_token_values_with_provider", "egress_routes_for_bottle", "egress_token_env_map", ] diff --git a/tests/unit/test_egress.py b/tests/unit/test_egress.py index 7d7107c..55b89c4 100644 --- a/tests/unit/test_egress.py +++ b/tests/unit/test_egress.py @@ -2,6 +2,7 @@ resolution (PRD 0017).""" import unittest +import unittest.mock from bot_bottle.egress import ( CODEX_HOST_CREDENTIAL_TOKEN_REF, @@ -9,6 +10,7 @@ from bot_bottle.egress import ( egress_manifest_routes, egress_render_routes, egress_resolve_token_values, + egress_resolve_token_values_with_provider, egress_routes_for_bottle, egress_token_env_map, ) @@ -349,5 +351,64 @@ class TestResolveTokenValues(unittest.TestCase): self.assertEqual({}, out) +class TestResolveTokenValuesWithProvider(unittest.TestCase): + def test_empty_map_returns_empty(self): + out = egress_resolve_token_values_with_provider({}, False, {}) + self.assertEqual({}, out) + + def test_empty_map_with_forward_credentials_returns_empty(self): + # forward_host_credentials=True but no slots → no codex call needed. + out = egress_resolve_token_values_with_provider({}, True, {}) + self.assertEqual({}, out) + + def test_manifest_tokens_resolved_without_forward_credentials(self): + out = egress_resolve_token_values_with_provider( + {"EGRESS_TOKEN_0": "GH_PAT"}, + False, + {"GH_PAT": "ghp_secret"}, + ) + self.assertEqual({"EGRESS_TOKEN_0": "ghp_secret"}, out) + + def test_codex_token_slotted_in_when_forward_credentials_and_matching_ref(self): + with unittest.mock.patch( + "bot_bottle.egress.codex_host_access_token", + return_value="codex-access-token", + ): + out = egress_resolve_token_values_with_provider( + {"EGRESS_TOKEN_0": CODEX_HOST_CREDENTIAL_TOKEN_REF}, + True, + {}, + ) + self.assertEqual({"EGRESS_TOKEN_0": "codex-access-token"}, out) + + def test_codex_token_not_slotted_when_no_matching_ref(self): + # forward_host_credentials=True but no CODEX_HOST_CREDENTIAL_TOKEN_REF + # slot in the map → manifest tokens only; Codex token is fetched but + # nothing to slot it into. + with unittest.mock.patch( + "bot_bottle.egress.codex_host_access_token", + return_value="codex-access-token", + ): + out = egress_resolve_token_values_with_provider( + {"EGRESS_TOKEN_0": "GH_PAT"}, + True, + {"GH_PAT": "ghp_secret"}, + ) + self.assertEqual({"EGRESS_TOKEN_0": "ghp_secret"}, out) + + def test_codex_not_called_when_forward_credentials_false(self): + called = [] + with unittest.mock.patch( + "bot_bottle.egress.codex_host_access_token", + side_effect=lambda *_: called.append(1) or "tok", + ): + egress_resolve_token_values_with_provider( + {"EGRESS_TOKEN_0": "GH_PAT"}, + False, + {"GH_PAT": "ghp_secret"}, + ) + self.assertEqual([], called) + + if __name__ == "__main__": unittest.main() -- 2.52.0 From 8c2b59ca949550cf9eef34321660fe7db56d9334 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 04:22:52 +0000 Subject: [PATCH 3/5] complete(prd): mark PRD 0030 active --- docs/prds/0030-deduplicate-egress-token-resolution.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/prds/0030-deduplicate-egress-token-resolution.md b/docs/prds/0030-deduplicate-egress-token-resolution.md index d81adb7..5a88c87 100644 --- a/docs/prds/0030-deduplicate-egress-token-resolution.md +++ b/docs/prds/0030-deduplicate-egress-token-resolution.md @@ -1,6 +1,6 @@ # PRD 0030: Deduplicate egress token resolution across backends -- **Status:** Draft +- **Status:** Active - **Author:** didericis-claude - **Created:** 2026-06-02 - **Issue:** #118 -- 2.52.0 From 0e29bcc8297f2ebcb89121f7bd70c562119621f2 Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 04:53:23 +0000 Subject: [PATCH 4/5] refactor(egress): use provisioned_env instead of sentinel for Codex token (PRD 0030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `provisioned_env: dict[str, str]` to `AgentProvisionPlan`. When `forward_host_credentials=True`, `agent_provision_plan` reads the host Codex access token at prepare time and stores it under `CODEX_HOST_CREDENTIAL_TOKEN_REF`. Both backends merge `provisioned_env` over `os.environ` before calling `egress_resolve_token_values`, so the token slot resolves like any other manifest-declared token ref. Removes `egress_resolve_token_values_with_provider` and the sentinel `continue` skip from `egress_resolve_token_values`. The function is now fully generic — it neither knows nor cares about provider identity. --- bot_bottle/agent_provider.py | 11 +++- bot_bottle/backend/docker/launch.py | 10 ++-- bot_bottle/backend/smolmachines/launch.py | 10 ++-- bot_bottle/egress.py | 29 ---------- tests/unit/test_agent_provider.py | 31 +++++++++++ tests/unit/test_egress.py | 65 +---------------------- 6 files changed, 49 insertions(+), 107 deletions(-) diff --git a/bot_bottle/agent_provider.py b/bot_bottle/agent_provider.py index cd271dc..c610116 100644 --- a/bot_bottle/agent_provider.py +++ b/bot_bottle/agent_provider.py @@ -12,7 +12,7 @@ from dataclasses import dataclass, field from pathlib import Path from typing import Literal -from .codex_auth import write_codex_dummy_auth_file +from .codex_auth import codex_host_access_token, write_codex_dummy_auth_file from .egress import CODEX_HOST_CREDENTIAL_TOKEN_REF, EgressRoute @@ -92,6 +92,7 @@ class AgentProvisionPlan: verify: tuple[AgentProvisionCommand, ...] = () egress_routes: tuple[EgressRoute, ...] = () hidden_env_names: frozenset[str] = field(default_factory=frozenset) + provisioned_env: dict[str, str] = field(default_factory=dict) _REPO_ROOT = Path(__file__).resolve().parent.parent @@ -139,6 +140,7 @@ def agent_provision_plan( runtime = runtime_for(template) resolved_guest_env = dict(guest_env or {}) env_vars: dict[str, str] = {} + provisioned_env: dict[str, str] = {} dirs: list[AgentProvisionDir] = [] files: list[AgentProvisionFile] = [] pre_copy: list[AgentProvisionCommand] = [] @@ -169,8 +171,12 @@ def agent_provision_plan( tls_passthrough=True, )) if forward_host_credentials: + _host_env = host_env or dict(os.environ) + provisioned_env[CODEX_HOST_CREDENTIAL_TOKEN_REF] = codex_host_access_token( + _host_env, + ) auth_file = state_dir / "codex-auth.json" - write_codex_dummy_auth_file(auth_file, host_env or dict(os.environ)) + write_codex_dummy_auth_file(auth_file, _host_env) files.append(AgentProvisionFile(auth_file, f"{auth_dir}/auth.json")) pre_copy.append(AgentProvisionCommand(( "find", auth_dir, @@ -220,6 +226,7 @@ def agent_provision_plan( verify=tuple(verify), egress_routes=tuple(egress_routes), hidden_env_names=hidden_env_names, + provisioned_env=provisioned_env, ) diff --git a/bot_bottle/backend/docker/launch.py b/bot_bottle/backend/docker/launch.py index a1a6f5e..9e28a00 100644 --- a/bot_bottle/backend/docker/launch.py +++ b/bot_bottle/backend/docker/launch.py @@ -42,7 +42,7 @@ from contextlib import ExitStack, contextmanager from pathlib import Path from typing import Callable, Generator -from ...egress import egress_resolve_token_values_with_provider +from ...egress import egress_resolve_token_values from ...log import info from . import network as network_mod from . import util as docker_mod @@ -176,11 +176,9 @@ def launch( # Step 7: compose up. Token values + the OAuth placeholder # flow through subprocess env; the compose file holds only # bare names for the secret-carrying entries. - bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) - token_values = egress_resolve_token_values_with_provider( - plan.egress_plan.token_env_map, - bottle.agent_provider.forward_host_credentials, - dict(os.environ), + effective_env = {**dict(os.environ), **plan.agent_provision.provisioned_env} + token_values = egress_resolve_token_values( + plan.egress_plan.token_env_map, effective_env, ) compose_env: dict[str, str] = { **os.environ, diff --git a/bot_bottle/backend/smolmachines/launch.py b/bot_bottle/backend/smolmachines/launch.py index 629eb2f..14d5a33 100644 --- a/bot_bottle/backend/smolmachines/launch.py +++ b/bot_bottle/backend/smolmachines/launch.py @@ -28,7 +28,7 @@ from typing import Callable, Generator from ...egress import ( EGRESS_ROUTES_IN_CONTAINER, - egress_resolve_token_values_with_provider, + egress_resolve_token_values, ) from ...pipelock import ( PIPELOCK_CA_CERT_IN_CONTAINER, @@ -423,12 +423,8 @@ def _resolve_token_env( """Resolve the egress token env-var values from the host's environ so they reach the bundle's process env via docker's `-e NAME` inheritance. Empty when no routes declare auth.""" - bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) - return egress_resolve_token_values_with_provider( - plan.egress_plan.token_env_map, - bottle.agent_provider.forward_host_credentials, - host_env, - ) + effective_env = {**host_env, **plan.agent_provision.provisioned_env} + return egress_resolve_token_values(plan.egress_plan.token_env_map, effective_env) def _ensure_smolmachine(image_ref: str, *, dockerfile: str = "") -> Path: diff --git a/bot_bottle/egress.py b/bot_bottle/egress.py index 19988d4..0e1bc0d 100644 --- a/bot_bottle/egress.py +++ b/bot_bottle/egress.py @@ -29,7 +29,6 @@ from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING -from .codex_auth import codex_host_access_token from .log import die if TYPE_CHECKING: @@ -342,8 +341,6 @@ def egress_resolve_token_values( a sealed mapping without touching `os.environ`.""" out: dict[str, str] = {} for token_env, token_ref in token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - continue value = host_env.get(token_ref) if value is None: die( @@ -361,31 +358,6 @@ def egress_resolve_token_values( return out -def egress_resolve_token_values_with_provider( - token_env_map: dict[str, str], - forward_host_credentials: bool, - host_env: dict[str, str], -) -> dict[str, str]: - """Resolve all egress token env-var values, including the optional - Codex host credential slot. - - Combines `egress_resolve_token_values` (manifest-declared token refs) - with the `forward_host_credentials` path (Codex ChatGPT bearer). - Returns an empty dict when `token_env_map` is empty. - - Pure function: `host_env` is passed in so tests can use a sealed - mapping without touching `os.environ`.""" - if not token_env_map: - return {} - token_values = egress_resolve_token_values(token_env_map, host_env) - if forward_host_credentials: - access_token = codex_host_access_token(host_env) - for token_env, token_ref in token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token - return token_values - - class Egress(ABC): """The per-bottle egress proxy. Encapsulates the host-side prepare (route lift + routes.yaml render + token-env-map derivation); the @@ -429,7 +401,6 @@ __all__ = [ "egress_manifest_routes", "egress_render_routes", "egress_resolve_token_values", - "egress_resolve_token_values_with_provider", "egress_routes_for_bottle", "egress_token_env_map", ] diff --git a/tests/unit/test_agent_provider.py b/tests/unit/test_agent_provider.py index 8d4f4b6..3f90db5 100644 --- a/tests/unit/test_agent_provider.py +++ b/tests/unit/test_agent_provider.py @@ -145,6 +145,37 @@ class TestAgentProviderRuntime(unittest.TestCase): self.assertNotIn("CLAUDE_CODE_OAUTH_TOKEN", plan.env_vars) self.assertEqual(frozenset(), plan.hidden_env_names) + def test_codex_forward_host_credentials_populates_provisioned_env(self): + access = _jwt(2000000000) + with tempfile.TemporaryDirectory(prefix="bb-provider.") as tmp: + home = Path(tmp) / "host-codex" + home.mkdir() + (home / "auth.json").write_text(json.dumps({ + "auth_mode": "chatgpt", + "tokens": {"access_token": access}, + })) + plan = agent_provision_plan( + template="codex", + dockerfile="", + state_dir=Path(tmp), + forward_host_credentials=True, + host_env={"CODEX_HOME": str(home)}, + ) + self.assertEqual( + {CODEX_HOST_CREDENTIAL_TOKEN_REF: access}, + plan.provisioned_env, + ) + + def test_codex_without_forward_host_credentials_has_empty_provisioned_env(self): + with tempfile.TemporaryDirectory(prefix="bb-provider.") as tmp: + plan = agent_provision_plan( + template="codex", + dockerfile="", + state_dir=Path(tmp), + forward_host_credentials=False, + ) + self.assertEqual({}, plan.provisioned_env) + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_egress.py b/tests/unit/test_egress.py index 55b89c4..f1129cc 100644 --- a/tests/unit/test_egress.py +++ b/tests/unit/test_egress.py @@ -2,7 +2,6 @@ resolution (PRD 0017).""" import unittest -import unittest.mock from bot_bottle.egress import ( CODEX_HOST_CREDENTIAL_TOKEN_REF, @@ -10,7 +9,6 @@ from bot_bottle.egress import ( egress_manifest_routes, egress_render_routes, egress_resolve_token_values, - egress_resolve_token_values_with_provider, egress_routes_for_bottle, egress_token_env_map, ) @@ -343,72 +341,13 @@ class TestResolveTokenValues(unittest.TestCase): {"GH_PAT": ""}, ) - def test_codex_host_credential_ref_is_resolved_by_launch(self): + def test_codex_host_credential_ref_resolved_via_provisioned_env(self): out = egress_resolve_token_values( {"EGRESS_TOKEN_0": CODEX_HOST_CREDENTIAL_TOKEN_REF}, - {}, + {CODEX_HOST_CREDENTIAL_TOKEN_REF: "codex-access-token"}, ) - self.assertEqual({}, out) - - -class TestResolveTokenValuesWithProvider(unittest.TestCase): - def test_empty_map_returns_empty(self): - out = egress_resolve_token_values_with_provider({}, False, {}) - self.assertEqual({}, out) - - def test_empty_map_with_forward_credentials_returns_empty(self): - # forward_host_credentials=True but no slots → no codex call needed. - out = egress_resolve_token_values_with_provider({}, True, {}) - self.assertEqual({}, out) - - def test_manifest_tokens_resolved_without_forward_credentials(self): - out = egress_resolve_token_values_with_provider( - {"EGRESS_TOKEN_0": "GH_PAT"}, - False, - {"GH_PAT": "ghp_secret"}, - ) - self.assertEqual({"EGRESS_TOKEN_0": "ghp_secret"}, out) - - def test_codex_token_slotted_in_when_forward_credentials_and_matching_ref(self): - with unittest.mock.patch( - "bot_bottle.egress.codex_host_access_token", - return_value="codex-access-token", - ): - out = egress_resolve_token_values_with_provider( - {"EGRESS_TOKEN_0": CODEX_HOST_CREDENTIAL_TOKEN_REF}, - True, - {}, - ) self.assertEqual({"EGRESS_TOKEN_0": "codex-access-token"}, out) - def test_codex_token_not_slotted_when_no_matching_ref(self): - # forward_host_credentials=True but no CODEX_HOST_CREDENTIAL_TOKEN_REF - # slot in the map → manifest tokens only; Codex token is fetched but - # nothing to slot it into. - with unittest.mock.patch( - "bot_bottle.egress.codex_host_access_token", - return_value="codex-access-token", - ): - out = egress_resolve_token_values_with_provider( - {"EGRESS_TOKEN_0": "GH_PAT"}, - True, - {"GH_PAT": "ghp_secret"}, - ) - self.assertEqual({"EGRESS_TOKEN_0": "ghp_secret"}, out) - - def test_codex_not_called_when_forward_credentials_false(self): - called = [] - with unittest.mock.patch( - "bot_bottle.egress.codex_host_access_token", - side_effect=lambda *_: called.append(1) or "tok", - ): - egress_resolve_token_values_with_provider( - {"EGRESS_TOKEN_0": "GH_PAT"}, - False, - {"GH_PAT": "ghp_secret"}, - ) - self.assertEqual([], called) - if __name__ == "__main__": unittest.main() -- 2.52.0 From e528d5c5af135e8556fe751ed88f828357cae59d Mon Sep 17 00:00:00 2001 From: claude Date: Tue, 2 Jun 2026 04:54:09 +0000 Subject: [PATCH 5/5] docs(prd): update PRD 0030 design to reflect provisioned_env approach Revises the Design section to describe the implemented solution: provisioned_env on AgentProvisionPlan rather than an intermediate egress_resolve_token_values_with_provider function. Drops the old sentinel/lazy-import design narrative. --- ...030-deduplicate-egress-token-resolution.md | 195 +++++------------- 1 file changed, 57 insertions(+), 138 deletions(-) diff --git a/docs/prds/0030-deduplicate-egress-token-resolution.md b/docs/prds/0030-deduplicate-egress-token-resolution.md index 5a88c87..2067347 100644 --- a/docs/prds/0030-deduplicate-egress-token-resolution.md +++ b/docs/prds/0030-deduplicate-egress-token-resolution.md @@ -7,14 +7,15 @@ ## Summary -Extract the duplicated egress token resolution block — which calls -`egress_resolve_token_values` and then slots in the Codex host -credential — into a single shared function in `bot_bottle/egress.py`, -and have both backends call it. +Eliminate the duplicated egress token resolution block — which resolved +manifest-declared tokens and the Codex host credential — by moving +provider-specific token reading into `AgentProvisionPlan.provisioned_env` +at prepare time, and having both backends merge that map into `os.environ` +before calling the now-generic `egress_resolve_token_values`. ## Problem -The same logic block appears verbatim in two places: +The same logic block appeared in two places: - `bot_bottle/backend/docker/launch.py` (~line 183): inline inside `launch()`, building `token_values` before `compose_up`. @@ -29,21 +30,25 @@ Both blocks: call `codex_host_access_token` and slot the result into any `token_env` whose `token_ref` is `CODEX_HOST_CREDENTIAL_TOKEN_REF`. -The duplication means any change to step 3 — new provider credential -type, token refresh logic, a new sentinel ref — must be applied twice. +The duplication means any change to step 3 must be applied twice. PRD 0029, which introduced `forward_host_credentials`, already had to -wire both backends; the next change will too. This is a near-certain +wire both backends; the next change would too. This is a near-certain future sync bug. +`egress_resolve_token_values` also carried a sentinel `continue` skip +for `CODEX_HOST_CREDENTIAL_TOKEN_REF`, which tied an otherwise generic +egress helper to a Codex-specific contract. + ## Goals / Success Criteria - The `forward_host_credentials` resolution logic exists in exactly one place in the codebase. -- Both `docker/launch.py` and `smolmachines/launch.py` call that - shared function. +- Both `docker/launch.py` and `smolmachines/launch.py` call + `egress_resolve_token_values` with no provider-specific arguments. +- `egress_resolve_token_values` is fully generic — it neither knows nor + cares about provider identity or the `CODEX_HOST_CREDENTIAL_TOKEN_REF` + sentinel. - No behaviour change for either backend. -- The shared function is unit-testable without a plan or backend - dependency. ## Non-goals @@ -53,145 +58,59 @@ future sync bug. ## Design -### New function: `egress_resolve_token_values_with_provider` +### `AgentProvisionPlan.provisioned_env` -Add to `bot_bottle/egress.py`: +Add `provisioned_env: dict[str, str]` (default empty) to +`AgentProvisionPlan`. This map holds host-side secrets that the +provisioning stage resolved and that egress needs injected into the +sidecar environ. -```python -def egress_resolve_token_values_with_provider( - token_env_map: dict[str, str], - forward_host_credentials: bool, - host_env: dict[str, str], -) -> dict[str, str]: - """Resolve all egress token env-var values from the host environ. - - Combines the manifest-declared token refs (via - `egress_resolve_token_values`) with the optional Codex host - credential slot (when `forward_host_credentials` is True). Returns - an empty dict when `token_env_map` is empty. - - Pure function: `host_env` is passed in so callers can pass a sealed - mapping in tests without touching `os.environ`. The Codex access - token is read lazily only when the flag is set and a matching slot - exists, so callers that don't use `forward_host_credentials` pay no - cost.""" - if not token_env_map: - return {} - token_values = egress_resolve_token_values(token_env_map, host_env) - if forward_host_credentials: - from .codex_auth import codex_host_access_token - access_token = codex_host_access_token(host_env) - for token_env, token_ref in token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token - return token_values -``` - -The import of `codex_auth` is deferred inside the branch to avoid a -circular import — `egress.py` is imported by `pipelock.py` which -`codex_auth.py` does not import, but `codex_auth.py` imports from -`egress.py` transitively via `agent_provider`. The lazy import sidesteps -the cycle without restructuring the module graph. +When `forward_host_credentials=True` for Codex, `agent_provision_plan` +calls `codex_host_access_token(host_env)` and stores the result under +`CODEX_HOST_CREDENTIAL_TOKEN_REF`. This is already the prepare-time +stage where `write_codex_dummy_auth_file` runs, so the access-token +read is colocated with all other Codex-specific provisioning. ### Backend call sites -**`docker/launch.py`** — replace the inline block: +Both backends merge `provisioned_env` over `os.environ` before calling +`egress_resolve_token_values`: ```python -# before -token_values: dict[str, str] = {} -if plan.egress_plan.routes: - token_values = egress_resolve_token_values( - plan.egress_plan.token_env_map, dict(os.environ), - ) - if plan.spec.manifest.bottle_for( - plan.spec.agent_name, - ).agent_provider.forward_host_credentials: - access_token = codex_host_access_token(dict(os.environ)) - for token_env, token_ref in plan.egress_plan.token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token - -# after -bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) -token_values = egress_resolve_token_values_with_provider( - plan.egress_plan.token_env_map, - bottle.agent_provider.forward_host_credentials, - dict(os.environ), -) +effective_env = {**dict(os.environ), **plan.agent_provision.provisioned_env} +token_values = egress_resolve_token_values(plan.egress_plan.token_env_map, effective_env) ``` -**`smolmachines/launch.py`** — replace `_resolve_token_env`: +`provisioned_env` values take precedence over same-named host env vars, +so the Codex token slot resolves from the map written at prepare time +rather than from a raw `os.environ` lookup. -```python -# before -def _resolve_token_env( - plan: SmolmachinesBottlePlan, host_env: object -) -> dict[str, str]: - ep = plan.egress_plan - if not ep.routes: - return {} - env = dict(host_env) - token_values = egress_resolve_token_values(ep.token_env_map, env) - if plan.spec.manifest.bottle_for( - plan.spec.agent_name, - ).agent_provider.forward_host_credentials: - access_token = codex_host_access_token(env) - for token_env, token_ref in ep.token_env_map.items(): - if token_ref == CODEX_HOST_CREDENTIAL_TOKEN_REF: - token_values[token_env] = access_token - return token_values +### `egress_resolve_token_values` -# after -def _resolve_token_env( - plan: SmolmachinesBottlePlan, host_env: dict[str, str], -) -> dict[str, str]: - bottle = plan.spec.manifest.bottle_for(plan.spec.agent_name) - return egress_resolve_token_values_with_provider( - plan.egress_plan.token_env_map, - bottle.agent_provider.forward_host_credentials, - host_env, - ) -``` +Remove the `CODEX_HOST_CREDENTIAL_TOKEN_REF` sentinel `continue` skip. +The function now resolves every slot in `token_env_map` from `host_env` +without special-casing any key name. The `CODEX_HOST_CREDENTIAL_TOKEN_REF` +key is present in `effective_env` (injected by `provisioned_env`) exactly +when it is needed. -Note: the `host_env: object` annotation on the smolmachines version is -also corrected to `dict[str, str]` — the annotation was wrong; the -function body immediately called `dict(host_env)` treating it as a -mapping. +## Implementation -### Imports - -- `docker/launch.py`: drop `codex_host_access_token` and - `CODEX_HOST_CREDENTIAL_TOKEN_REF` imports; add - `egress_resolve_token_values_with_provider`. -- `smolmachines/launch.py`: same drops; add - `egress_resolve_token_values_with_provider`. -- `egress.py`: add `egress_resolve_token_values_with_provider` to - `__all__`. - -## Implementation chunks - -1. **PRD (this commit).** Sets the design. -2. **Shared function.** Add `egress_resolve_token_values_with_provider` - to `bot_bottle/egress.py` with unit tests covering: no routes → - empty dict, routes with no `forward_host_credentials` → manifest - tokens only, routes with `forward_host_credentials` + matching ref → - Codex token slotted in, routes with `forward_host_credentials` + no - matching ref → manifest tokens only (Codex call still made but no - slot to fill). -3. **Docker backend.** Replace the inline block in `docker/launch.py`. -4. **Smolmachines backend.** Replace `_resolve_token_env` in - `smolmachines/launch.py`; fix the `host_env: object` annotation. -5. **Cleanup.** Remove now-unused imports from both backends. - -## Testing strategy - -- **Unit (must):** the four cases above in a new test in - `tests/unit/test_egress.py`, using a sealed `host_env` dict so tests - don't touch `os.environ` or the filesystem. -- **No integration changes needed:** both backends' existing integration - tests exercise the full launch path; a behaviour regression would - surface as a token-injection failure in those tests. +- `bot_bottle/agent_provider.py`: add `provisioned_env` field; populate + it for Codex when `forward_host_credentials=True`. +- `bot_bottle/egress.py`: remove sentinel skip; remove + `egress_resolve_token_values_with_provider` (the intermediate function + that was introduced and then superseded by this design); drop the + `codex_auth` import. +- `bot_bottle/backend/docker/launch.py`: replace the provider-aware + resolution block with the `effective_env` merge. +- `bot_bottle/backend/smolmachines/launch.py`: same replacement in + `_resolve_token_env`. +- `tests/unit/test_agent_provider.py`: add tests verifying + `provisioned_env` is populated (or empty) for Codex with and without + `forward_host_credentials`. +- `tests/unit/test_egress.py`: remove `TestResolveTokenValuesWithProvider`; + replace the sentinel-skip test with a test verifying the Codex token + ref resolves normally when present in `host_env`. ## References -- 2.52.0