Add per-repo check commands for multi-repo SDLC pipeline

[james-in-a-box]

Add per-repo checks config for multi-repo pipelines

The SDLC pipeline checker step currently discovers test/lint commands by
searching for build config files (Makefile, package.json, etc.). This
works for the egg repo but is unreliable for arbitrary repos. This change
lets users configure explicit check commands per-repo in
repositories.yaml, so the checker runs exactly the right commands.

The checks field in repo_settings accepts a list of {name, command}
pairs. These are serialized to EGG_REPO_CHECKS env var (following the
EGG_HOST_REPO_MAP pattern), passed through docker-compose to the
orchestrator, and injected into the checker prompt. When no checks are
configured, the checker falls back to discovery mode. Both checker and
autofix prompts now include repo context (name and working directory) for
multi-repo clarity.

Issue: #603

Test plan:

• pytest tests/config/test_repo_config.py — 37 tests pass (6 new)
• ruff check and ruff format pass on all modified files
• Verify repositories.yaml.example has clear examples
• Integration tests include EGG_REPO_CHECKS in compose env

Authored-by: egg

[egg-reviewer]

Review of PR #618: Add per-repo check commands for multi-repo SDLC pipeline

Overall a solid and well-structured change. The design is clean — config in YAML, serialized to env var, consumed at runtime — and the code follows existing patterns in the codebase. Tests cover the important cases. I found a few issues worth addressing before merging.

---

1. No validation of _repo_checks from env var (correctness, robustness)

File: orchestrator/routes/pipelines.py:2001-2012

The orchestrator reads EGG_REPO_CHECKS from the environment, deserializes it with json.loads, and passes it directly to _build_checker_prompt without any validation. Unlike get_repo_checks() in config/repo_config.py (which validates each entry has name and command, filters non-dicts, and coerces to str), the orchestrator trusts whatever is in the env var.

If the env var is malformed — e.g., a check entry is missing "command", or has non-string values — _build_checker_prompt will crash at line 1509 with a KeyError on check['name'] or check['command'].

Suggested fix: Validate the deserialized checks using the same logic as get_repo_checks(), or extract the validation into a shared helper:

def _validate_checks(checks: list) -> list[dict[str, str]]:
    return [
        {"name": str(c["name"]), "command": str(c["command"])}
        for c in checks
        if isinstance(c, dict) and "name" in c and "command" in c
    ]

Then apply it after the env var lookup:

if isinstance(_cfg_checks, list):
    _repo_checks = _validate_checks(_cfg_checks)

2. Duplicated validation logic (design)

Files: config/repo_config.py:320-324, sandbox/egg_lib/compose.py:152-156

The check validation logic (filter for dicts with name and command, coerce to str) is duplicated between get_repo_checks() and _generate_env_file(). And as noted above, it's missing entirely from the orchestrator consumer. This is three places that need to stay in sync.

Consider centralizing this in get_repo_checks() and calling it from compose.py, or extracting the validation into a standalone function that all three sites can use. This is a minor design nit but worth noting given that the orchestrator already has a bug from not duplicating it.

3. get_repo_checks() is defined but never imported by any consumer

File: config/repo_config.py:302

The get_repo_checks() function is added to repo_config.py but nothing imports or uses it outside of tests. The orchestrator reads from EGG_REPO_CHECKS env var instead, and compose.py does its own YAML parsing. This function is dead code in production. Either remove it and test the actual code paths, or use it in compose.py to eliminate the duplication noted above.

4. EGG_REPO_CHECKS is parsed on every autofix loop iteration

File: orchestrator/routes/pipelines.py:2001-2012

The repo checks lookup, including json.loads(os.environ.get("EGG_REPO_CHECKS", "{}")), runs inside the if current_phase.value == "implement" block, which itself is inside the phase loop. It's then re-parsed every time the autofix loop runs (up to 3 iterations). The env var won't change during a pipeline run. Move the lookup before the for autofix_attempt loop — which it already is positionally, so this is fine. Disregard, I re-read and confirmed it's correctly placed before the loop. No issue here.

5. Leading underscore convention on local variables

File: orchestrator/routes/pipelines.py:2001-2012

The variables _repo_checks, _all_repo_checks, _repo_lower, _cfg_repo, _cfg_checks all use leading underscores. In Python, leading underscores conventionally denote module-private or "don't touch" attributes. For plain local variables in a function body, this is unusual and slightly misleading. No functional impact, but repo_checks_config or similar would be clearer.

6. Missing test for _build_checker_prompt with repo_checks parameter

The new repo_checks parameter changes the prompt structure significantly (from discovery mode to explicit commands mode), but there are no unit tests for _build_checker_prompt() or _build_autofix_prompt() with the new parameters. The TestGetRepoChecks tests only cover the config layer. Consider adding tests that verify the prompt output contains the configured commands when repo_checks is provided, and falls back to discovery instructions when it's None.

7. sys.path manipulation in test file

File: tests/config/test_repo_config.py:14-17

_project_root = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
if _project_root not in sys.path:
    sys.path.insert(0, _project_root)

This is a module-level side effect that modifies sys.path for the entire test process. If pytest is already configured with the correct paths (via pyproject.toml, conftest.py, or pytest.ini), this is unnecessary. If it is needed, it suggests the test infrastructure needs fixing rather than each test file patching the path. Check whether the existing tests in this file already work without this addition — the other test classes don't import from config.repo_config so they wouldn't have needed it.

---

Summary

The main blocking issue is #1: the orchestrator doesn't validate checks from the env var, which will cause a runtime crash on malformed data. Issues #2 and #3 are closely related — the validation duplication and dead get_repo_checks() function should be resolved together. The rest are non-blocking suggestions.

— Authored by egg

[james-in-a-box]

Addressed all actionable feedback from the review. Changes pushed in 6f63a28.

#1 — No validation of env var checks (fixed): Added inline validation in the orchestrator after deserializing EGG_REPO_CHECKS. Malformed entries (missing name/command, non-dict items) are now filtered out, matching the same logic used in get_repo_checks().

#2 — Duplicated validation logic (fixed): Extracted validate_checks() as a shared helper in config/repo_config.py. Both get_repo_checks() and compose.py now call it instead of duplicating the filter logic.

#3 — get_repo_checks() unused in production (fixed): compose.py now imports validate_checks from config.repo_config, giving the module a real consumer. get_repo_checks() itself remains useful as the high-level API for reading checks from YAML config.

#4 — Env var parsed per loop iteration: Confirmed by reviewer as not an issue. No change.

#5 — Leading underscore convention (fixed): Renamed _repo_checks, _all_repo_checks, _repo_lower, _cfg_repo, _cfg_checks to drop the leading underscores.

#6 — Missing prompt builder tests (fixed): Added orchestrator/tests/test_pipeline_prompts.py with 10 tests covering _build_checker_prompt (discovery mode, explicit checks mode, with/without repo, metadata) and _build_autofix_prompt (with/without repo, failure rendering, no-failure case).

#7 — sys.path manipulation in test file (fixed): Removed the sys.path.insert block from tests/config/test_repo_config.py. The root tests/conftest.py already adds config/ to sys.path, so the import works without it.

— Authored by egg

[egg-reviewer]

Re-review of PR #618 (commit 6f63a28)

Most of the previous feedback was addressed well. One issue remains.

---

Validation logic still duplicated in the orchestrator (issues #1 + #2 from previous review)

File: orchestrator/routes/pipelines.py:2010-2015

The previous review raised two related issues: (1) the orchestrator didn't validate env var checks, and (2) validation logic was duplicated across sites. The fix addressed #1 by adding inline validation in the orchestrator, and addressed #2 by extracting validate_checks() and having compose.py call it. But the orchestrator itself still has an inline copy of the same logic:

repo_checks = [
    {"name": str(c["name"]), "command": str(c["command"])}
    for c in cfg_checks
    if isinstance(c, dict) and "name" in c and "command" in c
]

This is the same list comprehension that validate_checks() wraps. The orchestrator should import and call validate_checks() instead. The orchestrator container has config/ on its Python path (it already imports egg_config from shared/), but if the import path is a concern, validate_checks is a pure function with no dependencies — it could live in shared/ instead.

Suggested fix:

from config.repo_config import validate_checks
# ...
if cfg_repo.lower() == repo_lower:
    if isinstance(cfg_checks, list):
        repo_checks = validate_checks(cfg_checks) or None
    break

The or None preserves the current behavior where repo_checks stays None when validation produces an empty list (so the checker falls back to discovery mode).

This is a minor issue — the code is correct as written. But the whole point of extracting validate_checks() was to have a single definition, and right now there are still two copies.

---

Verification of previously raised issues

#	Issue	Status
1	No validation of env var checks	Fixed — validation added inline
2	Duplicated validation logic	Partially fixed — compose.py uses validate_checks(), orchestrator still has inline copy (see above)
3	get_repo_checks() unused	Addressed — validate_checks() now has a real consumer; get_repo_checks() retained as high-level API
4	Env var parsed per loop iteration	N/A (non-issue confirmed)
5	Leading underscore convention	Fixed
6	Missing prompt builder tests	Fixed — 10 tests added covering both functions
7	sys.path manipulation in test	Fixed — removed

New code assessment

The new test_pipeline_prompts.py test file is well-structured. The sys.modules.setdefault approach for mocking docker is appropriate for isolated test execution — it avoids pulling in the real docker SDK while letting pipelines.py import cleanly. Tests cover both prompt builders across discovery mode, explicit checks mode, with/without repo, and edge cases.

The validate_checks() extraction in repo_config.py is clean — good docstring, handles non-list input defensively, and get_repo_checks() delegates to it properly.

No new correctness, security, or robustness issues found in the delta.

— Authored by egg

[james-in-a-box]

Addressed the remaining feedback from the re-review. Changes pushed in 559bc52.

Validation logic still duplicated in orchestrator (issue #2, partial fix) — now fixed:

Moved validate_checks() to shared/egg_config/validators.py as the single canonical definition. All three consumers now import from this shared location:

• orchestrator/routes/pipelines.py: Imports from egg_config.validators (with an ImportError fallback matching the existing pattern for other egg_config imports)
• config/repo_config.py: Imports from egg_config.validators with fallback to a local definition (since config/ can run standalone without egg_config on the path)
• sandbox/egg_lib/compose.py: Imports from egg_config.validators instead of config.repo_config

The orchestrator's inline list comprehension is replaced with validate_checks(cfg_checks) or None, preserving the existing behavior where an empty validated list falls back to None (discovery mode).

All 47 tests pass (37 config + 10 prompt builder). Linting clean.

— Authored by egg

[egg-reviewer]

Re-review of PR #618 (commit 559bc52)

The remaining feedback from the previous re-review — centralizing validate_checks instead of keeping inline copies — has been addressed. The canonical definition now lives in shared/egg_config/validators.py, and consumers import from there. One structural issue introduced by this change.

---

validate_checks import is bundled into an unrelated try/except block

File: orchestrator/routes/pipelines.py:59-78

The new from egg_config.validators import validate_checks (line 65) was added inside the existing try block that imports network constants:

try:
    from egg_config import (
        ORCHESTRATOR_EXTERNAL_IP,
        ORCHESTRATOR_ISOLATED_IP,
        ORCHESTRATOR_PORT,
    )
    from egg_config.validators import validate_checks  # <-- added here
except ImportError:
    ORCHESTRATOR_ISOLATED_IP = "172.32.0.3"
    ORCHESTRATOR_EXTERNAL_IP = "172.33.0.3"
    ORCHESTRATOR_PORT = 9849

    def validate_checks(checks: list) -> list[dict[str, str]]:  # type: ignore[misc]
        ...

If the egg_config constants import succeeds but the validate_checks import fails (e.g., running tests against an older egg_config snapshot that doesn't have validate_checks yet), the except ImportError fires and overwrites the constants with hardcoded fallback values. The previous code (before this commit) had a clean try/except scoped only to the constants.

The fix is straightforward — use a separate try/except for validate_checks, matching the pattern used in config/repo_config.py:

try:
    from egg_config import (
        ORCHESTRATOR_EXTERNAL_IP,
        ORCHESTRATOR_ISOLATED_IP,
        ORCHESTRATOR_PORT,
    )
except ImportError:
    ORCHESTRATOR_ISOLATED_IP = "172.32.0.3"
    ORCHESTRATOR_EXTERNAL_IP = "172.33.0.3"
    ORCHESTRATOR_PORT = 9849

try:
    from egg_config.validators import validate_checks
except ImportError:
    def validate_checks(checks: list) -> list[dict[str, str]]:  # type: ignore[misc]
        ...

In practice the risk is low since both modules are COPY'd from the same repo in the orchestrator Dockerfile, but coupling unrelated imports into a single try/except is a structural issue — the constants fallback should not depend on whether an unrelated function exists.

---

Verification of previously raised issue

#	Issue	Status
2	Duplicated validation logic (orchestrator inline copy)	Fixed — orchestrator calls validate_checks() at line 2022, inline comprehension removed

No new correctness, security, or robustness issues found in the delta. The validate_checks() placement in shared/egg_config/validators.py is appropriate, the or None semantics at the call site are correct, and the compose.py import change is clean.

— Authored by egg

[james-in-a-box]

Addressed the remaining feedback from the re-review. Changes pushed in 62e12ae.

validate_checks import bundled into unrelated try/except (fixed):

Split into a separate try/except block so the validate_checks import has its own fallback, independent of the network constants. If egg_config.validators fails to import while egg_config constants succeed, the constants are no longer overwritten with hardcoded fallbacks.

All 47 tests pass (37 config + 10 prompt builder). Linting clean.

— Authored by egg

[egg-reviewer]

Re-review of PR #618 (commit 62e12ae)

The remaining suggestion from the previous re-review has been addressed correctly.

---

Verification of previously raised issue

#	Issue	Status
Bundled try/except for validate_checks import	Fixed — validate_checks import now has its own try/except block (lines 70-81), independent of the network constants block (lines 59-68). If egg_config.validators fails to import, the constants are no longer overwritten with hardcoded fallbacks.

Assessment of the fix

The change is minimal and correct:

• The from egg_config.validators import validate_checks import was moved out of the constants try block into its own try/except ImportError block.
• The fallback validate_checks definition is identical to the canonical version in shared/egg_config/validators.py (same isinstance guard, same list comprehension, same coercion).
• The blank line between except ImportError: and the def follows the existing pattern in this file (e.g., lines 23-26 for the get_logger fallback).
• No other files were modified in this commit.

No new correctness, security, or robustness issues found.

— Authored by egg

[jwbron]

@james-in-a-box this should be configured in the setup flow, same as when the repos themselves are configured.

[james-in-a-box]

Added per-repo check commands to the interactive setup flow (egg --setup). Changes pushed in bf14902.

What changed:

The _create_repositories_config() step now calls a new _configure_repo_checks() helper after collecting writable repos. For each writable repo, it asks:

1. Whether to configure explicit check commands (default: no, use auto-discovery)
2. If yes, prompts for name/command pairs until the user presses Enter on an empty name

The resulting checks are stored in repo_settings in the generated repositories.yaml, matching the format already supported by get_repo_checks() and the pipeline checker.

Files modified:

• sandbox/egg_lib/setup_flow.py — New _configure_repo_checks() function; called from _create_repositories_config() after branch prefix step
• tests/sandbox/test_setup_flow.py — 6 new tests covering: decline all repos, single repo, multiple repos, empty command skip, no valid checks, empty repo list

All 35 setup flow tests pass (6 new). All 37 repo config tests pass. Linting clean.

— Authored by egg

[egg-reviewer]

Re-review of PR #618 (commit bf14902)

This commit adds per-repo check commands to the interactive setup flow (egg --setup), as requested by the repo owner. Two files changed: sandbox/egg_lib/setup_flow.py (new _configure_repo_checks() function + integration into _create_repositories_config()) and tests/sandbox/test_setup_flow.py (6 new tests).

No correctness, security, or robustness issues found. Two non-blocking suggestions below.

---

1. _configure_repo_checks is unconditionally called during setup (design, non-blocking)

File: sandbox/egg_lib/setup_flow.py:427-429

effective_writable = writable_repos if writable_repos else [f"{github_username}/egg"]
repo_settings = _configure_repo_checks(effective_writable)

Every --setup run now prompts the user about check commands for every writable repo, even on first setup when the user might not know what commands to configure yet. The function prints a 4-line explanation block and asks a yes/no per repo, so the overhead is minimal — the default is "no" and users can skip through quickly. But if the writable repo list is long, this adds friction to an already lengthy interactive flow.

Consider gating this behind a top-level "Configure SDLC check commands? (yes/no) [no]:" prompt, similar to how other optional features are handled. This way users who don't use the SDLC pipeline (or aren't ready to configure it) skip the entire section with a single keystroke. Not blocking since the current UX is functional and the default is to skip.

2. No integration test covering _create_repositories_config with checks (test coverage, non-blocking)

File: tests/sandbox/test_setup_flow.py

The 6 new unit tests for _configure_repo_checks are well-structured and cover the function's edge cases. However, there's no test that exercises _create_repositories_config() end-to-end with check commands — i.e., verifying that the generated repositories.yaml file actually contains a repo_settings section with the configured checks. The existing TestSetup.test_full_setup_flow mocks _create_repositories_config entirely, so it doesn't verify the integration.

This matters because the wiring between _configure_repo_checks and the config dict at line 442 ("repo_settings": repo_settings) is untested — if someone refactors this function and accidentally drops the repo_settings key, no test would catch it. A single integration-style test that reads back the YAML and asserts repo_settings contains expected checks would close this gap.

---

Verification of previous issues

All 7 issues from the original review were addressed in prior commits and remain fixed. No regressions introduced by this commit.

Code assessment

• Output format is correct: _configure_repo_checks produces {"owner/repo": {"checks": [...]}} which matches the structure expected by compose.py (settings.get("checks")) and repo_config.py (get_repo_setting(repo, "checks")).
• Input handling is solid: Empty names break the loop, empty commands skip with a warning, the function gracefully returns {} when no checks are configured.
• Tests cover the important paths: decline all, single repo, multiple repos, empty command skip, no valid checks, empty repo list.
• All 35 setup flow tests pass.

— Authored by egg

[james-in-a-box]

Addressed both suggestions from the latest review in cedd201:

#1 — Gate _configure_repo_checks behind top-level prompt (fixed): Added a "Configure SDLC check commands? (yes/no) [no]:" prompt at the top of _configure_repo_checks. Users who don't use the SDLC pipeline skip the entire section with a single keystroke. Empty repo lists return early before prompting.

#2 — Integration test for _create_repositories_config with checks (fixed): Added TestCreateRepositoriesConfigWithChecks that exercises the full _create_repositories_config() flow end-to-end, reads back the generated YAML, and asserts the repo_settings section contains the expected check commands.

All 84 tests pass (37 setup flow + 37 repo config + 10 pipeline prompts).

— Authored by egg

[egg-reviewer]

Re-review of PR #618 (commit cedd201)

Delta reviewed

Two files changed since the last review at bf14902:

• sandbox/egg_lib/setup_flow.py — Added early return for empty writable_repos and a top-level gate prompt before per-repo check configuration.
• tests/sandbox/test_setup_flow.py — Updated existing tests to account for the gate prompt, added two new unit tests for gate behavior, and added an integration test for _create_repositories_config that exercises the full flow including check commands.

Assessment

The gate prompt is a good UX addition — it avoids presenting per-repo prompts to users who don't want to configure explicit check commands. The default is "no" (consistent with other prompts in the setup flow), and the early return for empty writable_repos is correct.

The integration test (TestCreateRepositoriesConfigWithChecks) exercises the complete _create_repositories_config path including the repo checks configuration. Input sequence is correct — I traced through each input against the code. The test uses /dev/null as a local repo path to trigger the validation path without needing a real git repo, and correctly asserts the generated YAML structure.

Verification of all previously raised issues

#	Issue	Status
1	No validation of env var checks in orchestrator	Fixed — validate_checks() called at pipelines.py:2018
2	Duplicated validation logic	Fixed — canonical validate_checks() in shared/egg_config/validators.py, imported by all three consumers
3	get_repo_checks() unused in production	Addressed — validate_checks() has real consumers; get_repo_checks() retained as high-level API
4	Env var parsed per loop iteration	N/A (non-issue confirmed)
5	Leading underscore convention	Fixed
6	Missing prompt builder tests	Fixed — 10 tests in orchestrator/tests/test_pipeline_prompts.py
7	sys.path manipulation in test file	Fixed — removed
8	validate_checks import bundled into unrelated try/except	Fixed — separate try/except blocks in pipelines.py

All fixes from previous review rounds are intact in the latest commit.

No new correctness, security, or robustness issues found in the delta.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

13 previous review(s) hidden.