feat(FAR-772): compose e2e coverage for Runners status strip states

[farnalabs]

Summary

Adds docker-marked e2e coverage for the three Runners status strip states against the FAR-773 compose rig, as a new file backend/tests/docker/test_runners_strip_e2e.py (the only backend change; no production code touched).

Each scenario drives the real probe tick (run_runner_health_probe) and reads the result through the running FastAPI app's GET /api/v1/runners/status read model:

1. engine down — MODULO_DOCKER_HOST points at a dead port; the tick records engine_unreachable, the strip aggregate shows engine_unreachable, the runner profile reads available=false, and the concurrency preflight reads unknown. Runs on any reachable local engine (no rig).
2. rig up — MODULO_DOCKER_HOST points at the socket-proxy; with alpine:3.20 present in the outer store (pre-pulled, the same seeding the CI job performs), the tick records healthy, the strip shows healthy, the profile is offered, and the preflight reads ok.
3. engine kill — MODULO_DOCKER_HOST points at the nested dind engine; with a workspace provisioned inside dind the first tick reads healthy; after killing the dind engine the second tick reads engine_unreachable, and the healthy→unreachable transition surfaces both an in-app notification (category=runner) and an error-dashboard event (signal=runner_unavailable), asserted as +1 count deltas so prior rows never matter.

Posture (mirrors production)

• The probe writes org-scoped cache rows as the DB superuser (BYPASSRLS — the testcontainer equivalent of modulo_system).
• The web app reads the strip through a non-superuser engine that SET ROLEs to a dedicated modulo_runners_e2e_app role, so RLS actually scopes what the strip sees, exactly like the modulo_app runtime role in production.
• Postgres is a module-scoped testcontainer migrated to heads by alembic (roles modulo_migrate/modulo_breakglass/modulo_app provisioned first, same as the integration suite).

Required local run

docker compose -f deploy/compose/runner-ci.yml --profile runner-ci up -d
cd backend
uv run pytest tests/docker/test_runners_strip_e2e.py -m docker --tb=short -q --timeout=600

Scenarios 2–3 skip cleanly without the rig (env MODULO_RUNNER_HARNESS_PROXY_HOST / MODULO_RUNNER_HARNESS_DIND_HOST); the docker-marked autouse engine gate skips everything without a Docker engine.

⚠️ Discrepancy with the task's CI assumption (not fixed — out of scope)

The task stated the new docker-marked file would be "picked up automatically by the container rig CI workflow". That is not true for this repo:

• .github/workflows/runner-ci-harness.yml invokes the suite with an explicit single-file pin, uv run --no-build --no-sync pytest tests/docker/test_bundled_runner_harness.py -m docker ..., not tests/docker as a directory.
• Its path trigger (paths filter) also lists only backend/tests/docker/test_bundled_runner_harness.py.

So runner-ci-harness.yml will not execute this new file. No CI/workflow change was made because workflow changes were explicitly out of scope for this PR. Recommend a follow-up that widens the harness job to pytest tests/docker -m docker (or adds the new file to the pin + paths list).

Verification performed here

• ruff check and ruff format --check: clean.
• tests/architecture/test_test_suite_quality.py (the quality gate — no empty-container comparisons, no compound boolean asserts, no computed/blocking sleeps): full suite passes.
• pytest --collect-only -q: 3 tests collected; module imports cleanly.
• All applicable cross-platform pre-commit hooks run manually on the change (ruff, ruff-format, bandit, gitleaks, import-linter, and the pure-file checks): passed.
• Did NOT run the docker suite end-to-end: this authoring sandbox has no Docker engine and no PostgreSQL, so testcontainers Postgres and the rig/dind can't execute here. The engine-dependent scenarios must run in the rig-backed CI/local environment listed above.

Bugs found

• None in product code that this PR did not already avoid. (The workflow pin discrepancy above is an operational/CI gap, reported separately.)

[farnalabs]

Review feedback (FAR-772 e2e coverage) — feedback only, not the formal merge decision.

Overall: solid addition. The three scenarios drive the REAL probe tick (run_runner_health_probe) and read results through the real GET /api/v1/runners/status endpoint with an RLS-scoped app role — exactly the round-trip coverage this state machine needed (engine-down, rig-up, and the healthy→unreachable transition asserting +1 deltas for both the in-app notification and the error-dashboard event).

Minor notes (non-blocking):

1. Line 70: _DINO_LABEL looks like a typo of _DIND_LABEL. The value (runner-ci-dind) matches the compose label in deploy/compose/runner-ci.yml, so behavior is correct — just the constant name reads oddly.
2. _kill_local_container_by_label kills the shared rig's dind container and never restarts it. Module ordering currently puts this file last among the docker modules, and test_bundled_runner_harness.py:633-634 uses the same kill-by-label pattern, but if another docker-marked module later needs dind (or if docker tests ever run under xdist on parallel workers), this kill becomes a cross-test hazard. Worth a comment in the module docstring.
3. Scenario 1 asserts result['transitions'] == 1 for the FIRST down probe. That is correct today (no prior row → defaults to 'was reachable'), but it depends on this test running before any other test probes a reachable row for _MACHINE_ID within the module. A one-line comment pinning that assumption would help.
4. _AllFeatures is now duplicated in ~7 test files (integration/conftest.py, bdd, unit). Consider extracting to tests/helpers when convenient — not required in this PR.

No production code changes; the pytest.skip guards are conditional env-based opt-ins mirroring the existing harness test convention (rig absent → clean skip), not silent test disabling.

[modulo-reviewbot]

APPROVE - PR #356 (head 3b1e815)

Adds a single new e2e test file (backend/tests/docker/test_runners_strip_e2e.py, 655 lines) covering the Runners status strip states (FAR-772). The tests drive the real probe tick (run_runner_health_probe) and read through the real GET /api/v1/runners/status endpoint with an RLS-scoped non-superuser app role, asserting engine-down -> engine_unreachable, rig-up -> healthy/offered, and the healthy->unreachable transition emitting both an in-app notification and an error-dashboard event (+1 deltas). Asserts match the actual response model fields in backend/src/modulo/api/routes/runners.py (aggregate_state, image_checks, probe_error, engine_cpu_count, engine_mem_total_mb, preflight).

No production code, no frontend changes, no test deletions; the pytest.skip guards are conditional env-based opt-ins mirroring the existing harness test convention. CI was pending on parallel checks; proceeded per policy on the current head SHA.

Non-blocking follow-ups (from review):

• _DINO_LABEL is a typo of _DIND_LABEL (value is correct, only the constant name reads oddly).
• _kill_local_container_by_label kills the shared rig's dind container without restart; add a docstring note re: cross-test hazard for future dind-dependent modules / parallel xdist.
• transitions==1 relies on definition-order first probe for _MACHINE_ID; a one-line comment would pin that assumption.
• _AllFeatures stub is a ~7th duplicate across tests; consider extracting to tests/helpers.

[farnalabs]

Automated fix: migration 0209 duplicate-column crash (BDD/E2E failure)

Root cause. The BDD suite was failing at alembic upgrade with psycopg.errors.DuplicateColumn: column "collection_install_id" of relation "schemas" already exists. Migration 0207_collection_install_tracking already adds collection_install_id to schemas/agents/pipelines (idempotently, via ADD COLUMN IF NOT EXISTS at line 226), while 0209_collection_install_id_entity_columns then tried to op.add_column the same column again (non-idempotent) and crashed. 0207 runs before 0209 in the chain (0206→0207→0208→0209), so the column always exists by the time 0209 runs.

Fix (commit 982b6e5130762152e649dd73697d3ce8ad0ec83a). Made 0209 idempotent:

• The column add is now guarded by an existence check (Postgres information_schema / SQLite PRAGMA table_info) so it is skipped when 0207 already created it.
• The index create (ix_{table}_collection_install_id, the ORM-declared index that 0207 never created) is likewise guarded by an existence check.
• downgrade now drops the index/column only if present.

This is safe whether or not 0207 has run, and on both Postgres and SQLite. The migration docstring was corrected — it previously claimed 0207 did NOT add the column, which was wrong and was the source of the bug.

Verification. All committed pre-commit gates passed (ruff, ruff-format, bandit, semgrep, gitleaks, import-linter, check-yaml/toml/json, merge-conflict, eof/whitespace, no-commit-to-branch). A SQLite simulation of the upgrade→re-run→downgrade cycle confirmed the column/index are created once and removed cleanly with no duplicate-column error.

[farnalabs]

Feedback only (the formal decision is posted separately by post-decision).

Merge conflicts with main — request changes.

1. backend/src/modulo/db/migrations/versions/0209_collection_install_id_entity_columns.py: main already merged its own idempotency fix for the identical bug in PR #355 (5f9bce7) using sqlalchemy.inspect inside upgrade(). This PR reimplements the same fix with custom _column_exists/_index_exists helpers (information_schema/pg_indexes + PRAGMA), which (a) diverges from main — the PR is currently unmergeable (mergeable: false, state dirty) — and (b) re-implements what sa.inspect() already provides, duplicating an abstraction that exists in main's version. Recommendation: rebase onto main and drop this PR's migration change entirely, keeping main's version; the PR should ship only the e2e test addition (its original purpose).

2. backend/tests/docker/test_runners_strip_e2e.py: the test content itself looks sound — three scenarios, module-scoped fixtures, and skip-guards for the rig consistent with the existing pattern in test_bundled_runner_harness.py. Caveat: because CI ran against the pre-rebase head, the migration must be re-validated against main's version of 0209 after rebase (the migrated_db_url fixture runs alembic upgrade heads, so it will exercise whichever 0209 wins the conflict).

Note also: the PR adds pytest.skip calls in the test file (guarded, consistent with existing harness tests) — flagged for the high-risk routing check.

[modulo-reviewbot]

Blocking findings (PR #356, head 982b6e5)

1. backend/src/modulo/db/migrations/versions/0209_collection_install_id_entity_columns.py — duplicates an already-merged fix on main (PR #355) with custom _column_exists/_index_exists helpers instead of reusing sa.inspect(). The two implementations diverge and produce merge conflicts (PR mergeable=false, state dirty). Required: rebase onto main and drop this PRs migration change, keeping mains version.

2. backend/tests/docker/test_runners_strip_e2e.py — new 655-line docker e2e suite for the Runners status strip (3 scenarios, module-scoped fixtures, skip-guards consistent with test_bundled_runner_harness.py). Content looks sound, but it adds pytest.skip() calls (high-risk routing signal; the policy router flagged this diff as high-risk) and must be re-validated against mains 0209 after rebase since the migrated_db_url fixture runs alembic upgrade heads.

CI (SonarCloud) was still pending; the head is unmergeable until the migration conflict is resolved and CI re-runs against the rebased head.

[farnalabs]

Automated fix — migration collision (resolves CHANGES_REQUESTED finding #1)

Root cause: PR #356 carried its own revision of 0209_collection_install_id_entity_columns.py (commit 982b6e5 added custom _column_exists/_index_exists idempotency helpers) which collided with main's already-merged 0209 from PR #355. Two implementations with the same revision/down_revision under the same filename made the branch CONFLICTING/DIRTY and unmergeable.

Fix (commit 91f32988aabd4af8cfb1b23d2406e9563191c0d6): Reverted backend/src/modulo/db/migrations/versions/0209_collection_install_id_entity_columns.py to match main exactly, dropping this PR's migration change and keeping main's version. Main's 0209 already adds both the collection_install_id column and its index to the entity tables (schemas/agents/pipelines), so the Runners-status-strip e2e suite (test_runners_strip_e2e.py) needs nothing beyond it — the migrated_db_url fixture's alembic upgrade heads now resolves against a single, clean head.

Verification: alembic heads shows exactly one head (0209_collection_install_id_entity_columns); ruff check + ruff format pass on changed paths; the global pre-commit allowlist (ruff, bandit, semgrep, gitleaks, import-linter, etc.) passed on commit.

Finding #2 (the e2e test's pytest.skip() guards) is noted — content was judged sound by the reviewer; it now re-validates cleanly against main's 0209 with no migration divergence.

[farnalabs]

Review feedback (PR #356, head 91f3298) — feedback only, formal decision posted separately.

Migration collision from the previous CR is resolved: 0209_collection_install_id_entity_columns.py is now byte-identical to main's version (checked via git diff origin/main HEAD -- <path> = empty), and 0207's ADD COLUMN IF NOT EXISTS confirms the fresh-DB reasoning. Mergeability is true.

The new backend/tests/docker/test_runners_strip_e2e.py looks sound:

• Transition semantics check out against health_probe.py:368-385: transitions only counts was-reachable→now-unreachable, so scenario 2's transitions == 0 after scenario 1's down tick is correct, and module-scoped DB sharing across the three tests does not break the deltas.
• The env-conditional pytest.skip guards mirror the existing pattern in backend/tests/docker/conftest.py; no xfail, no test removals.
• The alert-delta assertions (+1 notifications / +1 error_events with before/after counts) correctly avoid ordering dependence against prior rows.

Minor notes (non-blocking):

• migrated_db_url in the docker test duplicates the role-ceremony fixture from backend/tests/integration/conftest.py. Cross-package conftest reuse isn't trivial here, so this is acceptable as-is, but if a third copy appears consider extracting a shared helper.
• machines[0]["image_checks"] == {_IMAGE_REF: True} is an exact-dict assert on a live probe; if the probe ever records additional refs the suite will false-fail — acceptable since real_refs are filtered by the org's profile image.

Verdict leaning APPROVE; formal decision comes from the post-decision node.

[farnalabs]

Feedback review (non-blocking): CI fully green, mergeable, migration logic verified. Findings — (1) 0209 upgrade idempotence is correct: 0207 adds the column with ADD COLUMN IF NOT EXISTS but never creates ix_

_collection_install_id, so skipping the column while creating the index is right on both fresh and prod DBs. (2) Minor: downgrade() of 0209 unconditionally drops the index AND column even when 0209 skipped creating them (fresh DBs where 0207 already added the column) — after downgrade the revision chain lands below 0209 but above 0207 yet the provenance column 0207 created is gone; consider making the downgrade existence-checked symmetric to the upgrade. (3) test_runners_strip_e2e.py adds pytest.skip env-guards for the runner-ci rig (_require_proxy/_require_dind); they are deliberately conditional (harness CI job sets those env vars and passed) so not treated as silent test disabling. Verdict on this stage: approve.

[modulo-reviewbot]

PR #356 — APPROVE

Verified by the review node (FAR-772 compose e2e coverage for the Runners status strip states). Diff contains an idempotence fix to migration 0209 and a new 655-line docker e2e test file.

• Migration 0209 upgrade is correct on fresh and prod DBs (0207 adds the column via ADD COLUMN IF NOT EXISTS but never creates the entity indexes; skip-column/create-index is right). Minor non-blocking nit: downgrade() is not symmetric — it unconditionally drops the column/index even when the upgrade skipped creating them on fresh DBs.
• e2e asserts transitions counts (healthy-to-unreachable only) verified against the health-probe source; notification/error-event deltas are ordered correctly after engine kill. Env-gated pytest.skip guards for the runner-ci rig are deliberate and the harness CI job passed.
• CI fully green (15/15), no security issues, no API contract surface touched.

Non-blocking feedback has been posted to the PR. No high-risk paths flagged (is_high_risk=false).

[farnalabs]

Branch Fixer: resolved merge-queue migration collision (PR #356 vs PR #353)

Root cause. Both PR #356 and PR #353 modify the same Alembic migration file
backend/src/modulo/db/migrations/versions/0209_collection_install_id_entity_columns.py.
The merge queue squash-merges approved PRs in age order (fix/deploy #353 at
18:23 is older, so it lands first). When #353 is already on the queue branch,
#356's version of 0209 conflicts with #353's, so the squash-merge of #356 fails
with a content conflict on that file — blocking the merge.

Why the previous attempt didn't work. A prior commit on this branch
(91f32988a fix(db): revert 0209 to main's version) reverted 0209 to main's
original (non-idempotent) version. That still conflicts, because reverting to
main's original differs from #353's idempotent index-only version — git still has
to reconcile two different edits to the same lines.

This fix. Commit e818f6828cee00c0792eaf6c86451e6b6ade91ec aligns #356's 0209 to exactly #353's version
(CREATE INDEX IF NOT EXISTS on the column that 0207 already creates idempotently;
column add and inspect guards removed). Once #353 is on main, #356's 0209
diff is a no-op, so the squash-merge is clean and only the new
tests/docker/test_runners_strip_e2e.py is added. Verified locally: squash-merging
#353 then #356 produces a single Alembic head (0210_community_gate) with no
0209 conflict. All pre-commit hooks pass.

0209 is now identical to #353's, so this branch carries no migration change of
its own — the migration fix lives entirely in #353. The branch fixer leaves #353
untouched (it merges first by age).

Run: this fix was applied by the Branch Fixer bot.

[farnalabs]

Review feedback (pre-decision, non-blocking posting)

Verdict direction: CHANGES_REQUESTED — one major finding:

1. [Major] The new e2e suite is never executed by any CI lane (backend/tests/docker/test_runners_strip_e2e.py). The docker marker is excluded from default lanes (backend/pyproject.toml:505), and the only -m docker job — runner-ci-harness.yml:184 — runs exactly pytest tests/docker/test_bundled_runner_harness.py. The new file is not in that command, and it is also missing from the harness PATHS filter (runner-ci-harness.yml:107), so future edits to it won't even trigger the job. Meanwhile the harness job already boots the exact compose rig and exports MODULO_RUNNER_HARNESS_PROXY_HOST / MODULO_RUNNER_HARNESS_DIND_HOST — the precise env vars _require_proxy()/_require_dind() read — so scenarios 2 and 3 were clearly written for this job but can never run in it. Net effect: 655 lines of coverage that CI never exercises (scenario 1 only runs if someone invokes it manually; 2/3 of the scenarios permanently skip everywhere). Please add the file to the harness pytest invocation (and PATHS filter) so the suite actually gates.

2. Minor (same file): scenario 3 kills the shared compose-rig dind container (_kill_local_container_by_label) and never restarts it. Safe today because it is the last test in the file, but it will poison any docker test that runs after it if the file is reordered or added to a shared run — consider a note or teardown-restart guard.

3. Minor (0209 migration): the change itself is sound — byte-identical to main's post-#355 version; idempotent CREATE INDEX IF NOT EXISTS with names matching the ORM index=True convention; downgrade drops only the index since 0207 owns the column. Two stale docstring spots: the ROLE WIRING section still describes the removed ALTER TABLE ... ADD COLUMN ceremony, and the SQLite paragraph references op.add_column/op.create_index that no longer exist in the code.

Verified good: the tests assert the real Pydantic wire shape via the running ASGI app (no mocked httpx), alert assertions are +1 deltas so prior rows can't flake them, Settings/dependency-override wiring mirrors the established integration-conftest pattern, and all 15 CI checks on this head are green.

[modulo-reviewbot]

Changes requested

The review node's decision is CHANGES_REQUESTED. Blocking findings carried through:

MAJOR — new e2e suite is never executed by any CI lane
backend/tests/docker/test_runners_strip_e2e.py (the PR's entire deliverable, a 655-line docker-marked suite for the Runners status strip) is never run by any CI workflow:

• The docker marker is excluded from default lanes (backend/pyproject.toml:505).
• The only -m docker job (runner-ci-harness.yml:184) runs solely tests/docker/test_bundled_runner_harness.py, and the new file is missing from the harness PATHS filter (runner-ci-harness.yml:107), so edits to it will not even trigger the job.
• The harness already exports MODULO_RUNNER_HARNESS_PROXY_HOST/MODULO_RUNNER_HARNESS_DIND_HOST, which the suite's _require_proxy/_require_dind gates read — scenarios 2-3 were written for that job yet permanently skip everywhere.
• Required change: add the file to the harness pytest invocation and PATHS filter so the coverage actually gates.

minor — scenario 3 kills the shared compose-rig dind container
_kill_local_container_by_label destroys the shared rig's dind container and never restarts it. Safe only as the last test in the file; will poison any docker test run after it if reordered or added to a shared run. Should restore/restart the container.

minor — 0209 migration docstrings are stale
The migration change itself is sound (byte-identical to main's post-#355 idempotent CREATE INDEX IF NOT EXISTS version; index names match ORM index=True; downgrade drops only the index since 0207 owns the column). But the ROLE WIRING docstring still describes the removed ALTER TABLE ... ADD COLUMN ceremony, and the SQLite paragraph references op.add_column/op.create_index which no longer exist in the code.

Verified strengths: tests round-trip the real Pydantic wire shape via the running ASGI app (no mocked httpx), delta-based alert assertions are immune to prior rows, and Settings/dependency-override wiring mirrors the established integration-conftest pattern. CI is green (15/15) and the PR is otherwise mergeable.

[farnalabs]

Branch Fixer — addresses CHANGES_REQUESTED review (head e818f6828)

MAJOR — strip e2e suite never ran in CI:
The docker marker is excluded from the default lanes (backend/pyproject.toml:505), and the only -m docker job (runner-ci-harness.yml:184) only invoked tests/docker/test_bundled_runner_harness.py. backend/tests/docker/test_runners_strip_e2e.py was also missing from the harness PATHS filter (runner-ci-harness.yml:107), so it was never gated. Fix: added the file to both the PATHS filter and the pytest invocation (kept before the harness file so the e2e engine-kill scenario — which takes down the shared dind — runs last and the rig is still intact for the bundled harness).

Minor 1 — Scenario 3 killed the shared rig dind without restoring it:
_kill_local_container_by_label targets label runner-ci-dind, which is the compose-rig's dind container. Scenario 3 (test_engine_kill_flips_strip_and_emits_notification_and_error_event, the last test in the file) now restarts that container and waits for it to come back up in a suppressed guard, and the helper's docstring documents that it takes down the rig's dind engine.

Minor 2 — stale 0209 migration docstrings:
Refreshed the ROLE WIRING section (still described the removed ALTER TABLE ... ADD COLUMN ceremony) and the SQLite paragraph (referenced op.add_column / op.create_index no longer in the code) to match the migration's actual CREATE INDEX IF NOT EXISTS behaviour.

Local verification: ruff + ruff format pass on both changed Python files; all cross-platform pre-commit hooks pass. The docker-marked e2e itself still requires the runner-ci compose rig (dind + proxy) to execute, which only CI provisions.

Commit: afe2a8921f73718aba69b7cf568f29396c89a106

[farnalabs]

CHANGES_REQUESTED (feedback, not the formal decision): CI is failing on the current head (afe2a89). Failing check: "Bundled Runner harness (compose + socket-proxy + dind)" — https://github.com/farnalabs/modulo/actions/runs/34545439642/job/103096931840. All other checks (Test Backend, BDD full suite, Integration changed, Frontend and WCAG, Lint, migrations, schema freshness, secret scan) pass. Please fix or investigate the runner-harness job before merge.

[modulo-reviewbot]

Blocking finding: the "Bundled Runner harness (compose + socket-proxy + dind)" CI check failed on head afe2a89 — see https://github.com/farnalabs/modulo/actions/runs/34545439642/job/103096931840.

All other checks (Test Backend, BDD full suite, Integration tests (changed), Frontend and WCAG, Lint (Backend), Check migration heads, Schema freshness, Secret scan, SonarCloud coverage) pass or are not failing, and the PR is otherwise mergeable. Per review policy, a failing check blocks approval. Full code review was not performed this round because CI failed; the PR will be re-reviewed once the harness job is fixed.

[farnalabs]

Branch Fixer: runners-strip e2e harness setup failure

Root cause. The runner-ci harness job runs both test_bundled_runner_harness.py and the new tests/docker/test_runners_strip_e2e.py. The new file imports modulo.api.main, whose module-level get_settings() call (api/main.py:1048) requires FERNET_KEY and SECRET_KEY. The runner-ci rig exports DATABASE_URL but not the two secrets, so the app module import raised a pydantic ValidationError at every test's setup (_ ERROR at setup of test_engine_down_maps_to_unreachable_...).

Fix. Added a module-scoped autouse fixture _provide_required_settings_env to backend/tests/docker/test_runners_strip_e2e.py that sets FERNET_KEY/SECRET_KEY (32-char placeholders, matching the _VALID_32 the client fixture already uses) before the app is imported. The client fixture still overrides get_settings with explicit settings, so this only unblocks the import; behaviour is otherwise unchanged. DATABASE_URL is still supplied by the rig.

Verification. ruff check + ruff format --check pass on the changed file; pre-commit hooks pass. The docker suite itself still requires the compose rig (scenarios 2–3) / a reachable engine (scenario 1) to actually execute.

Commit: d508322e7da1e70e1f528b06920f94c9d25c32f7

[farnalabs]

Re-review after fix push d508322 (fresh review; prior CR was for the same harness job on afe2a89 — the FERNET_KEY/SECRET_KEY import gate is fixed, but the harness check STILL fails on this head).

Blocked check (run 34547647091, job 103103619242): Bundled Runner harness — 2 failures in backend/tests/docker/test_runners_strip_e2e.py:

1. test_rig_up_maps_to_healthy_and_profile_offered: AssertionError: assert 'exceeds_cpu' == 'ok' (line 610). Root cause is environment-dependent, not infra flake: with no explicit org cap, get_sandbox_concurrency_limit resolves the Docker-tier default 4, so needed_cpu = 4 * PER_CONTAINER_CPU (1.0) = 4.0 (backend/src/modulo/api/routes/runners.py:168), and the engine's reported cpu_count on a 2-to-4-vCPU GitHub runner can be lower. Fix suggestion: in the shared org fixture seed an explicit low sandbox_concurrency_limit in organisations.settings_json (e.g. 1) so preflight deterministically reads 'ok', or assert against the computed state (needed_cpu vs preflight.engine_cpu_count) instead of the literal.

2. test_engine_kill_flips_strip_and_emits_notification_and_error_event: 'engine at tcp://127.0.0.1:32769 never became ready' at line 634 — the dind engine was unreachable at test START, before any kill, so the healthy→unreachable transition was never exercised. Please make the pre-kill wait distinguish 'rig never came up' (skip with a clear reason) from 'kill paused the engine' (test body), so a dead rig doesn't surface as a red scenario-3 test; also consider whether 60s of polling could hang the shared rig for downstream suites.

Non-blocking review of the rest of the diff:

• Migration 0209 (column removed from this migration, index-only + IF NOT EXISTS): correct. Verified 0207_collection_install_tracking:226 already does ADD COLUMN IF NOT EXISTS collection_install_id, so fresh DBs get the column before 0209; DBs that already ran main's 0209 also have it. Downgrade now drops only the index, which matches the upgrade scope and leaves column ownership to 0207. SET ROLE/RESET ROLE + owner assert ceremony preserved.
• runner-ci-harness.yml path filter + pytest command wiring for the new file: fine.

The migration change and workflow change look good; the PR is blocked solely by the harness CI check above.

[modulo-reviewbot]

Changes requested (blocking)

Two test failures in backend/tests/docker/test_runners_strip_e2e.py (added by FAR-772) fail the CI check "Bundled Runner harness (compose + socket-proxy + dind)" on head d508322.

1. test_rig_up_maps_to_healthy_and_profile_offered (line 610) — environment-dependent assertion preflight["state"] == "ok". With no org-level cap seeded, get_sandbox_concurrency_limit defaults to 4, needed_cpu = 4 * PER_CONTAINER_CPU(1.0) = 4.0 (backend/src/modulo/api/routes/runners.py:168), and smaller CI engines report cpu_count below that -> "exceeds_cpu". Fix: seed an explicit low sandbox_concurrency_limit in the shared org fixture settings_json (e.g. 1), or assert the computed state instead of the "ok" literal.

2. test_engine_kill_flips_strip_and_emits_notification_and_error_event (line 634) — the pre-kill _wait_for_engine_up(dind_host) raised "engine at tcp://127.0.0.1:32769 never became ready" in CI; the shared dind was down before the test killed anything, so the healthy->unreachable transition and the notification/error_event +1 deltas were never asserted against a real transition. Make "rig never came up" a clean skip distinct from the kill scenario, and keep the shared-rig restore defensive.

Please fix the two blocking test issues above and re-push; I will re-review.

Non-blocking notes

• 0209_collection_install_id_entity_columns.py migration is sound (index-only upgrade, column left to idempotent 0207 ADD COLUMN IF NOT EXISTS, IF NOT EXISTS index creation, preserved SET ROLE/RESET ROLE + owner-assert ceremony, scope-matched downgrade).
• .github/workflows/runner-ci-harness.yml correctly gates the new test into the path filter and docker-marked invocation.

[farnalabs]

Branch Fixer: resolve Bundled Runner harness failures

The Bundled Runner harness (compose + socket-proxy + dind) check failed on head d508322 with 2 red scenarios in backend/tests/docker/test_runners_strip_e2e.py. Fixed in commit a6dd9185fe41f7d84ee16246557c714283b038a0 (scoped to the test file only — no product code touched).

1. test_rig_up_maps_to_healthy_and_profile_offered — assert 'exceeds_cpu' == 'ok' (line 610)
Root cause: the shared test_org fixture seeded an empty settings_json, so get_sandbox_concurrency_limit resolved the Docker-tier default (4). needed_cpu = 4 * PER_CONTAINER_CPU (1.0) = 4.0 then exceeded the cpu_count the runner-ci runner reports (a 2–4-vCPU GitHub runner), flipping the preflight to exceeds_cpu.
Fix: seed an explicit sandbox_concurrency_limit = 1 in the org settings_json so the cap always stays below the engine's reported cpu_count and the preflight deterministically reads ok on any runner size (chosen over asserting the computed state — keeps the scenario's intent: a rig with headroom offers the profile).

2. test_engine_kill_flips_strip_and_emits_notification_and_error_event — engine never became ready (pre-kill wait)
Root cause: the nested dind engine was unreachable at test start, so _wait_for_engine_up raised before any kill — the healthy→unreachable transition was never exercised and a dead rig surfaced as a red failure.
Fix: replaced the hard pre-kill wait with a bounded poll that pytest.skips with a clear reason when the rig never comes up, so a dead rig is a clean skip (not a failure) and the real kill assertion still runs when the engine is reachable. Also wrapped the post-kill dind-restart wait in suppress(Exception) — it is best-effort rig restore before the job tears the rig down, not test logic.

Verified: ruff + ruff format + bandit + gitleaks + the pure-file checks all pass under the commit hook.

[farnalabs]

Re-review of head a6dd918: both blocking issues from the previous review are fixed - (1) the shared org fixture seeds sandbox_concurrency_limit=1 in settings_json so the resource preflight deterministically reads ok, and get_sandbox_concurrency_limit does read that exact key (crud/run.py); (2) scenario 3 now probes engine reachability for up to 60s at test start and cleanly skips when the rig is unavailable, separating environment-dependent skips from real assertion failures. The 0209 index-only migration is consistent with 0207 (ADD/DROP COLUMN IF NOT EXISTS on the same three tables). No blocking issues found.

[modulo-reviewbot]

High-risk path flagged (changed file .github/workflows/runner-ci-harness.yml matches .github/workflows/** glob) - routed through HITL human-approval gate and approved there; posting APPROVE to execute it. Human-approved HITL gate for high-risk paths.

Approved on re-review of head a6dd918:

• Both prior blocking issues fixed: org fixture seeds sandbox_concurrency_limit=1 (confirmed read by get_sandbox_concurrency_limit from org settings_json) making the preflight assertion engine-independent; scenario 3 cleanly skips when the dind rig never becomes ready within 60s, separating env skips from real failures.
• 0209 migration is index-only and consistent with 0207 idempotent ADD COLUMN IF NOT EXISTS; downgrade scope matches.
• New E2E suite exercises the real probe tick and GET /api/v1/runners/status over ASGI with RLS-scoped app role, satisfying the contract round-trip.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud