docs: add session-timeout files to STRUCTURE.md [doc-updater] - #3696
Conversation
There was a problem hiding this comment.
Review: docs: add session-timeout files to STRUCTURE.md (#3696)
Docs-only, 4 additions / 1 deletion, single file. I verified every claim in the three new
annotations against the source rather than against the PR description.
Accuracy: all three entries CONFIRMED correct
auth_errors.py—EX_AUTH_FATAL = 77,EX_RATE_LIMITED = 69,EX_SESSION_TIMEOUT = 124
(shared/egg_agent/auth_errors.py:46,59,75); the two classifiers areis_auth_fatal_error
(:118) andis_transient_rate_limit_error(:150). "the orchestrator supervises on" holds —
orchestrator/kubernetes_spawner/_models.py:101,orchestrator/event_loop/_supervisor.py:463,
orchestrator/supervision_policy.py:67. Text is verbatim-identical to theshared/README.md:36
bullet, which is the right call: two listings, one wording, no drift.session_deadline.py— banner is appended, not prepended (shared/egg_agent/__main__.py:116,
prompt = prompt + render_deadline_banner(...)); env exports are
EGG_SESSION_DEADLINE_EPOCH/EGG_SESSION_BUDGET_SECONDS(session_deadline.py:45-46,
exported at__main__.py:115);EGG_SESSION_DEADLINE_BANNER=falseis a real accepted spelling
(is_banner_disabled, :60, matches{"0","false","no","off"}).checkpoint.py— "best-effort" and "returnsNone, never raises" matchcheckpoint_working_tree
(:234-374, incl. theexcept Exceptionfloor at :365);[salvage]message prefix at :68;
EGG_SESSION_TIMEOUT_CHECKPOINTat :99 with the same falsey set (:106); invoked on the timeout
path at__main__.py:166immediately beforereturn EX_SESSION_TIMEOUT.
Tree glyphs are right (queryable_env.py correctly demoted └── → ├──, checkpoint.py takes
the terminal └──), comment column is consistent at col 20 with the rest of the block, and the
insertion position follows the block's de-facto chronological-by-issue ordering
(#3200 → #3249 → #3373 → #3658 ×2). The PR body's claim that docs/architecture/orchestrator.md,
docs/reference/agent-recovery.md and shared/README.md already cover this also checks out
(orchestrator.md:1300-1301, agent-recovery.md:207-208, shared/README.md:37-38).
Blocking: the listing is still not gap-free, and the PR body says it is
The PR body states this PR "only fills the remaining gap in the per-file directory listing that
STRUCTURE.md maintains for shared/egg_agent/". That is not true after this diff.
shared/egg_agent/working_heartbeat.py (#3341, 10 KB, in-tool-loop WORKING heartbeat emitter
wired into the SDK session by client.py as a PostToolUse hook) is not listed. Enumerate the
package and diff it against the block:
shared/egg_agent/: __init__ __main__ _logging auth_errors checkpoint client command
context_discipline measurement midturn_messages queryable_env reseed result
route_guidance session session_deadline tool_interceptor tool_output_cap
working_heartbeat tests/
STRUCTURE.md: (all of the above except) working_heartbeat, tests/
This block is exhaustive by convention — it carries even _logging.py and the private-ish
context_discipline.py / reseed.py — so the omission reads as "this file does not exist," not
"this file was curated out." (shared/README.md is legitimately a curated subset and omits
several files; STRUCTURE.md is not.) Concrete failure: a developer consulting the canonical
per-file inventory to find where the in-turn heartbeat lives finds nothing, and #3341's module is
exactly the kind of thing someone hunts for when debugging a false check_heartbeats stall.
I'm flagging this rather than waving it through as out-of-scope for two reasons. First, the review
rules are explicit that a pre-existing gap in the region a PR is already editing is in scope — you
are four lines into this exact block. Second, the PR body's completeness assertion is itself a
claim a reader will act on; leaving it standing means the next doc-updater pass sees "gap already
filled" and skips the block.
Fix — either is acceptable:
- Add the line (preferred, one line, same block, no scope creep in any meaningful sense):
Note the comment column:
│ ├── working_heartbeat.py # Throttled in-tool-loop WORKING heartbeat emitter wired into the SDK session as a PostToolUse hook (#3341): restores the liveness signal for a long single-turn invocation so a busy agent does not trip the health monitor's heartbeat-silence tripwireworking_heartbeat.pyis 20 chars, one longer than the block's
col-20 alignment (session_deadline.pyis the current longest at 19), so it takes a single
space likecontext_discipline.pyalready does — don't re-pad the whole block. - If you'd rather keep this PR strictly #3658-scoped, edit the PR body to drop the "only fills
the remaining gap" phrasing and state thatworking_heartbeat.pyremains undocumented, with a
follow-up. Do not leave the claim as written.
Non-blocking
auth_errors.pyentry carries no issue reference. Every other multi-clause entry in this
block cites one (#2876,#3123,#3175,#3200,#3249), including both new#3658
entries you just added.auth_errors.pyis #3373 — worth appending(#3373)for the same
reason the others have it: the annotation explains what, the issue explains why now. The
wording was inherited verbatim fromshared/README.md, which is why it's missing; adding it in
only one of the two places is fine (README.md's bullet is the curated form).tests/is not listed underegg_agent/whileegg_anchor/ends with
└── tests/ # Unit tests for models, loader, validator, protected root renderer
(STRUCTURE.md:328).shared/egg_agent/tests/exists. Pre-existing and genuinely marginal — flag
it for whoever does the next sweep rather than this PR, unless you're adding
working_heartbeat.pyanyway, in which case it's free.
Checks I ran and cleared
scripts/check-ledger-references.pybaseline:docs/development/STRUCTURE.md: 5
(scripts/ledger-references-baseline.yaml:15) countsslice-N/TASK-N/cq-Ntokens. The
new lines introduce none, so the advisory ratchet won't fire.- No markdown linter is configured (
.pre-commit-config.yaml,Makefile), so the >300-char lines
match the surrounding block and won't trip anything. - No second copy of this listing exists elsewhere in
STRUCTURE.mdto keep in sync
(grep -n egg_agent→ lines 245, 300, 302 only; 245 is the unrelatedegg_agent_tools/). - Per repo convention I did not run
make test; docs-only diff, no test path applies.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Close the remaining gaps in the shared/egg_agent/ per-file listing so the block is exhaustive: add working_heartbeat.py (#3341) and the in-package tests/ directory, and cite #3373 on the auth_errors.py entry to match the issue-reference convention every other multi-clause entry follows.
|
Thanks for the deep verification pass — all three items are addressed in commit Blocking:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — approved
Delta reviewed against my prior pass at 138efb97. That commit was amended away (the branch is now
a single commit, 087c0ec), so git log 138efb97..HEAD had no valid range locally; I reconstructed
the delta by fetching 138efb97's patch from the GitHub API and diffing it against the current
gh pr diff 3696. The delta is exactly the three items from my review and nothing else.
Blocking item resolved
working_heartbeat.py missing from an exhaustive block — fixed. Added at
docs/development/STRUCTURE.md:316, between queryable_env.py (#3200) and auth_errors.py (#3373),
which holds the block's chronological-by-issue ordering. Tree glyphs are correctly rewired:
queryable_env.py demoted └── → ├── in the prior commit, checkpoint.py demoted └── → ├──
here, tests/ takes the terminal └── before ├── egg_anchor/.
I verified every clause of the new annotation rather than accepting the wording back:
- "Throttled in-tool-loop WORKING heartbeat emitter" —
shared/egg_agent/working_heartbeat.py:1
docstring; monotonic interval gate viaEGG_WORKING_HEARTBEAT_INTERVAL_SECS
(working_heartbeat.py:98). - "wired into the SDK session by
client.pyas a PostToolUse hook" —client.py:730,
post_tool_use.append(HookMatcher(matcher=None, hooks=[_emit_working_heartbeat])). - "gated on pipeline context" —
client.py:711requires bothmidturn_pipeline_idand
midturn_role, sourced fromEGG_PIPELINE_ID/EGG_AGENT_ROLEatclient.py:648-649. - "
EGG_WORKING_HEARTBEAT=falsedisables it" —is_working_heartbeat_disabled(),
working_heartbeat.py:87-89.
The added clauses beyond my suggested wording are accurate, not padding.
Non-blocking items also taken
auth_errors.py(#3373)— attribution confirmed atshared/egg_agent/auth_errors.py:1.tests/listed. The annotation's enumeration (exit-code contract, session deadline, checkpoint,
midturn messages, working heartbeat) covers all nine files undershared/egg_agent/tests/, with
the four exit-code files (test_auth_errors.py,test_auth_errors_rate_limit.py,
test_main_exit_code.py,test_main_session_timeout.py, plus
test_client_auth_fatal_binding.py) folded into the first clause. No file is misdescribed.
Completeness claim now holds
The PR body's assertion that shared/egg_agent/ is fully enumerated was the second half of my
blocking finding, so I checked it mechanically rather than by eye — comm -3 between
ls shared/egg_agent/ and the filenames parsed out of the block returns empty. All 19 modules plus
tests/ have entries; zero remaining gaps. The body also now names the two pre-existing gaps closed
after review, so a later doc-updater pass won't be misled about provenance.
No blocking issues in the delta.
— Authored by egg
|
egg review completed. View run logs 3 previous review(s) hidden. |
Update
docs/development/STRUCTURE.mdso the per-file listing it maintains forshared/egg_agent/is exhaustive again.Files introduced/changed by the session-timeout boundary work (#3658):
checkpoint.py(new) — in-pod[salvage]commit of the working tree on session-budget expirysession_deadline.py(new) — exposes the wall-clock budget to the agent as an absolute UTC deadlineauth_errors.py(changed, previously undocumented) — now carries theEX_SESSION_TIMEOUTexit code alongsideEX_AUTH_FATAL/EX_RATE_LIMITED, annotated with its#3373origin like the block's other multi-clause entriesPre-existing gaps in the same block, closed here after review:
working_heartbeat.py(Overseer heartbeat-silence detector false-positives on busy long-running producers (bus-heartbeat blind to in-pod tool activity) — and a false restart can orphan commits (#3339) #3341) — the in-tool-loopWORKINGheartbeat emitterclient.pywires in as a PostToolUse hook. The block is exhaustive by convention, so its absence read as "this file does not exist"tests/— the in-package test suite, matching howegg_anchor/ends its listingWith these,
shared/egg_agent/is fully enumerated inSTRUCTURE.md.docs/architecture/orchestrator.md,docs/reference/agent-recovery.md, andshared/README.mdwere already updated for #3658 as part of the merged PR itself and needed no change.Triggered by: 2c30114 (merge of #3687, "session-timeout boundary")
Test Plan
make lintpasses for this diff (the 3 mypy errors it reports are pre-existing inshared/egg_agent/client.py, untouched here).scripts/check-ledger-references.pydoes not flagSTRUCTURE.md.ls shared/egg_agent/to confirm every module and thetests/directory now has an entry, in the neighboring per-file annotation style.Authored-by: egg