docs: Document orchestrator worktree architecture [doc-updater] - #619
Conversation
Update documentation to reflect changes from PR #617 which fixed the orchestrator to read verdict/draft/check files from per-pipeline worktrees instead of the main repository. Changes: - Add "Per-Pipeline Worktrees" section to orchestrator architecture doc explaining how the orchestrator mounts and reads from .egg-worktrees - Clarify HOST_HOME comment in deployment guide to explain it's needed for orchestrator to mount .egg-worktrees and read pipeline artifacts The code changes in #617 were internal fixes to existing functionality, but the architecture was not fully documented. This update clarifies how the orchestrator interacts with gateway-created worktrees. Triggered by: #617 Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Documentation Review
The documentation structure and placement are appropriate. The "Per-Pipeline Worktrees" section logically follows the "Pipeline State Persistence" section, and the deployment.md comment improvement is helpful. However, there are factual inaccuracies that will mislead readers.
Issues
1. Review file path pattern is wrong (orchestrator.md line 54)
The docs say:
.egg-state/reviews/{issue}-{phase}-{reviewer}.json
The actual pattern in orchestrator/routes/pipelines.py:819-821 is:
.egg-state/reviews/{issue}-{phase}-{reviewer_type}-review.json
Two errors: the variable is reviewer_type (not reviewer), and the filename has a -review suffix before .json. A reader using this doc to locate or construct review file paths will fail.
2. Draft file path pattern is inaccurate (orchestrator.md line 53)
The docs say:
.egg-state/drafts/{issue}-{analysis|plan}.md
The actual patterns in orchestrator/routes/pipelines.py:839-850 are:
.egg-state/drafts/{issue}-analysis.md(refine phase).egg-state/drafts/{issue}-{phase}.md(other phases, e.g. plan)- No draft for
implementphase (return None)
The {analysis|plan} notation suggests only two possible values, but the code accepts any phase name. Additionally, analysis is not a phase name — it's a special case mapping from the refine phase. The docs should reflect the actual logic.
3. Check file path is overly vague (orchestrator.md line 55)
The docs say:
.egg-state/checks/*.json
The actual path used in orchestrator/routes/pipelines.py:1506 is specifically:
.egg-state/checks/implement-results.json
A glob pattern suggests many check files; the code reads exactly one. This matters for someone trying to understand the data flow.
4. "read-only" claim for orchestrator mount is incorrect (orchestrator.md line 48)
The docs say:
Orchestrator mounts
/home/egg/.egg-worktrees(read-only)
In docker-compose.yml:133, the orchestrator volume mount is:
- ${HOST_HOME:-/home/egg}/.egg-worktrees:/home/egg/.egg-worktreesThere is no :ro flag — this is a read-write mount. The gateway mount (docker-compose.yml:75) is also read-write. Whether the orchestrator should mount read-only is a separate question, but the docs should not state a restriction that isn't enforced.
deployment.md change
The HOST_HOME comment update (deployment.md line 73) is accurate and an improvement over the previous generic comment. No issues.
Summary
The four issues above are all factual inaccuracies in the new documentation section. Since this PR's purpose is to document the worktree architecture correctly, these need to be fixed before merge.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Address review feedback on PR #619: - Fix review file path pattern: use {reviewer_type}-review.json - Fix draft file path: document refine→analysis mapping and implement→None - Fix check file path: use specific implement-results.json, not glob - Remove incorrect read-only claim for orchestrator mount
There was a problem hiding this comment.
All four issues fixed in 436776c:
- Review file path: Updated to
{issue}-{phase}-{reviewer_type}-review.json— fixed both the variable name and the-reviewsuffix. - Draft file path: Now documents the actual logic —
refinemaps toanalysis,implementreturnsNone, other phases use{phase}. - Check file path: Replaced glob pattern with the specific
implement-results.jsonpath. - Read-only claim: Removed the incorrect
(read-only)annotation sincedocker-compose.ymlhas no:roflag on the orchestrator mount.
— 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. The delta since last review (436776c) fixes factual inaccuracies in documentation per the other reviewer's feedback — file path patterns, phase mapping logic, and mount flag accuracy. No agent workflows, prompts, or design patterns are affected.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-Review — All Issues Fixed
Verified the delta from f8951e8 → 436776c against the source code. All four issues from the previous review have been correctly addressed:
- Review file path — Fixed. Now matches
orchestrator/routes/pipelines.py:822:{issue}-{phase}-{reviewer_type}-review.json. - Draft file path — Fixed. Documents the
refine→analysisspecial case,implement→None, and generic{phase}fallback, matchingorchestrator/routes/pipelines.py:824-850. - Check file path — Fixed. Replaced glob with the specific
implement-results.json, matchingorchestrator/routes/pipelines.py:1506. - Read-only claim — Fixed. Removed the incorrect
(read-only)annotation.docker-compose.yml:133has no:roflag.
No new issues found in the updated content. The contract path (.egg-state/contracts/{issue}.json) is correct per shared/egg_contracts/loader.py:57. The WORKTREE_BASE_DIR and gateway/worktree_manager.py cross-references exist. The integration test named volume claim is accurate (integration_tests/*/docker-compose.yml). The deployment.md HOST_HOME comment improvement is factually correct.
LGTM — no blocking issues.
One minor observation (non-blocking): the "Key files read from worktrees" section lists contracts, but the orchestrator also writes contracts to worktrees (orchestrator/routes/pipelines.py:1756-1778). The heading "read from" is technically incomplete. Consider "Key artifact files in worktrees" or similar if you want precision, but this is cosmetic.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Agreed. Renamed the heading from "Key files read from worktrees" to "Key artifact files in worktrees" in 059eecc, since the orchestrator also writes contracts to worktrees. — Authored by egg |
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.
Re-Review — Delta from 436776c → 059eecc
The only change since my last review is the heading rename: "Key files read from worktrees" → "Key artifact files in worktrees" (commit 059eecc). This addresses my previous non-blocking suggestion about the heading being incomplete since the orchestrator also writes contracts to worktrees.
Verification
Re-verified all documented claims against source code. Everything remains accurate:
- WORKTREE_BASE_DIR: Matches
orchestrator/routes/pipelines.py:60andgateway/worktree_manager.py:45 - Review file path: Pattern matches
pipelines.py:819-821 - Draft file paths:
refine→analysis,implement→None, generic{phase}all matchpipelines.py:839-850 - Check results path: Matches
pipelines.py:1506 - Contract path: Matches
shared/egg_contracts/loader.py:23,57 - Volume mount: No
:roflag indocker-compose.yml:133— correctly not claimed as read-only - Integration test volumes: Named
worktreesvolume confirmed in both integration test docker-compose files
No issues found. LGTM.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs 10 previous review(s) hidden. |
Update documentation to reflect changes from PR #617 which fixed the orchestrator to read verdict/draft/check files from per-pipeline worktrees instead of the main repository.
Changes
.egg-worktreesto access pipeline artifacts written by agent containersHOST_HOMEcomment to explain it's needed for orchestrator to mount.egg-worktreesand read pipeline artifactsContext
The code changes in #617 were internal fixes to existing functionality (the orchestrator was reading from the wrong path), but the worktree architecture was not fully documented. This update clarifies how the orchestrator interacts with gateway-created per-pipeline worktrees.
Triggered by: #617
— Authored by egg