[issue-3200][slice-6/10] Session-resume substrate (#3186... - #3243
Conversation
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 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.
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
Contract Verification — PR #3243 (issue-3200, slice-6/10)
Verdict: Approve. All acceptance criteria for both tasks are met, the new tests pass locally (6/6), and the full CI rollup is green. Two non-blocking coverage notes below.
Note: the orchestrator was UNREACHABLE during this review (
egg-orch health→ orchestrator UNREACHABLE; gateway OK), soverify-criterionwrites could not be persisted to the contract. The verification below is authoritative; the criterion flags will need re-marking once the orchestrator is back.
task-6-1 — session-resume substrate + cold-start fallback ✅
- AC: client.py exposes a resume-by-session_id option threaded through the event-pump path. Verified.
run_agent_asyncgrows keywordresume: str | None = None(shared/egg_agent/client.py:248). It is threaded end-to-end through the one-shot event-pump path:__main__.pyadds--resumeand passes it torun_agent(shared/egg_agent/__main__.py), the sync wrapper forwards via**kwargs(client.py:954), andsandbox/llm/claude/runner.py:32carries it through. When enabled,options.resumeis set andfork_sessionis deliberately left unset so the session continues rather than branches (client.py:407-431). - AC: when no resumable session exists the path seeds fresh from the protected root rather than erroring. Verified.
resume_session_id = (resume or "").strip(); an absent/empty id (or a disabled flag) simply does not setoptions.resume, so the run cold-starts a fresh session — no raise.read_session_statecollapses every failure mode (missing/empty/corrupt file, non-object payload, missingsession_id) toNone(session.py:read_session_state), andwrite_session_stateswallows OS errors and returnsFalse(session.py:write_session_state). The__main__.pywrite-back is best-effort and never alters the exit code. - AC: resume is opt-in (default off) so rollout is staged. Verified, two-fold: the param defaults to
None, and even a valid id is ignored unlessEGG_SESSION_RESUMEis truthy (session_resume_enabled(),session.py). Substrate ships dark ahead of the slice-8 gate, as documented.
Sanity-checked the consumers: AgentResult.session_id and window_occupancy both exist (shared/egg_agent/result.py:49-50), so the __main__.py write-back round-trip has real fields to persist.
task-6-2 — tests: resume-by-id, cold-start, default-off ✅
tests/shared/egg_agent/test_client_resume.py asserts each required behaviour: resume-by-id when the flag is on (options.resume == "sess-abc"), id ignored when the flag is off, fresh on no-arg / resume=None / empty id, and the param-default-off signature check. The missing-session paths assert success without raising. Ran the file locally: 6 passed. CI Unit Tests and Python checks are SUCCESS.
Non-blocking suggestions
session.pyround-trip has no direct unit tests.read_session_state/write_session_state/resolve_session_state_path/_coerce_occupancyare exercised only indirectly (the write side via__main__.py; the read side is slice-8's concern). For a module whose entire contract is "never raise," the defensive branches (corrupt JSON, blank-env-var path, bool-vs-int occupancy, atomic-write OS error) would benefit from explicit coverage. Not required by the named ACs, which target theclient.pyresume plumbing.__main__.py's new--resume/--session-state-fileargs and thewrite_session_statecall aren't covered bytest_main.py. A small round-trip test would lock in the CLI plumbing.
For the record: the unparenthesized except ValueError, TypeError: (session.py:147) and except TypeError, ValueError: (test_protected_root.py:200, introduced by the formatting-fix commit) are valid PEP 758 syntax on the project's required Python >=3.14 (requires-python = ">=3.14", ruff target-version = "py314") — legitimate, not a bug. No change requested.
— Authored by egg
There was a problem hiding this comment.
Review: PR #3243 — Session-resume substrate (slice-6/10)
Reviewed every changed file, traced the resume path end-to-end (__main__.py → client.run_agent_async → ClaudeAgentOptions.resume → SDK), verified ClaudeAgentOptions actually exposes resume/fork_session, confirmed AgentResult.session_id/window_occupancy exist, and checked CI-safety of the new imports. No blocking correctness or security issues found. The substrate meets slice-6's stated scope and ships dark as designed. Findings below are non-blocking but should be addressed before slice-8 builds on this.
What's correct (verified, not assumed)
run_agent_async(resume=...)defaults toNone, only setsoptions.resumewhen the id is non-empty andsession_resume_enabled()is true — the two-fold default-off gate. Cold-start (omitted /None/ empty / flag-off) simply skipsoptions.resume; it never raises. ✓options.resumeandoptions.fork_sessionare real SDK dataclass fields (confirmed againstclaude_agent_sdk); leavingfork_sessionunset to continue rather than branch is the right call. ✓write_session_stateis correctly atomic (temp file in the destination dir +os.replace), and allOSErrors — includingmkdir, the write, and the rename — are inside the singletry, so a misconfigured path (parent is a file, target is a dir, permission denied) degrades toFalse, never a crash. ✓test_client_resume.pyexercises the realrun_agent_asyncand asserts onoptions.resume, mocking only the SDKqueryboundary. No self-seeding goldens, no fixture that bypasses the production path, no name-vs-behaviour contradictions. Imports are CI-safe (client lazy-imports the SDK; session.py is stdlib +egg_loggingfallback). ✓
Non-blocking findings
1. shared/egg_agent/session.py (205 LOC) has zero direct unit tests.
The test file covers client.py's plumbing only. The new module's own logic is untested: the write→read round-trip, the corrupt/empty/non-dict-JSON fallback in read_session_state, _coerce_occupancy's bool-exclusion and non-int→None coercion, resolve_session_state_path's blank-string-is-unset rule, and the atomic write. This matters more than usual because the write side is live in production today — __main__.py calls write_session_state(...) on every run when $EGG_SESSION_STATE_FILE is set — and this slice is a declared hard prereq of slice-8's reseed gate. A regression in the persistence format would not break any current test; slice-8 would inherit it silently. Please add a focused test_session.py (round-trip, each read_session_state fallback branch, occupancy coercion incl. True/False, blank-path resolution) before slice-8 depends on it.
2. read_session_state swallows every failure with no log — asymmetric with the write side.
write_session_state emits logger.warning("Failed to persist session state...") on failure, but read_session_state collapses missing-file / unreadable / malformed-JSON / non-dict / bad-session_id all to None with no signal. Silent cold-start is the right default (resume is an optimization, not operator config that must succeed), so this is not blocking. But once slice-8 wires the read path, an operator who turned EGG_SESSION_RESUME on and is silently cold-starting every event because of a corrupt state file will have nothing to diagnose with. Recommend a logger.debug(...) on the corrupt/unreadable branch (distinct from the legitimate "no path configured" / "no file yet" cases, which should stay quiet).
3. Unrelated formatting churn across 7 files (commit 11606eb "apply automated formatting fixes").
agent_model_resolution.py, routes/event_prompt.py, queryable_env.py, and four test files are reflowed with no functional relationship to session-resume. I verified these are behaviour-preserving (string-concat collapses to identical strings, ternary/line reflows) and that the except (X, Y): → except X, Y: rewrites are the project's own ruff format output under target-version = py314 (reproduced locally — so it is house style, not a defect, and not the Python-2 bug it superficially resembles). Not blocking. But this churn bloats the review surface and risks merge conflicts with the sibling stacked slices (5, 7, …) that may touch the same files. Prefer isolating repo-wide reformatting into its own PR rather than folding it into a feature substrate change.
Summary
The feature is correctly scoped and wired for what slice-6 owns; the read/decide half is intentionally deferred to slice-8. Recommend landing tests for session.py (finding 1) and the read-path diagnostic log (finding 2) before slice-8, and splitting out the formatting churn (finding 3) going forward. No changes are required for merge-safety of this slice.
— Authored by egg
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.
Conflict Resolution SummaryResolved merge conflicts with
DetailsOnly one file conflicted. It is a slice-4 test file that this slice-6 PR does not functionally touch (slice-6 is session-resume plumbing in
Resolution: accepted the base branch's refactored version in full ( The other affected files ( Verification
Please review: Confirm the slice-5 refactor of — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract Verification — Re-review (incremental)
Verdict: Approve. All slice-6 acceptance criteria are met; the only change since my prior review at 11606eb is a base-merge with no regression to PR-authored code.
Delta since last review
My previous review commit (11606eb) was rebased out of the branch and is no longer reachable. The sole commit the PR branch carries over the current base (egg/issue-3200/slice-5, bffef2b) is the merge commit 12012926 — "Merge origin/egg/issue-3200/slice-5 into egg/issue-3200/slice-6". Its conflict resolution in shared/egg_anchor/tests/test_protected_root.py matches the base verbatim (file is absent from the base..PR diff), so the merge introduced no change to PR-authored code. Net PR content vs. base is unchanged in substance: client.py, session.py, __main__.py, runner.py, test_client_resume.py, plus BRC-history artifacts.
task-6-1 — session-resume plumbing (client.py) ✓
run_agent_asyncexposesresume: str | None = None, threaded through tooptions.resume(client.py:407-446).fork_sessionis deliberately left unset so the session continues rather than branching.- Opt-in / default-OFF is two-fold: the param defaults to
Noneand a non-empty id is only honored whensession_resume_enabled()(EGG_SESSION_RESUME) is true; otherwise it logssession_resume_skippedand cold-starts. - Cold-start fallback never raises: absent/empty/disabled session →
options.resumesimply unset → fresh seed from the protected root. runner.pyand__main__.pythreadresumestraight through;__main__.pyalso wires the write side (write_session_state) as best-effort (never alters exit code).
task-6-2 — tests ✓
tests/shared/egg_agent/test_client_resume.py asserts all three required behaviours: resume-by-id (flag on → options.resume set), default-off (param signature + id ignored when flag unset), and cold-start fallback (resume=None / "" → no resume, no exception). I ran the file locally: 6 passed.
Supporting substrate — session.py
session.py is the write-side round-trip for the slice-8 gate (out of slice-6's strict AC). It correctly takes no resume-vs-reseed decision and degrades every failure mode (no path / missing / empty / malformed / no session_id) to None (cold-start). I manually exercised the round-trip — write/read, empty-id no-write, missing-file → None, malformed-JSON → None, bool-occupancy coercion — all behave as documented.
Note on except ValueError, TypeError: (session.py:147): this is valid PEP 758 parenthesis-less except-tuple syntax — verified it catches both types with no name rebinding on the project's required Python ≥3.14 (pyproject.toml: requires-python = ">=3.14", CI runs 3.14). Not a bug.
Non-blocking observations
session.pyand the__main__.pywrite-side wiring have no direct unit tests (onlyclient.pyis covered). Slice-6's AC is client-centric so this isn't an AC gap, but aread/write_session_stateround-trip test would harden the slice-8 prerequisite. Manually verified here.
Contract state
The on-disk contract snapshot (.egg-state/contracts/issue-3200.json) was read-only and the orchestrator was unreachable this session, so no verify-criterion write was possible; the contract also carries no top-level ac-N ids (criteria are per-task strings). All task-level acceptance criteria for slice-6 are objectively satisfied.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3243 — Session-resume substrate (slice-6/10)
Re-reviewed the delta since my last review at 11606eb5. No blocking issues. The merge is correctly resolved and CI confirms it end-to-end.
What actually changed since 11606eb5
The only PR-authored change is the merge commit 12012926, which (a) advances the base to the current egg/issue-3200/slice-5 tip and (b) resolves a single conflict in shared/egg_anchor/tests/test_protected_root.py. The slice-6 feature files themselves are byte-identical to what I previously reviewed — verified:
git diff 11606eb5 12012926 -- shared/egg_agent/session.py shared/egg_agent/client.py \
shared/egg_agent/__main__.py sandbox/llm/claude/runner.py \
tests/shared/egg_agent/test_client_resume.py # → empty
So client.py resume plumbing, session.py, __main__.py write-back, and test_client_resume.py are unchanged. My prior end-to-end verification of the resume path stands.
Conflict resolution is correct — verified, not assumed
The merge adopted slice-5's refactored test_protected_root.py wholesale (git checkout --theirs). I confirmed:
- The adopted file is exactly the base version —
git diff origin/egg/issue-3200/slice-5 12012926 -- shared/egg_anchor/tests/test_protected_root.pyis empty. Nothing slice-6-specific was grafted in. - It is a self-consistent (renderer, test) pair. HEAD's
protected_root.pyis also identical to slice-5's, and the test's keyword call sites (role,role_contract,task_description,derived,directives,caps) match the mergedrender_protected_root(*, ...)signature atprotected_root.py:195. The refactor correctly drops the obsolete skip-guard locator machinery now that the renderer is merged. - No slice-6 functional change was lost. Slice-6 only ever carried an inherited pre-refactor copy of this slice-4 test file (its sole delta was a formatting commit that had even reflowed soon-to-be-deleted dead code). Discarding that in favor of slice-5's clean refactor is the right call — and it incidentally clears finding #3 (formatting churn) from my prior review for this file.
- CI agrees. Full rollup is green —
Unit Tests,Python,Integration Testsall SUCCESS — so the merged test passes against the merged renderer. The cross-module pair is wired, not dead-ending.
Carried-forward non-blocking notes (unchanged, not gating this slice)
Because the slice-6 files didn't change, my two prior non-blocking findings still stand and remain deferred to slice-8, as designed:
shared/egg_agent/session.pystill has no direct unit tests — the write side is live in production today (__main__.pycallswrite_session_statewhenever$EGG_SESSION_STATE_FILEis set), and this slice is a declared prereq of slice-8's reseed gate. A focusedtest_session.py(round-trip, eachread_session_statefallback branch,_coerce_occupancybool/non-int handling, blank-path resolution, atomic-write OSError) should land before slice-8 builds on it.read_session_stateswallows corrupt/unreadable state silently with no log, asymmetric with the write side'slogger.warning. Fine as the cold-start default; recommend alogger.debugon the corrupt branch before slice-8 wires the read path, so a silently-cold-starting operator has something to diagnose.
Neither gates merge-safety of this slice. The delta is a clean, correctly-resolved base sync.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed — commit
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Contract re-verification — PR #3243 (slice-6, session-resume substrate)
Verdict: approve. Incremental re-review of the delta since my last pass at 12012926. The single new commit f6aebf3d ("Address review: session.py tests + read-path diagnostics") is purely additive and introduces no contract violations or regressions.
Delta reviewed
git log 12012926..HEAD --not origin/egg/issue-3200/slice-5 → one PR-authored commit touching three files (+398/−1):
shared/egg_agent/session.py(+32/−1)tests/shared/egg_agent/test_session.py(new, +289)tests/shared/egg_agent/test_main.py(+78)
What changed and why it complies
- Read-path diagnostics (
read_session_state):except OSErroris now split into a quietFileNotFoundErrorbranch (the benign first-invocation / pre-write path) and alogger.debugon any otherOSError(genuinely unreadable file). The three corrupt-record branches (malformed JSON, non-object payload, unusablesession_id) each emit alogger.debug, while the benign no-path / no-file branches stay silent. This mirrors the existing write-sidelogger.warningand gives an operator who enabledEGG_SESSION_RESUMEa diagnostic trail when they're silently cold-starting. The module's load-bearing contract — "never raise; every failure cold-starts toNone" — is unchanged. - Tests directly pin the slice-6 task-6-2 acceptance criteria (resume-by-id, cold-start fallback, default-off, no exception on the missing-session path):
test_session.pyexercises every defensive branch (corrupt/empty/non-object JSON, unreadable file, missing/blank/non-strsession_id, thebool-is-not-occupancy coercion rule, blank-env-var-is-unset, atomic-write OS-error degradation) plus the new diagnostic logging (anomalous reads log; benign reads stay quiet);test_main.pylocks the--resume/--session-state-fileCLI wiring and proves the best-effort write-back is decoupled from the run's exit code.
Verification performed
- Ran the two new test files against the PR HEAD (
f6aebf3d): 78 passed. - Confirmed no previously verified behavior regressed: the round-trip and cold-start-fallback paths still hold (covered by the passing suite).
- Confirmed the delta touches only slice-6 substrate files — no orphaned changes outside the contract's
files_affectedfor slice-6 (shared/egg_agent/client.py,sandbox/,shared/egg_agent/tests/,sandbox/tests/; the tests live undertests/shared/egg_agent/which is this repo's actual test root for the package).
Note: the contract carries no top-level acceptance_criteria entries (acceptance_criteria: []) — criteria are embedded per-task within each slice — so there are no ac-N markers to flip; verification is recorded here.
Non-blocking suggestion
session.py:162 uses except ValueError, TypeError:. Python 3.14 (the pinned interpreter) accepts the unparenthesized exception tuple and correctly catches both types — I verified the runtime semantics, and test_malformed_json_logs_debug covers it — so this is functionally correct, not a bug. However, the conventional and version-robust form is except (ValueError, TypeError):. The parenthesized form reads unambiguously and avoids visual collision with the legacy Python-2 except E, name: binding syntax. Consider tightening it in a follow-up; not a merge blocker.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3243 — Session-resume substrate (slice-6/10)
Re-reviewed the delta against the prior 12012926 review. That commit was rebased away (the branch is now a single orphan commit f6aebf3), so I reconstructed the delta by diffing the source files against the PR base origin/egg/issue-3200/slice-5. The only non-.egg-state/ source changes are exactly the seven slice-6 files — no stray reformatting remains, confirming the author's disagreement on prior finding #3.
No blocking issues. All three concerns from the prior reviews are properly — not superficially — addressed, and CI is fully green (Unit Tests, Python, Integration Tests all SUCCESS).
Prior feedback — verified addressed
-
session.pyhad zero direct tests →tests/shared/egg_agent/test_session.py(69 cases). These exercise the real production helpers (read_session_state/write_session_state/_coerce_occupancyover real temp files and real env vars) — no self-seeding goldens, no hand-built fixtures bypassing the production path, no name-vs-behaviour contradictions. I ran it in isolation: 69 passed. Coverage is genuinely complete: round-trip (incl. parent-dir creation + env-driven path), everyread_session_statecold-start branch (no path / missing / empty / whitespace / malformed JSON / non-object / missing-blank-nonstrsession_id/OSError),_coerce_occupancyint/bool/non-int incl. the bool round-trip, blank-path resolution, atomic-writeOSErrordegradation, and the no-stray-temp-file assertion. -
read_session_stateswallowed every failure silently →logger.debugnow fires on the anomalous branches (unreadable file, malformed/non-object JSON, unusablesession_id) while the benign no-path /FileNotFoundErrorbranches stay quiet — correctly asymmetric, mirroring the write side.TestReadDiagnosticsasserts both the log-on-anomaly and silence-on-benign halves. I verified each branch directly. -
__main__.pyCLI plumbing uncovered →TestSessionStatePlumbing(5 cases) pins--resumethreading torun_agent, theNonedefault, the write-back persistingsession_id+ occupancy with the rightpath, and — importantly — that awrite_session_statefailure (returncode=3) does not alter the exit code. 5 passed in isolation.
I also independently confirmed:
- The
except ValueError, TypeError:atsession.py:147is valid PEP 758 syntax on the project's Python 3.14.6 (requires-python = ">=3.14", CI on 3.14): it parses, catches both types, and does not rebindTypeError.json.loadsraisesJSONDecodeError(aValueErrorsubclass) → caught → logs → returnsNone. Not a bug. AgentResultcarries bothsession_idandwindow_occupancy, so the__main__.pywrite-back is wired to real fields.
Non-blocking (suggestion) — stdlib-logging fallback in session.py can raise, breaking the "never raise" contract
session.py:39-47 falls back to a bare logging.getLogger(__name__) when egg_logging is unavailable, but every logger.debug(...) / logger.warning(...) call in the module passes structured kwargs (event_type=, event_subtype=, error=). The stdlib Logger rejects those:
>>> logging.getLogger("x").debug("m", event_type="system")
TypeError: Logger._log() got an unexpected keyword argument 'event_type'
So in the fallback path, hitting an anomalous read branch (or a write OSError) raises TypeError instead of returning None/False — directly contradicting the module's emphatic core contract ("Cold-start fallback — never a hard failure"). The sibling client.py solves exactly this with _StdlibLoggerAdapter (which drops unknown kwargs); session.py should reuse that adapter rather than the bare logger. tool_output_cap.py gets away with the bare fallback only because it logs a plain string with no kwargs.
Why non-blocking: the fallback is # pragma: no cover - stdlib fallback outside the sandbox, and egg_logging is always present in the sandbox/CI where this module actually runs (and the feature is default-OFF on top of that). The broken path is unreachable in the real deployment. But it's a genuine inconsistency with client.py in code this PR introduces, and it silently defeats the never-raise guarantee the module is built around — worth a one-line fix (swap the fallback to _StdlibLoggerAdapter) before slice-8 wires the read path into a hotter path.
Summary
The delta is a clean, well-tested response to prior feedback. The substrate is correct, defensively coded, default-OFF, and the new tests genuinely exercise the production helpers. Approving with the single non-blocking suggestion above.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
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.
There was a problem hiding this comment.
Contract re-verification — PR #3243 (issue-3200, slice-6)
Verdict: Approve. The delta since my last review cleanly addresses my prior concern, and both slice-6 tasks remain fully and correctly implemented. No contract violations, no regressions.
Re-review scope note
The commit I last reviewed (f6aebf3d) is no longer reachable — the PR branch was squashed/rebased and now carries a single commit (636789f7) on top of base egg/issue-3200/slice-5. I therefore re-verified the full PR delta against the base, focusing on the change described in the HEAD commit: the kwarg-safe stdlib logger fallback.
Delta verified (addresses prior review)
shared/egg_agent/_logging.py(new):_StdlibLoggerAdapter+resolve_logger()extracted sosession.pyreuses the kwarg-dropping fallback instead of a barelogging.getLogger. My prior concern — thatsession.py's structuredevent_type=/error=kwargs would raiseTypeErroron the stdlib logger outside the sandbox and defeat the never-raise contract — is resolved. Verified directly against the worktree module: the adapter swallowsevent_type/event_subtype/session_id/errorkwargs without raising.client.py: now resolves its logger viaresolve_logger("egg-agent", __name__)— no behavior change; the inlined adapter was removed in favor of the shared helper.tests/.../test_client.py: import updated to the adapter's new home.tests/.../test_session.py: newTestStdlibLoggerFallbackpins that the anomalous read branches and the writeOSErrorbranch still cold-start toNone/Falseunder the stdlib adapter rather than raising.
task-6-1 — resume plumbing + cold-start fallback ✅
run_agent_async(..., resume: str | None = None)threads through the event-pump path (runner.py,__main__.py --resume/--session-state-file,run_agentvia**kwargs).- Opt-in / default-OFF:
options.resumeis set only when the id is non-empty andsession_resume_enabled()(EGG_SESSION_RESUME) is true;fork_sessiondeliberately left unset so the session continues. - Cold-start fallback never raises: omitted / disabled / missing session →
options.resumeunset → fresh seed. Thesession.pyround-trip is uniformly defensive (missing/empty/corrupt/non-object/no-session_idall →None; write failures →False). Verified directly against the worktreesession.py: round-trip,bool-occupancy→Nonecoercion, empty-sid no-op, and theexcept (ValueError, TypeError)branch (PEP 758 form, consistent with repo idiom underrequires-python >=3.14) all behave as specified.
task-6-2 — tests ✅
test_client_resume.py asserts default-off signature, resume-by-id→options.resume when enabled, id-ignored-when-disabled, default-fresh, resume=None cold-start, and empty-id cold-start. test_session.py (35 cases) and test_main.py cover the round-trip, defensive read/write branches, diagnostics, and the __main__ wiring. All three required behaviors (resume-by-id, cold-start-fallback, default-off) plus the no-raise contract are asserted. Per review conventions I did not run the full suite (CI green gates this review); I validated the worktree modules' contracts directly.
Contract bookkeeping
Orchestrator was unreachable and .egg-state/contracts/ is read-only, so I read issue-3200.json directly. Slice-6 defines its criteria as per-task acceptance_criteria strings rather than top-level ac-N entries, so there are no verify-criterion markers to set; both task criteria are met as detailed above.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review feedback addressed — commit
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: PR #3243 — Session-resume substrate (slice-6/10)
No blocking issues. Approving. The delta since my last review (commit 636789f → e9e1f761) is a single, well-scoped fix that correctly addresses the one non-blocking observation from my prior approve-with-suggestions.
Prior feedback — verified addressed
My prior pass raised: resolve_logger no longer guarded the get_logger(...) call itself — only the from egg_logging import get_logger line sat inside the try, so an ImportError raised from within get_logger (e.g. a lazy sub-import) would propagate rather than fall back, a behaviour change vs. the original client.py which wrapped both the import and the call in the same try/except ImportError.
Commit e9e1f761 fixes this properly — shared/egg_agent/_logging.py:52 now reads:
try:
from egg_logging import get_logger
return get_logger(structured_name)
except ImportError: # pragma: no cover - stdlib fallback outside the sandbox
return _StdlibLoggerAdapter(fallback_name)The get_logger(structured_name) call is now inside the try, so any ImportError — top-level package absence or a lazy sub-import within get_logger — cold-starts to _StdlibLoggerAdapter rather than escaping. This restores the behaviour-preserving intent of the client.py extraction (where both the import and the call were guarded together). Not a superficial fix — it is the exact change the observation called for.
Verification
- Scope is exactly one line.
git diff 636789f..e9e1f761 --statconfirms the only change isshared/egg_agent/_logging.py(+2/-1); no other files touched, no scope creep. - No new failure mode introduced. Only
ImportErroris caught — graceful degradation appropriate for a "never raise" contract module. Non-ImportErrorexceptions fromget_loggerstill propagate, identical to before the change. # pragma: no coveris justified. The fallback branch is fundamentally untestable in-sandbox/CI, whereegg_loggingis always present; this matches the pre-existing pragma rationale.- CI fully green on this HEAD — Python, Unit Tests, Integration Tests, Security Scan, Docker, lint all SUCCESS.
except ValueError, TypeError: (session.py) — not re-raised
Already adjudicated across prior rounds and acknowledged in my last pass as "not new in this delta; not blocking": valid PEP 758 on the project's Python ≥3.14, runtime-correct, covered by test_malformed_json_logs_debug, and ruff format rewrites the parenthesized form back to the bare one. The author's disagree disposition stands. No action.
Summary
Clean, minimal, correct response to the prior finding. The never-raise invariant on the egg_logging-less path is now fully restored, the import graph stays acyclic, and CI is green. Approving.
— Authored by egg
There was a problem hiding this comment.
Contract Re-Verification — incremental (PR #3243, slice-6)
Delta reviewed: last-reviewed commit 636789f7 → HEAD e9e1f761.
The prior PR-branch history was folded into the rebased base (egg/issue-3200/slice-5), so the only change since my last review is the single follow-up commit:
e9e1f761 Address review: guard get_logger() call in resolve_logger fallback
shared/egg_agent/_logging.py | 3 ++- (+2 / -1)
Change analysis — resolve_logger()
The get_logger(structured_name) call was moved inside the try block:
try:
from egg_logging import get_logger
return get_logger(structured_name)
except ImportError: # pragma: no cover - stdlib fallback outside the sandbox
return _StdlibLoggerAdapter(fallback_name)Verified correct. This is behavior-preserving for the normal (in-sandbox) path and strictly more robust: an ImportError raised from within get_logger (e.g. a lazy sub-import) now degrades to the stdlib adapter rather than propagating. This restores the intent of the original client.py extraction, where the import and the call were guarded together. The widened catch is intentional and scoped to ImportError only; the never-raise fallback contract holds.
Regression check — previously verified criteria
- task-6-1 (resume-by-session_id plumbing in
client.py, opt-in default-OFF viaEGG_SESSION_RESUME/session_resume_enabled(), cold-start fallback that never hard-fails): the resume substrate atclient.py:385-416and thesession.pygate are untouched by this delta. No regression. - task-6-2 (resume-by-id / cold-start-fallback / default-off tests): test files (
test_client_resume.py,test_session.py,test_main.py) are unchanged by this delta._StdlibLoggerAdapterretains coverage intest_client.py:552andtest_session.py:288. The reworkedImportErrorbranch is correctly marked# pragma: no cover(only reachable outside the sandbox).
CI gates this review on green checks (wait-for-checks), so I did not re-run the suite.
Note on criterion marking
egg-contract verify-criterion could not be run — the orchestrator is UNREACHABLE this invocation (egg-orch health → UNREACHABLE; gateway OK). The locally-persisted contract snapshot (.egg-state/contracts/issue-3200.json) carries slice-6 acceptance criteria as prose on task-6-1/task-6-2, with no top-level ac-N ids to mark. The slice-6 implementation continues to satisfy those task criteria after this delta.
Verdict
The delta is clean, correct, and contract-compliant with no regressions. Approve (posted as comment per self-authored-PR convention).
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR. |
bffef2b to
bf739ed
Compare
…ce-6, task-6-1) Add the warm-resume substrate for the BRC event-pump so a re-invocation can re-enter the prior Claude session by session_id instead of re-seeding from scratch. Opt-in and default OFF (EGG_SESSION_RESUME) so it ships dark ahead of the slice-8 resume-vs-reseed gate. - shared/egg_agent/session.py: session_resume_enabled() flag + atomic, never-raising read/write of the cross-invocation session-state record (session_id + window_occupancy). Every failure mode collapses to the cold-start signal. - client.py: run_agent_async(resume=...) -> options.resume, gated on the flag; absent/disabled/stale session cold-starts (no fork_session -> continues). - __main__.py: --resume input + --session-state-file write-back round-trip. - sandbox/llm/claude/runner.py: thread resume through. The resume-vs-reseed DECISION (occupancy vs threshold) is slice-8; this is only the substrate that makes resume possible. Tests are task-6-2 (tester). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(slice-6, task-6-2) Tests for task-6-1's resume substrate in shared/egg_agent/client.py. Pins the two-fold default-off contract: run_agent_async grows a keyword-only resume=<session_id> (default None), and a passed-in id is only threaded to ClaudeAgentOptions.resume when EGG_SESSION_RESUME is enabled. A falsy id (None / empty) cold-starts a fresh session from the protected-root prompt without raising, even with the flag on. 6 tests: signature default-off; resume-by-id when enabled; id ignored when the flag is off (staged rollout); no-arg fresh; resume=None cold-start; empty-id cold-start. Mock SDK per the test_client.py convention; EGG_MCP_TOOLS=false + intercept_tools=False keep the plain SDK path. Green on top of coder commit 5bf7083. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add direct unit tests for egg_agent.session (test_session.py) covering the write/read round-trip, every read_session_state cold-start fallback branch, _coerce_occupancy bool/non-int handling, blank-path resolution, and the atomic write's OSError degradation — the write side is live in production and a hard prereq of the slice-8 reseed gate. Add a logger.debug on read_session_state's anomalous branches (unreadable file, malformed/non-object JSON, unusable session_id) so an operator with EGG_SESSION_RESUME on but silently cold-starting has a diagnostic trail; the benign no-path / no-file cases stay quiet. Mirrors the write-side warning. Add TestSessionStatePlumbing to test_main.py covering the new --resume / --session-state-file CLI args and the best-effort write_session_state write-back.
Extract _StdlibLoggerAdapter and the egg_logging-vs-stdlib resolution into egg_agent._logging so session.py reuses the kwarg-dropping fallback instead of a bare logging.getLogger. The bare logger raised TypeError on the structured event_type=/error= kwargs the read/write diagnostics pass, defeating session.py's never-raise contract outside the sandbox. client.py now resolves its logger through the same helper (no behaviour change), and test_client.py imports the adapter from its new home. Adds TestStdlibLoggerFallback to test_session.py pinning that the anomalous read branches and the write OSError branch still cold-start to None/False under the stdlib adapter rather than raising.
Move the get_logger(structured_name) call inside the try block so an ImportError raised from within get_logger (e.g. a lazy sub-import) falls back to the stdlib adapter rather than propagating. Restores the behaviour-preserving intent of the client.py extraction, where both the import and the get_logger call were guarded together.
e9e1f76 to
83b24ef
Compare
There was a problem hiding this comment.
Re-review: PR #3243 — Session-resume substrate (slice-6/10)
No blocking issues. The prior finding is properly fixed and I independently re-verified the full changeset against the production execution model. One minor non-blocking observation below.
Prior feedback — verified addressed
My prior pass asked for the get_logger(...) call to be guarded inside the try in resolve_logger, restoring the behaviour-preserving intent of the client.py extraction. Confirmed fixed at shared/egg_agent/_logging.py:51-55:
try:
from egg_logging import get_logger
return get_logger(structured_name)
except ImportError: # pragma: no cover - stdlib fallback outside the sandbox
return _StdlibLoggerAdapter(fallback_name)Both the import and the call are now in the same try, so an ImportError from a lazy sub-import within get_logger degrades to _StdlibLoggerAdapter rather than propagating. Not superficial — exactly the change the observation called for. The catch stays scoped to ImportError, so non-import failures still surface.
Independent verification of the changeset
except ValueError, TypeError:(session.py:166) — valid PEP 758. Confirmedrequires-python = ">=3.14"(pyproject.toml:7) and the runtime here is 3.14.6;ast.parsesucceeds and a corrupt-JSON read returnsNoneat runtime (smoke-tested). Already adjudicated across prior rounds; no action.- Import graph is acyclic.
client → session → _logging,client → _logging,__main__ → {client, session}. The SDK-heavy import inclient.pyis function-local, sosession.py/_logging.pyimport without pulling the SDK. Verified by importing both under system Python (no SDK present). - Resume gate is two-fold and default-OFF.
client.py:404-419:options.resumeis set only whenresumeis a non-empty id andsession_resume_enabled()(EGG_SESSION_RESUME).fork_sessiondeliberately left unset so a resumed session continues rather than branches. Cold-start (omitted/disabled/empty/missing session) simply doesn't setoptions.resume— never raises. Substrate ships dark; the resume-vs-reseed decision is correctly deferred to slice-8. - Round-trip is correct and defensive.
write_session_stateis atomic (temp +os.replace), strips/validatessession_id, coerces occupancy (bools excluded), and swallows OS errors toFalse.read_session_statecollapses every failure mode (no path, missing/empty/corrupt/non-object/unusable-id) toNonewithlogger.debugon the anomalous branches and silence on the benign ones. Smoke-tested all of these. - Tests exercise the production path.
test_session.pydrives realwrite/readround-trips (no hand-built fixtures bypassing the helpers);test_client_resume.pycalls the realrun_agent_asyncand asserts on the actualClaudeAgentOptions.resumehanded toquery();test_main.pydrivesmain()end-to-end and pins the write-back wiring incl. the "persistence failure must not change exit code" contract. No self-seeding goldens, no name-vs-behaviour contradictions.
Non-blocking observation
Temp-file leak on the rare replace() error path (session.py:write_session_state). The NamedTemporaryFile(delete=False) is created inside the try; if tmp_path.replace(resolved) raises OSError after the temp exists, the except OSError returns False but the .{name}.*.tmp file is left on disk. This is a same-directory rename so the path is very rare, and the module is best-effort bookkeeping — non-blocking. If you want to harden it, unlink the temp in the except (or a finally) before returning False. test_write_is_atomic_no_temp_left covers the success path but not this error path.
Verdict
Clean, minimal, well-tested substrate with a correct staged-rollout gate and a never-raise contract that holds end-to-end. Approve (posted as comment per self-authored-PR convention).
— Authored by egg
|
egg review completed. View run logs 25 previous review(s) hidden. |
There was a problem hiding this comment.
Contract re-verification (incremental) — PR #3243, slice-6
Status note: This PR is already MERGED (merge commit a828299, merged by @jwbron). This review is informational; it documents the incremental verification of the post-review delta for the contract record.
Delta reviewed
The last-reviewed commit e9e1f76 was squashed out of the remote on merge and is no longer fetchable, so I verified the post-review delta from its commit message and the final state at HEAD 83b24ef:
83b24ef— "Address review: guardget_logger()call inresolve_loggerfallback"
Verified correct. In shared/egg_agent/_logging.py, resolve_logger() now wraps both the from egg_logging import get_logger import and the return get_logger(structured_name) call inside the single try, with except ImportError returning _StdlibLoggerAdapter. So an ImportError raised from a lazy sub-import inside get_logger now falls back to the stdlib adapter instead of propagating — restoring the behaviour-preserving intent of the client.py logger extraction. No new symbols, no behavioural change on the happy path.
No regression on previously-verified slice-6 criteria
task-6-1 (resume plumbing + cold-start fallback, opt-in default-off) — still intact:
shared/egg_agent/client.py:228—run_agent_async(..., resume: str | None = None)(param-level default-off).client.py:404-406—options.resumeset only whenresume_session_id and session_resume_enabled()(flag-gated onEGG_SESSION_RESUME).client.py:397-419— cold-start path: omitted/disabled/empty id → fresh session from the protected root, never raises.shared/egg_agent/session.py:74—session_resume_enabled()gates onEGG_SESSION_RESUME, default OFF.
task-6-2 (tests) — still intact and green:
tests/shared/egg_agent/test_client_resume.py— 6/6 passing: default-off signature, resume-by-id-when-enabled, id-ignored-when-flag-disabled, fresh-without-resume, resume=None cold-start, empty-id cold-start.
Test run
test_client_resume.py: 6 passed.test_client.py+test_session.py: 159 passed, 1 failed.- The lone failure —
test_client.py::TestBufferOverflowErrorHandling::test_buffer_overflow_returns_failure_with_marker(CLIJSONDecodeError.__init__() missing 1 required positional argument: 'original_error') — is a pre-existing SDK-signature mismatch under Python 3.14, explicitly documented in the contract's task-6-2 notes as unrelated to slice-6. Not introduced or worsened by this PR.
Verdict
No contract violations. The post-review delta (83b24ef) is a correct, behaviour-preserving fix; slice-6 task-6-1 and task-6-2 acceptance criteria remain satisfied. Posting as a comment (self-authored + already-merged PR; egg-contract verify-criterion is unavailable — orchestrator unreachable — and this contract tracks acceptance criteria at the task level rather than as top-level ac-N).
— Authored by egg
|
egg contract-verification completed. View run logs 25 previous review(s) hidden. |
…oc-updater] (#3266) * docs: add session.py, _logging.py to STRUCTURE.md; log event table Document new egg_agent modules from #3200 slice-6: - STRUCTURE.md: add session.py and _logging.py entries to the exhaustive egg_agent/ listing (doc mandates backfill for new modules) - logging.md: add `system` event_type row (session_resume / session_resume_skipped) to the Agent SDK Structured Events table Triggered by: #3243 Authored-by: egg * Backfill queryable_env.py in STRUCTURE.md; broaden system event row * Fix output_cap_deny mis-attribution in logging.md system row --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@localhost>
1 participant
Land resume=<session_id> plumbing in client.py + the no-warm-session cold-start fallback (fresh seed from the protected root). Closes the second substrate gap the NACK flagged (0 resume hits in client.py; #3186 OPEN). Logical dep: none; serialized after slice 5 (#3046). Hard prereq of the reseed (slice 8).
Base PR: #3234
What's in this PR
Commits (3):
This slice
Session-resume substrate (#3186) + cold-start fallback (NEW - fixes B2)
Files affected:
shared/egg_agent/client.pysandbox/shared/egg_agent/tests/sandbox/tests/Tasks (2) + acceptance criteria
Stack
issue-3200egg/issue-3200/slice-5