fix: preserve shell precedence for runtime dotenv config - #69
Conversation
webup
left a comment
There was a problem hiding this comment.
Thanks for fixing the source precedence and adding coverage for the richer dotenv syntax. The intended config dotenv < CLI dotenv < launch shell ordering behaves correctly under the locked environment, and hosted CI is green. I found two production blockers that the lockfile does not exercise, plus a test-isolation regression: the new import breaks the declared pydantic-settings lower bound, dotenv interpolation can read ambient variables outside the container allowlist, and existing lower-precedence tests now depend on the developer shell. Please address these before merge.
| from pathlib import Path | ||
| from typing import TextIO | ||
|
|
||
| from pydantic_settings.sources.providers.dotenv import dotenv_values |
There was a problem hiding this comment.
[P1] Preserve the declared pydantic-settings compatibility
pyproject.toml still permits pydantic-settings>=2.4.0, but 2.4.0 exposes pydantic_settings.sources as a module and has no sources.providers package. In an isolated allowed install, importing agentseek_api.cli fails at this line with ModuleNotFoundError, disabling every CLI command. The locked 2.14.1 environment masks this. Please use a stable public parser import with a direct dependency, or intentionally raise and test the minimum supported version.
| key, value = line.split("=", maxsplit=1) | ||
| values[key.strip()] = value.strip().strip("\"'") | ||
| return values | ||
| values = dotenv_values(env_file) |
There was a problem hiding this comment.
[P1] Keep dotenv expansion inside the selected container environment
dotenv_values() defaults to interpolation against the full process os.environ, even when build_container_env() deliberately supplies only _ambient_container_env() as base_env. Repro: with host PR69_DISALLOWED_SECRET=host-sensitive-value and an env file containing OPENAI_API_KEY=${PR69_DISALLOWED_SECRET}, the source key is excluded by the container allowlist but this parser still produces OPENAI_API_KEY=host-sensitive-value, which docker run -e then forwards. The previous parser kept ${...} literal. Please disable interpolation or resolve it only against the explicitly selected/filtered environment.
| assert env["AGENTSEEK_GRAPHS"] == str(config_path.resolve()) | ||
|
|
||
|
|
||
| def test_build_runtime_env_shell_values_override_config_and_cli_dotenv(tmp_path: Path) -> None: |
There was a problem hiding this comment.
[P2] Make the lower-precedence tests independent of the ambient shell
The new precedence is correct, but existing main() tests still assert config/CLI values without clearing the same keys from os.environ. For example, setting OPENAI_API_KEY makes test_dev_command_loads_config_env_mapping_and_auth_path fail at line 290, and setting TOKEN/SHARED makes test_dev_command_merges_config_env_file_before_cli_env_file fail at line 326. This new test stays hermetic via base_env, but the existing lower-precedence cases need monkeypatch.delenv(...) (especially for commonly set provider keys).
Follow-up: scope of the latest changesThis PR now covers the runtime dotenv compatibility and security regressions raised in review:
Validation completed locally:
The full Docker runtime suite is wired into CI. Local execution was blocked by an OrbStack/containerd image-layer issue while pulling the MySQL test image, not by an application assertion failure. |
webup
left a comment
There was a problem hiding this comment.
Requesting further changes on exact head 3237c3237d0a97a5d1d851f21eba79f7bd7cb291.
[P1] Preserve safe dotenv interpolation for agentseek-api up
build_container_env() now calls build_runtime_env(..., interpolate_env_file=False) at src/agentseek_api/cli.py:623-630. That prevents the original host-secret expansion, but it also disables legitimate references to values defined earlier in the same dotenv file.
For example:
API_ORIGIN=https://api.example.test
OPENAI_BASE_URL=${API_ORIGIN}/v1The exact-head behavior is:
runtime=https://api.example.test/v1
container=${API_ORIGIN}/v1
docker-run handoff=${API_ORIGIN}/v1
_execute_up_command() passes these values to Docker as explicit -e KEY=value argv entries, and Docker does not recursively expand the placeholder. As a result, dev and up interpret the same dotenv file differently and the application receives a broken URL under up.
Please resolve placeholders against a controlled context: prior dotenv entries and the intended lower-precedence sources, plus only _ambient_container_env() as the ambient source. References to unavailable host-only variables must remain literal. Add an up-path regression covering both a legitimate same-file reference and the existing disallowed-host-secret boundary.
[P2] Make the minimum-dependency smoke exercise all declared floors
scripts/test_minimum_cli_dependencies.py:18 pins pydantic-settings==2.4.0, but requests python-dotenv>=1.0 and leaves direct pydantic unconstrained. The current smoke resolved python-dotenv 1.2.2 and pydantic 2.13.4, so it does not prove the declared lower-bound combination. Pin python-dotenv==1.0.0 and pydantic==2.8.0, or apply equivalent generated lower-bound constraints. An independent Python 3.12 probe confirmed that exact floor combination imports and runs the CLI successfully.
The dependency import fix, host-secret non-expansion, and ambient-test isolation from the previous review are otherwise addressed, and all hosted checks on this head are green.
Follow-up: dotenv compatibility regression addressedThanks for the additional review. ProblemThe controlled interpolation resolver added for
This could make Root causeWe had to disable FixThe resolver now:
This preserves same-file references such as Regression coverageAdded both
Validation: The fix is pushed as |
Follow-up: minimum-dependency smoke coverageThe other P2 from the review was also addressed and should be called out separately. ProblemThe original minimum-dependency smoke pinned only: but left FixThe smoke now installs the exact declared floor combination: It then installs ValidationThe exact floor combination installed successfully in a fresh Python 3.12 environment, and the real CLI entrypoint completed successfully. This closes the dependency-compatibility P2 independently of the dotenv interpolation fix. |
webup
left a comment
There was a problem hiding this comment.
Requesting further changes on exact head 2b491b1ccbbb2112ac60197b14ffbf1e89398ed7.
The two previously requested follow-ups are addressed: same-file references now resolve through the final Docker argv without exposing a disallowed host secret, and the minimum-dependency smoke now exercises the exact Python 3.12 floors. However, the replacement interpolation path still changes normal runtime semantics and the new regressions are not hermetic.
[P1] Keep normal runtime interpolation compatible with python-dotenv
The custom resolver at src/agentseek_api/cli.py:152-167 preserves an unavailable ${NAME} literally, and build_runtime_env() applies that behavior to dev, serve, worker, and scheduler—not only to the protected container handoff.
Fresh exact-head reproduction:
OPTIONAL=prefix-${MISSING}-suffix
python-dotenv==1.0.0: prefix--suffix
current head: prefix-${MISSING}-suffix
The custom regex is also narrower than python-dotenv for dotted or digit-leading names and physical multiline defaults. Literal preservation is a container security policy; it should not silently replace established dotenv behavior for every runtime command.
[P2] Preserve physical binding order during interpolation
At src/agentseek_api/cli.py:177-185, dotenv_values(..., interpolate=False) first collapses duplicate keys, then the custom resolver iterates that dictionary. This makes a later binding visible to an earlier reference:
API_ORIGIN=https://first.example
OPENAI_BASE_URL=${API_ORIGIN}/v1
API_ORIGIN=https://second.exampleFresh result:
python-dotenv==1.0.0: https://first.example/v1
current head: https://second.example/v1
Resolve ordered bindings as they are parsed; do not collapse them before interpolation.
[P2] Make the container regressions independent of the developer shell
The new security tests and scripts/test_container_env_boundary.py inherit allowlisted provider keys while asserting lower-precedence dotenv values. With OPENAI_API_KEY=ambient-key, the standalone smoke fails at line 35 before Docker. The related unit tests similarly fail when OPENAI_BASE_URL or OPENAI_API_KEY is commonly present. Clear and restore every asserted allowlisted key, then keep separate hostile-shell cases that intentionally verify precedence and secret isolation.
Recommended design direction
Please pause the incremental regex fixes and define the contract before the next patch:
- Separate the two policies explicitly:
- normal runtime: upstream python-dotenv semantics;
- container handoff: the same grammar and ordering, but interpolation only against selected prior values plus the ambient allowlist.
- Preserve source order: config dotenv -> config mapping -> CLI dotenv -> final shell override. Define which values are visible while each layer is interpolated.
- Use one complete ordered grammar. If the public API cannot accept an explicit interpolation context, either use upstream parser machinery behind a tested version range or vendor a complete resolver with conformance tests. Do not extend
_ENV_REFERENCEcase by case. - Add a table-driven conformance matrix covering same-file and cross-layer references, missing/default/bare references, dotted and digit-leading names, multiline values/defaults, duplicate keys, empty and valueless bindings, allowlisted and disallowed host keys.
- Run the matrix under both clean and hostile ambient shells, against
python-dotenv==1.0.0and the locked version, and through the final Docker argv/container handoff.
All hosted checks on this SHA are green, but they do not exercise the reproductions above.
|
Addressed the requested changes on new head Runtime compatibility
Ordered interpolation and layer contract
Hermetic and shared conformance coverage
Verification
Hosted checks are now starting on the new head; the results above are the fresh local verification for this commit. |
Summary
Fix runtime dotenv handling so the launch shell has the documented highest precedence after project and CLI dotenv sources are loaded, while keeping Docker handoff interpolation isolated from disallowed host values.
This is the runtime prerequisite for ob-labs/agentseek#172 and agentseek-ai/agentseek-templates#14.
Runtime contract
dev,serve, worker, and scheduler) use python-dotenv grammar, physical binding order, and missing-variable semantics.Implementation
python-dotenv>=1.0,<1.3.parse_stream()bindings in physical order instead of collapsing duplicate keys before interpolation.Regression coverage
One shared table-driven corpus covers:
The corpus runs in explicit clean and hostile ambient modes:
pydantic-settings==2.4.0,pydantic==2.8.0,python-dotenv==1.0.0);python -m agentseek_api.cli up-> Docker argv -> container inspection path.Validation
git diff --check: passed.Hosted Actions for the current cross-repository head require maintainer approval before jobs are created.