fix(master): load per-challenge embed.env into isolated child env - #48
Merged
Conversation
Embedded challenges start under `env -i` so Prism never inherits CHALLENGE_* and agent-challenge never inherits PRISM_*. That isolation is deliberate, but the passed-through list was hardcoded with no extension point, so every operator setting was silently dropped -- including CHALLENGE_PHALA_ATTESTATION_ENABLED and CHALLENGE_ATTESTED_REVIEW_ENABLED. Agent Challenge therefore booted with attestation off even though the operator env file enabled it, producing unattested zero scores, no raw weight snapshot, and a withheld epoch that fell back to burn. Add the supported extension point: a per-challenge env file (default <data-dir>/embed.env, overridable via BASE_MASTER_AC_ENV_FILE and BASE_MASTER_PRISM_ENV_FILE) whose allowlisted keys are appended after the built-in defaults, so operator values win. Prefixes stay disjoint per challenge, preserving cross-challenge isolation. Values are never logged.
📝 WalkthroughWalkthroughThe entrypoint now loads optional per-challenge ChangesChallenge environment overrides
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant master-entrypoint.sh
participant load_challenge_env_file
participant Prism
participant agent-challenge
Operator->>master-entrypoint.sh: provide optional embed.env files
master-entrypoint.sh->>load_challenge_env_file: parse Prism overrides
load_challenge_env_file->>Prism: apply allowlisted environment
master-entrypoint.sh->>load_challenge_env_file: parse agent-challenge overrides
load_challenge_env_file->>agent-challenge: apply allowlisted environment
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_master_embed_env_file.py (1)
48-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Prism-side env-file contract test.
All cases populate only the AC file, so
PRISM_ENV_FILEparsing and precedence are untested. Extend the helper with an optional Prism file body and verify aPRISM_*override reaches Prism only and replaces its default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_master_embed_env_file.py` around lines 48 - 91, Extend _run_entrypoint with an optional Prism env-file body, create the Prism env file when provided, and pass the corresponding configuration into the subprocess environment. Add or update a contract test to define a PRISM_* override in that file and assert it appears in the Prism dump, does not appear in the AC dump, and replaces the default value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_master_embed_env_file.py`:
- Around line 25-36: The test fake uvicorn must not persist raw API-key values.
In tests/unit/test_master_embed_env_file.py lines 25-36, redact or hash
sensitive environment values before writing dumps; at lines 94-115, replace the
raw PHALA_CLOUD_API_KEY fixture and assertion with a SHA/digest-based value; and
at lines 118-128, remove the raw API-key fixture from the isolation test while
preserving propagation and isolation coverage.
---
Nitpick comments:
In `@tests/unit/test_master_embed_env_file.py`:
- Around line 48-91: Extend _run_entrypoint with an optional Prism env-file
body, create the Prism env file when provided, and pass the corresponding
configuration into the subprocess environment. Add or update a contract test to
define a PRISM_* override in that file and assert it appears in the Prism dump,
does not appear in the AC dump, and replaces the default value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 456959b7-5d58-47da-af63-56e14f1ec1e6
📒 Files selected for processing (2)
docker/master-entrypoint.shtests/unit/test_master_embed_env_file.py
Comment on lines
+25
to
+36
| FAKE_UVICORN = """#!/usr/bin/env bash | ||
| # Dump the isolated child environment so the test can assert on it. | ||
| # The dump path is baked in: `env -i` strips DUMP_DIR from the child env. | ||
| target="unknown" | ||
| for arg in "$@"; do | ||
| case "${arg}" in | ||
| prism_challenge.app:app) target="prism" ;; | ||
| agent_challenge.app:app) target="ac" ;; | ||
| esac | ||
| done | ||
| env > "__DUMP_DIR__/${target}.env" | ||
| """ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not persist raw API-key values in test artifacts.
The fake uvicorn dumps the full child environment, including the raw PHALA_CLOUD_API_KEY; the related fixtures and assertions also retain that value. Redact or hash sensitive variables in the dump, and use a SHA/digest fixture when validating propagation.
tests/unit/test_master_embed_env_file.py#L25-L36: redact/hash sensitive environment values before writing the dump.tests/unit/test_master_embed_env_file.py#L94-L115: replace the raw API-key fixture/assertion with a digest-based value.tests/unit/test_master_embed_env_file.py#L118-L128: remove the raw API-key fixture from the isolation test.
As per coding guidelines: “Never log, document, or include evidence containing private keys, wallet mnemonics, API tokens, or full secret values; only names, digests, and SHAs are permitted.”
📍 Affects 1 file
tests/unit/test_master_embed_env_file.py#L25-L36(this comment)tests/unit/test_master_embed_env_file.py#L94-L115tests/unit/test_master_embed_env_file.py#L118-L128
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_master_embed_env_file.py` around lines 25 - 36, The test fake
uvicorn must not persist raw API-key values. In
tests/unit/test_master_embed_env_file.py lines 25-36, redact or hash sensitive
environment values before writing dumps; at lines 94-115, replace the raw
PHALA_CLOUD_API_KEY fixture and assertion with a SHA/digest-based value; and at
lines 118-128, remove the raw API-key fixture from the isolation test while
preserving propagation and isolation coverage.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
env -i, and the passed-through variable list was hardcoded with no extension point, so every operator-supplied setting was silently dropped.phala_attestation_enabled=Falseandattested_review_enabled=Falseeven though the operator env file enabled both.embed.envmerged into the isolated child environment, without weakening cross-challenge isolation.Why this matters (production impact)
Observed on the live master before this fix:
False/False(env file said true)score=0.0,attested=Nonesource_challenges.agent-challenge ok:false error:"missing"uids:[0] weights:[1.0]Root cause is exactly one line of intent:
env -i "${ac_env[@]}"wipes the ambientenvironment, and the allowlist below it never included the attestation switches.
The isolation itself is correct and is kept — only the missing extension point is added.
Changes
docker/master-entrypoint.shload_challenge_env_file()merges allowlistedKEY=VALUElines into a child env array.<data-dir>/embed.env, overridable viaBASE_MASTER_AC_ENV_FILE/BASE_MASTER_PRISM_ENV_FILE.CHALLENGE_/BASE_CHALLENGE_for AC,PRISM_for Prism;PHALA_,DSTACK_,OPENROUTER_API_KEYshared) so isolation is preserved.of quotes; missing file is non-fatal; unreadable file fails loudly.
tests/unit/test_master_embed_env_file.py— new behavioural coverage.Test plan
Tests execute the real entrypoint with a stubbed
uvicornthat dumps its isolatedchild environment, so this asserts actual runtime behaviour rather than script text.
test_ac_env_file_supplies_phala_settings_to_isolated_child— Phala switches + key reach ACtest_ac_env_file_does_not_leak_into_prism_child— isolation preservedtest_ac_env_file_overrides_builtin_default— file wins over defaultstest_missing_env_file_is_not_fatal— unchanged behaviour when absenttest_env_file_ignores_comments_blanks_and_malformed_keys— parser hardeningRED→GREEN verified against this PR's exact base (
origin/main@a13d42a6):Regression + lint:
CI
Notes
embed.envexists — existing deployments are unaffected.set_weightspath touched; sealer/aggregation untouched.Trueand an attested score lands before re-enabling emissions.Summary by CodeRabbit
New Features
Tests