Skip to content

fix: preserve shell precedence for runtime dotenv config - #69

Open
kic635 wants to merge 11 commits into
ob-labs:developfrom
kic635:fix/runtime-dotenv-precedence
Open

fix: preserve shell precedence for runtime dotenv config#69
kic635 wants to merge 11 commits into
ob-labs:developfrom
kic635:fix/runtime-dotenv-precedence

Conversation

@kic635

@kic635 kic635 commented Aug 11, 2026

Copy link
Copy Markdown

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

  • Normal runtime commands (dev, serve, worker, and scheduler) use python-dotenv grammar, physical binding order, and missing-variable semantics.
  • Container handoff uses the same grammar and order, but interpolation sees only selected prior layers plus the ambient provider allowlist. Unavailable host-only references remain literal and their values never enter Docker argv.
  • Source visibility and final precedence are explicit: config dotenv -> literal config mapping -> CLI dotenv -> launch shell override.
  • Valueless bindings affect later interpolation within their own dotenv file but do not delete a lower-precedence exported value or mask it from a later dotenv file.

Implementation

  • Declare and bound the direct parser dependency to the tested range python-dotenv>=1.0,<1.3.
  • Consume parse_stream() bindings in physical order instead of collapsing duplicate keys before interpolation.
  • Use python-dotenv variable atoms rather than a partial regular-expression grammar.
  • Keep normal runtime and protected container interpolation policies separate.

Regression coverage

One shared table-driven corpus covers:

  • same-file and cross-layer references;
  • missing, default, and bare references;
  • dotted and digit-leading names;
  • physical multiline defaults;
  • duplicate bindings;
  • empty and valueless bindings;
  • config dotenv, config mapping, CLI dotenv, and final shell precedence;
  • allowlisted and disallowed ambient host values.

The corpus runs in explicit clean and hostile ambient modes:

  • against the locked python-dotenv version;
  • against the exact Python 3.12 dependency floors (pydantic-settings==2.4.0, pydantic==2.8.0, python-dotenv==1.0.0);
  • through the real python -m agentseek_api.cli up -> Docker argv -> container inspection path.

Validation

  • Full suite: 817 passed, 24 skipped.
  • Ruff and git diff --check: passed.
  • Python 3.12 exact minimum-dependency conformance: passed.
  • Clean and hostile real Docker conformance matrix: passed.
  • Independent pre-commit code review: no remaining Critical, Important, or Minor findings; ready to merge.

Hosted Actions for the current cross-repository head require maintainer approval before jobs are created.

@webup webup left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/agentseek_api/cli.py Outdated
from pathlib import Path
from typing import TextIO

from pydantic_settings.sources.providers.dotenv import dotenv_values

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread src/agentseek_api/cli.py Outdated
key, value = line.split("=", maxsplit=1)
values[key.strip()] = value.strip().strip("\"'")
return values
values = dotenv_values(env_file)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread tests/unit/test_cli.py
assert env["AGENTSEEK_GRAPHS"] == str(config_path.resolve())


def test_build_runtime_env_shell_values_override_config_and_cli_dotenv(tmp_path: Path) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

@kic635

kic635 commented Aug 12, 2026

Copy link
Copy Markdown
Author

Follow-up: scope of the latest changes

This PR now covers the runtime dotenv compatibility and security regressions raised in review:

  • Replaced the unstable pydantic_settings.sources.providers.dotenv import with the public python-dotenv API and added python-dotenv>=1.0 as a direct dependency.
  • Verified the CLI through the installed agentseek-api console entrypoint under the minimum supported pydantic-settings==2.4.0 environment, so a newer lockfile is not masking import failures.
  • Kept dotenv interpolation within the selected runtime environment. Container handoff does not resolve ${...} against arbitrary host variables; disallowed host-only values remain literal.
  • Added coverage for comments, quotes, escapes, export, shell/config/CLI precedence, malformed valueless dotenv entries, and host-secret isolation.
  • Added a Docker regression to the existing agentseek-api up path. It starts a real application container and verifies the final docker exec environment, rather than checking only the pre-Docker environment map.

Validation completed locally:

  • tests/unit/test_cli.py: 66 passed
  • Ruff: passed
  • Minimum dependency console-script smoke: passed
  • Standalone container environment boundary smoke: passed

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 webup left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}/v1

The 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.

@kic635

kic635 commented Aug 12, 2026

Copy link
Copy Markdown
Author

Follow-up: dotenv compatibility regression addressed

Thanks for the additional review.

Problem

The controlled interpolation resolver added for agentseek-api up initially supported ${NAME} but did not preserve all relevant python-dotenv semantics:

  • ${NAME:-default} was left literal instead of using the default value;
  • bare $NAME was expanded, while python-dotenv intentionally leaves bare references unchanged.

This could make dev and up behave differently for existing dotenv files.

Root cause

We had to disable python-dotenv's built-in interpolation so it would not read arbitrary host os.environ values during container handoff. The first replacement resolver therefore implemented only a partial interpolation grammar, which introduced the compatibility regression.

Fix

The resolver now:

  • continues to parse dotenv syntax with dotenv_values(..., interpolate=False);
  • supports ${NAME} and ${NAME:-default};
  • leaves bare $NAME unchanged;
  • keeps unavailable ${NAME} references literal;
  • resolves values only from the controlled context (previous dotenv entries, lower-precedence selected sources, and the allowlisted ambient container environment).

This preserves same-file references such as API_ORIGINOPENAI_BASE_URL, while still preventing disallowed host-only secrets from being expanded into Docker -e arguments.

Regression coverage

Added both dev and up tests covering:

  • ${NAME:-default} fallback;
  • bare $NAME preservation;
  • same-file reference expansion;
  • disallowed host-secret non-expansion on the up path.

Validation: 69 passed, Ruff passed, and git diff --check passed.

The fix is pushed as 2b491b1 on the PR head.

@kic635

kic635 commented Aug 12, 2026

Copy link
Copy Markdown
Author

Follow-up: minimum-dependency smoke coverage

The other P2 from the review was also addressed and should be called out separately.

Problem

The original minimum-dependency smoke pinned only:

pydantic-settings==2.4.0

but left python-dotenv as >=1.0 and pydantic unconstrained. The resolver therefore selected newer versions such as python-dotenv==1.2.2 and pydantic==2.13.4. That could hide compatibility failures at the declared lower bounds.

Fix

The smoke now installs the exact declared floor combination:

pydantic-settings==2.4.0
pydantic==2.8.0
python-dotenv==1.0.0

It then installs agentseek-api without pulling a newer dependency set and executes the installed agentseek-api version console script, rather than importing an internal function directly.

Validation

The 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 webup left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.example

Fresh 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:

  1. 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.
  2. Preserve source order: config dotenv -> config mapping -> CLI dotenv -> final shell override. Define which values are visible while each layer is interpolated.
  3. 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_REFERENCE case by case.
  4. 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.
  5. Run the matrix under both clean and hostile ambient shells, against python-dotenv==1.0.0 and 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.

@kic635

kic635 commented Aug 13, 2026

Copy link
Copy Markdown
Author

Addressed the requested changes on new head cb84614bf6c24fc3e44e3504b533552e98e192e2.

Runtime compatibility

  • Removed the incremental _ENV_REFERENCE regex resolver.
  • Normal runtime interpolation now uses python-dotenv's parser and variable atoms with upstream missing-variable behavior, so OPTIONAL=prefix-${MISSING}-suffix becomes prefix--suffix.
  • Container-only literal preservation is now a separate policy and is not used by dev, serve, worker, or scheduler.
  • The python-dotenv implementation API dependency is bounded to the tested range python-dotenv>=1.0,<1.3.

Ordered interpolation and layer contract

  • Dotenv bindings are consumed directly from parse_stream() in physical order; duplicate keys are no longer collapsed before interpolation.
  • The explicit visibility/precedence contract is: config dotenv -> literal config mapping -> CLI dotenv -> final shell override.
  • Added valueless-binding tombstones so a higher-precedence valueless binding consistently masks a lower exported value while still participating in later interpolation.

Hermetic and shared conformance coverage

  • Added one shared table-driven corpus covering missing/default/bare references, dotted and digit-leading names, physical multiline defaults, duplicate bindings, empty and valueless bindings, and allowlisted/disallowed ambient references.
  • The same corpus is compared with upstream python-dotenv under both the locked 1.2.2 version and the exact 1.0.0 floor.
  • The same corpus is also executed through the real python -m agentseek_api.cli up path, including final Docker argv construction and inspection of the resulting container environment.
  • Added cross-layer container cases for config dotenv and config mapping tombstoned by the CLI dotenv, plus an allowlisted shell value restoring the final key.
  • Tests explicitly remove every allowlisted and corpus variable before clean-shell cases. I also reran the Docker matrix with all relevant variables deliberately polluted in the developer shell.

Verification

  • tests/unit/test_cli.py tests/unit/test_graph_manifest.py: 98 passed under a hostile ambient shell.
  • Coverage-backed unit + integration suite: 809 passed, 5 skipped; 90.64% coverage (90% required).
  • Python 3.12 exact floors (pydantic-settings==2.4.0, pydantic==2.8.0, python-dotenv==1.0.0): passed, including the shared upstream comparison.
  • Locked python-dotenv 1.2.2 shared comparison: passed.
  • Clean and hostile real agentseek-api up Docker conformance matrix: passed.
  • uv lock --check, Ruff, compileall, and git diff --check: passed.

Hosted checks are now starting on the new head; the results above are the fresh local verification for this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants