[issue-3200][slice-8/10] Threshold reseed (resume-vs-reseed, AC-3) - #3251
Conversation
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 1, "Test/Unit Tests": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ground slice-10 (emit-only BRC context-discipline surfaces) in the codebase. Key finding: the slice-1 occupancy field (#3236) and slice-8 reseed signals (#3251) the issue says to derive from are UNMERGED open PRs; origin/main has neither. Registered blocking HITL OQ-1 (substrate dependency, options A-D). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Operator HITL gate resolved the substrate-dependency open question to Option D: emit-only against existing AgentResult fields plus a single adapter seam that degrades to null/zero until slice-1 (#3236) / slice-8 (#3251) merge. No hard dependency, no stacking on egg/issue-3200/*, no vendored substrate. Bake the single-seam direction, emit-only hard scope, and synthetic-sequence AC-3 into the refine analysis. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b01a66c to
1a1195d
Compare
…ask-8-2) Decision-boundary tests for the slice-8 threshold reseed gate (AC-3): - occupancy just-under threshold -> resume; at/over -> reseed-from-root - None/unknown occupancy -> reseed (never a lossy resume) - no warm session -> reseed (never a hard failure) - reseed verdict yields no resume session id (fresh protected-root path) - threshold computed against the REAL backend window, not the [1m] alias (the sub-1M mis-trigger regression carried from slice-2) Boundary occupancy values are derived from the merged slice-2 reseed_threshold helper so they track the floor/margin knobs without edits. Parallel-BRC convention: a signature-introspecting locator skip-guards until the coder's task-8-1 decision symbol merges, then asserts at PR assembly. Validated 9/9 PASS against a throwaway "resume id or None" reference; the sub-1M regression FAILs against a [1m] mis-trigger reference before removal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the slice-8 resume-vs-reseed decision on top of the slice-6 warm-resume
substrate. At each one-shot event-pump re-invocation the gate reads the prior
run's persisted window occupancy (cache_read+cache_creation+input, slice-1),
compares it to the reseed threshold computed against the model's REAL backend
window (slice-2), and decides:
- occupancy known and < threshold -> RESUME the cached session (slice-6)
- occupancy >= threshold -> RESEED: fresh session from the protected
root (slice-4), bulk re-pulled JIT (slice-5)
Every ambiguous case biases to a safe reseed, never a lossy resume:
no warm session, unknown/None occupancy, no resolvable threshold, or resume
disabled (EGG_SESSION_RESUME off, the staged-rollout default). On a reseed the
gate yields no resume session id, so run_agent cold-starts from the protected
root rather than warm-continuing the history the reseed exists to discard.
- shared/egg_agent/reseed.py (new): decide_session_action() pure decision core;
decide_resume_session() reads SessionState + resolves threshold;
resolve_reseed_threshold() takes $EGG_RESEED_THRESHOLD (cross-boundary channel
for the sandbox where orchestrator is off PYTHONPATH) else a defensive lazy
import of orchestrator.agent_model_resolution.reseed_threshold, else None
(-> safe reseed). No hard shared->orchestrator dependency.
- __main__.py: route --resume through the gate; pass resume=decision.session_id
(None -> cold start) to run_agent.
- session.py: fix a Python 2 'except ValueError, TypeError:' that is a hard
SyntaxError under standard CPython 3 (the gate imports from this module).
Trigger uses occupancy (not billed input) and the REAL window (no [1m]
mis-trigger). Activatable purely via env so the orchestrator bash wrapper stays
byte-identical (golden unchanged); dark by default. Satisfies tester task-8-2
(test_reseed_decision.py): 9/9 pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
slice-8 routes --resume through decide_resume_session (the resume-vs-reseed gate) rather than threading it straight to run_agent. Update the slice-6 CLI plumbing test to assert the gate receives explicit_resume and its decision drives run_agent's resume kwarg.
8949da7 to
b1d5ab7
Compare
There was a problem hiding this comment.
Contract Verification — PR #3251 (issue-3200, slice-8: Threshold reseed / resume-vs-reseed, AC-3)
Verdict: Contract-compliant. Both slice-8 tasks are fully implemented and their acceptance criteria are objectively met. PR is already merged; this is the final contract check.
task-8-1 — resume-vs-reseed decision gate ✅
Implemented in shared/egg_agent/reseed.py (new) + shared/egg_agent/__main__.py wiring.
| Acceptance criterion | Evidence |
|---|---|
Resume when occupancy known & < min(400k, 0.80*real_window) |
decide_session_action resumes only on occupancy < safe_threshold with a real int occupancy (reseed.py:154-181). Threshold from resolve_reseed_threshold → orchestrator.agent_model_resolution.reseed_threshold = min(400_000, int(0.80*real_backend_window)) (agent_model_resolution.py:481-494). |
Reseed when occupancy >= threshold OR None/unknown OR no warm session |
Explicit branches no_warm_session, no_threshold, unknown_occupancy, at_or_above_threshold — every non-resume path returns session_id=None. |
| Reseeded session = protected root + JIT re-pull only | Reseed yields ResumeDecision(session_id=None); __main__.py passes resume=None → run_agent cold-starts from the protected root (slice-4) with JIT pull (slice-5). |
Trigger uses occupancy (cache_read+cache_creation+input), not billed input |
Occupancy sourced from SessionState.window_occupancy, documented as cache_read + cache_creation + input (session.py:89-97). |
| None-occupancy & no-warm-session → safe reseed, never lossy resume | Both collapse to reseed before any resume branch is reachable; bool occupancy also treated as unknown. |
Additional correctness: dark-by-default via EGG_SESSION_RESUME (resume_disabled short-circuit); no hard shared→orchestrator dependency (env override → defensive lazy import → None→safe reseed); never raises.
task-8-2 — decision-boundary tests ✅
tests/shared/egg_agent/test_reseed_decision.py (9 tests) + tests/shared/egg_agent/test_main.py gate-threading update.
Coverage confirmed: under→resume, at→reseed, over→reseed, None→reseed, no-warm-session→reseed (incl. low-occupancy variant), reseed leaks no resume id, and the real-window regression (kimi-k2.7-code[1m]: occupancy between the real ~209,715 threshold and the 400k [1m]-implied floor → reseed; symmetric below-threshold resume).
Ran locally (targeted, per review policy): test_reseed_decision.py + test_main.py → 18 passed. CI on HEAD: 19 checks green.
Notes for the human reviewer (non-blocking)
- The boundary tests resolve the pure core
decide_session_action(threshold=…), passing in a threshold the test derives from the merged slice-2 helper. This proves the decision core honors a real-window threshold and that the helper yields a sub-400k value for sub-1M backends — but the production wiringdecide_resume_session → resolve_reseed_thresholdis not exercised end-to-end for the[1m]case. Production uses the same slice-2 helper, so the risk is low; an integration test of that wiring would close the gap. - "JIT re-pull asserted" is covered indirectly (reseed yields no resume id ⇒ cold-start path), not via slice-5 mechanics — appropriate for the gate's scope.
- The
except TypeError, ValueError:form in the new test file is valid under Python 3.14 (PEP 758, optional parens) and matches existingclient.py/tool_output_cap.pyusage — verified, not a defect.
Criterion marking
This contract carries no top-level ac-N criteria (acceptance_criteria is empty; criteria live per-task as prose), so egg-contract verify-criterion has no target — and the orchestrator endpoint was unreachable this session regardless. Both per-task criteria are recorded here as verified.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: [issue-3200][slice-8/10] Threshold reseed (resume-vs-reseed gate)
Thorough review of the new shared/egg_agent/reseed.py, its wiring into __main__.py, and the slice-8 tests. The core decision logic is correct and the bias-to-reseed design is sound. Two issues warrant changes, plus context worth confirming. (PR is already merged and bot-authored, so this is posted as a comment for the follow-up.)
1. (Blocking) reseed.py reintroduces the exact bare-stdlib logger fallback that _logging.resolve_logger was built to eliminate — verified TypeError
reseed.py:60-67 builds its logger with the raw stdlib fallback:
try:
from egg_logging import get_logger
logger: Any = get_logger("egg-agent")
except ImportError: # pragma: no cover - stdlib fallback outside the sandbox
import logging
logger = logging.getLogger(__name__)…and then at reseed.py:221-231 calls it with structured kwargs:
logger.info(
"BRC event-pump resume-vs-reseed decision: %s",
"resume" if decision.resume else "reseed",
event_type="system", event_subtype="resume_gate_decision",
resume=..., reason=..., occupancy=..., threshold=..., model=model,
)When egg_logging is unimportable (the documented "outside the sandbox" case), logger is a plain logging.Logger, and once logging is enabled at INFO this raises:
TypeError: Logger._log() got an unexpected keyword argument 'event_type'
I reproduced this directly. It is masked today only because (a) logger.info short-circuits when the effective level is above INFO, and (b) egg_logging happens to be importable in the sandbox/test env.
This is a direct regression of an established codebase pattern created for this exact failure mode. The sibling module in this same feature, session.py:44-48, carries an explicit comment:
The bare
logging.getLoggerfallback this replaced raisedTypeErroron theevent_type=/error=kwargs below, which would have defeated the module's "never raise" contract (#3200 review).
session.py uses resolve_logger("egg-agent", __name__) from egg_agent/_logging.py, whose _StdlibLoggerAdapter drops structured kwargs precisely so the fallback never raises. reseed.py ignores that helper and reintroduces the un-hardened pattern — and decide_resume_session's own docstring claims "Never raises," a contract this logger.info violates in the egg_logging-absent + INFO case.
Fix (one line):
from egg_agent._logging import resolve_logger
logger: Any = resolve_logger("egg-agent", __name__)This is latent (gated behind EGG_SESSION_RESUME being on, since the logger.info is only reached when resume is enabled, plus egg_logging absent), but it is a concrete defect with an established trivial fix and should not ship inconsistent with its sibling.
2. (Blocking — testing) The new module's two I/O functions are untested, and the "real-window / no-[1m]" tests don't exercise threshold resolution
reseed.py has three public functions. Only one is exercised:
decide_session_action(pure comparator) — tested bytest_reseed_decision.py.decide_resume_session(the actual gate: enable-flag short-circuit, state read,explicit_resumeoverride, logging) — only ever mocked (test_main.py:186), never exercised.resolve_reseed_threshold(env-override parse,_positive_intguard, orchestrator fallback,Nonebias) — zero tests (grep-confirmed).
The decision tests bind to decide_session_action and inject the threshold themselves via _threshold_for(model) (confirmed: the locator resolves to egg_agent.reseed.decide_session_action, signature (*, session_id, occupancy, threshold)). Because the threshold is supplied externally, test_sub_1m_backend_uses_real_window_not_1m_alias does not actually verify the gate computes the real-window threshold — it passes the already-correct value in and re-asserts the comparator. A regression in resolve_reseed_threshold (e.g. resolving 80% off the [1m]-implied 1M window) would not break a single test in this PR. The AC "the threshold is computed against the real backend window (no [1m] mis-trigger)" is asserted only against slice-2's helper, not against this slice's resolution code — this is the "fixture bypasses the production code path" anti-pattern, and there's a name-vs-behavior gap between what the sub-1M test claims and what it checks.
Recommend adding:
- direct
resolve_reseed_thresholdtests: a parseableEGG_RESEED_THRESHOLDoverride wins; a non-int/non-positive override is rejected; with neither override nor importable orchestrator →None; and the real-window value for a sub-1M model is genuinely below the 400k floor (the regression that actually lives in resolution). - a
decide_resume_sessiontest that runs unmocked:EGG_SESSION_RESUMEoff →resume_disabledreseed; a persisted low-occupancy record → resume;explicit_resumeoverrides the session id but occupancy still comes from the record.
3. (Non-blocking) Invalid EGG_RESEED_THRESHOLD is swallowed with no operator signal
In resolve_reseed_threshold (reseed.py:126-134), a malformed or non-positive operator-supplied override hits except ValueError: override = None and falls through silently. In the sandbox — where orchestrator is off PYTHONPATH — that fall-through reaches None → the gate always reseeds, with no diagnostic for the operator who deliberately set the var. Behavior degrades to the safe path, so this isn't blocking, but a logger.debug/warning on parse failure (mirroring the trail session.py:read_session_state already emits for corrupt state) would save an operator from wondering why their threshold is ignored.
4. (Context — confirm, not blocking) The slice is entirely dark in the sandbox today
Nothing exports EGG_RESEED_THRESHOLD (grep: only reseed.py references it), consensus_wrapper.py does not set EGG_SESSION_STATE_FILE or pass --session-state-file, and EGG_SESSION_RESUME defaults OFF. So in the real sandbox the gate currently always reseeds (= legacy cold-start), and because orchestrator isn't importable there, the resume half is unreachable until the env-export channel is wired. This matches the documented staged-rollout intent and is correct for an 8-of-10 slice — just confirm the threshold-export + event-pump wiring are tracked in a later slice so the resume path doesn't stay permanently inert in the sandbox.
What's good
- Bias-to-reseed on every ambiguity (no warm session / unknown occupancy / no threshold / disabled) is the right safety posture and is cleanly expressed in
decide_session_action's ordered branches. - Boundary is correct and exclusive for resume (
occupancy >= thresholdreseeds); the comparator boundary cases are well covered. _positive_intcorrectly excludesbool(consistent withsession.py), and the round-trip write-back in__main__.pycorrectly persists the fresh session after a reseed.
The blocking items are #1 (logger fallback regression, one-line fix) and #2 (resolution/integration functions untested). Neither touches the comparator logic, which is correct.
— Authored by egg
|
egg review completed. View run logs 7 previous review(s) hidden. |
Resume-vs-reseed decision at re-invocation, comparing occupancy to the threshold; reseed a fresh session from the protected root, pre-empting CC compaction. None/unknown occupancy and no-warm-session both bias to reseed. Logical deps: slices 1, 2, 4, 6, 7 (all upstream in the serialized chain via slice 7).
Base PR: #3234
What's in this PR
Commits (3):
This slice
Threshold reseed (resume-vs-reseed, AC-3)
Files affected:
sandbox/shared/egg_agent/sandbox/tests/shared/egg_agent/tests/Tasks (2) + acceptance criteria
Stack
issue-3200egg/issue-3200/slice-7