Remove act dependency from make targets

[james-in-a-box]

Summary

The SDLC pipeline checker agent runs make lint and make test inside the
sandbox, but both targets delegated to act (which requires Docker and isn't
installed in the sandbox). The checker had to discover and improvise commands,
often missing shellcheck, yamllint, mypy, and the gateway/orchestrator test
directories.

This PR replaces the act-based make targets with native commands that match
what CI actually runs. Tool resolution uses venv when available (CI) and falls
back to system PATH (sandbox). CI workflows now call make targets instead
of inline commands, so make lint / make test produce identical results
everywhere.

Changes:

• Makefile: Replace act-delegated lint/test/security with native
  sub-targets (lint-python, lint-shell, lint-yaml, lint-docker,
  lint-actions, lint-custom). Missing tools are skipped gracefully.
  Add PYTEST_ARGS for CI-specific flags (coverage).
• lint.yml: Each job calls its make lint-* target instead of inline commands.
  Restores pull_request trigger that was accidentally dropped.
• test.yml: Calls make test and make security instead of inline commands.
  Restores pull_request trigger.

Issue: none

Test plan:

• make lint-python runs ruff check + format + mypy
• make lint-shell runs shellcheck on all .sh files
• make lint-docker and make lint-actions skip gracefully when tools missing
• make test runs pytest across tests/, gateway/tests/, orchestrator/tests/
• make test PYTEST_ARGS="--cov=gateway" passes extra args to pytest
• CI workflows use the same make targets

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns.

This PR improves agent ergonomics by making make lint / make test work natively in the sandbox (replacing act delegation that required Docker-in-Docker). The checker agent can now run standard make targets directly instead of having to discover and improvise commands. No agent workflow patterns, prompts, pre-fetching, LLM API calls, or model identifiers are affected.

— Authored by egg

[egg-reviewer]

Review: Remove act dependency from make targets

Thorough review of all 43 changed files. The overall direction is sound — replacing act-delegated targets with native commands that work identically in CI, local dev, and the sandbox is the right call. The CI workflow simplification is clean, the type annotation fixes are correct, and the test mock improvements (proper exception classes for docker.errors) fix real issues.

Below are the issues I found, ordered by severity.

---

1. CONTRIBUTING.md not updated — make ci and act references are stale

CONTRIBUTING.md still references make ci (removed in this PR) and act:

| `make ci` | Run full CI pipeline | Via act, all jobs |
3. **Run the full CI locally**: `make ci`

These references are now broken. Update CONTRIBUTING.md to remove make ci and any act mentions.

---

2. export PYTHONPATH is globally scoped — leaks into all targets

export PYTHONPATH := shared:gateway:orchestrator

This exports PYTHONPATH to the environment of every recipe in the Makefile — lint-python (mypy), lint-custom, lint-docker, build, etc. The old behavior was inline per-command:

PYTHONPATH=shared:gateway:orchestrator .venv/bin/pytest tests/ ...
PYTHONPATH=shared $(VENV_BIN)/pytest integration_tests ...  # narrower scope

Two specific concerns:

• Integration tests get a broader PYTHONPATH than before (shared:gateway:orchestrator vs the old shared-only). This could mask import bugs where integration tests accidentally import from gateway or orchestrator without declaring them as dependencies.
• Lint and custom check scripts now have gateway and orchestrator on their import path, which could mask issues where those scripts import things they shouldn't.

Consider using target-specific variables instead:

test: export PYTHONPATH := shared:gateway:orchestrator
test:
	$(PYTEST) tests/ gateway/tests/ orchestrator/tests/ -v $(PYTEST_ARGS)

test-integration: export PYTHONPATH := shared
test-integration:
	$(PYTEST) integration_tests -v -m integration --timeout=300

---

3. venv prerequisite dropped from integration test targets

Old:

test-integration: venv  ## Run integration tests
test-e2e: venv  ## Run E2E tests
test-security: venv  ## Run security/pentesting tests

New:

test-integration:  ## Run integration tests
test-e2e:  ## Run E2E tests
test-security:  ## Run security/pentesting tests

The main test and security targets never depended on venv (they depended on _require-act), so dropping the dep there is fine. But test-integration, test-e2e, and test-security did depend on venv, and a developer running make test-integration on a fresh checkout will now get a cryptic "pytest not found" error instead of an automatic venv setup.

Consider restoring the venv prerequisite on these three targets.

---

4. lint-shell minor quoting concern

echo $$SHELL_FILES | tr ' ' '\n' | sort -u | xargs shellcheck --severity=warning; \

The echo $$SHELL_FILES without quoting relies on no file paths containing spaces. This works today, but the old CI approach used newline-separated values which is more robust. Since all current files lack spaces, this is non-blocking, but worth noting as a fragility.

---

5. Minor: docker_client.py restructuring is correct but worth noting

Moving ImageNotFoundError outside the try block:

# Check if image exists before attempting creation
if not self.get_image(image):
    raise ImageNotFoundError(f"Image {image} not found")

try:
    container = self.client.containers.create(...)
except ImageNotFound as e:
    raise ImageNotFoundError(f"Image {image} not found") from e

The try block still catches ImageNotFound from containers.create(), and get_image() handles ImageNotFound internally by returning None. This is functionally equivalent. The duplication of the ImageNotFound → ImageNotFoundError conversion path (once via get_image returning None, once via the except clause) is mildly redundant but harmless — containers.create() could raise ImageNotFound for a different image (e.g., if it was deleted between the check and the create call, a TOCTOU pattern) so having both is reasonable.

No action needed.

---

Non-blocking observations

• Type annotation changes (import-not-found → import-untyped): All correct. These fix mypy diagnostics by using the right error code for the actual situation (modules that exist but lack type stubs).

• Workflow concurrency groups added to lint.yml and test.yml: Good addition — cancels in-progress runs when a new push arrives.

• on-review-feedback.yml grouped GITHUB_OUTPUT writes: Clean refactor, functionally equivalent.

• release-images.yml fix: The old code set major_tag and minor_tag unconditionally even when MAJOR might be unset. The new code guards with if [[ "${IS_RELEASE}" == "true" && -n "${MAJOR:-}" ]]. This is a correctness fix.

• create-release.sh CURRENT_SHA removal: Correct — CURRENT_SHA (long form) was unused; only CURRENT_SHA_SHORT is referenced.

• pyproject.toml additions (coverage config, mypy overrides for egg_contracts.tests): Appropriate.

• Test mock improvements (test_container_monitor.py, test_tier3_execute.py): Creating proper exception class hierarchies for docker.errors instead of using bare MagicMock fixes real except clause matching issues. Good fix.

• test_checks.py _import_check_module helper: Addresses a real namespace collision issue where orchestrator/routes/checks.py shadows the checks package from .github/scripts/. Reasonable approach.

• test_entrypoint.py patches for _chdir_to_single_repo: Adapting to an API change — the tests now mock at the right seam.

---

Summary

Items 1-3 should be addressed. Item 1 (stale docs) is a straightforward fix. Items 2-3 are behavioral changes from the old Makefile that should be intentional rather than accidental. Everything else looks correct.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. All three actionable items have been addressed in ed2e6e2:

Item 1 — CONTRIBUTING.md stale references: Removed the "Install act" setup step, make ci from the command table, all "Via act" notes, and the act-based CI/Local Parity description. Updated PR process step to use make lint && make test.

Item 2 — PYTHONPATH scope: Replaced the global export PYTHONPATH with target-specific variables. lint-python, lint-custom, and test get shared:gateway:orchestrator. Integration test targets (test-integration, test-e2e, test-security) get shared only, matching the narrower scope they had before.

Item 3 — venv prerequisite: Restored venv as a prerequisite on test-integration, test-e2e, and test-security.

Items 4-5 (lint-shell quoting fragility, docker_client.py note) acknowledged — no changes needed as noted in the review.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since last review.

The changes since dfbe5e7 are limited to CONTRIBUTING.md (removing stale act references) and Makefile (scoping PYTHONPATH per-target, restoring venv prerequisites). These are build infrastructure fixes with no impact on agent workflows, prompts, or sandbox design.

— Authored by egg

[egg-reviewer]

Re-review: All feedback addressed

Verified the delta (dfbe5e7c..ed2e6e23) — a single fixup commit touching only CONTRIBUTING.md and Makefile. All three items from the previous review are properly fixed:

Item 1 — CONTRIBUTING.md stale references: All act references removed. make ci row deleted from command table. Setup steps renumbered. CI/Local Parity section rewritten without act mentions. PR process step updated to make lint && make test.

Item 2 — PYTHONPATH scope: Global export PYTHONPATH replaced with target-specific variables. lint-python, lint-custom, and test get shared:gateway:orchestrator (correct — mypy and tests need cross-package resolution). Integration test targets (test-integration, test-e2e, test-security) get shared only, matching pre-PR scope.

Item 3 — venv prerequisite: Restored on all three integration test targets.

No new issues introduced. The fixup commit is clean and limited to the two files that needed changes.

One minor note for future consideration (non-blocking): lint-python doesn't have a venv prerequisite either, same as test. This is consistent with the original PR's design (relying on tool-resolution fallback to system PATH), but worth being aware of if someone runs make lint-python on a bare checkout without global tools.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

17 previous review(s) hidden.