docs: Update docs for phase restrictions [doc-updater] - #244
Conversation
Update gateway and architecture documentation to reflect the phase-based operation filtering system introduced in PR #243. This adds: - Phase-based policy rules table showing allowed/blocked operations per phase - New phase API endpoints (/api/v1/phase/*) - Updated file listing with phase_filter.py, phase_transition.py, phase_api.py - Phase-based access control in architecture overview These updates ensure the documentation accurately reflects the new phase restrictions that prevent operations like git push during refine/plan phases and gh pr create during implement phase.
Documentation ReviewThis PR documents the phase-based operation restrictions implemented in PR #243. The documentation is largely accurate, but I found one issue that needs correction. Issue Found (Blocking)gateway/README.md: Incorrect required parameter documentation The documentation for However, in the implementation ( if not issue_number:
return make_phase_error("Missing issue_number")Fix: Change Minor Observations (Non-Blocking)
These are minor and can be addressed in a follow-up if desired. What Looks Good
Please fix the required parameter issue for the — Authored by egg |
|
egg review completed. View run logs |
|
@james-in-a-box address the above feedback |
The issue_number parameter is required, not optional, per the implementation in gateway/phase_api.py:350-351. Moved it to the front of the parameter list and removed the ? suffix to indicate required.
|
Agreed. Fixed the Regarding the minor observations:
— Authored by egg |
|
egg finished successfully. View run logs |
There was a problem hiding this comment.
Documentation Accuracy Issues
This PR documents the phase-based operation restrictions from PR #243, but there are several discrepancies between the documentation and the actual implementation.
1. Incomplete Allowed Operations Table
The phase permissions table omits several operations that are actually allowed:
Current documentation:
| Phase | Allowed Operations |
|---|---|
| refine | gh issue comment/edit, egg-contract add-decision |
| plan | gh issue comment/edit, egg-contract add-decision |
| implement | git push, egg-contract add-commit/mark-task |
| pr | gh pr create/edit, git push |
Missing from documentation:
- All phases:
egg-contract show *is allowed in refine, plan, implement, and pr phases but not documented - implement phase: Missing
egg-contract update-notes *andegg-contract mark-phase *
This could cause confusion when operators try to run egg-contract show in refine/plan phases and expect it to be blocked based on the documentation.
Suggested fix: Either:
- Add the missing operations to the table, or
- Add a note that
egg-contract showis implicitly allowed in all phases for contract state viewing
2. "Exit Requires" Column Inconsistency
The table shows:
| pr | ... | Human merge |
But the actual implementation in .egg/phase-permissions.json uses:
"exit_requires": "human"The schema only allows values: "human", "reviewer", "implementer".
"Human merge" is semantically correct (the PR phase exits when a human merges the PR), but the documentation should be consistent with how the system actually works. Consider either:
- Change to "Human approval" for consistency, or
- Add a note explaining that "human" means "human merge" in the context of the PR phase
3. Minor: Missing actor Parameter Documentation
The /api/v1/phase/advance endpoint documentation shows:
Request: {issue_number, repo_path?, reason?}
But the implementation also accepts an optional actor parameter (defaults to "agent"):
actor = data.get("actor", "agent")This is a minor omission since it has a default value, but for completeness it could be added.
The core documentation structure is good. These are accuracy issues that should be addressed before merging to avoid confusion between the documentation and the actual behavior.
— Authored by egg
|
egg review completed. View run logs |
|
@james-in-a-box if it seems appropriate, pull in context around phase 1 of this work as well #217 |
- Document contract API endpoints in gateway README - Add contract_api.py, auth.py, test_contract_api.py to file listings - Add SDLC Contracts section to architecture docs - Document role-based field ownership (implementer, reviewer, human) - Add egg_contracts library to components table Authored-by: egg
|
Agreed. Added documentation for the contract system from PR #217 (Phase 1: Contract schema and core library): gateway/README.md:
docs/architecture/README.md:
— Authored by egg |
|
egg finished successfully. View run logs |
There was a problem hiding this comment.
Re-Review: Documentation Updates for Phase Restrictions
This is a re-review of changes since commit f643257e36d8bda7012944276b9763555811025a.
Issue Addressed ✓
issue_number parameter documentation fixed: The /api/v1/phase/filter endpoint now correctly shows issue_number as required (not optional).
Outstanding Issues
1. Phase permissions table still incomplete (Previously raised, not addressed)
The phase permissions table in gateway/README.md continues to omit several operations that are actually allowed:
Documented:
| Phase | Allowed Operations |
|---|---|
| refine | gh issue comment/edit, egg-contract add-decision |
| plan | gh issue comment/edit, egg-contract add-decision |
| implement | git push, egg-contract add-commit/mark-task |
| pr | gh pr create/edit, git push |
Actually allowed per .egg/phase-permissions.json:
- All phases:
egg-contract show *(not documented anywhere) - implement phase: Also allows
egg-contract update-notes *,egg-contract mark-phase *
The author's response said these are "documented implicitly via the phase-permissions.json reference." This is insufficient—the table's purpose is to provide a quick reference of what's allowed without reading the JSON file. Either document all operations or add a note like "plus egg-contract show in all phases" to indicate implicit read access.
Fix: Add egg-contract show * to the table, or add a footnote: "Note: egg-contract show * is allowed in all phases for contract state viewing."
2. Missing actor parameter in /api/v1/phase/advance (Previously raised, not addressed)
Documentation:
POST /api/v1/phase/advance
Request: {issue_number, repo_path?, reason?}
Implementation (phase_api.py:215):
actor = data.get("actor", "agent")The optional actor parameter is not documented. Add actor? to the request schema.
3. Role-based field ownership documentation incomplete (New)
The new documentation in docs/architecture/README.md lists:
- `implementer`: `tasks[].commit`, `tasks[].notes`
- `reviewer`: `tasks[].status`, `phases[].status`, `acceptance_criteria[].verified`
Actual implementation (shared/egg_contracts/roles.py):
# Implementer also owns:
"phases.*.tasks.*.files_affected": Role.IMPLEMENTER
# Reviewer also owns:
"phases.*.review_feedback": Role.REVIEWER
"current_phase": Role.REVIEWERMissing from documentation:
implementer:files_affectedreviewer:review_feedback,current_phase
Also, human owns more than just decisions[].resolved—it also owns decisions[].resolution, decisions[].resolved_by, decisions[].resolved_at.
Summary
The blocking issue_number fix was addressed. However, the documentation accuracy issues remain. This PR adds new documentation that introduces additional discrepancies with the implementation.
For a documentation PR, accuracy is the primary requirement. Please update the phase permissions table and role ownership lists to match the actual implementation.
— Authored by egg
|
egg review completed. View run logs |
|
@james-in-a-box address the outstanding review items above |
- Add footnote noting egg-contract show is allowed in all phases - Add missing implement phase operations (update-notes, mark-phase) - Add actor? parameter to /api/v1/phase/advance endpoint docs - Complete role-based field ownership lists: - implementer: add files_affected - reviewer: add review_feedback, current_phase - human: add resolution, resolved_by, resolved_at Authored-by: egg
|
Agreed. Addressed all outstanding review items:
— Authored by egg |
|
egg finished successfully. View run logs |
There was a problem hiding this comment.
Re-Review: Documentation for Phase Restrictions and Contracts
This is a re-review of changes since commit 21ac20cec2136c18a96fc9c6cbb8f4c6892033f9.
All Previously Raised Issues Addressed ✓
1. issue_number parameter documentation — Fixed
/api/v1/phase/filternow correctly showsissue_numberas required (first parameter, no?suffix)
2. Phase permissions table — Fixed
- Added footnote: "Note:
egg-contract show *is allowed in all phases for contract state viewing" - Added missing implement phase operations:
update-notes,mark-phase
3. actor parameter for /api/v1/phase/advance — Fixed
- Documentation now shows
{issue_number, repo_path?, reason?, actor?}
4. Role-based field ownership — Fixed
implementer: Now includestasks[].files_affected✓reviewer: Now includesphases[].review_feedback,current_phase✓human: Now includesdecisions[].resolution,decisions[].resolved_by,decisions[].resolved_at✓system: Documented in architecture README ✓
Verification Against Implementation
I verified the documentation against the actual implementation:
Phase permissions (gateway/README.md lines 60-67) matches .egg/phase-permissions.json:
- refine:
gh issue comment/edit,egg-contract add-decision✓ - plan:
gh issue comment/edit,egg-contract add-decision✓ - implement:
git push,egg-contract add-commit/mark-task/update-notes/mark-phase✓ - pr:
gh pr create/edit,git push✓ - Footnote correctly notes
egg-contract show *allowed in all phases ✓
Role ownership (gateway/README.md lines 159-162, docs/architecture/README.md lines 49-53) matches shared/egg_contracts/roles.py:
- All implementer fields documented ✓
- All reviewer fields documented ✓
- All human fields documented ✓
API endpoints match implementations in phase_api.py and contract_api.py:
- Request/response schemas accurate ✓
- Required vs optional parameters correct ✓
Summary
All outstanding documentation accuracy issues from the previous review have been properly addressed. The documentation now accurately reflects the implementation of both the phase-based operation restrictions (PR #243) and the contract system (PR #217).
No blocking issues found. Ready for human review and merge.
— Authored by egg
|
egg review completed. View run logs |
docs: Update gateway and architecture docs for phase restrictions [doc-updater]
Update documentation to reflect changes from PR #243 (Phase 2: Phase-based operation restrictions).
Changes:
/api/v1/phase/advance,/api/v1/phase/filter, etc.)phase_filter.py,phase_transition.py,phase_api.pyWhy these updates are needed:
The phase-based operation restrictions fundamentally change how agents interact with git/gh during different pipeline phases. Without these docs, users would not understand why operations are blocked (e.g.,
git pushduring refine/plan phases) or how to configure phase permissions.Triggered by: #243
Authored-by: egg