Skip to content

fix(#3277): forward EGG_CONTEXT_MEASUREMENT into agent pods - #3281

Merged
jwbron merged 1 commit into
mainfrom
egg/issue-3277-forward-measurement-flag
Jun 26, 2026
Merged

fix(#3277): forward EGG_CONTEXT_MEASUREMENT into agent pods#3281
jwbron merged 1 commit into
mainfrom
egg/issue-3277-forward-measurement-flag

Conversation

@jwbron

@jwbron jwbron commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Summary

EGG_CONTEXT_MEASUREMENT was missing from the orchestrator's flag-forward tuple (_FORWARDED_DISCIPLINE_ENV_KEYS in orchestrator/kubernetes_spawner.py), so the #3249 emit-only measurement surfaces (#3271) never fired in production. Agent pods received EGG_CONTEXT_DISCIPLINE (mechanism active) but not EGG_CONTEXT_MEASUREMENT — the flag record_measurement() gates on — so it no-op'd in every pod and zero metrics were emitted.

The omission dates 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 MEASUREMENT_ENV = "EGG_CONTEXT_MEASUREMENT", so the rationale no longer holds.

Changes

  • orchestrator/kubernetes_spawner.py — add "EGG_CONTEXT_MEASUREMENT" to _FORWARDED_DISCIPLINE_ENV_KEYS; replace the stale "not forwarded yet / no consumer" comment with one pointing at the feat(#3249): emit-only per-event context-discipline measurement surfaces #3271 consumer.
  • orchestrator/tests/test_kubernetes_spawner.py — pin the regression: test_flags_forwarded_into_pod_env_when_set asserts the key forwards when set; test_flags_absent_when_unset asserts it's absent when unset. (test_covers_every_declared_key already auto-covers it via the helper.)
  • docs/architecture/context-discipline.md — list EGG_CONTEXT_MEASUREMENT among the flags forwarded from the orchestrator deployment.

Testing

  • Targeted: 9 forward-flag tests pass.
  • Full make test: 18,357 passed. The only 2 failures are the known pre-existing reap-stale-egg-images safety-gate tests (exit 127) that always fail on this btrfs-root host — unrelated to this change.

Scope note

This PR is fix + test pin only. The redeploy + live-pod verification (issue step 3 — confirm a spawned pod carries the flag and context-measurement progress events land) is a deploy-time action handled separately.

Closes #3277

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.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No agent-mode design concerns. This is an infrastructure fix (forwarding EGG_CONTEXT_MEASUREMENT through _FORWARDED_DISCIPLINE_ENV_KEYS) with a test pin and doc update — no pre-fetching, human-facing structured output, post-processing pipelines, prompt-level constraints, direct Anthropic API calls, or pinned model identifiers in scope.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approve. Correct, minimal, well-targeted fix. I traced the full chain and the bug is real and the fix resolves it.

Verification

The bug is a genuine cross-module silent no-op. The in-pod consumer reads the flag, but nothing wired it into the pod env:

  • shared/egg_agent/__main__.py:123 calls record_measurement(...).
  • record_measurement (shared/egg_agent/measurement.py:423) gates on measurement_enabled(), which reads os.environ.get(MEASUREMENT_ENV) where MEASUREMENT_ENV = "EGG_CONTEXT_MEASUREMENT" (measurement.py:89,182).
  • The only production channel into the pod env is _FORWARDED_DISCIPLINE_ENV_KEYS (envFrom absent, sandbox_env built from pipeline fields — confirmed by the comment at kubernetes_spawner.py:209-217). Before this PR the tuple carried EGG_CONTEXT_DISCIPLINE and EGG_SESSION_RESUME but not EGG_CONTEXT_MEASUREMENT, so measurement_enabled() always saw an absent flag in-pod → record_measurement returned early in every pod. The PR description matches the code exactly.

The fix is correct. Adding "EGG_CONTEXT_MEASUREMENT" to the tuple (kubernetes_spawner.py:230) routes it through _forwarded_discipline_env, applied at kubernetes_spawner.py:1801before the extra_env merge at line 1804, so per-spawn override still wins (covered by the existing test_extra_env_overrides_forwarded_flag). Default-OFF semantics are preserved by the non-blank filter in _forwarded_discipline_env: unset/blank → not forwarded → in-pod default-OFF parse is byte-identical. The key is correctly not in _PROTECTED_ENV_KEYS, matching the other two discipline flags.

Tests exercise the production path. test_flags_forwarded_into_pod_env_when_set / test_flags_absent_when_unset drive spawn_agent_job_forwarded_discipline_env(os.environ) and assert on the resulting result.environment — no hand-built fixtures, no self-seeded goldens. test_covers_every_declared_key independently auto-covers the new key against the live tuple.

Doc update is accurateEGG_CONTEXT_MEASUREMENT is now listed among the forwarded flags in docs/architecture/context-discipline.md.

Non-blocking notes

  1. Emit-only safety holds. record_measurement also requires EGG_PIPELINE_ID and swallows all exceptions (measurement.py:425,430), so forwarding the flag cannot change agent outcomes even if a snapshot build fails — good. No concern, just confirming the blast radius.
  2. Attribution nit (doc + code comment). Both the doc ("EGG_CONTEXT_MEASUREMENT added in #3277") and the spawner comment cite issue #3277; the actual change rides in this PR (#3281). Citing the issue is fine and consistent with the existing #3272 style, so this is purely cosmetic — no change required.
  3. Scope note acknowledged. Live-pod verification (spawn a pod, confirm the flag rides and context-measurement progress events land) is correctly deferred to deploy time. That is the right call for a fix+test-pin PR; the static chain is fully proven here.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor
egg is addressing review feedback...

@james-in-a-box

Copy link
Copy Markdown
Contributor

Thanks for the thorough trace on both reviews — both came back APPROVED with no blocking concerns. Disposition of every non-blocking note below.

  1. Emit-only safety holds (review note 1) — disagree (no change needed). This was confirmation, not a request: record_measurement additionally requires EGG_PIPELINE_ID and swallows all exceptions, so forwarding the flag cannot change agent outcomes. Nothing to do.

  2. Attribution nit — #3277 vs #3281 (review note 2) — disagree (style preference, no technical impact). The reviewer explicitly wrote "Citing the issue is fine and consistent with the existing #3272 style, so this is purely cosmetic — no change required." The doc and the spawner comment intentionally cite the issue (EGG_CONTEXT_MEASUREMENT not forwarded to agent pods — #3249 measurement surfaces are inert in prod (#3272 dropped the key) #3277), matching the existing fix(#3200): forward context-discipline flags into spawned agent pods #3272 convention, rather than the PR number. Leaving as-is per the reviewer's own call.

  3. Live-pod verification deferred to deploy (review note 3) — disagree (no change needed). The reviewer confirmed this is the right call for a fix + test-pin PR; the static chain is fully proven by the tests. No action.

No code changes were required for this round — the static chain, default-OFF semantics, and per-spawn override precedence are all already covered by the existing tests at HEAD (1dc220f).

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor

egg feedback addressed. View run logs

2 previous review(s) hidden.

@jwbron
jwbron merged commit 5bfd5df into main Jun 26, 2026
32 checks passed
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.

EGG_CONTEXT_MEASUREMENT not forwarded to agent pods — #3249 measurement surfaces are inert in prod (#3272 dropped the key)

1 participant