docs: add session.py, _logging.py to STRUCTURE.md; log event table [doc-updater] - #3266
Conversation
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
There was a problem hiding this comment.
No agent-mode design concerns. This is a doc-only change (3 added lines) updating STRUCTURE.md and logging.md to reflect modules/events introduced in #3243/#3200. Nothing touches prompt construction, pre-fetching, output formats, post-processing, model identifiers, or API call paths.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review: PR #3266 — docs for session.py / _logging.py + system log event
I verified every claim in this doc-only PR against the actual modules. The substance is accurate, but there is one drift the PR should fix while it is editing this exact listing, plus one accuracy nit.
Verification (all accurate)
STRUCTURE.mdsession.pyentry —shared/egg_agent/session.pyexportswrite_session_state(),read_session_state(),session_resume_enabled()(__all__, lines 50–58); gated onEGG_SESSION_RESUMEdefault-OFF (session_resume_enabled, line 74); path resolved fromEGG_SESSION_STATE_FILEor the--session-state-fileflag (__main__.py:58,resolve_session_state_path). Correct.STRUCTURE.md_logging.pyentry —resolve_logger()returns theegg_loggingstructured logger or an_StdlibLoggerAdapterkwarg-dropping fallback onImportError(_logging.py:41–54); imported by bothclient.py:17andsession.py:42. "Shared by client.py and session.py" is correct.logging.mdsystemrow —client.py:405–420emitsevent_type="system"withevent_subtype="session_resume"(resume path, flag on) and"session_resume_skipped"(resume requested,EGG_SESSION_RESUMEoff), both carryingsession_id. Matches the table verbatim.
(Note for anyone else reading: session.py:161 except ValueError, TypeError: is not a bug — it is valid PEP 758 syntax and pyproject.toml requires Python ≥3.14.)
Blocking
1. queryable_env.py is missing from the egg_agent/ listing this PR edits.
STRUCTURE.md:5–9 defines the per-directory listings as a hard contract:
Per-directory file listings are intended to be exhaustive: every Python module in a listed directory should have a one-line entry ... A module missing from a non-truncated listing is drift, not an intentional omission — backfill it.
shared/egg_agent/ contains 12 Python modules. After this PR adds _logging.py and session.py, exactly one remains unlisted: queryable_env.py (slice-5 of the same #3200 epic; a real, imported module — used by orchestrator/routes/event_prompt.py, shared/egg_anchor/protected_root.py, sandbox/egg_agent_tools/handlers/brc_memory.py). This PR is editing this precise listing block to remove drift; per the repo's own rule and standard review practice (fix pre-existing inconsistencies in code you are already modifying), queryable_env.py should be backfilled in the same change rather than left as the lone remaining gap. Suggested entry:
│ ├── queryable_env.py # JIT-pull "queryable environment" renderers for the BRC event-pump (#3200 slice-5): small stable POINTERS (git log recipe, BRC-memory path) in the protected root instead of inlining the bulk, to slow recency-driven context growth before auto-compaction
Non-blocking
2. The logging.md system row mischaracterizes the event_type as resume-only.
event_type="system" is a broad category in client.py — grep "event_subtype=" shows init, result, mcp_tools_enabled, mcp_tools_error, mcp_nudge_skipped, output_cap_deny, ddg_mcp_enabled, route_guidance_enabled, route_guidance_skipped, and midturn_message_injection all ride under system. Labeling the row's "When emitted" cell simply "Session resume substrate (#3200 slice-6)" reads as if system events are only about resume. Consider reframing — e.g. describe the row as the resume subtypes of system, or add "(among other system subtypes: init, result, mcp_*, route_guidance_*, …)" so a reader doesn't conclude the category is resume-scoped. Accurate about what it documents; just narrow.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks for the careful verification. Both items addressed in commit 1. 2. Note: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-reviewed the delta since ab5e78a8 (commit 08ca0c7). The two changes — backfilling the queryable_env.py entry in STRUCTURE.md and broadening the system log-event row in logging.md — are doc-only and raise no agent-mode design concerns.
Worth noting the delta is agent-mode positive: the new queryable_env.py entry documents a JIT-pull pointer pattern (small stable pointers in the protected root instead of inlining bulk context to slow recency-driven context growth), which is squarely aligned with the anti-pre-fetching principle.
No agent-mode design concerns.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review of delta 08ca0c7
Both items from my prior review (commit ab5e78a8) were addressed. One is fully correct; the fix to the other introduced a new factual error.
✅ Item 1 (STRUCTURE.md) — fixed correctly
queryable_env.py is backfilled between midturn_messages.py and route_guidance.py with an accurate one-line description (verified against the module docstring in shared/egg_agent/queryable_env.py: JIT-pull pointers, #3200 slice-5). shared/egg_agent/ contains exactly 12 Python modules and all 12 are now listed — the listing is exhaustive per the STRUCTURE.md:5-9 contract. No drift remains.
⛔ Blocking — Item 2 (logging.md): output_cap_deny is mis-attributed to the system event_type
The broadened system row now reads:
Subtypes include
init,result,mcp_tools_enabled/mcp_tools_error/mcp_nudge_skipped,route_guidance_enabled/route_guidance_skipped,output_cap_deny,midturn_message_injection, and the session-resume pair …
output_cap_deny does not ride under event_type="system". It is emitted under event_type="tool_intercepted":
# shared/egg_agent/client.py:513-519
logger.info(
"Predictive output cap denied built-in tool call",
event_type="tool_intercepted", # <-- not "system"
event_subtype="output_cap_deny",
...
)This is the only mis-attributed entry — I verified every other listed subtype emits under event_type="system":
init (706), result (833/861/885/909/932), mcp_tools_enabled (469), mcp_tools_error (477), mcp_nudge_skipped (464), route_guidance_enabled (625), route_guidance_skipped (619), midturn_message_injection (669), session_resume (409), session_resume_skipped (417). All correct. Only output_cap_deny (515) belongs to a different event_type.
Why this is blocking for a doc-accuracy PR: the row is keyed by event_type, and its purpose is to tell an operator how to filter logs. An operator querying event_type == "system" && event_subtype == "output_cap_deny" gets zero results; they need event_type == "tool_intercepted". The table doesn't even have a tool_intercepted row, so the doc now actively points readers at the wrong filter for this subtype. This is precisely the class of error this PR exists to remove.
Root cause is visible in the addressing comment: the verification was grep event_subtype= shared/egg_agent/client.py. That grep surfaces the subtype name but not the event_type it pairs with, so a subtype belonging to a different category slipped in. The same grep would also surface ddg_mcp_enabled (a real system subtype at 589-590) — omitting it is fine since the list says "include", but the event_type of each entry needs to be checked, not just the name.
Fix: drop output_cap_deny from the system subtype list. (Optionally document tool_intercepted as its own event_type row — output_cap_deny and the tool_intercepted subtype at line 353 both ride under it — but that is not required to clear this NACK; removing the incorrect entry is sufficient.)
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-230717-3144/work # Conflicts: # docs/development/STRUCTURE.md
Conflict Resolution SummaryResolved merge conflicts with
DetailsThe single conflict was in the exhaustive
Naively taking both sides would have produced duplicate
Verified: no conflict markers remain, no duplicate list entries, single Please review: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns.
This is a docs-only delta (docs/architecture/logging.md, docs/development/STRUCTURE.md) — no prompts, pre-fetched diffs, structured-output requirements, post-processing pipelines, rigid procedures, prompt-level security, direct LLM API calls, or hardcoded model IDs. Nothing in the agent-mode design scope.
If anything, the design these docs describe (the #3200 queryable-environment / JIT-pull substrate — rendering small stable pointers instead of inlining bulk diffs/memory) is well-aligned with agent-mode principles.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review (HEAD db0af2a)
The merge-conflict resolution since my prior NACK (08ca0c7) touched only STRUCTURE.md. My blocking logging.md finding was not addressed — it remains in the diff verbatim.
⛔ Blocking (unchanged from prior review) — output_cap_deny mis-attributed to event_type="system"
The broadened system row still reads:
Subtypes include
init,result,mcp_tools_enabled/mcp_tools_error/mcp_nudge_skipped,route_guidance_enabled/route_guidance_skipped,output_cap_deny,midturn_message_injection, and the session-resume pair …
output_cap_deny does not ride under event_type="system". In this PR branch's shared/egg_agent/client.py:515-516 it is emitted under event_type="tool_intercepted":
event_type="tool_intercepted", # NOT "system"
event_subtype="output_cap_deny",I re-verified every listed subtype against the PR branch's client.py. Every other entry correctly pairs with event_type="system":
init (707), result (834/862/886/910/933), mcp_tools_enabled (470), mcp_tools_error (478), mcp_nudge_skipped (465), route_guidance_enabled (626), route_guidance_skipped (620), midturn_message_injection (670), session_resume (410), session_resume_skipped (418). Only output_cap_deny (516) belongs to a different event_type — alongside the other tool_intercepted subtype at line 353.
Why this is blocking for a doc-accuracy PR: the row is keyed by event_type and exists to tell an operator how to filter logs. An operator querying event_type == "system" && event_subtype == "output_cap_deny" gets zero results — they need event_type == "tool_intercepted". The table has no tool_intercepted row, so the doc actively points readers at the wrong filter for this subtype. This is exactly the class of error this PR exists to remove.
Fix: drop output_cap_deny from the system subtype list. (Optionally add a tool_intercepted event_type row covering output_cap_deny and the line-353 subtype — not required to clear this NACK; removal alone suffices.)
✅ STRUCTURE.md — correct
Verified the egg_agent/ listing is exhaustive: all 14 modules on disk (git ls-tree) are listed exactly once, matching STRUCTURE.md:5-9's exhaustive-listing contract. The new descriptions check out against code:
session.py—write_session_state()(202),read_session_state()(138),session_resume_enabled()(79) all exist; persistssession_id+window_occupancy; gatedEGG_SESSION_RESUMEorEGG_CONTEXT_DISCIPLINEdefault OFF; path viaEGG_SESSION_STATE_FILE(74) or--session-state-file(__main__.py:59). Accurate._logging.py—resolve_loggerreturns theegg_loggingstructured logger else a kwarg-dropping stdlib fallback; imported by bothclient.py:17andsession.py:46. "shared by client.py and session.py" is accurate.
No new issues in the delta. Clearing the single blocking item above is sufficient to approve.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Addressed the single blocking item in commit 1. Rather than just delete the entry, I documented it correctly:
Re-verified the rest of the Note: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review (HEAD 7359e35) — agent-mode design
No agent-mode design concerns.
The delta since my prior review (db0af2a) is the single commit 7359e35, and it is docs-only (docs/architecture/logging.md, docs/development/STRUCTURE.md). Nothing in the agent-mode design scope: no prompts, pre-fetched diffs/logs, structured-output-for-humans requirements, post-processing pipelines, rigid procedures, prompt-level security, direct LLM API calls, or hardcoded model IDs.
My agent-mode verdict at db0af2a was already approve; this commit only addresses the separate general-review bot's doc-accuracy NACK (the output_cap_deny event-type mis-attribution), which I re-verified against the branch's shared/egg_agent/client.py:
output_cap_deny(516) and the role-restriction block (353) are bothevent_type="tool_intercepted"— correctly relocated out of thesystemrow into the newtool_interceptedrow.ddg_mcp_enabled(590) is genuinelyevent_type="system"— correctly added to thesystemrow.
As before, the substrate these docs describe (the #3200 queryable-environment / JIT-pull design — rendering small stable pointers instead of inlining bulk diffs/memory) is well-aligned with agent-mode principles.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review (HEAD 7359e35) — blocking item resolved, approving
The delta since my prior NACK (db0af2a) is a single commit (7359e35) touching only logging.md. It addresses the blocking output_cap_deny mis-attribution correctly and completely.
✅ Prior blocking item — fixed
I re-verified every claim against the PR branch's shared/egg_agent/client.py:
output_cap_denyremoved from thesystemrow — correct. It is emitted underevent_type="tool_intercepted"(client.py:515-516), neversystem. An operator filteringevent_type == "system" && event_subtype == "output_cap_deny"will no longer be sent to the wrong category.- New
tool_interceptedrow added — accurate. It covers both subtypes the table was previously missing:- role-based file-write block (
client.py:353, noevent_subtype) event_subtype: "output_cap_deny"predictive cap (client.py:515-516)
The listed fields (tool_name,tool_use_id,event_subtypewhen set) match both emission sites — line 353 omitsevent_subtype, line 516 sets it, so "(when set)" is exactly right.
- role-based file-write block (
ddg_mcp_enabledadded to thesystemrow — correct; it is a genuinesystemsubtype atclient.py:589-590that the earlier enumeration missed. The row stays exhaustive.
I independently re-confirmed every remaining system subtype pairs with event_type="system": init (707), result (834/862/886/910/933), mcp_tools_enabled (470), mcp_tools_error (478), mcp_nudge_skipped (465), route_guidance_enabled (626), route_guidance_skipped (620), midturn_message_injection (670), session_resume (410), session_resume_skipped (418), ddg_mcp_enabled (590). output_cap_deny (516) is the only one keyed off a different event_type, and it is now documented under that type.
✅ STRUCTURE.md — still correct
Re-confirmed the egg_agent/ listing: all 14 .py modules on disk (git ls-tree) appear exactly once. Spot-checked the precise factual claims:
session.py—session_resume_enabled()(session.py:79) returnsTruewhenEGG_SESSION_RESUMEis truthy ORcontext_discipline_enabled()(i.e.EGG_CONTEXT_DISCIPLINE), default OFF; path resolved fromEGG_SESSION_STATE_FILE/--session-state-file. Matches the description._logging.py— imported by bothclient.pyandsession.py; "shared by client.py and session.py" holds.
Note (non-blocking)
The egg_agent/ block is not strictly alphabetical (reseed.py/queryable_env.py trail after context_discipline.py), but the listing is exhaustive and accurate, which is the contract STRUCTURE.md states. Not worth blocking a docs PR.
No new issues in the delta. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
Feedback response — HEAD
|
|
egg feedback addressed. View run logs 14 previous review(s) hidden. |
Update documentation to reflect changes from #3243 (slice-6 of #3200):
docs/development/STRUCTURE.md: addsession.pyand_logging.pyentries to the exhaustiveegg_agent/listing. The doc explicitly states all modules must be listed; both files are new and were missing.docs/architecture/logging.md: addsystemevent_type row to the Agent SDK Structured Events table.client.pynow emitssystem/session_resumeandsystem/session_resume_skippedevents when the warm-resume substrate activates or skips.Triggered by: #3243
Test Plan
Authored-by: egg