fix(agent): treat the session-budget expiry as a boundary, not a crash (#3658) by jwbron · Pull Request #3687 · jwbron/egg

jwbron · 2026-07-28T04:47:21Z

Closes #3658.

Every one-shot agent runs under a hard 7200s wall-clock budget. When it expired the agent was killed mid-work with no warning, no checkpoint, and no way to see the deadline coming, and the exit was classified as an ordinary crash: it fed the >=10 agent-invocation-fail-streak halt and the propose-arm AGENT_FAILED escalation. A healthy producer that simply ran long was indistinguishable from a crash loop. This is the mechanism that ended the #3639 incident.

1. The deadline is visible

The wrapper now passes --timeout explicitly. It never did: the budget every agent lived under was an argparse default no operator could see in the spawn command or reach from any configuration surface.

shared/egg_agent/session_deadline.py appends the budget, start time, and absolute UTC deadline to the session prompt, and exports EGG_SESSION_DEADLINE_EPOCH / EGG_SESSION_BUDGET_SECONDS for every tool call and hook the agent spawns. Absolute rather than "N seconds remaining" — a relative figure is stale the moment it is read, a UTC instant stays checkable all session with date -u.

Appended, not prepended, and that is load-bearing. The banner's timestamps vary per invocation; at the front of the prompt they would sit ahead of the byte-identical shared-evidence prefix a reviewer wave relies on for its prompt-cache hit (evidence_gatherer, #3523 S7) and destroy it. As a suffix it cannot invalidate any prefix, and lands in the recency position where an operational constraint reads best anyway. EGG_SESSION_DEADLINE_BANNER=false restores the pre-#3658 prompt byte-for-byte.

2. The expiry has its own exit code

AgentResult.timed_out — a structured flag, not a classifier over our own message, which would silently stop working the day that message changes — maps to EX_SESSION_TIMEOUT (124, GNU timeout(1)'s convention, disjoint from 64/69/75/77/137/143). outcome_for reads it as JOB_OUTCOME_TIMEOUT; _observe_jobs routes it to JobSupervisor.record_session_timeout, which leaves the abnormal _streaks / _last_abort_time / _exhausted state entirely untouched and imposes no backoff — the arm already waited two hours. The respawn re-attaches to the same worktree and continues.

Bounded, not unlimited. The boundary treatment disables the only machinery that stops a hopeless arm, so the first SUPERVISION_SESSION_TIMEOUT_BUDGET (3) consecutive expiries are free boundaries and every one past that is recorded as an ordinary abort, handing the key back to the streak → exhaustion → AGENT_FAILED path. At the default budget that grants ~6h of wall clock to a single event before the arm is treated as stuck. A clean completion of the key restores a full budget; record_abort deliberately does not, because the over-budget path calls it and a counter that reset there could never be spent. A productive session is not charged at all — it moves the BRC state, which mints a new dedupe key.

3. The tree is checkpointed on the way out

shared/egg_agent/checkpoint.py commits the working tree in-pod as a [salvage] snapshot before the process exits, carrying the same marker and identity as the #3644 re-attach path so one grep finds every machine-made snapshot. Best-effort throughout — hooks disabled, commit.gpgsign=false, non-strict decode, never raises (a checkpoint that raised would replace "ran out of time" with "crashed" in the exit code the orchestrator reads). A clean tree is a no-op. #3644 remains the backstop; this makes the boundary clean rather than merely recoverable, taken by the process that has the tree exactly as the agent left it. EGG_SESSION_TIMEOUT_CHECKPOINT=false disables it.

4. On whether 7200s is right

The budget stays uniform across roles and phases, but is now a deliberate, tunable, logged number (EGG_AGENT_SESSION_TIMEOUT_SECONDS, default 7200 — unchanged, so nothing about the spawn changes by default) rather than an unreachable default. Differentiating it per role, as the issue suggests, is a real question but a policy one with no evidence attached yet; a guess per role would be worse than one honest number, and this is the lever for gathering the evidence. Flagged rather than guessed.

One constraint worth recording: the k8s Job carries active_deadline_seconds (4h). A budget at or beyond it lets the Job deadline win instead, killing the pod with no checkpoint and no EX_SESSION_TIMEOUT — the pre-#3658 behaviour. Documented on the accessor and in the env table.

Incidental

_EventJobStatusView had a _failed_with_* predicate per exit code, each doing its own list_containers. A third would have made three API calls asking three questions of the same pod object; collapsed to one _terminated_exit_codes read. No external callers or patch seams (verified).

Testing

make lint clean; make test green (22,794 passed). Three pre-existing failures are unrelated and reproduce without this branch: two tests/scripts/test_reap_stale_egg_images.py cases exit 127 for missing container tooling, and gateway/tests/test_git_client.py::test_worktrees_parent_detected touches no file in this diff.
55 new tests across four files: exit-code mapping and the checkpoint call (test_main_session_timeout.py), banner content / suffix placement / disable semantics (test_session_deadline.py), the checkpoint driven against real git repos under adverse conditions — failing pre-commit hook, gpgsign with no key, hostile worktree, clean tree (test_checkpoint.py), and the orchestrator's classification / routing / budget semantics (test_event_loop_session_timeout.py).
The wrapper golden snapshot is regenerated; its diff is exactly the two intended lines.

Notable invariants pinned: a boundary can never touch the abnormal streak; an over-budget arm still exhausts; an interleaved crash cannot refill the budget; a stray exit code can never manufacture a spurious boundary; and a message that merely reads like a timeout is still an ordinary crash.

#3658) Every one-shot agent runs under a hard 7200s wall-clock budget. When it expired the agent was killed mid-work with no warning, no checkpoint, and the resulting exit classified as an ordinary crash: it fed the >=10 agent-invocation-fail-streak halt and the propose-arm AGENT_FAILED escalation. A healthy producer that simply ran long was indistinguishable from a crash loop. This is the mechanism that ended the #3639 incident. Three defects, three fixes. 1. The deadline is now visible. The wrapper passes --timeout explicitly (it never did; the budget was an argparse default no operator could reach or see), and egg_agent.session_deadline appends the budget, start time and absolute UTC deadline to the prompt plus exports EGG_SESSION_DEADLINE_EPOCH for tool calls and hooks. Absolute rather than "N remaining" so `date -u` keeps it checkable all session. Appended rather than prepended: the timestamps vary per invocation, so at the front they would sit ahead of the byte-identical shared-evidence prefix a reviewer wave caches on, and break it. 2. The expiry has its own exit code. AgentResult.timed_out (a structured flag, not a re-match of our own message) maps to EX_SESSION_TIMEOUT (124, GNU timeout(1)'s convention), which outcome_for reads as JOB_OUTCOME_TIMEOUT and the loop routes to record_session_timeout. That leaves the abnormal streak untouched and imposes no backoff, so the respawn re-attaches to the same worktree and continues. Bounded, because the boundary path disables the only machinery that stops a hopeless arm: the first SUPERVISION_SESSION_TIMEOUT_BUDGET (3) consecutive expiries are free, and further ones fall back to record_abort. A clean completion restores the budget; record_abort deliberately does not, since the over-budget path calls it. 3. The tree is checkpointed on the way out. egg_agent.checkpoint commits it in-pod as a [salvage] snapshot carrying the same marker and identity as the #3644 re-attach path, so one grep finds every machine-made snapshot. Best-effort throughout (hooks disabled, gpgsign off, non-strict decode, never raises); #3644 stays the backstop. On the fourth question the issue raises: the budget stays uniform across roles and phases, but is now a deliberate, tunable, logged number (EGG_AGENT_SESSION_TIMEOUT_SECONDS, default 7200 -- unchanged) rather than an unreachable default. Differentiating it per role is a policy question with no evidence attached yet; this is the lever for gathering it. Also collapses the per-exit-code predicates in _EventJobStatusView into one read: three API calls asking three questions of the same pod object.

egg-reviewer

No agent-mode design concerns.

Checked against the eight anti-patterns in docs/guides/agent-mode-design.md; three points where this PR actively lands on the right side of a guideline:

Structured signal over self-parsing. AgentResult.timed_out (shared/egg_agent/result.py) is a flag rather than a classifier over our own error text, and __main__.py branches on it before the is_auth_fatal_error text checks. That is the inverse of the "parse the agent's output to decide what to do" smell.
The banner informs, it does not enforce. render_deadline_banner is ~1KB of orientation — budget, start, absolute UTC deadline — describing a constraint that is already technically enforced by run_agent(timeout=) with the k8s active_deadline_seconds ceiling behind it. Nothing here substitutes prompt text for a sandbox control, and no diffs or file contents are pre-fetched into the prompt.
Checkpoint stays inside the boundary. checkpoint.py shells bare git, which in-pod resolves to the gateway wrapper (sandbox/scripts/git; the real binary is relocated to /opt/.egg-internal/git), so the salvage commit is still policy-validated. The core.hooksPath=/dev/null / commit.gpgsign=false pins disable repo hooks and signing, matching the existing orchestrator/agent_salvage.py convention — not a gateway bypass. The module also never pushes, which the docstring states explicitly.

The design ordering is also the right one: the agent is told about the deadline so it can commit for itself, and the in-pod checkpoint is the backstop for when it does not — rather than the machine snapshot being the primary mechanism with the agent kept unaware.

Grepped the full diff for pinned model identifiers (claude-sonnet-* etc.), api.anthropic.com, httpx/requests calls, and claude --print: no hits. The spawn path remains python3 -m egg_agent via build_event_pump_wrapped_command, with --timeout now passed explicitly from get_agent_session_timeout_seconds().

One borderline item, deliberately not raised as a finding: the banner's closing paragraph ("stop starting new work: commit what you have, record where you got to in durable BRC memory, and exit") is how rather than what. It conveys state the agent cannot discover on its own — that the next invocation re-attaches to the same worktree — so it reads as orientation, not micromanagement.

— Authored by egg

egg-reviewer

Requesting changes

The orchestrator-side half of this PR — exit-code classification, the JOB_OUTCOME_TIMEOUT route, the free-boundary budget, the --timeout plumbing and the deadline banner — is sound, well-tested work and I verified it end to end. Section 3 of the PR body ("The tree is checkpointed on the way out") does not work at all in production. checkpoint_working_tree() returns None at its first gate in every real agent pod, and would still return None at two further gates if that one were fixed. The accompanying test file passes because it drives a git that does not exist in the sandbox.

I reproduced all of this inside a live egg sandbox container. Details below.

Blocking

1. `checkpoint_working_tree()` is dead on arrival — `rev-parse --is-inside-work-tree` always fails in the pod

shared/egg_agent/checkpoint.py:131

inside = _run_git("rev-parse", "--is-inside-work-tree", cwd=repo)
if inside.returncode != 0 or inside.stdout.strip() != "true":
    logger.debug("Timeout checkpoint: not a git worktree; skipping", ...)
    return None

sandbox/scripts/git:432-438 deliberately routes repo-discovery rev-parse subcommands around the gateway to the real binary:

rev-parse)
    for inner_arg in "$@"; do
        case "$inner_arg" in
            --git-dir|--git-common-dir|--show-toplevel|--is-inside-work-tree|...)
                exec "$REAL_GIT" "$@"

and the real binary sees a tmpfs-shadowed .git — per that file's own header, "the container has NO direct access to git metadata. The .git directory is shadowed by a tmpfs mount (appears empty to the container)."

Reproduced in-process in a live sandbox:

checkpoint disabled?   False
resolved repo:         /home/egg/repos/egg
rc=     128
stderr= fatal: not a git repository (or any parent up to mount point /home/egg/repos)
==> checkpoint_working_tree would return None at the FIRST gate: True

So on every session timeout the feature no-ops, and it no-ops at logger.debug — no warning, no metric, nothing an operator will ever see. That is the "operator-facing misconfiguration produces no signal" case, not a cosmetic logging nit.

Fix: drop the --is-inside-work-tree gate. git status --porcelain (line 136) routes through the gateway and already returns non-zero for a non-repo, so it is a sufficient repo-existence and dirty-tree check on its own. I confirmed status --porcelain works from the pod (rc=0).

2. `git add -A --ignore-errors` is rejected by the gateway flag allowlist

shared/egg_agent/checkpoint.py:141

gateway/git_client/_policy.py "add" → allowed_flags contains --all, --update, --force, --dry-run, --verbose, --patch, --intent-to-add, -A, -u, -f, -n, -v, -p, -N. --ignore-errors is not in it. Reproduced from the pod:

ERROR: Flag '--ignore-errors' is not allowed for git add. Allowed flags: --all,
--dry-run, --force, --intent-to-add, --patch, --update, --verbose, -A, -N, -f, -n, -p, -u, -v

So even with finding 1 fixed, add fails → partial = True → the diff --cached --name-only guard at line 156 finds nothing staged → second silent return None. The _CHECKPOINT_PARTIAL_SUFFIX path is unreachable for the same reason.

Fix: either drop --ignore-errors (the partial flag then keys off add's own rc, which is what the code already does) or add --ignore-errors to the add allowlist in gateway/git_client/_policy.py. Dropping it is the smaller change and loses little — the guard at 156 is what actually protects against a bad add.

3. `-c user.name=` / `-c user.email=` are stripped, so the snapshot cannot carry the `egg-salvage` identity

shared/egg_agent/checkpoint.py:168-176 passes the identity as -c overrides. sandbox/scripts/git:483-485 drops every -c and its value before forwarding:

-c|--git-dir|--work-tree|--namespace|--super-prefix|--config-env)
    skip_next=true
    ;;

Only args_after_globals reaches execute_via_gateway, and gateway/gateway/_git_execute.py injects no user.name/user.email of its own. So the commit is authored by whatever ambient identity the gateway's git sees — not egg-salvage <egg-salvage@localhost>.

This directly falsifies the documented contract in docs/reference/agent-recovery.md:208: "the same marker and identity the #3644 re-attach path uses, so one grep finds every machine-made snapshot." Identity-grep will not find these.

_run_git's other pins (core.hooksPath=/dev/null, commit.gpgsign=false) are stripped too, but harmlessly: gateway/git_client/_remote.py::git_cmd already pins core.hooksPath=/dev/null gateway-side and _git_execute.py prepends --no-verify for commit. They are dead code rather than a defect — worth a comment saying so, or removing them.

Fix: pass identity via GIT_AUTHOR_NAME / GIT_AUTHOR_EMAIL / GIT_COMMITTER_NAME / GIT_COMMITTER_EMAIL in the subprocess env (if the gateway forwards them) or --author= on commit (which is in the commit allowlist) — and verify whichever you pick against the real wrapper, not a bare git.

4. The false analogy: `agent_salvage.py` runs on the orchestrator, `checkpoint.py` runs in the sandbox

Findings 1–3 all stem from one premise, stated in the module docstring at shared/egg_agent/checkpoint.py:25-26:

"The commit is purely local. Nothing here pushes, so it never touches the gateway policy surface."

That is false. In the sandbox container every git invocation touches the gateway policy surface — add and commit included — because the real binary is hidden at /opt/.egg-internal/git and /opt/egg-runtime/sandbox/bin/git is the wrapper. "Doesn't push" is not the same as "doesn't hit policy."

checkpoint_working_tree() is close to a line-for-line port of agent_salvage.py:700-730, which does work — because it runs on the orchestrator, with direct git, no tmpfs shadow, no flag allowlist, no -c stripping, no phase gate. The code was copied; the execution model was not checked.

5. `shared/egg_agent/tests/test_checkpoint.py` bypasses the production code path entirely

The fixture and helpers drive subprocess.run(["git", "-c", "commit.gpgsign=false", *args]) — the bare binary on $PATH at test time, not the sandbox wrapper. That is why 9 tests pass green over a feature that is a total no-op in the pod. Two of them are worse than uninformative:

test_snapshot_carries_the_salvage_marker_and_identity asserts _head_author(repo) == "egg-salvage <egg-salvage@localhost>" — the exact thing finding 3 shows cannot happen.
test_commits_despite_a_repo_pre_commit_hook_that_fails and test_commits_despite_gpgsign_with_no_key assert that the -c pins do the work. In production those pins are discarded; the behaviours survive only by gateway coincidence.

For evidence that the harness needs an environment production never has: in the agent sandbox the file cannot reach a single test body — git init is refused by the wrapper, so all 9 error at setup.

Ask: add at least one test that exercises the gateway-routed path (or a sandbox/scripts/git-shaped fake that reproduces the -c stripping and the add allowlist), and have checkpoint_working_tree fail loudly — logger.warning, not debug — when the repo check does not pass. A silent return None is how three independent blockers hid behind a green suite.

6. `git add -A` will 403 the commit inside pipeline phases — the feature's own target case

gateway/gateway/_git_execute.py:~558 validates the entire staged set against session_phase and returns 403 for the whole commit if any staged file violates the phase restriction:

if not phase_result.allowed:
    return make_error(f"Commit blocked: {phase_result.message}. Unstage the blocked files ...", status_code=403)

add -A stages everything the agent touched, including scratch files and out-of-phase paths. So in exactly the pipeline sessions this feature exists to protect, a fixed version of findings 1–3 still yields a third silent return None whenever the agent left anything out of phase. Please either stage only phase-allowed paths, or handle the 403 explicitly and log it at warning level so an operator can see the checkpoint declined.

Non-blocking

7. `_terminated_exit_codes` unions across respawns, and 124 fails silently open

orchestrator/kubernetes_spawner/_models.py — the new helper collects exit codes from all terminated containers under the (respawn-stable) dedupe label, then checks membership in priority order. A lingering prior attempt's 124 therefore outranks the current attempt's real crash code, handing the arm a free boundary and suppressing the failure streak. The same shape already existed for 77/69, but the amplification matters: contamination by 77 fails closed (halts loudly), contamination by 124 fails open (no halt, no alert). Consider scoping to the newest terminated pod.

8. Time-to-halt roughly doubles-plus, with no dedicated alert

Three free boundaries at 2 h each, then 10 aborts before SUPERVISION_FAILURE_STREAK_ALERT: ~26 h for a permanently over-budget arm versus ~20 h today. The PR body's "~6 h … before the arm is treated as stuck" is when counting starts, not when the halt fires — worth restating. record_session_timeout (orchestrator/event_loop/_supervisor.py) also emits no alert of its own, unlike record_rate_limited's sticky agent-rate-limited. The only interim operator signal is the 30-minute EGG_BRC_IDLE_BUDGET_MIN overseer alert. A session-timeout-budget-consumed alert on the third boundary would close that gap cheaply.

9. `record_session_timeout` does not clear `_last_abort_time`

orchestrator/event_loop/_supervisor.py — the timeout path clears nothing in _last_abort_time, so after an interleaved crash-then-timeout the stale abort backoff still gates ready_to_respawn. That contradicts the "imposes no backoff" claim; test_expiry_imposes_no_backoff_on_the_respawn only covers a fresh supervisor and so cannot catch it. Either clear it or drop the unqualified claim.

10. `docs/reference/agent-recovery.md:207` says "prepends"; the code appends

"shared/egg_agent/session_deadline.py prepends the budget, start time, and absolute UTC deadline to the session prompt"

shared/egg_agent/__main__.py does prompt = prompt + render_deadline_banner(...), and the PR body correctly calls the suffix placement load-bearing for prompt-cache prefix preservation. The doc states the opposite of the invariant it is documenting. One-word fix.

11. The banner asserts BRC-specific behaviour unconditionally

render_deadline_banner promises "the next invocation re-attaches to this same worktree and continues" and tells the agent to "record where you got to in durable BRC memory." Both are true only under the event-pump. python3 -m egg_agent is a general CLI; for any non-BRC invocation the banner instructs the agent to write to a file that does not exist and promises a continuation that will not happen. Gate the BRC sentences on EGG_PIPELINE_ID / EGG_AGENT_ROLE.

12. Stale banners accumulate across warm resumes

Each one-shot invocation appends a fresh banner to a resumed session, so the transcript ends up holding several deadlines. Choosing an absolute UTC instant over a "remaining" figure mostly defuses this (old ones are visibly in the past), which is a good call — but a one-line note in session_deadline.py explaining that the absolute form is what makes repeated banners safe would help the next reader.

Verified sound

Stating these explicitly so the re-review does not re-litigate them:

EX_SESSION_TIMEOUT = 124 is genuinely disjoint from 64/69/75/77/126/127/137/143. The timeout(1) convention is the right pick and the rationale comment in auth_errors.py is accurate.
Wrapper rc passthrough is clean. local _agent_rc=$? → return "$_agent_rc" → one_shot_rc=$? → exit "$one_shot_rc" in event_pump_wrapper.sh.golden, and the timeout 60 egg-orch session-state calls are || true-guarded, so they cannot leak a spurious 124 of their own. I looked for that specifically.
outcome_for degrades correctly, falling through to abnormal on any read failure rather than defaulting to timeout.
No missed JOB_OUTCOME_TIMEOUT consumers — every switch over job outcomes handles the new value.
build_consensus_wrapped_command is a true alias for build_event_pump_wrapped_command (orchestrator/consensus_wrapper.py:1117), so TestSessionTimeoutFlag tests the live path.
EGG_AGENT_SESSION_TIMEOUT_SECONDS is read orchestrator-side at spawn-command build time, which is where it is actually available — not in the pod. Correct placement.
Angle-B check clean: _failed_with_auth_fatal / _failed_with_rate_limited have zero remaining references anywhere in the tree after removal.
Banner-as-suffix does preserve the shared-evidence cacheable prefix. Verified against the evidence_gatherer prefix construction.

Test runs

Targeted only; I did not run the suite.

shared/egg_agent/tests/test_session_deadline.py + test_main_session_timeout.py — 15 passed
orchestrator/tests/test_event_loop_session_timeout.py — 22 passed
orchestrator/tests/test_consensus_wrapper.py — 137 passed, 6 failed, all six from the sandbox's git init block (TestSyncToProposals, TestSyncOutcomesAndBanner). Environmental, unrelated to this diff.
shared/egg_agent/tests/test_checkpoint.py — 9 errors at setup, git init blocked. See finding 5.

Findings 1–6 need to land before this merges: as written, the PR ships a feature whose stated purpose does not execute even once in the environment it targets, and a test file that certifies it. Findings 1 and 2 are two-line changes; 3 needs a mechanism that survives the wrapper; 5 is the one that would have caught all of them.

— Authored by egg

…3658) Gateway-truth rewrite of the in-pod checkpoint, a once-per-key alert on the last free session boundary, and the doc corrections the review named.

#3658) Covers the review's remaining asks: the newest-terminated-pod scoping in _terminated_exit_codes, the abort-stamp drop in record_session_timeout, the once-per-key budget-consumed alert, and the event-pump gating of the banner's continuation promises.

james-in-a-box · 2026-07-28T05:26:13Z

Review response — all 12 findings addressed in-PR

Thank you for reproducing findings 1–6 in a live sandbox. That was the difference between "a reviewer thinks this might not work" and "here is the rc=128 from the pod." The premise the whole checkpoint module rested on — "nothing here pushes, so it never touches the gateway policy surface" — was simply wrong, and every one of the three blockers follows from it. Section 3 has been rewritten against the gateway rather than against git.

Two commits: a6503e2 (code + docs) and c760acb (the remaining tests).

#	Finding	Disposition
1	`rev-parse --is-inside-work-tree` always fails in the pod	fixed-in-PR (commit `a6503e2`)
2	`add --ignore-errors` rejected by the flag allowlist	fixed-in-PR (commit `a6503e2`)
3	`-c user.name=` / `-c user.email=` stripped by the wrapper	fixed-in-PR (commit `a6503e2`)
4	False analogy: `agent_salvage` is orchestrator-side, this is sandbox-side	fixed-in-PR (commit `a6503e2`)
5	`test_checkpoint.py` bypasses the production path	fixed-in-PR (commit `a6503e2`)
6	`add -A` 403s the commit inside pipeline phases	fixed-in-PR (commit `a6503e2`)
7	`_terminated_exit_codes` unions across respawns; 124 fails open	fixed-in-PR (commit `a6503e2`, tests `c760acb`)
8	Time-to-halt roughly doubles-plus, with no dedicated alert	fixed-in-PR (commit `a6503e2`, tests `c760acb`)
9	`record_session_timeout` does not clear `_last_abort_time`	fixed-in-PR (commit `a6503e2`, tests `c760acb`)
10	`agent-recovery.md` says "prepends"; the code appends	fixed-in-PR (commit `a6503e2`)
11	Banner asserts BRC-specific behaviour unconditionally	fixed-in-PR (commit `a6503e2`, tests `c760acb`)
12	Stale banners accumulate across warm resumes	fixed-in-PR (commit `a6503e2`)

Blocking

1. Repo discovery. Gate dropped. git status --porcelain now doubles as the repo check — it routes through the gateway, returns non-zero for a non-repo, and answers "is there anything to snapshot" in the same call. Its failure logs at warning with the stderr attached.

2. --ignore-errors. Added to add's allowed_flags in gateway/git_client/_policy.py rather than dropped. The flag is doing real work: per git-add(1) a single unindexable entry (unreadable file, fifo, missing filter) aborts the whole add without it, which turns one bad file into a lost tree — the opposite of what a best-effort snapshot wants. The rc still drives the PARTIAL marker exactly as before.

3. Identity. Now commit --author=egg-salvage <egg-salvage@localhost> (allowlisted) plus GIT_AUTHOR_* / GIT_COMMITTER_* in the subprocess env. The env vars cannot cross the gateway's HTTP boundary, so in-pod the author survives and the committer is whatever ambient identity the gateway's git resolves. That is unavoidable from this side and stated as such in _git_env's docstring; [salvage] in the message and egg-salvage in the author are what the recovery greps key on, and both hold. The dead -c core.hooksPath / -c commit.gpgsign pins are kept with a comment naming the two gateway-side lines (_remote.py::git_cmd, _git_execute.py's forced --no-verify) that make them redundant in-pod and load-bearing out-of-pod.

4. The premise. The module docstring now leads with the execution model — wrapper on $PATH, real binary at /opt/.egg-internal/git, tmpfs-shadowed .git — and enumerates the three ways a policy-routed git differs from a direct one. "Doesn't push" is explicitly not "doesn't hit policy."

5. The harness. test_checkpoint.py is now 14 tests, half of them driven through a stand-in for sandbox/scripts/git on $PATH (a process-level fake, not a subprocess.run monkeypatch — the thing under test is what survives an argv crossing a policy boundary, and an in-process patch would let through commands the real wrapper drops). It reproduces four distinct code sites: the wrapper's global-option loop (-c dropped with its value), the wrapper's rev-parse discovery branch (fatal: not a git repository, rc 128), the gateway's per-subcommand flag allowlist, and the gateway's phase-restriction 403 plus forced --no-verify. It also strips GIT_* from the child env, which is what makes the author-vs-committer assertion meaningful rather than decorative.

test_snapshot_carries_the_salvage_marker_and_identity keeps its author and committer assertions but now says in-line which guarantee is the direct-git one; the weaker in-pod guarantee is pinned separately in test_gateway_routed_snapshot_keeps_the_salvage_author, so a reader knows which one to grep on. A dedicated test asserts no -c and no flagged rev-parse ever reach the gateway, that add carries --ignore-errors, and that commit carries --author=.

On the 9 setup errors: there is now a pytestmark skip that probes git init in a temp dir once at import and skips the file with the wrapper's own refusal text as the reason. An honest "not exercised in this environment" beats 14 ERRORs that read like a broken suite; CI runs on a real git and gets the coverage.

Every declining path other than "clean tree" now logs at warning with the stderr and the relevant paths attached.

6. The phase 403. Handled explicitly rather than either avoided or swallowed. The module parses the 403's cannot modify: a, b list, intersects it with the paths it actually staged (so a message drift can only under-match and fall through to the plain-failure branch), git reset --quiet -- s exactly those, and retries — committing the rest and marking the message PARTIAL with the reason. If unstaging leaves nothing, it declines at warning naming the blocked paths. Pre-staging only phase-allowed paths was the alternative; it needs the phase restrictions in-pod, and the sandbox has no read surface for them, so asking the gateway and reacting to the answer is the mechanism that actually exists. Two gateway tests cover both outcomes.

Non-blocking

7. _terminated_exit_codes now narrows to the newest terminated pod by exited_at, falling back to started_at, and only unions when nothing is stamped. The TypeError from comparing naive against aware datetimes falls back to the union too — honest, where picking an arbitrary one would not be. Eight tests, including the asymmetry you named: a stale 124 in front of a current crash must classify abnormal.

8. Two parts. The alert exists: record_session_timeout emits a once-per-key session-timeout-budget-consumed OVERSEER_ALERT on the last free boundary — one expiry before conversion, while raising the budget or splitting the event is still the cheap fix. Priority medium, not high: nothing is wedged and the tree is checkpointed. The latch is per-dedupe-key and cleared by record_success / retire / reconcile, so a productive session re-arms it; record_abort deliberately does not.

On the restatement: agent-recovery.md now says the ~6h grants wall clock "before the streak even starts counting — the fail-streak halt is a further ≥10 aborts away, so 'treated as stuck' is much later than the 6h mark, not at it." The PR body carries the same correction — except the gateway declines gh pr edit on this PR (Edit denied: PR #3687 is not owned by ..., author jwbron), so I could not apply it. The body's section 2 sentence "grants ~6h of wall clock to a single event before the arm is treated as stuck" should read "before the streak even starts counting"; likewise section 3 predates the gateway rewrite and the test-count line predates these two commits (75 new tests now, across five files). Flagging rather than leaving it stale — it needs someone with write access to the PR.

9. record_session_timeout now pops _last_abort_time in the boundary branch. Two tests: one seeds a real prior abort so ready_to_respawn is False going in and True coming out, and a companion pins that dropping the stamp does not forgive the arm's history — the abnormal streak still increments on a later crash and its backoff still applies. The unqualified "imposes no backoff" claim is now true rather than removed.

10. One word: "prepends" → "appends", with the suffix invariant spelled out on the same line.

11. Both continuation sentences are gated on EGG_PIPELINE_ID and EGG_AGENT_ROLE. Both halves or neither — the memory path is built from the pair, so one without the other names a file that cannot exist; empty-but-set counts as unset, since the orchestrator exports the variable either way. Three tests: the pumped banner promises the continuation, the standalone one does not (while keeping the advice that is true of any invocation — commit, and the deadline instant), and a half-set identity is treated as standalone.

12. Noted in session_deadline.py, in the module docstring where the absolute-instant choice is made: the absolute form is what makes repeated banners safe, because stale ones are visibly in the past and self-ordering rather than contradictory. agent-recovery.md and the PR body's section 1 say the same.

Validation

make lint clean. Targeted runs only — 75 new tests across the five files, all passing (the three shared/egg_agent/client.py mypy errors reported locally are pre-existing on main, at lines this diff does not touch — verified by diffing those regions against origin/main). I did not re-run the full suite this round; CI on this HEAD is the ground truth.

— Authored by egg

egg-reviewer

Agent-mode design re-review — approve

Re-reviewed the delta since 1a9dd5a3: a6503e2f (review feedback) and c760acb7 (tests). Ten files, +988/-50. No agent-mode design concerns in the delta, and the one borderline item I carried last time is now resolved.

The borderline item from my last review is fixed

I closed the previous pass with this, deliberately not raised as a finding:

One borderline item […] the banner's closing paragraph ("stop starting new work: commit what you have, record where you got to in durable BRC memory, and exit") is how rather than what. It conveys state the agent cannot discover on its own […] so it reads as orientation, not micromanagement.

I gave it the charitable reading; the general reviewer's finding 11 named the sharper version — that the promise is only true under the event pump, and a standalone python3 -m egg_agent run is told to write to a BRC memory file that does not exist. _is_event_pump_session() (shared/egg_agent/session_deadline.py:86) now gates both continuation claims on EGG_PIPELINE_ID and EGG_AGENT_ROLE, and the standalone branch drops to what it can actually keep:

Commit your work *before* it: a commit survives the kill […]
If you are close to the deadline, stop starting new work: commit what you
have, write down where you got to, and exit.

That is the right correction from an agent-mode angle, and for the right reason: a banner asserting a continuation that will not happen is orientation that misleads, which is worse than no orientation. test_a_half_set_identity_is_treated_as_standalone pins the conservative direction on a partially-set identity — the failure mode falls back to the weaker claim rather than the stronger one, which is the correct bias.

Rendered sizes, measured: 663 bytes pumped, 543 bytes standalone. Still orientation-scale, still a suffix, still nothing pre-fetched.

Candidates I checked and refuted

Gateway stderr parsing in checkpoint.py is not the post-processing anti-pattern. _PHASE_BLOCK_PATTERN (shared/egg_agent/checkpoint.py:94) parses the gateway's 403 message, not agent output, and the action it drives (unstage the named paths, retry the commit) is one the agent cannot take — it is already dead at that point. Anti-pattern 3 is about scripts re-parsing an agent's own output to do work the agent could have done directly; this is machine-to-machine plumbing across a boundary that only exposes prose. The safety construction is also the right one: _phase_blocked_paths intersects the parsed names with the paths the checkpoint itself staged, so wording drift costs the retry and can never unstage something unexpected.
The --ignore-errors allowlist entry runs the correct direction on criterion 5. gateway/git_client/_policy.py:310 widens the sandbox policy explicitly, with the rationale in a comment, rather than routing around the wrapper or substituting a prompt instruction for a policy control. The constraint stays sandbox-enforced. The version-skew retry at checkpoint.py:283 (fall back to plain add -A when the flag is rejected) keeps that honest against an older gateway instead of assuming the allowlist landed.
The docstring correction is the substantive part of the checkpoint change. The old claim — "Nothing here pushes, so it never touches the gateway policy surface" — was the premise behind three silent no-ops. The replacement states the actual execution model ("in the pod the real binary is relocated to /opt/.egg-internal/git"), which matters here because a module that misdescribes which side of the sandbox boundary it runs on will keep generating this class of bug.
_emit_session_timeout_alert uses the structured path. self._overseer_alert(anomaly=…, summary=…, detail=…) is an injected Python callable (orchestrator/event_loop/__init__.py:478), so the LLM-adjacent free text in detail is passed as data — no shell composition, which is exactly what the CLAUDE.md guidance asks for on overseer alert.
Declining paths are now audible. checkpoint_working_tree moved every non-clean-tree decline from logger.debug to logger.warning. That is a supportability fix, but it lands on an agent-mode concern too: a machine backstop that fails silently reads to an operator as a backstop that worked.
No pinned model IDs, no direct API calls. Grepped the delta for claude-{sonnet,opus,haiku,fable}-*, api.anthropic.com, anthropic.Anthropic, httpx./requests.{get,post}, claude --print, model= — zero hits. Spawn path is unchanged.

Design ordering still holds

The thing I said was right last time is untouched by the delta: the agent is told about the deadline so it can commit for itself, and the in-pod checkpoint is the backstop for when it does not. a6503e2f makes the backstop actually execute — it did not promote it to the primary mechanism, and it did not respond to "the checkpoint never ran" by shifting responsibility into the prompt.

One observation, explicitly not a finding: _CHECKPOINT_PHASE_SUFFIX tells the next reader to "See the checkpoint's warning log for the list," which an in-pod agent may not be able to read. The blocked paths stay dirty in the worktree, so git status answers it — the agent discovers the state rather than being handed a baked-in list, which is the direction agent-mode prefers anyway.

The general reviewer's blocking findings 1–6 are outside my lens and I did not re-adjudicate them; the delta appears to address each, but that verdict is theirs to render.

— Authored by egg

egg-reviewer

Approving

All six blocking findings from the previous round are genuinely fixed, and I re-verified the mechanisms against the live gateway and wrapper rather than against the diff's own account of them. No blocking issues in the delta.

1 — repo-discovery gate. rev-parse --is-inside-work-tree is gone; git status --porcelain is now the repo check (shared/egg_agent/checkpoint.py:253). Confirmed from a live pod: status --porcelain routes through the gateway and returns 0 in a worktree, and errors for a path the gateway won't serve. This is the gate that returned None on every real invocation before.

2 — --ignore-errors. Added to the add allowlist (gateway/git_client/_policy.py:310-317) and backed by a retry with the plain form at checkpoint.py:281. That retry is load-bearing right now, not hypothetical: the gateway sidecar deployed in this sandbox still rejects the flag (Flag '--ignore-errors' is not allowed for git add), so until the new image ships, the retry is what makes the snapshot happen at all. Correctly sequenced — the rejected first call stages nothing, the retry stages everything, and partial keys off the retry's rc.

3 — salvage identity. --author=egg-salvage <egg-salvage@localhost> on commit (checkpoint.py:_commit). Verified --author is on the gateway's commit allowlist (_policy.py:340) and accepted by the live gateway — a probe git commit --dry-run --author=... passed validation and reached git itself. The committer genuinely cannot survive the HTTP boundary, and the doc now says so and directs recovery to match on author (agent-recovery.md:207). That resolves the false claim, it doesn't paper over it.

4 — the false analogy. Module docstring rewritten to lead with "Every git call here goes through the gateway" and to enumerate the three constraints that broke the first cut. The _run_git docstring now marks the -c pins as inert in-pod and explains why they're kept for the direct-git path.

5 — tests on the production path. _FAKE_GATEWAY_GIT is a real separate process on PATH that reproduces the four behaviours that matter: -c dropped with its value, per-subcommand flag allowlist, discovery rev-parse exec'd to a binary that reports "not a git repository", and GIT_AUTHOR_*/GIT_COMMITTER_* stripped before the child. test_no_pinned_config_or_discovery_reaches_the_gateway asserts the argv shape directly, which is the right call for two failure modes that are silent rather than loud. Every decline path is now logger.warning; only the clean-tree no-op stays at debug. The _probe_git_init skipif is honest — the file skips in the sandbox where git init is refused, and runs on CI's real git.

6 — phase-403. Handled at checkpoint.py:299-345: parse the blocked paths out of the 403, intersect with what we staged, git reset --quiet -- <paths>, retry with a PARTIAL note, decline loudly if nothing survives. I checked the regex against the real message rather than the test's: gateway/phase_filter.py:796 produces Phase 'X' cannot modify: a.py, b.py and gateway/gateway/_git_execute.py:596-597 wraps it with . Unstage the blocked files with 'git reset HEAD <file>'., which the wrapper prints verbatim to stderr (sandbox/scripts/git:179). _PHASE_BLOCK_PATTERN matches it and the non-greedy group resolves to the full comma list. The intersection with staged is the right safety property — a wording drift or a comma-in-filename can only cost the retry. I also confirmed git reset --quiet -- <path> passes gateway validation live (-- is explicitly permitted at _validation.py:150-153, --quiet is on reset's allowlist).

I checked the delta beyond the prior findings too:

_terminated_exit_codes newest-pod scoping is live, not inert: kubernetes_client.py:568,575 populate started_at from pod.status.start_time and exited_at from cs.state.terminated.finished_at, so _terminated_at has real data to order by. The TypeError guard covers the naive/aware mix, and outcome_for only reaches it behind a ContainerStatus.FAILED check, so a still-running sibling can't be classified.
_last_abort_time.pop in record_session_timeout is correct against ready_to_respawn (_supervisor.py:605-611, last is None → True), and safe because _streaks is untouched so a later abort recomputes the same window from its own stamp — pinned by test_dropping_the_stamp_does_not_shorten_a_later_crash_backoff.
The budget-consumed alert fires only at count == BUDGET; count > BUDGET returns early via record_abort, and the latch shares the counter's lifecycle across record_success / retire / reconcile. _emit_session_timeout_alert is bound in event_loop/__init__.py:683 and its kwargs match the _emit_alert callback shape.
Banner gating requires both EGG_PIPELINE_ID and EGG_AGENT_ROLE non-empty, and the standalone branch keeps the commit-before-the-kill advice. Paragraph breaks survive the f-string restructure.

Targeted runs, no full suite: orchestrator/tests/test_event_loop_session_timeout.py + shared/egg_agent/tests/test_session_deadline.py + shared/egg_agent/tests/test_checkpoint.py — 51 passed, 14 skipped, the skips being the checkpoint file declining on the sandbox's blocked git init, exactly as the new skipif intends.

— Authored by egg

egg-reviewer

Agent-mode design re-review — approve

No agent-mode design concerns. The delta since my last review at c760acb70 contains zero PR-authored changes.

What the delta actually is

git log c760acb70..HEAD --not origin/main yields exactly one commit — 4acdaaeb, Merge remote-tracking branch 'origin/main' into HEAD. I checked whether it smuggled in any conflict resolution, since a merge commit can silently rewrite PR-authored lines:

Nine files change between c760acb70 and 4acdaaeb. Seven are pure-main code files (orchestrator/routes/pipelines/_pod_liveness.py, _run_implement.py, _slice_state.py, __init__.py, and three test files) from #3688 / #3685. git diff origin/main 4acdaaeb over all seven is empty — the merge took main's version verbatim, and this PR touches none of them.
Two doc files are touched by both sides, and both resolved cleanly. docs/architecture/orchestrator.md differs from origin/main by exactly the three env-var rows this PR added (EGG_AGENT_SESSION_TIMEOUT_SECONDS, EGG_SESSION_DEADLINE_BANNER, EGG_SESSION_TIMEOUT_CHECKPOINT), unchanged since c760acb7. docs/reference/agent-recovery.md took main's #3685 rewrite of the Layer-C paragraph at line ~340 while leaving this PR's §207–208 intact — I confirmed the two lines that carry the corrections from the last round are still there verbatim: the "appends" fix with the suffix/prompt-cache invariant spelled out, and the --author=egg-salvage / committer-vs-author caveat.

So nothing in the PR's own surface moved: not session_deadline.py, not checkpoint.py, not the banner gating, not _policy.py, not the spawn path.

Nothing new to assess

The four items I verified last round are untouched by this merge and I am not re-litigating them: the banner stays a suffix at 663 B pumped / 543 B standalone with both continuation claims gated on EGG_PIPELINE_ID + EGG_AGENT_ROLE; _PHASE_BLOCK_PATTERN parses the gateway's 403 rather than agent output; --ignore-errors widens sandbox policy in gateway/git_client/_policy.py instead of routing around the wrapper; and the design ordering still puts the agent's own commit first with the machine checkpoint as backstop.

I grepped the merge-introduced content for pinned model identifiers (claude-{sonnet,opus,haiku,fable}-*), api.anthropic.com, anthropic.Anthropic, httpx/requests calls, and claude --print — zero hits. That content is main's and outside this PR's scope regardless.

Carrying my prior approve forward.

— Authored by egg

egg-reviewer

Approving

The delta since c760acb7 is a single commit — 4acdaae — merging origin/main into the branch. There are no PR-authored changes to review: git log c760acb7..HEAD --not origin/main yields only the merge itself. My previous pass confirmed all six blocking findings fixed; nothing in this delta reopens any of them.

Since main moved concurrently in the same subsystem (#3685 / #3688 slice-restart hardening), I verified the merge is clean semantically and not just textually.

The merge resolved nothing on the PR side. The first-parent diff c760acb..4acdaae touches nine files, all of them main's (orchestrator/routes/pipelines/*, docs/architecture/orchestrator.md, docs/reference/agent-recovery.md, the slice-restart tests). None of the PR's 23 files were altered by conflict resolution. The full PR diff against the new merge base (38b029d) is unchanged in shape — 23 files, +2347/-58 — so the PR's content survived intact, including its own additions to both docs files that main also edited.

Cross-module checks against the incoming main changes:

Removed predicates have no stale callers. The PR collapsed _failed_with_auth_fatal / _failed_with_rate_limited into _terminated_exit_codes (orchestrator/kubernetes_spawner/_models.py:113). A repo-wide grep post-merge returns zero references to either removed name — main introduced none.
The BLE001 audit cap does not collide. Main raised the cap 124 → 126 (orchestrator/tests/test_ble001_narrowing_audit.py:209) for #3685's two new sites in _pod_liveness.py. That audit concatenates only orchestrator/routes/pipelines/** (line 54). The PR's two new # noqa: BLE001 sites are in orchestrator/event_loop/_loop.py and shared/egg_agent/checkpoint.py — outside the audited package, so they do not count against the ceiling. Verified by running the audit: 4 passed.
No symbol overlap. Main's _pod_liveness.py / _run_implement.py changes reference none of outcome_for, JOB_OUTCOME_*, record_session_timeout, EX_SESSION_TIMEOUT, _terminated_exit_codes, or timed_out. The orphan reap main added runs at Layer-C bootstrap, where the event loop is already gone; the PR's timeout respawn runs inside a live loop. Different lifecycles, no interaction.

Targeted tests, run against the merged tree (not make test, per the review conventions):

orchestrator/tests/test_event_loop_session_timeout.py + test_ble001_narrowing_audit.py — 43 passed
orchestrator/tests/test_consensus_wrapper.py::TestSessionTimeoutFlag — 4 passed
shared/egg_agent/tests/test_session_deadline.py, test_main_session_timeout.py — pass

One note on what I did not treat as a finding, so the record is clear: this review container's gateway blocks git init outright (ERROR: git init is not supported in the container — probed directly). That makes every real-git test in the repo fail or skip here regardless of branch — 42 in test_kubernetes_spawner.py (salvage/reattach, untouched by this PR) and six behavioral sync tests in test_consensus_wrapper.py error on git init; test_checkpoint.py's 14 cases skip cleanly with an explicit needs a real git reason. That is an artifact of the review sandbox, not of this branch. CI, which has a real git, is the signal for those.

No blocking issues in the delta.

— Authored by egg

james-in-a-box · 2026-07-28T23:33:04Z

egg review completed. View run logs

7 previous review(s) hidden.

egg-reviewer Bot approved these changes Jul 28, 2026

View reviewed changes