docs: update refs for gateway.py decomposition [doc-updater] - #3514
Conversation
This comment has been minimized.
This comment has been minimized.
Autofix tracking{"Lint/Python": 2, "Lint/Custom Checks": 1, "Test/Unit Tests": 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review — docs: update refs for gateway.py decomposition
I verified every claim in the three doc edits against the live tree. Most are accurate, but one newly-introduced line states a count that contradicts its own enumeration, and there's an undocumented out-of-scope code change.
Blocking
1. docs/development/STRUCTURE.md — wrong submodule count ("12" should be "13")
The new Gateway Structure line reads:
gateway/… barrel__init__.py+__main__.pyentry point + 12_<cluster>.pysubmodules:_helpers,_health,_git_ops,_git_execute,_gh_ops,_gh_execute,_jira,_jira_writes,_confluence,_worktree,_sessions,_proxy,_server
The prose says 12, but the inline list enumerates 13 submodules, and there are in fact 13 on disk:
$ ls gateway/gateway/_*.py | grep -v '^.*/__' | wc -l
13
(_confluence, _gh_execute, _gh_ops, _git_execute, _git_ops, _health, _helpers, _jira, _jira_writes, _proxy, _server, _sessions, _worktree)
The enumeration is complete and correct — only the numeral is wrong. Since this PR's entire purpose is to make the docs match reality after the split, and this is freshly authored text (the pre-split line carried no count), shipping a self-contradicting "12 … <13 items>" is exactly the class of inaccuracy this change is meant to eliminate. Change 12 → 13.
Non-blocking
2. Undocumented out-of-scope change in orchestrator/event_loop/_supervisor.py
The PR title and body describe a docs-only change ("Automated: none (docs-only change)"), but the diff also collapses a two-line logger.info(...) format string into one line:
- "JobSupervisor: operator reset cleared %d exhausted key(s) — "
- "fresh spawn budgets: %s",
+ "JobSupervisor: operator reset cleared %d exhausted key(s) — fresh spawn budgets: %s",This is behaviorally identical (same format specifier, same args, same rendered output) and harmless, but it's unrelated to the gateway-decomposition doc sync and unmentioned in the PR body. Either drop it from this PR or note it in the description so the scope is honest. Not blocking on its own.
Verified accurate (no action needed)
python3 -m gatewaylaunch claim — confirmed atgateway/entrypoint.sh:343,352.gateway/gateway/_sessions.py:535citation for theaudit_log("session_created", …)call — accurate; the call spans lines 534–535.docs/guides/decomposition-pattern.mdrepoint — the "route registrations stay in__init__.py" convention holds: real@app.routedecorators live only in the barrel; the@app.routementions in submodules are all docstring text, not decorators.- The remaining
gateway.pyreferences inSTRUCTURE.md(lines 246, 258) belong to thesandbox/egg_lib/tree, unrelated to the decomposed gateway sidecar — correctly left untouched.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…y split - check-hardcoded-ports.py: allowlist entry pointed at the retired gateway/gateway.py; update to the gateway/gateway/ package files (__init__.py, _helpers.py, _server.py) introduced by the #3312 slice-3 decomposition. - _git_ops.py, _jira.py, _jira_writes.py, _confluence.py: fix mypy type: ignore mismatches (missing/unused import-untyped codes, and no-any-return on Any-typed barrel accessor calls) left over from the same decomposition.
…tion' into egg/egg-exec-20260706-170455-3133/work
This comment has been minimized.
This comment has been minimized.
|
Thanks for the review — both items addressed. 1. 2. Undocumented out-of-scope change in Note: the 12 pre-existing mypy errors surfaced by — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — docs: update refs for gateway.py decomposition
Both concerns from my prior review are properly resolved, and I re-verified every claim plus the new changes against the live tree.
Previously-blocking item — resolved
1. STRUCTURE.md submodule count (12 → 13) — ✅ Fixed correctly.
ls gateway/gateway/_*.py | grep -v '/__' | wc -l → 13, and the inline enumeration (_helpers, _health, _git_ops, _git_execute, _gh_ops, _gh_execute, _jira, _jira_writes, _confluence, _worktree, _sessions, _proxy, _server) matches the numeral and the files on disk exactly. No self-contradiction remains.
Previously non-blocking item — resolved
2. _supervisor.py out-of-scope formatting — ✅ Documented honestly. The PR body's "Also included" section explains the single-line collapse is a ruff format --check-required normalization from the automated formatting step and cannot be dropped without failing CI lint. Behaviorally identical (same format specifier/args). Acceptable.
New changes since last review — verified
check-hardcoded-ports.pyallowlist — Necessary and correct. The stalegateway/gateway.pyentry is replaced with the three split files that actually carry the source-of-truth port literals:__init__.py:574(DEFAULT_PORT = 9848),_helpers.py:182and_server.py:82(socket.create_connection(("127.0.0.1", 3129), …))._health.pyis correctly not allowlisted — its only port references are comment-only lines, which the checker skips (stripped.startswith("#")). Ranpython3 scripts/check-hardcoded-ports.py→OK: No hardcoded port numbers found(exit 0). This is squarely in-scope for a "update refs for gateway.py decomposition" PR.type: ignore[...]code refinements in_confluence.py,_git_ops.py,_jira.py,_jira_writes.py— Comment-only edits from the automated mypy-fix step. The annotated runtime lines are unchanged; adding/removingimport-untyped/no-any-returncodes is exactly whatmypy --warn-unused-ignoresrequires. Behaviorally inert; CI mypy is the ground truth here.
Re-verified accurate (no action needed)
python3 -m gatewaylaunch claim —gateway/entrypoint.sh:343,352._sessions.py:535audit_log("session_created", …)citation — accurate (call spans 534–535).decomposition-pattern.mdroute-convention repoint — holds: all@app.routeoccurrences in_*.pysubmodules are docstring text (line 4); real decorators live only in the barrel__init__.py(50 of them).
No blocking issues remain. The docs now match the decomposed tree, and the config/lint edits needed to keep the branch green are correct.
Minor (non-blocking): the type: ignore refinements and the check-hardcoded-ports.py allowlist update aren't called out in the PR body's "Also included" section alongside _supervisor.py. Not required, but a one-line note would make the full scope explicit.
— 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 re-review and approval. Addressing the remaining minor suggestion: Minor (non-blocking):
No code changes were needed for this item — it's a description-only scope clarification, so no new commit was pushed and HEAD ( — Authored by egg |
This comment has been minimized.
This comment has been minimized.
…y-decomposition # Conflicts: # gateway/gateway/_confluence.py # scripts/check-hardcoded-ports.py
There was a problem hiding this comment.
Re-review — docs: update refs for gateway.py decomposition
Delta since my last review (8290f10) — clean
The only new commit is the merge of origin/main (59df153). I verified it carries no risk:
- The conflict resolutions in
gateway/gateway/_confluence.pyandscripts/check-hardcoded-ports.pyare byte-identical to main —git diff origin/main HEAD -- <file>is empty for both. The merge did not silently revert or corrupt anything from main. - The merge-base of
origin/mainand HEAD is currentorigin/main(4b27008b5), so the net PR is now exactlymain + 3 doc files(git diff origin/main...HEAD→STRUCTURE.md,decomposition-pattern.md,per-agent-models.md, 5/5 lines).
Previously-verified claims — re-confirmed against the live tree
- 13 gateway submodules, names match the STRUCTURE.md enumeration exactly;
__init__.py+__main__.pypresent. python3 -m gatewaylaunch —gateway/entrypoint.sh:343,352._sessions.py:535audit_log("session_created", …)— accurate.- Route seam (
decomposition-pattern.md) — 50 real@app.routedecorators in the barrel__init__.py; every@app.routeoccurrence in a_*.pysubmodule is docstring text only.
Blocking — the same fenced block still has two stale module→package refs from the same decomposition
The PR's purpose is to make the STRUCTURE.md gateway block accurate for the gateway.py → gateway/gateway/ decomposition (issue #3312). But the very same fenced block (docs/development/STRUCTURE.md) still lists two sibling modules as .py files that were split into packages under the same #3312 initiative:
- Line 60 —
├── git_client.py # Git operation handler
gateway/git_client.pyno longer exists; it is now the packagegateway/git_client/(__init__.py+_attribution.py,_branch_ops.py,_credentials.py,_policy.py,_push_analysis.py,_remote.py,_validation.py). Its barrel__init__.pysays: "Decomposition note (#3312 slice-11): the pre-split single-file module is now a sub-package." - Line 80 —
├── worktree_manager.py # Git worktree lifecycle
gateway/worktree_manager.pyno longer exists; it is now the packagegateway/worktree_manager/(_cleanup.py,_common.py,_create.py,_fsutil.py,_query.py,_remove.py). Its barrel says: "Sub-package barrel (#3312 slice-12). worktree_manager.py (2,507 lines) was split…"
These are the identical module→package change the PR documents for gateway.py, in the same block, from the same parent issue (#3312). Leaving them as .py files is self-inconsistent: the block now claims a package for gateway/ while pointing readers at two sibling files that don't exist. Per the review convention, a pre-existing inconsistency in code the PR is already editing should be fixed while in the area — and here it's not even orthogonal drift, it's the same decomposition effort.
Suggested fix (mirror the gateway/ line's style):
├── git_client/ # Git operation handler package (barrel __init__.py + _<cluster>.py submodules: _attribution, _branch_ops, _credentials, _policy, _push_analysis, _remote, _validation; #3312 slice-11)
├── worktree_manager/ # Git worktree lifecycle package (barrel __init__.py + _cleanup, _common, _create, _fsutil, _query, _remove; #3312 slice-12)
(Every other file listed in the block was verified to still exist as-named.)
Once these two lines are repointed, the block will be internally consistent and the PR will fully deliver on "update refs for … decomposition."
— 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 re-review — the remaining blocking item is addressed. Blocking: Repointed both entries to packages, mirroring the
The submodule enumerations were verified against the tree, and the numeral/enumeration for the — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — docs: update refs for gateway.py decomposition
Delta since my last review (59df153) — the one blocking item is fixed
The only new commit is eee618c ("docs: repoint git_client/worktree_manager to packages in STRUCTURE.md"), a 2-line docs change that directly addresses the blocking finding from my prior review.
Blocking finding — resolved and verified against the live tree
Both sibling modules I flagged as still listed as .py files (from the same #3312 decomposition) are now repointed to packages, and every claim in the new text checks out on disk:
git_client/—ls gateway/git_client/→__init__.py+_attribution.py,_branch_ops.py,_credentials.py,_policy.py,_push_analysis.py,_remote.py,_validation.py. Matches the enumeration exactly.gateway/git_client.pyis gone. Slice tag#3312 slice-11matches the barrel's own note (git_client/__init__.py: "Decomposition note (#3312 slice-11)…").worktree_manager/—ls gateway/worktree_manager/→__init__.py+_cleanup.py,_common.py,_create.py,_fsutil.py,_query.py,_remove.py. Matches exactly.gateway/worktree_manager.pyis gone. Slice tag#3312 slice-12matches the barrel (worktree_manager/__init__.py: "Sub-package barrel (#3312 slice-12)…").
Whole-block consistency re-swept
I re-checked every ├──/└── entry in the gateway fenced block against the filesystem (34 entries) — all 34 resolve, including the two just-fixed packages. The block is now fully internally consistent: it no longer claims a package for gateway/ while pointing readers at sibling files that don't exist.
Scope
Net PR vs origin/main remains 3 docs files (STRUCTURE.md, decomposition-pattern.md, per-agent-models.md) — no code touched, no new risk introduced by the delta.
No remaining blocking issues. Approving.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pipelines.py was decomposed into the pipelines/ package; the test's hardcoded pipelines.py path no longer exists, so it errored at fixture setup. Read the whole package (all *.py submodules) instead.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…conflict in test_artifact_spec.py
Conflict Resolution SummaryResolved merge conflicts with
Details: Both this PR branch (
The comment prose differs cosmetically between the two sides; Verification:
Please review: The resolution is byte-identical to — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review — docs: update refs for gateway.py decomposition
Re-reviewed the delta since my last approval (eee618c). Two commits landed: da2a8f0 (test fix) and ac539c4 (merge of origin/main). No blocking issues; all previously-blocking items remain resolved and every claim re-verified against the live tree.
Delta since last review — verified
1. da2a8f0 — test_artifact_spec.py repointed at the pipelines/ package ✅
pipelines.py was decomposed into the orchestrator/routes/pipelines/ package, so the test's hardcoded pipelines.py path errored at fixture setup. The fix moves PIPELINES_PATH to the package dir and concatenates every *.py for the ratchet. Verified correct:
- The package is flat (
find orchestrator/routes/pipelines -mindepth 1 -type d→ none), so the non-recursiveglob("*.py")captures the full prompt-building surface — including_prompt_agent.py,_populate.py,_drafts.py, the three submodules that actually callresolve_artifact_path. No prompt code is missed. test_pipelines_py_is_readablecorrectly switched from.exists()to.is_dir().- The ratchet reads production source text directly — not a self-seeding golden, not a hand-built fixture bypassing the production path. A real regression would still break it.
- This change also landed independently in
main, sogit diff origin/main HEAD -- shared/egg_contracts/tests/test_artifact_spec.pyis empty — it's no longer part of the net PR delta.
2. ac539c4 — merge of origin/main, conflict in test_artifact_spec.py ✅
Clean resolution: the file at HEAD is byte-identical to main (empty diff), and there are no leftover conflict markers in any changed file. The merge did not revert or corrupt the doc edits — the net PR vs merge-base (5251b5b) is exactly the 3 doc files (STRUCTURE.md, decomposition-pattern.md, per-agent-models.md).
Doc claims — re-verified against the live tree
STRUCTURE.mdgateway block —gateway/gateway/has exactly 13_<cluster>.pyfiles (excluding the two dunders__init__.py/__main__.py); numeral and inline enumeration match each other and disk exactly.git_client/→ 7 submodules (_attribution, _branch_ops, _credentials, _policy, _push_analysis, _remote, _validation) andworktree_manager/→ 6 (_cleanup, _common, _create, _fsutil, _query, _remove) both match the on-disk packages precisely. Every other.pyentry in the block still exists as a file — none silently became a package.decomposition-pattern.mdroute seam — holds: 50 real@app.routedecorators in the barrel__init__.py; every@app.routeoccurrence in a_<cluster>.pysubmodule is docstring text (line 4 only).per-agent-models.md—gateway/gateway/_sessions.py:535audit_log("session_created", …)citation accurate (call spans 534–535);python3 -m gatewaylaunch confirmed viagateway/gateway/__main__.py.
Non-blocking (no action required)
The STRUCTURE.md gateway block omits several existing modules (artifact_api.py, confluence_*.py, jira_adf.py, jira_idempotency.py, orchestrator_pipelines.py, routing_policy.py, upstream_registry.py, etc.). This is pre-existing non-exhaustiveness — the block is illustrative, not a complete file listing — and unrelated to the #3312 module→package decomposition this PR documents. Not introduced or worsened here, so out of scope for this doc-sync PR.
The docs now match the decomposed tree and the branch is internally consistent. LGTM.
— Authored by egg
|
egg review completed. View run logs 18 previous review(s) hidden. |
Update documentation to reflect the
gateway/gateway.py→gateway/gateway/sub-package split from #3312 (slice-18):docs/development/STRUCTURE.md: replace the flatgateway.pyentry in the Gateway Structure tree with the newgateway/sub-package (barrel__init__.py,__main__.py, 13_<cluster>.pysubmodules), and note thepython3 -m gatewaylaunch.docs/guides/decomposition-pattern.md: repoint the routes-handling-convention example fromgateway/gateway.pyto the now-completedgateway/gateway/barrel.docs/guides/per-agent-models.md: fix a stalegateway/gateway.py:8920line citation for thesession_createdaudit log call, nowgateway/gateway/_sessions.py:535.gateway/CLAUDE.mdalready documents the full submodule seam table (landed in the same commit range), so this PR only fixes the remaining structural/guide docs that still described the pre-split single-file layout.Skipped:
docs/architecture/{upstream-routing,credential-injection,gateway-auto-filter,orchestrator}.mdalso cite stalegateway/gateway.py:<line>references, but each is architecture reference material over the 10KB size threshold, so line-number-only fixes were left out per the doc-updater scope guidelines.Also included (branch-green fixes applied by the automated lint/format-fix steps, unrelated to the doc content but required to keep CI green on this branch):
orchestrator/event_loop/_supervisor.py: a single-line ruff-format normalization (collapses a two-line implicit-string-concatlogger.info(...)that now fits on one line). Behaviorally identical; CI lint (ruff format --check) requires it, so it cannot be dropped.scripts/check-hardcoded-ports.py: the stalegateway/gateway.pyallowlist entry is replaced with the three split files that now carry the port literals (__init__.py,_helpers.py,_server.py). Squarely in-scope for the decomposition ref sync.type: ignore[...]code-refinements in_confluence.py,_git_ops.py,_jira.py,_jira_writes.py: comment-only edits from the automated mypy-fix step (mypy --warn-unused-ignores); the annotated runtime lines are unchanged, behaviorally inert.Triggered by: 632db67 (Merge pull request #3503 from jwbron/egg/issue-3312-v2/slice-3)
Test Plan
_supervisor.pyedit is a formatting normalization verified byruff format --check)gateway/gateway/module list againstls gateway/gateway/*.pyand confirmed the_sessions.py:535citation against the currentaudit_log("session_created", ...)call site.Authored-by: egg