fix(#3200): forward context-discipline flags into spawned agent pods - #3272
Conversation
The #3200 context-discipline flags (EGG_CONTEXT_DISCIPLINE) and the #3249 measurement flag (EGG_CONTEXT_MEASUREMENT) are read *in-pod* — by the agent and the wrapper's in-pod prompt composer — but nothing wired them into the spawned-agent Job env: no envFrom/ConfigMap, sandbox_env is built from pipeline fields, and build_agent_command never passes them. So `kubectl set env` on the orchestrator deployment was a no-op for agents, leaving no operator path to actually enable the mechanism (or the measurement emit) for a proving run. Forward a curated set of context-discipline flags from the orchestrator's own env into every agent pod's env at the single spawn chokepoint (spawn_agent_job's base environment dict, which the event-pump path also routes through). Default-OFF preserved: an unset/blank flag is not forwarded, so the legacy path stays byte-for-byte. The forward runs before the extra_env merge, so a per-spawn override still wins. The per-spawn substrate (EGG_SESSION_STATE_FILE / EGG_RESEED_THRESHOLD) is deliberately NOT a blind forward — it needs real per-pod wiring (slice-scoped persistent state path + per-model threshold export), a tracked follow-up. This minimal forward unblocks the prompt-split (protected-root / JIT-pull) activation and the emit-only measurement. Refs #3200, #3249.
There was a problem hiding this comment.
Approve with non-blocking suggestions. I traced the full data-flow end-to-end and the core wiring is correct: the change closes a real gap and the primary mechanism works in its actual runtime environment.
What I verified (functional)
EGG_CONTEXT_DISCIPLINEreaches its real in-pod consumers. The wrapper runs in-pod and shells out to/opt/egg-runtime/orchestrator/routes/event_prompt.py(consensus_wrapper.py:181), whose_context_discipline_enabled()reads the pod'sos.environ(event_prompt.py:1722);egg_agent.context_discipline/session/reseedlikewise read pod env. So forwarding into the Job env is genuinely necessary and sufficient —kubectl set envon the orchestrator now activates the protected-root / JIT-pull split. End-to-end path confirmed.EGG_SESSION_RESUMEdegrades safely without the deferred substrate. With resume on butEGG_SESSION_STATE_FILEunset,resolve_session_state_path()returnsNoneandread_session_state()returnsNone(never raises) → cold-start, exactly as the PR claims ("every event cold-reseeds").session.py:60-160.- Override ordering / coverage. The forward runs before the
extra_envmerge (kubernetes_spawner.py:1792), the three keys are not in_PROTECTED_ENV_KEYS, so a per-spawn override wins as documented. There is no early-return spawn path between env construction (1652) and the forward, andspawn_event_jobroutes throughspawn_agent_job(:2125), so every BRC role is covered. - Default-OFF preserved. Unset/blank omitted (
if value and value.strip()); pod parse is byte-identical to absence. - Tests exercise the production path (real
spawn_agent_job+os.environvia monkeypatch, asserting onresult.environment) — not hand-built fixtures. Good.
Non-blocking
1. EGG_CONTEXT_MEASUREMENT is forwarded but has no consumer — the measurement claim is currently inert. Grepping the whole repo, the literal EGG_CONTEXT_MEASUREMENT appears only in this PR (kubernetes_spawner.py:226 + its tests). Nothing in-pod reads it: there is no egg_agent.measurement module, and #3249's emit-only surfaces (its scope item 1) are unbuilt — #3249 is OPEN and blocked on the still-unmerged #3236 / #3251. So forwarding this key is a silent no-op today, and the PR description's "unblocks … the emit-only measurement" / "the #3249 measurement emit" overstates it: an operator who sets EGG_CONTEXT_MEASUREMENT=1 gets no measurement and no signal that nothing happened.
This isn't harmful (no crash, no security impact, and it's reasonable to pre-wire an operator knob ahead of its consumer), which is why it's non-blocking. But please either (a) drop EGG_CONTEXT_MEASUREMENT from the forward until #3249's emit consumer lands, or (b) reword the description/inline comment to say the measurement knob is pre-wired but currently inert rather than active — and ideally pin the eventual flag name, since the codebase's flag-name auto-discovery convention (sandbox/tests/test_context_discipline_flag.py) means the unbuilt consumer's name isn't fixed yet; if it lands under a different name this forward silently won't match it.
2. "truthy" wording vs. actual behavior. _forwarded_discipline_env's docstring ("present (truthy)") and the test name test_selects_only_set_truthy_flags describe a truthiness filter, but the implementation forwards any non-blank value — EGG_CONTEXT_DISCIPLINE=false would be forwarded (then correctly parsed OFF in-pod, so behavior is fine). The same docstring's other phrasing ("set and non-blank") is the accurate one; "truthy" is misleading. Minor — align the wording/test name with the non-blank semantics.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on PR #3272: - Remove EGG_CONTEXT_MEASUREMENT from the forwarded set. It has no in-pod consumer (no egg_agent.measurement module exists) and #3249's emit surface is unbuilt, so forwarding it was a silent no-op. The flag-name auto-discovery convention also means the eventual consumer's name isn't pinned, so a guessed forward could silently never match. Documented as a re-add point once #3249's emit consumer lands. - Fix 'truthy' wording in _forwarded_discipline_env docstring and rename test_selects_only_set_truthy_flags -> _non_blank_flags: the filter is non-blank, not truthiness ('false' is forwarded as-is, parsed OFF in-pod).
|
Thanks for the thorough end-to-end trace. Addressed both non-blocking items in-PR (commit e017877). 1. One thing I couldn't do: edit the PR description (the bot account doesn't own this PR — author is jwbron, so 2. "truthy" wording vs. non-blank behavior —
Targeted tests green (6 passed), ruff clean. — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approve. Re-reviewed the delta (e017877) against the merge base. Both non-blocking items from my prior review are properly resolved at the code level — not superficially — and no new issues were introduced.
Prior feedback — verified resolved
1. EGG_CONTEXT_MEASUREMENT inert forward → resolved (option a). Dropped from _FORWARDED_DISCIPLINE_ENV_KEYS entirely; the list now carries exactly EGG_CONTEXT_DISCIPLINE + EGG_SESSION_RESUME (kubernetes_spawner.py:231-234). Confirmed by grep that EGG_CONTEXT_MEASUREMENT no longer appears as a forwarded key anywhere in orchestrator/, sandbox/, or shared/ — its only remaining occurrence is the explanatory NOTE comment (:224) documenting the re-add point once #3249's emit consumer lands under a pinned name. This is the right call given the flag-name auto-discovery convention (sandbox/tests/test_context_discipline_flag.py): a guessed name would be a silent no-op now and could fail to match later. The two remaining forwarded keys both have real in-pod consumers — EGG_CONTEXT_DISCIPLINE (shared/egg_agent/context_discipline.py:53, session.py, event_prompt.py:1700) and EGG_SESSION_RESUME (session.py:68, reseed.py, client.py, runner.py) — so the forward is genuinely necessary and sufficient.
2. "truthy" wording vs. non-blank behavior → resolved. The _forwarded_discipline_env docstring now states "set and non-blank" and explicitly calls out it is a non-blank filter, not a truthiness filter ("false" is forwarded as-is and parsed OFF in-pod). The test was renamed test_selects_only_set_non_blank_flags and now asserts a falsey-but-non-blank EGG_SESSION_RESUME=false is forwarded — name and behavior agree, no name-vs-behavior contradiction.
Re-verified (still correct after the delta)
- Override ordering / coverage. Forward runs at
:1801, before theextra_envmerge loop (:1804+). Neither forwarded key is in_PROTECTED_ENV_KEYS, so a per-spawnextra_envoverride genuinely wins, as documented and as the newtest_extra_env_overrides_forwarded_flagasserts. No early-return path exists between env construction and the forward, so every spawn (includingspawn_event_job → spawn_agent_job) is covered. - Default-OFF preserved.
if value and value.strip()omits unset/blank/whitespace-only values, never forwarding an empty string — pod parse is byte-identical to absence. - Tests exercise the production path. The integration tests drive real
spawn_agent_jobwithos.environvia monkeypatch and assert onresult.environment; the helper unit tests cover the pure selector directly. No self-seeded goldens, no hand-built fixtures bypassing the production helper. Ran the 6 targeted tests locally — all pass.
The author already noted the PR description still references the (now-removed) measurement wiring and that the bot can't edit it since jwbron owns the PR. That's a prose-only staleness in the description, not a code issue — @jwbron may want to trim the "unblocks … #3249 measurement emit" line, but it does not block merge.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
The orchestrator's `_FORWARDED_DISCIPLINE_ENV_KEYS` forwarded `EGG_CONTEXT_DISCIPLINE` and `EGG_SESSION_RESUME` but omitted `EGG_CONTEXT_MEASUREMENT`, the flag #3271's `record_measurement()` gates on. With the flag absent in-pod, the #3249 emit-only measurement surfaces no-op in every agent, so an instrumented proving run captures zero metrics even while context discipline is active. The omission dated to #3272, whose comment said the measurement knob had no in-pod consumer yet. That consumer landed in #3271 as `egg_agent.measurement` under the fixed name `EGG_CONTEXT_MEASUREMENT`, so the rationale no longer holds. - Add `EGG_CONTEXT_MEASUREMENT` to the forward tuple; rewrite the stale "not forwarded yet" comment. - Pin the regression in test_kubernetes_spawner.py: assert the key forwards when set and is absent when unset. - Update docs/architecture/context-discipline.md to list the flag among those forwarded from the orchestrator deployment.
Summary
Wires the operator path to actually turn on the #3200 BRC context discipline (and the #3249 measurement emit) for a spawned-pipeline run. Without this, the merged mechanism is unreachable from the operator side.
The gap
The context-discipline switches are all read in-pod — by the agent (
egg_agent.context_discipline/egg_agent.measurement) and by the wrapper's in-pod prompt composer (event_prompt._context_discipline_enabled) — but nothing wired them into the spawned-agent Job env:envFrom/ ConfigMap on agent Jobs,sandbox_envis built from pipeline fields (doesn't forward the orchestrator'sos.environ),build_agent_commandnever passes them,So
kubectl set env EGG_CONTEXT_DISCIPLINE=trueon the orchestrator deployment was a no-op for agents — they stayed on the legacy path, and the measurement would never emit. There was no operator path to enable the mechanism for a proving run.The fix
Forward a curated set of context-discipline flags from the orchestrator's own env into every agent pod at the single spawn chokepoint —
spawn_agent_job's baseenvironmentdict, which the event-pump path (spawn_event_job→spawn_agent_job) also routes through, so it covers every BRC role:extra_envmerge.kubectl set envon the orchestrator deployment is now the single operator knob.Deliberately out of scope
The per-spawn substrate —
EGG_SESSION_STATE_FILE(a slice-scoped persistent path shared across the one-shot event pods) andEGG_RESEED_THRESHOLD(per-model, computed orchestrator-side) — is not a blind forward; it needs real per-pod wiring and is a tracked follow-up. This minimal forward unblocks the protected-root / JIT-pull prompt-split activation and the emit-only measurement; the warm-resume/reseed substrate stays inert (every event cold-reseeds), which the measurement faithfully records.Tests
orchestrator/tests/test_kubernetes_spawner.py— pure-helper selection (set/unset/blank/coverage) + spawn-path wiring (forwarded when set, absent when unset,extra_envoverrides). Full file green (163); ruff clean; no new mypy errors. CI full suite is the ground truth.Refs #3200, #3249.