feat(#3249): emit-only per-event context-discipline measurement surfaces - #3271
Conversation
Build the single Option-D adapter seam (#3200 phase 10, folding the #3258 surfaces) that emits the six BRC context-discipline metrics agent-side, after the SDK call, through the two surfaces an in-pod agent can write: the structured progress event and the heartbeat ping. The orchestrator event loop never reads AgentResult/logs/tokens back (#3164), so the measurement must emit in-pod. shared/egg_agent/measurement.py: - build_snapshot() (pure) binds the six metrics from the slice-1 occupancy capture (AgentResult.window_occupancy/token_usage) and the slice-8 resume-vs-reseed verdict (ResumeDecision), degrading to null/zero when a field is absent (partial-usage LiteLLM routes). - emit_snapshot() routes the full payload as the detail of an 'egg-orch progress emit' and a compact summary as an 'egg-orch message heartbeat' body; best-effort, never raises. - record_measurement() is the in-pod entry point, gated default-OFF behind EGG_CONTEXT_MEASUREMENT + EGG_PIPELINE_ID so the legacy / standalone path stays byte-identical. shared/egg_agent/__main__.py: one bare record_measurement() call after the SDK call; return discarded, exit code untouched. Emit-only: nothing gates runtime control flow on a metric value. Ships the structural assertions (call-site discard + no branch-condition reads a snapshot metric) and a synthetic >=1-reseed event test exercising all six metrics, plus routing/gate/degradation coverage.
There was a problem hiding this comment.
Review: emit-only per-event context-discipline measurement surfaces (#3271)
I traced the full path: __main__.record_measurement → build_snapshot → _resolve_real_window/_resolve_threshold → emit_snapshot → egg-orch progress emit / egg-orch message heartbeat → the progress_emit / message_heartbeat handlers → orchestrator POST. The CLI surfaces match (progress emit takes --step/--state/--detail; message heartbeat takes --state WORKING/--body), the token_usage keys match what client._usage_components produces, the list-form subprocess + json.dumps give a clean no-injection path, every failure is swallowed, and the data genuinely lands at the orchestrator endpoints (not a silent no-op). The default-OFF double gate keeps the legacy path byte-identical. The structural emit-only tests parse the real source. Good shape overall.
One blocking issue, plus non-blocking notes.
Blocking — two of the six headline metrics are structurally None in the production pod, and the tests assert the opposite
_resolve_real_window (measurement.py:213-228) resolves the backend window only by importing orchestrator.agent_model_resolution. But the production agent process runs in the sandbox with PYTHONPATH="/opt/egg-runtime/sandbox:/opt/egg-runtime/shared" (sandbox/Dockerfile:373) — orchestrator is not importable there. This is the same cross-boundary constraint reseed.py documents at length, and _resolve_real_window's own docstring acknowledges it ("the sandbox runs with orchestrator off PYTHONPATH … the measurement degrades to a null window there").
The consequence isn't a rare edge — it is every production event:
real_backend_window→None(import always fails in-pod)window_utilization=occupancy / real_window→None(metric 2, "peak utilization under resume")- the working-set-vs-real-window comparison →
None(metric 3)
So 2 of the "six metrics" the PR is built to emit are dead in the exact environment the feature targets. Note the asymmetry the PR introduces: the threshold has a cross-boundary env channel (EGG_RESEED_THRESHOLD, consumed via resolve_reseed_threshold) precisely because orchestrator isn't importable in-pod — but real_backend_window was given no equivalent channel, so window_utilization is null while reseed_threshold (potentially) populates.
What makes this blocking rather than a doc nit is that the tests assert the values that are null in production:
# test_synthetic_reseed_event_builds_all_six_metrics
assert snap.real_backend_window == 1_000_000
assert snap.reseed_threshold == 400_000
assert snap.window_utilization == pytest.approx(120_000 / 1_000_000)These pass only because CI/dev runs with the repo root on sys.path (orchestrator importable). They validate the import-succeeds path, which never executes in production, and give false confidence that these fields populate. This is exactly the "verify the analogy holds at the execution-model level" / "tests must exercise the production code path" failure mode: the test's mechanism (orchestrator importable) ≠ the pod's mechanism (orchestrator absent), and there is no test for the production-realistic null-window snapshot.
The underlying information isn't irretrievably lost — the snapshot emits model and window_occupancy, so an offline consumer running orchestrator-side can recompute real_backend_window(model) and derive utilization. That gives you two acceptable fixes; please pick one and make the code, docstrings, and tests agree:
- Add an
EGG_REAL_BACKEND_WINDOW(or reuse a spawn-time export) cross-boundary channel mirroringEGG_RESEED_THRESHOLD, so_resolve_real_windowreads the env first and the in-pod snapshot actually populates these fields. Then keep the current assertions but add a test that monkeypatches the import to fail and asserts the env-fed value still resolves (and assertsNonewhen neither is available). This is the option that makes the metric real in-pod. - Relabel metrics 2/3 as offline-derived — drop or clearly mark
real_backend_window/window_utilizationas "reconstructed offline frommodel+occupancy," fix the module/PR framing that presents them as per-event populated values, and add a test asserting the production path (_resolve_real_window→None⇒real_backend_window is None,window_utilization is None) so CI reflects what the pod actually emits.
Right now the PR ships green tests that contradict production behavior for three snapshot fields; that needs to be reconciled before merge.
Non-blocking
-
The emit-only structural test is narrower than its name suggests.
test_no_branch_condition_reads_a_snapshot_metriconly flags a branch whose condition is a directsnapshot.<metric>attribute access. It is trivially evaded by aliasing to a local first — which_summaryalready does:occ = snapshot.window_occupancy; … "n/a" if occ is None else str(occ)branches on a metric value via a local and is not caught. The invariant holds operationally (those branches only null-format an emit-only string, never steer agent control flow), but the test's guarantee is weaker than the docstring's "no … condition reads a builtsnapshot.<metric>field" implies. Consider either walking aliased locals or documenting the test's scope so a future reader doesn't over-trust it. -
Doc gap:
EGG_CONTEXT_MEASUREMENTisn't registered in the flag table indocs/architecture/context-discipline.md(≈ lines 197-200) alongsideEGG_CONTEXT_DISCIPLINE/EGG_SESSION_RESUME/EGG_RESEED_THRESHOLD. Adding a row keeps the staged-rollout flags discoverable in one place. -
Emit latency on measured runs: the two emits are sequential
subprocess.run(..., timeout=5)calls, so a stalled orchestrator adds up to ~10s per event before the pod returns its exit code. Default-OFF makes this prod-safe, but on the #2270 proving run it's ~10s/event worst case. A note (or a single combined emit / tighter cap) would be worth considering. -
Minor: the PR body calls
build_snapshot"pure," but it readsEGG_AGENT_ROLE/EGG_PHASE/EGG_SLICE_IDfromos.environ(the module docstring is accurate; the PR summary is slightly off).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Resolve the blocking review issue: real_backend_window / window_utilization were structurally None on every production event because orchestrator is off PYTHONPATH in the sandbox pod. Add an EGG_REAL_BACKEND_WINDOW cross-boundary env channel mirroring EGG_RESEED_THRESHOLD so _resolve_real_window reads the exported integer first and the metrics populate in-pod. Tests: keep the import-succeeds (dev/CI) six-metric assertions and add production-path coverage that blocks the orchestrator import — env override resolves, null when neither source available, non-positive override rejected. Also: register EGG_CONTEXT_MEASUREMENT / EGG_REAL_BACKEND_WINDOW in the context-discipline flag table, document the structural test's direct-access scope, note the ~10s worst-case emit latency, and drop the inaccurate (pure) qualifier on build_snapshot (it reads identity env).
|
Thanks for the trace-through. Addressed in Blocking — metrics 2/3 structurally
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
This is emit-only telemetry infrastructure (shared/egg_agent/measurement.py), not an agent workflow, and it aligns cleanly with the agent-mode design principles:
- No pre-fetching. Nothing is baked into an agent prompt. The module reads
AgentResultfields after the SDK call and emits them — it never constrains what any agent can see. - Structured vs. human-facing output is split correctly. The JSON
--detailpayload is genuine machine telemetry consumed offline (#3249 items 2-3 tune the route-aware reseed cap) — the legitimate "machine-readable output for genuine automation" case. The human-skimmable surface (_summary→ heartbeat body) is natural language. - Not a post-processing pipeline. It reads structured dataclass fields (
token_usage,window_occupancy, the slice-8ResumeDecision), not parsing an agent's free text to take an action the agent could take itself. - Sandbox boundary respected. Lives in
shared/but makes no direct Anthropic API call — it routes through the existingegg-orch progress/heartbeatsurfaces, the two channels an in-pod agent can write (EGG200 clean). - Model aliases.
modelis threaded as a variable; no pinned dated identifiers (EGG201 clean). - Emit-only, default-OFF, best-effort. The structural tests pinning that nothing gates control flow on a metric value, and that the legacy path stays byte-identical, are a good safeguard against the instrumentation perturbing agent behavior.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: emit-only per-event measurement surfaces (#3271) — 933c1b5
The single prior blocking concern is properly resolved, and all four non-blocking notes are addressed. I re-traced the full path and independently scrutinized the new code. No blocking issues. Approving with two minor forward-looking notes.
Blocking concern from last review — resolved ✅
Last review flagged that metrics 2/3 (real_backend_window, window_utilization) are structurally None in the production pod (orchestrator off PYTHONPATH), while the tests asserted the populated values via a dev/CI-only import path that never runs in production — green tests contradicting production behavior.
The PR took Option 1 exactly as specified:
_resolve_real_window(measurement.py:213) now readsEGG_REAL_BACKEND_WINDOWfirst and only falls back to the orchestrator import. I diffed it againstreseed.resolve_reseed_threshold— same env-first →_positive_int→ import-fallback shape, so the asymmetry the last review called out (threshold had a cross-boundary channel, window didn't) is gone.- The import-succeeds assertions in
test_synthetic_reseed_event_builds_all_six_metricsare kept but annotated dev/CI-only. - The three production-realistic tests asked for are present:
test_real_window_resolves_from_env_in_pod,test_real_window_null_in_pod_without_override,test_real_window_override_rejects_non_positive_int.
I verified the production-path tests are not a silent no-op: sys.modules["orchestrator.agent_model_resolution"] = None genuinely raises ModuleNotFoundError at the from … import statement, so test_real_window_null_in_pod_without_override truly drives the import-fails branch and asserts the honest None. Ran the 9 production-path/routing/degradation tests targeted — all green. The test/production contradiction is reconciled.
Non-blocking notes from last review — all addressed ✅
- Structural-test scope — documented in
test_no_branch_condition_reads_a_snapshot_metric's docstring (flags only directsnapshot.<metric>access, not aliased locals; explicitly a tripwire, not a proof). - Doc gap — both
EGG_CONTEXT_MEASUREMENTandEGG_REAL_BACKEND_WINDOWadded to the flag table indocs/architecture/context-discipline.md. - Emit latency —
emit_snapshotdocstring now documents the ~10s worst case, bounded by default-OFF + the per-call cap. - "pure" wording — corrected to "free of any side effect."
Independent scrutiny of the new code — clean
- Integration (
__main__.py:120):record_measurementis a bare statement afterrun_agent/write_session_state;result/resume_decisionare in scope; the success path still returnsresult.returncode. Even if it raised,record_measurementswallows everything internally — the emit cannot perturb the exit code. Matches the structural assertions. - CLI surfaces are real: verified
egg-orch progress emit --step/--state/--detail(choices includeworking) andegg-orch message heartbeat --state WORKING --bodyexist insandbox/egg_lib/orch_cli.pywith the exact flags used — the emit lands, not a silent no-op. - No injection: list-form
subprocess.run(no shell), JSON--detailpayload. - Schemas match:
AgentResult.window_occupancy/token_usage/num_turnsandResumeDecision.resume/reason/occupancyall line up with the snapshot build. - Graceful degradation on partial/absent usage; default-OFF double gate (
EGG_CONTEXT_MEASUREMENT+EGG_PIPELINE_ID) keeps the legacy path byte-identical.
Non-blocking — for follow-up
-
The producer side of
EGG_REAL_BACKEND_WINDOWisn't wired in this PR. Until the orchestrator exports the computed window into each pod's env at spawn time,real_backend_window/window_utilizationresolve toNonein-pod on every event. This is honestly documented (the docstring says "may export … would beNone… without this override") and it exactly mirrors the already-mergedEGG_RESEED_THRESHOLD, which likewise has no exporter yet — so it's consistent and out of scope for this reader-side PR. The raw inputs (model+window_occupancy) are still emitted, so the two metrics are offline-derivable regardless. Worth tracking the spawn-time export as a follow-up so the #2270 proving run gets these populated from the stream rather than via post-processing. One small wording nit:_resolve_real_windowpoint 1 ("This is the path that runs in production") reads as if the env path yields a value in prod today; until the export lands it always falls through to theNonebranch. -
Trailing
WORKINGheartbeat at process exit: the heartbeat emit fires after the SDK call near process exit withstate=WORKING. Benign here — default-OFF, proving-run-only, and the orchestrator sequences on k8s exit / BRC consensus state rather than heartbeat content — but if a future health-monitoring change keys off the last-seen heartbeat state, a staleWORKINGping at exit is something to keep in mind.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for the re-review and the approval. Both forward-looking notes addressed. Note 1 — producer side of
|
|
Feedback contract violation detected. The response comment posted during this run violates the contract in Violations: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — no agent-mode design concerns.
The only source delta since my last review (commit a777688) is a docstring correction in shared/egg_agent/measurement.py::_resolve_real_window. It directly addresses the wording nit from my prior review: point 1 previously read as if the $EGG_REAL_BACKEND_WINDOW env path yields a value in-pod today, when in fact the orchestrator does not yet export it at spawn time. The reworded text now states plainly that the override is the only channel that can populate the field in the sandbox, that it is currently unset in-pod so resolution falls through to None on every production event, and that it becomes the populating path only once the spawn-time export lands.
This is a documentation-accuracy fix with no behavioral effect and no agent-mode design surface. The emit-only invariant is preserved, the producer-side wiring remains correctly deferred to a follow-up (#3273), and the raw inputs (model + window_occupancy) stay emitted so the metric is offline-derivable in the interim.
Design remains aligned. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: emit-only per-event measurement surfaces (#3271) — delta a777688
The only change since my last review (933c1b5) is the single commit a777688, an 8/−5 docstring-only edit to _resolve_real_window in shared/egg_agent/measurement.py. No logic, no tests, no other files changed (confirmed against the commit's file list and patch).
Addresses the prior non-blocking nit — correctly ✅
My approving review of 933c1b5 left one wording nit: point 1 of _resolve_real_window's docstring read "This is the path that runs in production," which implied the EGG_REAL_BACKEND_WINDOW env path yields a value in-pod today, when in fact no orchestrator producer exports that variable yet, so resolution always falls through to None in production.
The new docstring (measurement.py:213-221) fixes this precisely and honestly:
The orchestrator does not yet export this variable at spawn time, so today the override is unset in-pod and resolution falls through to
None—real_backend_window/window_utilizationareNoneon every production event until that spawn-time export lands. Once it does, this is the path that populates the field in production.
I re-verified the underlying facts:
grepforEGG_REAL_BACKEND_WINDOW/EGG_RESEED_THRESHOLDacrossorchestrator/,sandbox/,shared/confirms no producer wires either var — both are reader-side only. The docstring's "does not yet export … until that spawn-time export lands" is accurate.- The code path is unchanged and still correct: env-first →
_positive_int→ import-fallback (fails in-pod) →None. This matches the production-realistic tests already added in933c1b5.
The corrected wording is now consistent with the module-level docstring (lines 34-41, "When neither the env override nor the (dev/CI-only) orchestrator import yields a value, both metrics degrade to None"). No contradiction remains between docstring and production behavior.
Non-blocking — one parallel docstring left slightly behind
For symmetry: _resolve_threshold's docstring (measurement.py:253) still describes EGG_RESEED_THRESHOLD as "the cross-boundary channel the orchestrator exports into the sandbox" — present-tense "exports," asserting it happens today. But per the same grep, EGG_RESEED_THRESHOLD has no producer either, and reseed.py:40 itself phrases it carefully as "the orchestrator side … may export it." So after this commit the window docstring is honest about the missing exporter while the threshold docstring one screen up still reads as if the export exists. Worth a one-word softening ("may export") to keep the two parallel resolvers telling the same story. Pure doc nit — no behavioral impact.
No blocking issues. The delta does exactly what it set out to do.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for the re-review and approval. The one non-blocking doc nit is addressed. Non-blocking —
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review (#3271) — delta a777688..2eb9836
The only change since my last review is the single commit 2eb9836, a +4/−2 docstring-only edit to _resolve_threshold in shared/egg_agent/measurement.py. No logic, no tests, no other files changed (confirmed against the commit file list and patch).
Addresses my prior non-blocking nit — correctly ✅
My previous review flagged that after a777688 the two parallel resolvers told different stories: _resolve_real_window's docstring was honest about the missing exporter, while _resolve_threshold (measurement.py:253) still described EGG_RESEED_THRESHOLD as "the cross-boundary channel the orchestrator exports into the sandbox" — present-tense, asserting the export exists today.
The new wording (measurement.py:253-256) fixes this precisely:
the cross-boundary channel the orchestrator side may export into the sandbox — like
$EGG_REAL_BACKEND_WINDOW, no producer wires it yet, so today this falls through to the import path
I re-verified the underlying facts:
grepforEGG_RESEED_THRESHOLD/EGG_REAL_BACKEND_WINDOWacrossorchestrator/andsandbox/confirms no producer wires either var — noenviron[…]=, setenv, or export anywhere. Both are reader-side only. The "no producer wires it yet" claim is accurate.- The softened phrasing now matches
reseed.py:40-41("the orchestrator side … may export it into the sandbox env") and the parallel_resolve_real_windowdocstring (measurement.py:212-221). The two resolvers andreseed.pynow tell one consistent story. - The code path is unchanged and still correct: env-first →
_positive_int/import-fallback →None.
No behavioral effect; the emit-only invariant is preserved and the raw inputs remain emitted so the metric stays offline-derivable until the spawn-time export lands. The delta does exactly what it set out to do.
No blocking issues. (PR already merged.)
— Authored by egg
There was a problem hiding this comment.
Re-review — no agent-mode design concerns.
The only source delta since my last review (commit a777688) is the single commit 2eb9836, a docstring-only edit to _resolve_threshold in shared/egg_agent/measurement.py. It addresses the exact non-blocking nit from my prior round: the resolver previously read as if $EGG_RESEED_THRESHOLD is "the cross-boundary channel the orchestrator exports into the sandbox" (present-tense, asserting the export exists today), while the parallel _resolve_real_window docstring was already honest that no producer wires its env var yet.
The reworded text now states plainly that the override is the channel the orchestrator side may export — "like $EGG_REAL_BACKEND_WINDOW, no producer wires it yet, so today this falls through to the import path." Both parallel resolvers now tell the same story and match reseed.py's own "may export" phrasing.
This is a documentation-accuracy fix with no behavioral effect and no agent-mode design surface. The emit-only invariant is preserved, raw inputs (model + window_occupancy) stay emitted so the metrics remain offline-derivable, and the producer-side spawn-time wiring remains correctly deferred to #3273.
Design remains aligned. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
|
egg agent-mode-design completed. View run logs 12 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
Builds scope item 1 of #3249 — the emit-only per-event measurement surfaces (the deferred #3200 phase 10, folding the #3258 surfaces). After each one-shot
python3 -m egg_agentBRC event the agent holds everything the six context-discipline metrics derive from (the slice-1 occupancy capture + the slice-8 resume-vs-reseed verdict), but the orchestrator event loop never readsAgentResult/logs/tokens back (#3164 — it sees only the pod exit code). So the measurement must emit agent-side, in-pod, after the SDK call, through the two surfaces a pod can write: the structured progress event and the heartbeat ping.This is the single Option-D adapter seam. Its prerequisites — the entire #3200 slice stack (#3236 slice-1 occupancy … #3251 slice-8 reseed) — are merged on
main; this binds to those fields.What it does
shared/egg_agent/measurement.py(new):build_snapshot()(pure) — binds the six metrics fromAgentResult.window_occupancy/token_usage(slice-1) andResumeDecision(slice-8), degrading to null/zero when a field is absent (partial-/no-usageLiteLLM routes). The six: window occupancy, peak-utilization input (occupancy / real_window+resumed), single-event working-set vs real backend window, reseed frequency (reseeded+reseed_reason), root-cache hit rate, tokens/event.emit_snapshot()— routes the full metric payload as the--detailofegg-orch progress emitand a compact summary as anegg-orch message heartbeat --body; best-effort, list-form (no shell), never raises, each surface independent.record_measurement()— the in-pod entry point, gated default-OFF behindEGG_CONTEXT_MEASUREMENTand the presence ofEGG_PIPELINE_ID, so the standalone / legacy path stays byte-identical.shared/egg_agent/__main__.py: one barerecord_measurement(...)call after the SDK call — return discarded, exit code untouched.Emit-only invariant
Nothing gates runtime control flow on a metric value; the metrics are consumed offline (#3249 items 2-3 tune the route-aware reseed cap). Enforced by two structural tests:
__main__call site discards the return and still returnsresult.returncode;if/while/ternary/assertcondition in the module reads a builtsnapshot.<metric>field.(The second test caught a real violation during development — a cosmetic
if snapshot.reseededbranch in the summary line — which is now removed.)Tests
tests/shared/egg_agent/test_measurement.py— 21 tests: the two emit-only structural assertions, a synthetic ≥1-reseed event exercising all six metrics, both-surface routing, the default-OFF / outside-a-pipeline gates, and graceful degradation on absent SDK usage. Green locally; ruff + mypy clean. (Full CI suite is the ground truth.)How this fits
Lands the instrumentation so the #2270 overseer-overhaul pipeline can be run as a measured proving run for the context-discipline mechanism (already deployed on
main/ the cluster, flag default-OFF). Items 2–5 of #3249 (run the measurement, tune the route-aware cap, monitoring) consume these surfaces and are out of scope here.Refs #3249, #3200, #3258.