feat(#3279): compute + inject EGG_RESEED_THRESHOLD per event spawn - #3284
Conversation
The in-pod resume-vs-reseed gate (#3200 slice-8) and the #3249 measurement resolve their threshold via egg_agent.reseed.resolve_reseed_threshold, which reads $EGG_RESEED_THRESHOLD first and otherwise tries to import orchestrator.agent_model_resolution.reseed_threshold. The agent pod runs with orchestrator off PYTHONPATH, so without the override the threshold resolves to None and the gate takes its no_threshold safe-reseed branch every event. Compute reseed_threshold(decision.claude_code_alias) in _build_event_spawn_params (the orchestrator has the model decision; the pod can't) and inject it as EGG_RESEED_THRESHOLD in the event pod env. Resolved against claude_code_alias so the injected value matches the --model flag and the emitted #3249 measurement, and sub-1M LiteLLM models resolve against their real backend window rather than the [1m]-implied 1M. Inert unless a discipline/resume/measurement flag is on in the pod. Session-state / transcript persistence (the rest of the warm-resume substrate) is #3278.
There was a problem hiding this comment.
Approve. Thorough trace of the full wiring — correct, well-scoped, and genuinely inert until a flag is on. No blocking issues. A couple of non-blocking notes below.
What I verified
End-to-end wiring is real (not just well-structured). I traced the injected value the whole way to the pod env:
_build_event_spawn_paramsreturns the 4th tuple element;spawn_eventwritesenv["EGG_RESEED_THRESHOLD"](concurrent_executor.py:165).envcomes fromget_agent_env, which builds a fresh dict every call (concurrent_executor.py:382), so the mutation has no shared-state aliasing risk.extra_envflows throughspawn_event_job's merge (kubernetes_spawner.py:2106-2107, caller keys preserved) →spawn_agent_job'sextra_envmerge.EGG_RESEED_THRESHOLDis not in_PROTECTED_ENV_KEYS(kubernetes_spawner.py:143-206), so it isn't stripped — and the comment atkubernetes_spawner.py:221explicitly flags this var as needing "real per-pod wiring," which is exactly what this PR adds.
reseed_threshold(decision.claude_code_alias) is the correct input. Using the alias (not upstream_model) is right: upstream_model is None on the Claude path, so the alias is the only string that resolves all three classes. Confirmed against the live resolver:
opus/opus[1m]/sonnet→400000(1M window, floored)kimi-k2.7-code(and the[1m]variant, normalised to bare) →209715=int(0.80 * 262_144)— guards the #3200 task-2-1 mis-trigger- regular/unknown LiteLLM (
qwen3-coder-30b[1m]) →160000(conservative 200K window) - empty/garbage alias →
160000, never raises
No bypass paths. The only production caller of _build_event_spawn_params (concurrent_executor.py:157) is updated, and the only production spawn_event invocation (event_loop.py:853) routes through the modified adapter. The 3→4 tuple signature change has no other consumers.
Genuinely inert (matches the PR's claim). decide_resume_session short-circuits to a safe reseed when session_resume_enabled() is false (reseed.py:204), and that defaults OFF (session.py:79-95, both EGG_SESSION_RESUME and the master EGG_CONTEXT_DISCIPLINE unset). The measurement knob EGG_CONTEXT_MEASUREMENT is deliberately not yet forwarded (kubernetes_spawner.py:224-228). So a default pod ignores the injected value; the default-Claude wire shape is unchanged.
Tests exercise the production path. TestEventSpawnReseedThreshold drives a real ConcurrentPhaseExecutor + _ExecutorEventSpawner with a mock spawn_fn, asserting on the captured extra_env — no hand-built fixtures, no self-seeded goldens; the 400000 / 209715 expectations are independently computed. I ran the 3 tests locally: all pass.
Non-blocking suggestions
-
Coverage gap — the conservative LiteLLM branch is untested here. The two new tests cover Claude (
400000) and sub-1M (209715), but not a regular/unregistered LiteLLM model resolving to160000(the[1m]-stripped → conservative-200K path). That's the branch most likely to surprise an operator (their model isn't in_SUB_1M_CONTEXT_MODELS, so it silently gets the 200K window). A third case (e.g.qwen3-coder-30b→160000) would lock it in. The logic itself is covered byagent_model_resolution's own tests, so this is additive only. -
Docstring nuance. The
_build_event_spawn_paramsdocstring and thespawn_eventcomment say the injected threshold and the #3249 measurement "agree by construction" because both resolve againstargs.model/claude_code_alias. Strictly, both consumers read theEGG_RESEED_THRESHOLDoverride first (reseed.py:126-134,measurement.py:252-262), so once this PR always injects it on the event path, they agree because they read the same env var, regardless ofargs.model. The conclusion holds; the stated mechanism is just a hair imprecise.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…env docstring Add a third TestEventSpawnReseedThreshold case asserting an unregistered LiteLLM model (qwen3-coder-30b) resolves to the conservative 200K window, yielding EGG_RESEED_THRESHOLD=160000 — the branch an operator whose model isn't in _SUB_1M_CONTEXT_MODELS silently lands on. Refine the _build_event_spawn_params docstring: the injected threshold and the #3249 measurement agree because both consumers read the same EGG_RESEED_THRESHOLD env var first, not because each independently re-resolves args.model.
|
Thanks for the thorough trace and the two non-blocking notes — both addressed in
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Approve. Both non-blocking notes from my prior review (commit 825ec687e) are properly addressed in 9b52597 — substantively fixed, not papered over. The delta is documentation + test coverage only; no production logic changed, so my earlier end-to-end trace still holds. No new issues.
Re-review of the delta (825ec687e..HEAD)
1. Coverage gap → fixed and verified independent. test_event_spawn_threshold_conservative_for_unregistered_litellm_model drives the real _ExecutorEventSpawner.spawn_event path (same path event_loop.py:853 uses) with qwen3-coder-30b and asserts on the captured extra_env["EGG_RESEED_THRESHOLD"]. I checked it's a genuine production-path exercise, not a hand-built fixture, and the 160000 golden is independently derivable:
qwen3-coder-30bis not in_SUB_1M_CONTEXT_MODELS(onlykimi-k2.7-codeis) and is not a Claude alias →real_backend_windowreturns_conservative_unknown_window()=200_000.reseed_threshold=min(400_000, int(0.80 * 200_000))=160_000. Matches the assertion exactly — no self-seeding.
This locks in the branch an operator whose model isn't registered silently lands on, which was exactly the untested case.
2. Docstring nuance → fixed and accurate. The _build_event_spawn_params docstring no longer claims the threshold and the #3249 measurement "agree by construction" via independent args.model resolution. It now states the real mechanism — both in-pod consumers read the EGG_RESEED_THRESHOLD override first — and cites the call sites. I verified both refs are correct:
reseed.py:126-134(resolve_reseed_threshold): readsRESEED_THRESHOLD_ENVand returns the positive override before the orchestrator-import fallback. ✓measurement.py_resolve_threshold: reusesresolve_reseed_threshold, so it reads the same override first. ✓
Verification
- Ran
TestEventSpawnReseedThreshold(all 4 cases incl. the new one): 4 passed. - Confirmed the registry, resolver math, and cited line references against the live source.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
What
Compute the per-model reseed threshold in the orchestrator and inject it into every BRC event pod as
EGG_RESEED_THRESHOLD, so the in-pod resume-vs-reseed gate (#3200 slice-8) and the #3249 measurement resolve a real-window boundary instead ofNone.Why
egg_agent.reseed.resolve_reseed_threshold(model)resolves in order:$EGG_RESEED_THRESHOLDoverride →import orchestrator.agent_model_resolution.reseed_threshold. The agent pod runs withorchestratoroff itsPYTHONPATH, so without the override the import fails, the threshold resolves toNone, and the gate takes itsno_thresholdsafe-reseed branch every event. Nothing computed/exported the override — this wires it.How
ConcurrentPhaseExecutor._build_event_spawn_paramsalready resolves the model decision; it now also computesreseed_threshold(decision.claude_code_alias)and returns it._ExecutorEventSpawner.spawn_eventinjectsEGG_RESEED_THRESHOLDinto the event pod'sextra_env.claude_code_alias— the same string passed to--modeland the Measure + monitor + tune the BRC context-discipline knobs (route-aware reseed cap/threshold) — consumes #3200 measurement surfaces #3249 measurement (args.model), so the injected threshold and the emitted measurement agree by construction. Sub-1M LiteLLM models carry their real-backend identity in the bare alias, so they resolve against their real window, not the[1m]-implied 1M (the mis-trigger BRC context discipline: protected directive root + queryable environment, bounded by a deterministic threshold reseed #3200 task-2-1 calls out).Scope / notes
no_warm_sessionthere regardless.session_resume_enabled) and the measurement (EGG_CONTEXT_MEASUREMENT). A plain default pod ignores it. This change removes the gate'sno_thresholdbranch; it does not turn warm resume on — resume stays inert until the session-store/transcript persistence substrate (Persist the Claude Code session store per (pipeline,slice,role) so #3200 warm resume survives one-shot event pods #3278) lands. Don't read a green merge here as "warm resume is firing."Tests
TestEventSpawnReseedThreshold(3): default opus →400000; env wiring throughspawn_event; sub-1Mkimi-k2.7-code→209715(guards the real-window-not-[1m]resolution).make testgreen (the 2test_reap_stale_egg_imagesfailures are pre-existing btrfs-host env noise, #3222, unrelated).Related
Closes #3279