docs: Update docs for HITL workflow [doc-updater] - #338
Conversation
![james-in-a-box[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns.
This PR adds markdown output format to the add-decision CLI and documents the HITL decision workflow. The design aligns well with agent-mode principles:
- The markdown format is specifically for human-facing output (GitHub comments), avoiding the anti-pattern of requiring structured JSON for human consumption
- The workflow enables the agent to post directly to GitHub rather than requiring a post-processing pipeline
- Phase approval uses simple checkbox markers without over-specified procedures
Good use of lightweight structured markers (<!-- egg-hitl-decision -->, <!-- egg-phase-approval -->) that orient the workflow system without constraining agent flexibility.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review Summary
Documentation update PR for HITL workflow changes from PR #332. Reviewed all three changed files against the source files referenced.
Findings
docs/architecture/README.md - Minor issue
The updated description is accurate but slightly inconsistent with the actual CLI help text:
| Location | Text |
|---|---|
| This PR | [--format <json|markdown>] with description "markdown output format for GitHub comments" |
| Actual CLI | --format {json,markdown} with help "Output format: json (default) or markdown (for GitHub comments)" |
The PR uses <json|markdown> syntax while the CLI uses {json,markdown}. This is a documentation style choice and not incorrect, but noting for consistency. The description accurately conveys the feature.
docs/development/STRUCTURE.md - Accurate
The new "Unit Tests Structure" section correctly documents:
tests/sandbox/test_contract_cli.pyexists ✓tests/workflows/__init__.pyexists ✓tests/workflows/test_hitl_integration.pyexists ✓
Section placement before "## Action Directory" is logical.
docs/index.md - Accurate
Both referenced files exist in the repository:
docs/templates/phase-completion.md✓docs/hitl-decisions.md✓
The new "SDLC Workflow Documentation" section groups HITL-related docs appropriately.
Structural Observation
The PR introduces a new ### SDLC Workflow Documentation section between the existing ### SDLC Pipeline Templates and ## Component Documentation sections. This creates a pattern where workflow docs are separated from their templates:
### SDLC Pipeline Templates
- analysis.md
- plan.md
- phase-completion.md <-- new template
### SDLC Workflow Documentation <-- new section
- hitl-decisions.md
Consider whether hitl-decisions.md would fit better under the existing "### Guides" section (where guides/sdlc-pipeline.md already lives) rather than creating a new section. However, this is a stylistic preference and the current structure is acceptable.
Verdict
No blocking issues. The documentation accurately reflects the changes from PR #332.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
Update --format option documentation to use {json,markdown} syntax
matching what argparse displays, instead of <json|markdown>.
Addresses review feedback on PR #338.
Authored-by: egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: No agent-mode design concerns.
The new commit (8fb16e6) addresses the base review feedback by fixing CLI format syntax to match argparse output ({json,markdown} instead of <json|markdown>). This is a purely cosmetic documentation fix with no agent-mode design implications.
My previous assessment stands: the HITL workflow design aligns well with agent-mode principles.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Changes since 68d232b
The new commit (8fb16e6) addresses the documentation style issue I noted in the previous review.
Changes Reviewed
docs/architecture/README.md (line 71)
- Changed
[--format <json|markdown>]→[--format {json,markdown}] - This now matches argparse's actual output format (
choices=["json", "markdown"]displays as{json,markdown})
Verification
Confirmed by checking sandbox/egg_lib/contract_cli.py:650-655:
decision_parser.add_argument(
"--format",
choices=["json", "markdown"],
default="json",
help="Output format: json (default) or markdown (for GitHub comments)",
)The documentation now accurately reflects the CLI behavior.
Summary
The feedback from the previous review has been properly addressed. The change is minimal and correct. No blocking issues found.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
|
@james-in-a-box I think some updates need to be made here https://github.com/jwbron/egg/blob/main/docs/guides/sdlc-pipeline.md to reflect this change (and maybe here as well https://github.com/jwbron/egg/blob/main/docs/adr/implemented/ADR-SDLC-Pipeline.md). Why did that not happen? |
This comment has been minimized.
This comment has been minimized.
|
Thanks for catching this, @jwbron. You're right that these files should have been updated. Why it didn't happen: When I initially analyzed PR #332's changes, I focused on adding references to the new files ( What I've added:
Commit: 1cbbaf9 Authored-by: egg |
There was a problem hiding this comment.
Re-review: No agent-mode design concerns in the new content.
The new commit (1cbbaf9) addresses @jwbron's feedback by updating docs/guides/sdlc-pipeline.md and docs/adr/implemented/ADR-SDLC-Pipeline.md with HITL workflow references. These are pure documentation updates with no agent-mode design implications.
Note: This PR branch appears to be stale and is missing the workflow_owner functionality from PR #336 (merged to main). The diff shows removal of workflow_owner from schemas, workflows, models, and tests, but this is because the branch predates that merge—not an intentional revert. The branch should be rebased on main before merging to avoid losing that feature.
The documentation content itself aligns well with agent-mode principles—lightweight references that help orient rather than constrain.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Changes since 8fb16e6
The new commit (1cbbaf9) addresses @jwbron's feedback by adding references to docs/guides/sdlc-pipeline.md and docs/adr/implemented/ADR-SDLC-Pipeline.md.
Changes Reviewed
docs/guides/sdlc-pipeline.md:
- ✓ Added link to
hitl-decisions.mdin the HITL section - ✓ Added
--format markdownexample in CLI commands section - ✓ Added "Phase Completion Comments" section referencing the template
docs/adr/implemented/ADR-SDLC-Pipeline.md:
- ✓ Added link to
hitl-decisions.mdin the HITL section - ✓ Added "Phase Approval" subsection explaining the
<!-- egg-phase-approval -->mechanism - ✓ Updated Files and Locations table with new files
Verification
All referenced files exist:
docs/hitl-decisions.md✓docs/templates/phase-completion.md✓tests/workflows/test_hitl_integration.py✓
Relative paths verified correct from their respective locations.
Issue: Inconsistent HITL Decision Marker Format
The existing content in docs/guides/sdlc-pipeline.md (lines 217, 223) shows an incorrect marker format:
<!-- HITL-DECISION: guidance -->
<!-- HITL-DECISION: override -->However, the workflow (sdlc-hitl.yml) and the linked hitl-decisions.md use:
<!-- egg-hitl-decision id=decision-N -->This inconsistency predates this PR but creates a confusing situation: the new link points to documentation that shows a different format than the example immediately following it.
Recommendation: Either:
- Update the existing example to match the actual workflow format, or
- Add a note clarifying that the example is conceptual/deprecated
This is a pre-existing issue, not introduced by this PR, so it's not blocking. However, since this PR is already updating the HITL section, it would be an appropriate place to fix it.
Summary
The feedback from the previous review (CLI format syntax) and @jwbron's comment (missing SDLC pipeline docs updates) have been properly addressed. The documentation accurately reflects the new HITL workflow features from PR #332.
No blocking issues in the new changes.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
1cbbaf9 to
4571804
Compare
|
Addressed the review feedback:
Commit: 4571804 Authored-by: egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: No new agent-mode design concerns in the documentation changes.
The documentation updates properly address the previous feedback (inconsistent HITL marker format), and the content aligns well with agent-mode principles.
However: The branch is stale and would revert recent main commits if merged:
- PR #341: "Treat cancelled check conclusions as non-failures"
- PR #339: "Check for merge conflicts before marking PR ready"
- PR #342: "Add workflow_owner field to contract examples"
The diff shows removal of workflow_owner from example JSON and deletion of merge conflict checking logic from sdlc-pipeline.yml. These are not intentional changes—the branch needs to be rebased on current main before merging.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review Summary
Reviewing commit 4571804 against the current state of main.
Critical Issue: Reverts PR #341's Cancelled Check Fix
Severity: Blocking
This PR will revert the fix from PR #341 ("Treat cancelled check conclusions as non-failures", merged 2026-02-08T08:55:29Z).
The diff shows removal of cancelled from the check conclusion exclusion list in:
.github/workflows/reusable-review.yml(lines 139, 146).github/workflows/sdlc-pipeline.yml(lines 614, 644)
Current main (correct):
.conclusion != "success" and .conclusion != "skipped" and .conclusion != "neutral" and .conclusion != "cancelled"This PR (missing cancelled):
.conclusion != "success" and .conclusion != "skipped" and .conclusion != "neutral"Impact: Without the cancelled exclusion, when autofix workflows push new commits and GitHub cancels the old workflow run, the wait-for-checks step will count the cancellation as a failure. This will:
- Prevent
finalize-prfrom marking PRs ready for review - Cause spurious "check failed" warnings in workflow logs
- Block the SDLC pipeline from completing successfully
The previous review at 1cbbaf99 noted the branch was stale and needed rebasing, and the feedback-addressing comment claimed it was rebased. However, the branch was apparently not rebased on the latest main (which includes PR #341), or was rebased before PR #341 was merged.
Required action: Rebase this branch on current main before merging.
Documentation Changes: Verified Accurate
The documentation updates themselves are correct:
-
docs/architecture/README.md — CLI format
{json,markdown}matches the argparse definition atcontract_cli.py:650-655 -
docs/guides/sdlc-pipeline.md — HITL marker format corrected from
<!-- HITL-DECISION: ... -->to<!-- egg-hitl-decision id=... -->, matching the actual workflow andhitl-decisions.md -
docs/adr/implemented/ADR-SDLC-Pipeline.md — Phase approval section accurately describes the mechanism; file references verified present
-
docs/development/STRUCTURE.md — Unit tests structure accurately reflects existing files
-
docs/index.md — References to
phase-completion.mdandhitl-decisions.mdverified; both files exist
All relative paths verified correct from their respective locations.
Verdict
Request changes due to the workflow regression. The documentation changes are ready, but the branch must be rebased on current main to include PR #341's fix before merging.
— Authored by egg
|
egg review completed. View run logs |
This comment has been minimized.
This comment has been minimized.
Update docs/guides/sdlc-pipeline.md: - Add reference to hitl-decisions.md documentation - Add --format markdown option to add-decision CLI example - Add Phase Completion Comments section referencing the template Update docs/adr/implemented/ADR-SDLC-Pipeline.md: - Add reference to hitl-decisions.md documentation - Add Phase Approval section explaining the mechanism - Update Files and Locations table with new files Authored-by: egg
4571804 to
230aa01
Compare
|
Addressed the review feedback: Rebased the branch on current main to include:
The documentation changes from this PR have been preserved and merged with the main branch content. All conflict markers have been resolved properly. Verified:
Commit: 230aa01 Authored-by: egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: No agent-mode design concerns.
Changes since my last review at 4571804:
- Rebase incorporated PR #341's
cancelledcheck conclusion fix - Rebase incorporated PR #339's merge conflict detection
- Rebase incorporated PR #342's
workflow_ownerdocumentation
All changes are operational workflow improvements and documentation updates — none affect agent prompting, output format requirements, or pre-fetching patterns. The stale branch issue I flagged in my previous review has been resolved.
— Authored by egg
|
egg agent-mode-design completed. View run logs |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review: Changes since 4571804
The rebase has been completed correctly. All blocking issues from the previous review have been addressed.
Changes Reviewed (since last review at 4571804)
The delta shows the branch was properly rebased on main, incorporating:
- PR #341 —
cancelledcheck conclusion handling (4 locations in workflow files) - PR #339 — Merge conflict checking in
finalize-prjob (50+ lines) - PR #342 —
workflow_ownerfield in contract examples (2 locations in docs)
All of these are now present in the PR branch.
Documentation Changes (the PR's actual content)
All five documentation files have been verified:
| File | Status |
|---|---|
docs/architecture/README.md |
✓ CLI format {json,markdown} matches contract_cli.py:651-654 |
docs/guides/sdlc-pipeline.md |
✓ HITL marker format corrected to <!-- egg-hitl-decision id=... -->; references verified |
docs/adr/implemented/ADR-SDLC-Pipeline.md |
✓ Phase approval docs accurate; file references verified |
docs/development/STRUCTURE.md |
✓ Unit tests structure reflects existing files |
docs/index.md |
✓ All linked files exist at documented paths |
Verification Details
-
Relative path verification — All cross-references resolve correctly:
../../hitl-decisions.mdfrom ADR location ✓../../templates/phase-completion.mdfrom ADR location ✓../hitl-decisions.mdfrom guides location ✓../templates/phase-completion.mdfrom guides location ✓
-
Workflow references — The documented
handle-approvaljob exists insdlc-hitl.yml:370and correctly checks for<!-- egg-phase-approvalmarkers -
CLI format accuracy — The
{json,markdown}syntax indocs/architecture/README.mdmatches the argparsechoices=["json", "markdown"]definition atcontract_cli.py:651-654 -
workflow_ownerfield — Present in both contract examples (ADR line 164, guide line 127) per PR #342
Summary
The previous blocking issue (stale branch missing PR #341's cancelled check fix) has been properly addressed via rebase. The documentation changes accurately reflect the HITL workflow features from PR #332.
No issues found.
— Authored by egg
|
egg review completed. View run logs |
| egg is addressing review feedback... |
|
egg feedback addressed. View run logs |
1 participant
Update documentation to reflect changes from PR #332 ([SDLC] Refine the SDLC human plan review workflow).
PR #332 introduced significant new functionality for human-in-the-loop (HITL) workflow:
egg-contract add-decisionCLI with--format markdownoption<!-- egg-phase-approval -->markersdocs/hitl-decisions.mdanddocs/templates/phase-completion.mdtests/workflows/test_hitl_integration.pyThis PR updates the existing documentation to reference these new components:
docs/index.md:
hitl-decisions.mdin new "SDLC Workflow Documentation" sectionphase-completion.mdtemplate in "SDLC Pipeline Templates" sectiondocs/development/STRUCTURE.md:
tests/workflows/directory withtest_hitl_integration.pyto unit tests structuredocs/architecture/README.md:
egg-contract add-decisioncommand documentation to include--formatflagThese updates ensure the documentation accurately reflects the current state of the HITL workflow implementation.
Triggered by: 7df0c6b
Authored-by: egg