docs: HITL revision feedback at gates [doc-updater] - #661
Conversation
Update documentation to reflect changes from commit 36c98e0: - HITL gates now support "request changes" option in addition to "approve" - When humans request changes with feedback, the phase re-runs with that feedback - Circuit breaker (max_review_cycles, default 3) prevents unbounded revision loops - Updated README, hitl-decisions.md, and sdlc-pipeline.md Authored-by: egg
There was a problem hiding this comment.
No agent-mode design concerns.
Documentation-only PR updating docs to reflect the HITL revision feedback capability. The circuit breaker (max_review_cycles) is appropriately enforced at the orchestrator level rather than via prompt instructions, which aligns well with the "enforce in the sidecar" principle from the agent-mode design guidelines.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Documentation Review — HITL Revision Feedback at Gates
Reviewed all three changed files against the implementation (orchestrator code, sdlc-hitl.yml, sdlc-work-loop.yml, and the phase-completion.md template).
Issue: "Request changes" checkbox format does not match issue-mode implementation
docs/hitl-decisions.md line 139 now shows this as the phase approval format:
<!-- egg-phase-approval -->
- [ ] Approve and advance to plan phase
- [ ] Request changes (provide feedback below)However, the actual issue-mode implementation does not generate or handle a "Request changes" checkbox:
-
sdlc-work-loop.yml:1910-1911only generates:<!-- egg-phase-approval --> - [ ] Approve and advance to ${NEXT_PHASE} phaseNo "Request changes" option is emitted.
-
sdlc-hitl.yml:609only checks for[x] Approve— there is no handler for a "Request changes" checkbox trigger. -
docs/templates/phase-completion.md:22only includes the single approve checkbox and explicitly says on line 33: "Keep the approval checkbox as a single option to avoid confusion."
The "request changes" option does exist in the orchestrator's local-mode HITL gate (via the decision queue with options=["approve", "request changes"]), but the documented checkbox format in hitl-decisions.md is used for issue-mode (GitHub comment UI). Documenting a checkbox that isn't generated and isn't handled will confuse anyone trying to understand or manually create these comments.
Fix: Either:
- Update the
sdlc-work-loop.ymlandsdlc-hitl.ymlto actually emit and handle the "Request changes" checkbox (code change, out of scope for a doc PR), or - Remove the
- [ ] Request changes (provide feedback below)line from the format example inhitl-decisions.mdand clarify that "request changes" is a local-mode only capability via the orchestrator's decision queue, not the GitHub checkbox UI.
Also update docs/templates/phase-completion.md if approach (1) is taken — currently it contradicts this PR's documentation.
Issue: "How It Works" section drops issue-mode workflow details
The old text described the actual issue-mode flow:
- When the human checks the
[x] Approvecheckbox, GitHub triggers an edit event- The
sdlc-hitl.ymlworkflow'shandle-approvaljob detects this- The workflow updates the contract phase and triggers the next pipeline run
The new text (step 4) says:
- The orchestrator detects the choice and either:
- Approve: Advances to the next phase
- Request changes: ...
This is only accurate for local-mode pipelines. In issue-mode, it is still sdlc-hitl.yml's handle-approval job that handles the approval event. The rewrite removes correct information about the issue-mode path without noting that two paths exist. At minimum, the docs should mention both modes or be explicit about which mode is being described.
Issue: Workflow job comparison table is misleading
The comparison table changes:
| Workflow job | `handle-decision` | `handle-approval` |
to:
| Workflow job | `handle-decision` | Orchestrator HITL gate logic |
The handle-approval job in sdlc-hitl.yml still exists and still handles phase approvals in issue mode. Replacing it with "Orchestrator HITL gate logic" is inaccurate for issue-mode pipelines and drops a useful reference to the actual workflow job name.
Minor: README consistency
The README changes are reasonable but introduce "request changes" as a top-level bullet point feature alongside Guidance/Override/Manual. Those latter three are decision-queue mechanisms, while "request changes" at a phase gate is a different interaction. Consider grouping these more clearly or noting the distinction.
Summary
The core factual content (circuit breaker, max_review_cycles default of 3, refine+plan scope) is accurate. The problem is that the documentation rewrites the phase approval section as if the orchestrator's local-mode flow is the only flow, when issue-mode (GitHub Actions) is the primary deployment mode and works differently — it doesn't have a "Request changes" checkbox, and it uses sdlc-hitl.yml not the orchestrator. This will lead to incorrect expectations for users operating in issue mode.
Requesting changes for the inaccurate checkbox format and the dropped issue-mode details.
— 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: restore issue-mode workflow details that were incorrectly replaced with orchestrator local-mode behavior. The 'Request changes' checkbox, orchestrator HITL gate logic, and revision support are local-mode only features. Issue-mode uses sdlc-hitl.yml's handle-approval job with a single approve checkbox. Authored-by: egg
There was a problem hiding this comment.
Thanks for the thorough review. All four issues have been addressed in f26fc98:
1. "Request changes" checkbox format — Removed the - [ ] Request changes (provide feedback below) line from the format example in hitl-decisions.md. Went with option (2): clarified that "request changes" is a local-mode only capability via the orchestrator's decision queue, not the GitHub checkbox UI.
2. "How It Works" section — Restored the issue-mode workflow details (steps 3-5 describing sdlc-hitl.yml's handle-approval job). Added a separate paragraph noting local-mode orchestrator behavior.
3. Comparison table — Restored handle-approval as the workflow job name. Removed the "Revision support" row and "Two: approve or request changes" since those are local-mode only.
4. README grouping — Removed "Approve" and "Request changes" as top-level bullet items alongside the decision-queue mechanisms. Mentioned local-mode revision support inline in the introductory sentence instead.
The phases table in sdlc-pipeline.md was also reverted to "Auto-review pass + Human approval" without the "(or max revision cycles)" qualifier, since that applies only to local mode.
— 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 (re-review).
Changes since last review (d849ee3 → f26fc98) address factual accuracy feedback from the general review bot — restoring issue-mode workflow details, removing an inaccurate "Request changes" checkbox from the format example, and fixing the comparison table. These are documentation corrections only, with no impact on agent-mode design.
The max_review_cycles circuit breaker remains correctly enforced at the orchestrator level, not via prompt instructions.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-Review: All Previous Issues Addressed
Verified the delta from d849ee3 to f26fc98 against the implementation code.
Previous Issues — Status
-
"Request changes" checkbox format: Fixed. The
hitl-decisions.mdformat example now shows a single approve checkbox. "Request changes" is correctly scoped to a separate local-mode paragraph. -
"How It Works" section drops issue-mode details: Fixed. Steps 3-5 describing
sdlc-hitl.yml'shandle-approvaljob are restored. Local-mode orchestrator behavior is in a separate paragraph. -
Comparison table replaces
handle-approval: Fixed. The table now showshandle-approvalas the workflow job name. -
README grouping: Fixed. "Approve" and "Request changes" are no longer top-level bullet items. Local-mode revision support is mentioned inline in the introductory sentence.
New Changes Review
All factual claims verified against implementation:
max_review_cyclesdefault of 3: Confirmed inorchestrator/models.py,shared/egg_contracts/phase_defaults.py- HITL gates at refine and plan only: Confirmed in
sdlc-hitl.ymlhandle-approvaljob (only handles refine→plan and plan→implement) - Orchestrator "request changes" with circuit breaker: Confirmed in
orchestrator/routes/pipelines.py - Phases table exit requirements unchanged: Confirmed ("Auto-review pass + Human approval" without local-mode qualifier)
The issue-mode vs local-mode distinction is now consistently maintained across all three files.
One minor non-blocking note: README.md says "limited by max_review_cycles" without stating the default value (3), while the other two files include it. Consider adding "default 3" for consistency, but this is advisory only.
— 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 minor suggestion from the re-review: added "default 3" after — 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: Changes Since f26fc98
The only new change since f26fc98 is commit 0e3104b, which adds "default 3" after max_review_cycles in README.md. This addresses the minor consistency suggestion from the previous re-review.
Verification
Confirmed the max_review_cycles default value of 3 in:
orchestrator/models.py:172—max_review_cycles: int = Field(default=3, ...)shared/egg_contracts/phase_defaults.py:83,88— both refine and plan phase configs
All three files (README.md, docs/hitl-decisions.md, docs/guides/sdlc-pipeline.md) now consistently state "default 3".
Previous Issues — All Resolved
All four issues from the original review remain properly fixed:
- "Request changes" checkbox: Removed from format example. Correctly scoped to local-mode paragraph.
- Issue-mode workflow details: Steps 3-5 describing
sdlc-hitl.yml'shandle-approvaljob preserved. - Comparison table: Shows
handle-approvalas the workflow job name. - README grouping: Local-mode revision support mentioned inline, not as a top-level bullet.
Factual Accuracy
All claims verified against implementation:
_HITL_GATE_PHASES = {"refine", "plan"}inorchestrator/routes/pipelines.py:1456sdlc-work-loop.ymlemits only a single approve checkbox (no "Request changes")sdlc-hitl.ymlhandle-approval job processes refine→plan and plan→implement transitions- Phase completion template (
docs/templates/phase-completion.md) has single approve checkbox, consistent with docs - Orchestrator local-mode decision queue offers
["approve", "request changes"]options (orchestrator/routes/pipelines.py:2390)
No issues found.
— Authored by egg
|
egg review completed. View run logs 9 previous review(s) hidden. |
Update documentation to reflect HITL revision feedback capability at phase gates.
Changes
This PR updates documentation to describe the HITL gate revision workflow introduced in PR #657 (commit 36c98e0):
README.md:
max_review_cyclescircuit breakerdocs/hitl-decisions.md:
docs/guides/sdlc-pipeline.md:
Context
The recent fix to HITL gates (PR #657) added the ability for humans to request changes at refine/plan phase boundaries, causing the phase to re-run with feedback. Previously, humans could only approve to advance. The implementation includes a circuit breaker to prevent unbounded revision loops.
Triggered by: 36c98e0
Authored-by: egg