docs: Update local-mode PR phase behavior [doc-updater]#693
Conversation
Update documentation to reflect changes from dae1fc5: - Gateway now allows PR operations in local mode during PR phase - Orchestrator no longer switches to public mode for PR phase - Local pipelines include PR phase as terminal phase Changes: - docs/architecture/orchestrator.md: Update PR phase behavior description - docs/guides/local-quickstart.md: Document PR phase in local pipelines - README.md: Update phase permissions table, rename Merge to PR Triggered by: #689 Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Documentation Review
The documentation updates correctly reflect the architectural change from PR #689 (gateway now handles PR-phase access in local mode via phase permissions rather than mode switching). However, there is one issue that needs to be fixed.
Issue: Contradictory statement left in docs/architecture/orchestrator.md
File: docs/architecture/orchestrator.md, line 47
The bullet point for None (auto) network mode still says:
local→localfor non-PR phases (switches topublicfor the PR phase to enable push access)
This directly contradicts the paragraph updated by this PR at line 63, which correctly states:
Local-mode pipelines use
localgateway mode throughout all phases, including the PR phase.
The old text at line 47 describes the pre-#689 behavior that was removed. It should be updated to something like:
- **`None`** (auto): Falls back based on pipeline mode — `issue` → `public`, `local` → `local`
Minor observations (non-blocking)
-
docs/guides/local-quickstart.md: "During refine and plan phases, the gateway blocks push and gh operations" is slightly misleading. The gateway allowsgit pushfor state files (.egg-state/) andgh issue comment/editduring refine and plan phases perphase-permissions.json. It blocks PR operations and code pushes, not all push/gh operations. Consider: "During refine and plan phases, the gateway restricts pushes to state files and blocks PR operations." -
README.md PR phase row: Lists
gh pr create/edit/commentbutphase-permissions.jsononly listspr createandpr editas allowed operations. The gateway code does allowpr commentvia its hardcoded allowlist ingh_execute, so the README is technically accurate for what happens at runtime, but it's inconsistent with the config file. Worth aligningphase-permissions.jsonto includepr commentif that's the intended behavior (separate from this PR's scope).
— 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 thorough review. Pushed fixes in a1d5792: Blocking issue — contradictory Non-blocking observation 1 — Non-blocking observation 2 — README — 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 3a0775e → a1d5792
Previous blocking issue: Fixed
The contradictory None (auto) network mode description in docs/architecture/orchestrator.md line 47 has been corrected. The stale "switches to public for the PR phase" text is removed.
Previous non-blocking #1: Fixed
docs/guides/local-quickstart.md line 92 now accurately says "the gateway restricts pushes to state files and blocks PR operations" during refine/plan, instead of the previous overly broad claim.
Previous non-blocking #2: Acknowledged but new instance introduced
The pr comment vs phase-permissions.json inconsistency was acknowledged as out of scope, but the fix commit introduced a new instance of the same problem.
New issue: Incorrect attribution of pr comment to phase-permissions.json
File: docs/architecture/orchestrator.md, line 63
The updated text says:
During the PR phase, the gateway allows PR-specific operations (
gh pr create,gh pr edit,gh pr comment) based on phase permissions (.egg/phase-permissions.json)
This is factually incorrect. .egg/phase-permissions.json defines pr create and pr edit for the PR phase, but does not include pr comment. The pr comment operation is allowed by hardcoded gateway logic in gateway.py (the gh_pr_comment() function and gh_execute() allowlist), not by phase-permissions.json.
Suggested fix — either:
-
Remove
gh pr commentfrom the parenthetical list:...allows PR-specific operations (
gh pr create,gh pr edit) based on phase permissions... -
Or acknowledge the dual mechanism:
...allows PR-specific operations (
gh pr create,gh pr edit) based on phase permissions (.egg/phase-permissions.json), andgh pr commentvia the gateway's hardcoded allowlist...
Option 1 is simpler and avoids documenting an implementation detail that may change.
Minor observation (non-blocking)
File: README.md, line 102
The phase permissions table omits egg-contract add-decision from the Refine row, but phase-permissions.json includes it in both refine and plan phases. The table lists it only under Plan. This predates this PR (the old table didn't have it either), so it's not introduced by this change — just noting it for completeness.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Pushed fix in f3e8e63: Blocking issue — incorrect Non-blocking observation — — 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 my last review (a1d5792 → f3e8e63) is two documentation accuracy fixes — correcting the pr comment attribution in orchestrator.md and adding add-decision to the Refine row in README.md. No changes to agent interaction patterns, prompt construction, or sandbox design.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Delta from a1d5792 → f3e8e63
Previous issues: All addressed
-
Blocking — incorrect
pr commentattribution to phase-permissions.json (orchestrator.md): Fixed.gh pr commentremoved from the parenthetical list. The text now correctly statesgh pr create,gh pr editas the operations governed by phase-permissions.json. -
Non-blocking —
add-decisionmissing from Refine row (README.md): Fixed. Refine row now includesegg-contract add-decision, matchingphase-permissions.jsonwhich definesadd-decisionfor both refine and plan phases.
New changes: Verified correct
Both changes in f3e8e63 are accurate:
orchestrator.mdline 63: PR-specific operations list now matches whatphase-permissions.jsonactually defines for the PR phase (pr createandpr edit).README.mdRefine row: Now symmetric with the Plan row, both listingegg-contract add-decision. This matches the config where both phases have identical allowed operations.
Remaining known inconsistency (pre-existing, out of scope)
The PR row in README.md still lists gh pr create/edit/comment, but phase-permissions.json only defines pr create and pr edit. pr comment is allowed by hardcoded gateway logic, not phase permissions. The author acknowledged this in the first round and explicitly deferred it to a separate PR. Not a regression from this delta.
No other issues found.
— Authored by egg
|
egg review completed. View run logs 9 previous review(s) hidden. |
Update documentation to reflect changes from #689 (commit dae1fc5).
What Changed
The gateway now handles PR-phase access in local mode through phase-based permissions rather than switching gateway modes. This architectural change affects how local pipelines interact with GitHub during the PR phase.
Documentation Updates
docs/architecture/orchestrator.md:
docs/guides/local-quickstart.md:
README.md:
.egg/phase-permissions.jsonTriggered by: #689
Authored-by: egg