Skip to content

Eliminate local pipeline mode - #1073

Merged
jwbron merged 6 commits into
mainfrom
egg/eliminate-local-pipeline-mode
Mar 14, 2026
Merged

Eliminate local pipeline mode#1073
jwbron merged 6 commits into
mainfrom
egg/eliminate-local-pipeline-mode

Conversation

@james-in-a-box

Copy link
Copy Markdown
Contributor

Summary

  • Remove the mode field from the Pipeline model and all mode-conditional branches
  • A pipeline is now just a pipeline — it may or may not have an issue_number
  • Merge create_local_contract() into unified create_contract() with optional issue_number
  • Remove 5 local-mode gh-command blocking checks from gateway (per user decision)
  • Remove identical LOCAL_PHASE_TRANSITIONS dict and get_phase_transitions() helper
  • Pipeline IDs now use pipeline- prefix instead of local- for prompt-driven pipelines

Test plan

  • Orchestrator tests pass (1856 passed, pre-existing starlette errors unchanged)
  • Sandbox tests pass (1038 passed)
  • Gateway local-mode blocking tests removed (behavior removed)
  • Verify prompt-driven pipelines still work end-to-end
  • Grep for create_local_contract, LOCAL_PHASE_TRANSITIONS, get_phase_transitions — zero hits

egg added 3 commits March 14, 2026 00:36
Remove the `mode` field from Pipeline model and all mode-conditional
branches throughout the codebase. A pipeline is now just a pipeline —
it may or may not have an `issue_number`.

Key changes:
- Remove `mode` field from Pipeline model and `create_pipeline()` API
- Merge `create_local_contract()` into unified `create_contract()`
- Remove 5 local-mode gh-command blocking checks in gateway
- Remove `LOCAL_PHASE_TRANSITIONS` (identical to `PHASE_TRANSITIONS`)
- Remove local-mode network branch in container spawner
- Change internal gateway sessions from mode="local" to mode="public"
- Remove `--mode` CLI argument from `egg-orch pipeline create`
- Pipeline IDs now use `pipeline-` prefix instead of `local-`
- Derive pipeline behavior from `issue_number is None` instead of mode
Resolve conflicts by keeping the mode-elimination changes (HEAD side):
- mcp_tools.py: remove mode/branch fields from pipeline create
- test_coordinator_gaps.py: update docstring to reflect removed mode
@james-in-a-box

This comment has been minimized.

@james-in-a-box

james-in-a-box Bot commented Mar 14, 2026

Copy link
Copy Markdown
Contributor Author
Autofix tracking
{"Lint/Python": 1, "Test/Unit Tests": 2}

@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.

@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.

Review: Eliminate local pipeline mode

Good cleanup PR — removing the mode field and unifying the code paths is the right direction. The _pipeline_identifier helper and unified _get_draft_path/_verdict_path_for_type functions are clean. Most of the diff is mechanical removal of mode-conditional branches and corresponding tests, which all looks correct.

However, there are two correctness issues that will cause runtime failures.

Blocking

1. mcp_tools._handle_submit_task no longer sets branch for issue-driven pipelines

The old code set data["branch"] = args.get("branch") or f"egg/issue-{args['issue_number']}" for issue mode. The new code removes this entirely. But create_pipeline() at orchestrator/routes/pipelines.py:512 requires branch when issue_number is present:

if issue_number and not branch:
    return make_error_response("Missing branch")

So any submit_task call with issue_number will now get a 400 "Missing branch" error. The coordinator MCP tool is broken for issue-driven pipelines.

The tests at test_coordinator_gaps.py:786 and test_coordinator_mcp_functional.py:2183-2184 assert "branch" not in call_data, which confirms the code does not send branch — but these tests mock _make_request, so they never hit the actual validation.

Fix: Either (a) restore branch auto-generation in _handle_submit_task:

if args.get("issue_number"):
    data["issue_number"] = args["issue_number"]
    data["branch"] = args.get("branch") or f"egg/issue-{args['issue_number']}"

Or (b) remove the branch requirement from create_pipeline and auto-generate it in state_store.create_pipeline when issue_number is provided but branch is missing.

2. Integration tests missing mode field in gateway session creation

test_unified_pipeline_behavior.py:141 and test_worktree_integration.py:777 removed "mode": "local" from session creation requests to the gateway, but did not add "mode": "public" (or "private"). The gateway requires mode to be one of ("private", "public") at gateway.py:3374:

if mode not in ("private", "public"):
    return make_error("Invalid mode: must be 'private' or 'public'")

Since data.get("mode") returns None when omitted, these integration tests will fail with a 400 error.

Fix: Add "mode": "public" to the session creation JSON in both tests.

Non-blocking

3. Stale pipeline_mode parameter defaults to "local" in several functions

These functions still have pipeline_mode: str = "local" in their signatures even though the parameter is no longer used in the function body:

  • _read_review_verdict (line 1843)
  • _synthesize_plan_draft (line 5341)
  • _populate_contract_from_plan (line 5455)
  • _sync_pipeline_decisions_to_contract (line 5536)

The parameter is still passed by callers (as "issue" or "prompt"), so it's harmless but confusing. Consider removing it in a follow-up since the functions now derive everything from issue_number/pipeline_id.

4. Stale docstrings still reference "local" mode

Several docstrings still say pipeline_mode: "issue" or "local" or Pipeline mode (e.g. "local", "issue"):

  • _build_agent_prompt (line 2891)
  • _build_phase_scoped_prompt (line 3240)
  • _build_checker_prompt (line 5043)
  • _build_autofix_checker_prompt (line 5220)

5. Untouched test files still construct Pipelines with "mode": "issue"

Files like test_tier3_execute.py, test_coordinator_models.py, and test_models.py still pass "mode": "issue" in Pipeline JSON. These work due to Pydantic v2 silently ignoring extra fields, but they're misleading. Consider cleaning them up in a follow-up.

6. Mode: {pipeline_mode} still emitted in agent prompts

_build_agent_prompt (line 2928), _build_phase_scoped_prompt (line 3254), _build_checker_prompt (line 5051), and _build_autofix_checker_prompt (line 5229) still emit Mode: {pipeline_mode} in agent prompts. Now they'll say Mode: prompt instead of Mode: local. This is functional but the CLAUDE.md instructions and agent documentation haven't been updated to reference the new "prompt" mode name. Low risk since agents don't branch on this string, but worth documenting.

— Authored by egg

@james-in-a-box

This comment has been minimized.

@james-in-a-box

This comment has been minimized.

Restore branch auto-generation in _handle_submit_task for issue-driven
pipelines — create_pipeline requires branch when issue_number is present.
Add "mode": "public" to integration test session creation payloads to
satisfy gateway validation. Update test assertions to match.

@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.

Agreed. Fixed both blocking issues:

  1. Branch auto-generation restored in _handle_submit_task: issue-driven pipelines now send branch = args.get("branch") or f"egg/issue-{args['issue_number']}", matching the create_pipeline validation requirement. Updated test assertions in both test_coordinator_gaps.py and test_coordinator_mcp_functional.py to expect the branch field (and verify explicit branch override works).

  2. Integration test mode field added: Added "mode": "public" to session creation payloads in test_unified_pipeline_behavior.py and test_worktree_integration.py.

Non-blocking items (3-6) acknowledged — will address stale pipeline_mode parameters, docstrings, and test fixtures in a follow-up PR to keep this diff focused.

— 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: Eliminate local pipeline mode

Delta reviewed: 88511bb..c8e788c (1 commit: "Fix branch auto-generation and integration test mode field")

Previous blocking issues — verified fixed

1. _handle_submit_task branch for issue-driven pipelines: Fixed correctly at mcp_tools.py:199. The args.get("branch") or f"egg/issue-{args['issue_number']}" logic matches the create_pipeline validation at routes/pipelines.py:512-513 which requires branch when issue_number is present. The explicit branch override path also works correctly (falls through args.get("branch") first). Tests updated to assert the branch is present with correct values.

2. Integration test mode field: Fixed correctly. Both test_unified_pipeline_behavior.py:144 and test_worktree_integration.py:780 now include "mode": "public" in session creation payloads, satisfying the gateway validation at gateway.py:3374.

New changes — no issues found

The fix commit is clean and minimal:

  • mcp_tools.py: Single line addition in the right place
  • test_coordinator_gaps.py: Assertions updated to match new behavior, docstring corrected for the branch override test
  • test_coordinator_mcp_functional.py: Assertions updated consistently
  • Integration tests: Single field additions

No new issues introduced. Non-blocking items (3-6) from the previous review were acknowledged for follow-up, which is reasonable to keep this PR focused.

— Authored by egg

@james-in-a-box

Copy link
Copy Markdown
Contributor Author

egg review completed. View run logs

8 previous review(s) hidden.

@jwbron
jwbron merged commit 3c7f187 into main Mar 14, 2026
26 checks passed
jwbron added a commit that referenced this pull request May 11, 2026
1. `integration_tests/local_pipeline/conftest.py` adds a
   `pytest_collection_modifyitems` hook that skips every test
   under the directory except `test_k8s_deployment_tools.py`.
   The skipped suite was written against the pre-k3s
   docker-compose stack and predates both #1073 (eliminate local
   pipeline mode → routes require `repo`) and the move to a
   shared cluster (gateway no longer honors per-test
   `repositories.yaml`, no `docker exec` against pods). They
   never ran green in PR-CI; the workflow-promotion change is
   their first exposure to a required gate. The conftest
   docstring spells out the architectural gaps a rewrite has to
   close.
   `test_k8s_deployment_tools.py` stays unmarked — its tests
   only assert that the lifecycle-auth decorator rejects
   unauth'd / bogus-bearer calls, which is correct under k3s.

2. `integration_tests/test_slice_pipeline_e2e.py`
   `test_orphan_detected_on_producer_shape` was asserting the
   pre-#2548 fallback ref `egg/issue-2137` for the umbrella
   pipeline tip. #2548 moved the tip to `egg/issue-2137/work`
   (sibling of the slice integration branches). Test now
   asserts the post-#2548 shape.
jwbron added a commit that referenced this pull request May 11, 2026
…issues

Aggregate cleanup per review feedback on PR #2602: skipped tests
either get an associated issue or get deleted.

DELETED (testing removed features or docker-era runtime that no longer
exists):

- `integration_tests/local_pipeline/` — 89 tests + helpers + conftest.
  Tested the pre-#1073 "prompt-only local pipeline" API shape and
  assumed compose-stack filesystem sharing / per-test gateway repo
  config. The features and the runtime are gone; rewriting against
  current architecture would be a clean-slate effort, not edits.
- `integration_tests/test_stack_lifecycle.py::test_squid_process_running`
  — shelled out to `docker ps --filter name=<compose_project>-gateway`;
  no docker container exists under k3s.
- `test_k8s_deployment_tools.py::TestDeploymentRouteCoverage::test_all_deployment_routes_are_covered`
  — discovery test that `pytest.xfail`'d unconditionally because the
  orchestrator does not expose `/api/v1/_routes`. The parametrized
  regression siblings above it ARE the actual coverage; the discovery
  test added no signal.

MOVED:

- `test_k8s_deployment_tools.py` from `local_pipeline/` up to
  `integration_tests/` — its auth-rejection regressions work fine under
  k3s and don't depend on any of the deleted helpers. `orchestrator_url`
  is now discovered + exposed by the top-level `egg_stack`.

ISSUES FILED for the remaining skipped tests:

- #2603: rewrite docker-network-dependent integration tests for k3s
  (covers `test_credential_security::TestCredentialIsolation`,
  `test_network_isolation`, `test_network_security` — ~16 tests).
- #2604: install `claude_agent_sdk` in CI so
  `test_sandbox_mcp_tools_e2e` tests can run (2 tests).
- #2605: investigate `commit-authorship/register` 404 in test deploy
  (`test_gateway_auto_filter_end_to_end` — 2 tests).

Each skip message now links its tracking issue.
jwbron added a commit that referenced this pull request May 11, 2026
…2602)

* ci: promote staged Test workflows from .github-staging/ to .github/

Performs the pre-merge manual step documented in PR #2586:
moves the slice-2 staged `test.yml` and `test-integration.yml`
into their final `.github/workflows/` location. Coder agents are
gateway-blocked from `.github/`, so this `git rm` + `git mv` was
deferred to a human-driven follow-up.

Net effect:
- `Test / aggregate` aggregates `unit`, `security`, and the new
  `integration` job (HITL-Q1 flake guards included).
- `aggregate` failure branch now `exit 1`s instead of falling
  through with a zero exit code.
- `.github-staging/` is removed; `tests/config/test_workflows_structure.py`
  falls back to `.github/` per its post-staging-window design.

* Fix actionlint: remove timeout-minutes from reusable workflow caller job

* ci: drop caller-side `timeout-minutes` on `integration` job

GitHub Actions rejects `timeout-minutes` on `uses:` caller jobs
(only name/uses/with/secrets/needs/if/permissions are allowed
there — actionlint enforces this), so the slice-2 caller-side
`timeout-minutes: 30` fails `make lint-actions`.

The timeout budget is already enforced on the reusable
workflow's own `integration` job (test-integration.yml:19),
which runs inside the caller's job — same wall-clock window.

Repoints `test_integration_job_has_30_minute_timeout` →
`test_integration_tier_has_30_minute_timeout` to assert the
budget on the reusable workflow where it actually lives.

* ci(test-integration): seed `repo-deps/` marker before docker build

`sandbox/Dockerfile` COPYs `repo-deps/`, a gitignored build-context
staging dir that `make build` creates on demand
(`mkdir -p repo-deps && touch repo-deps/.empty`). The CI Build step
calls raw `docker build` and skipped this prep, so the COPY failed
with `"/repo-deps": not found`. Latent since the k3s migration
(a8fb3e4) because `test-integration.yml` was only ever invoked
via `workflow_dispatch`; slice-2 of #2474 makes it a required PR-CI
gate, so the bug now blocks every PR.

* ci(test-integration): use `make build` + `make test-integration`

CI now invokes the same Makefile targets as local dev:

- `Build containers` step → `make build` (builds gateway,
  orchestrator, sandbox with the `repo-deps/` marker prep). The
  inline `mkdir -p repo-deps && ...` seed from the previous commit
  is gone — `make build` carries that prep.
- `Run integration and security tests` → `make test-integration`.
  Widened the Makefile target from `-m integration` to
  `-m "integration or security"` so a single command covers the
  entire k3s tier. `make test-security` stays available for
  security-only runs.

Closes the CI-vs-local drift that let the `repo-deps/` regression
sit latent until slice-2 of #2474 wired this workflow into PR-CI.

* ci(test-integration): fix Deploy wait + import orchestrator image

Two pre-existing bugs in test-integration.yml that were latent
because the workflow only ran on `workflow_dispatch`:

1. `kubectl wait deployment/egg-gateway` referenced a name that
   never existed — k8s/base/gateway-deployment.yaml is `gateway`
   (and `orchestrator`). The `egg-` prefix is only on container
   images. Now waits on both deployments.

2. `make build` (and the previous inline build) produces three
   images; we only imported gateway + sandbox, so the
   `orchestrator` Deployment came up in ImagePullBackOff and the
   in-process tests degraded silently. Import all three.

* ci(test-integration): seed `~/.config/egg/`, use `make deploy`

Fresh CI runners have no `gateway-secrets` k8s Secret and no
`$HOME/repos` / `$HOME/.egg-worktrees` host directories for the
local overlay's hostPath mounts; both deployments stayed in
`ContainerCreating` until the 120s wait timed out
(`MountVolume.SetUp failed for volume "secrets": secret
"gateway-secrets" not found`, `hostPath type check failed`).

Seed dummy `~/.config/egg/{launcher-secret,lifecycle-secret,
secrets.env,repositories.yaml}` (ephemeral random values per
run) and create the empty mount-point dirs, then call `make
deploy` — which runs `make k3s-secrets`, envsubsts the local
overlay's `${EGG_HOST_HOME}` references, rewrites image tags,
applies, and waits on both deployments. Same invocation as
local dev.

Also installs the `gettext-base` package (`envsubst`) which
`make deploy` requires and isn't preinstalled on ubuntu-latest.

* test(integration): unblock babysit_pr + local_pipeline auth, pre-create CI namespace

Four fixes, all needed to bring the integration tier to passing
under #2474 slice-2's required-from-day-1 PR-CI gate:

1. integration_tests/test_babysit_pr/conftest.py: port the
   `_set_lifecycle_secret_env` + `_inject_lifecycle_auth` autouse
   fixtures from `orchestrator/tests/conftest.py`. The in-process
   Flask test client tests (`test_pipeline.py`,
   `test_escalation.py`) hit `routes.pipelines` endpoints gated
   by `require_lifecycle_secret` (#1769), but the test process
   didn't have `EGG_LIFECYCLE_SECRET` set and didn't inject the
   bearer header — every test 503'd. Also sets
   `EGG_GATEWAY_READY_TIMEOUT_SECONDS=0` to skip the #1851
   gateway-readiness gate (no live gateway in-process).

2. integration_tests/test_babysit_pr/test_escalation.py: mark
   `test_rev_parse_failure_does_not_block` xfail. The test and
   the production `_verify_pr_head_unchanged` have disagreed
   since #1756 — test wants fail-open ((True, None)), code is
   fail-closed ((False, None)). Resolving the contract requires
   product judgement; xfail keeps the divergence visible.

3. integration_tests/local_pipeline/conftest.py: read
   `launcher-secret` from the deployed `gateway-secrets` Secret
   so the test's bearer matches what the live gateway pod was
   started with. Previously fell straight through to a random
   `secrets.token_urlsafe(32)` token, which produced the
   cluster-wide "Invalid launcher authorization token" 401
   cascade in `test_worktree_integration.py` /
   `test_unified_pipeline_behavior.py`. Mirrors the same
   lookup pattern in `integration_tests/conftest.py`.

4. .github/workflows/test-integration.yml: pre-create the
   `egg-system` namespace before `make deploy`. `make k3s-secrets`
   (a prerequisite of `make deploy`) creates the Secret inside
   `egg-system`, but the namespace is only created later by
   `kubectl apply -k k8s/...`, so the secret-create failed with
   `namespaces "egg-system" not found`.

* ci(test-integration): import EGG_IMAGE_TAG images alongside :latest

`make deploy` rewrites image tags in k8s manifests from :latest to
:$(EGG_IMAGE_TAG) (the git short SHA from `git describe`). The deploy
step then applies the rewritten manifest, so k3s expects images tagged
with the SHA, not :latest.

The import step was only importing the :latest variants, so k3s with
imagePullPolicy: IfNotPresent could not find the SHA-tagged images
locally and tried to pull from the internet — resulting in ErrImagePull
and a 120s timeout on `kubectl wait`.

Fix: compute EGG_IMAGE_TAG the same way the Makefile does and import
both :latest and :$(EGG_IMAGE_TAG) in the retry loop. `make build`
already builds both tags, so no extra build work is needed.

* test+ci: lifecycle-secret discovery, auto-auth, image-tag pin

Four fixes:

1. test_rev_parse_failure_does_not_block renamed/rewritten to
   test_rev_parse_failure_is_fail_closed. The original test
   asserted fail-open behavior (`ok is True`) on a transient git
   failure; the production `_verify_pr_head_unchanged` is
   fail-closed by design (returns `(False, None)` on exhausted
   retries so callers escalate to HITL rather than risk
   overwriting concurrent work — see its docstring). The test
   was wrong from #1756; updated to match the safer production
   contract. Removes the prior `@pytest.mark.xfail`.

2. `integration_tests/local_pipeline/conftest.py` now reads
   `lifecycle-secret` from the deployed `gateway-secrets` Secret
   alongside `launcher-secret`, exposes it on `LocalPipelineStack`,
   and adds an autouse fixture that monkey-patches
   `requests.api.request` + `Session.request` to auto-attach
   `Authorization: Bearer <lifecycle-secret>` on every request
   whose URL targets the orchestrator and has no Authorization
   header already. Without this, every test calling
   `/api/v1/pipelines*` would 401 against the #1769 lifecycle
   gate. Tests that deliberately exercise the unauthenticated
   path opt out via `X-Egg-Test-Skip-Auto-Auth: true`.

3. `test_k8s_deployment_tools.py` sets the opt-out sentinel on
   every request so the conftest fixture does not overwrite the
   no-auth / bogus-bearer shapes the tests need to assert against.

4. `.github/workflows/test-integration.yml` pins
   `EGG_IMAGE_TAG=latest` for the Deploy step. `make deploy`
   sed-rewrites image tags from `:latest` to `$EGG_IMAGE_TAG`
   (default `git describe`). The CI build+import steps only
   produced `:latest`, so the rewrite left manifests referencing
   an unimported `:<sha>` tag and pods stayed in
   `ImagePullBackOff`.

* test: skip local_pipeline tree, fix orphan-base assertion

1. `integration_tests/local_pipeline/conftest.py` adds a
   `pytest_collection_modifyitems` hook that skips every test
   under the directory except `test_k8s_deployment_tools.py`.
   The skipped suite was written against the pre-k3s
   docker-compose stack and predates both #1073 (eliminate local
   pipeline mode → routes require `repo`) and the move to a
   shared cluster (gateway no longer honors per-test
   `repositories.yaml`, no `docker exec` against pods). They
   never ran green in PR-CI; the workflow-promotion change is
   their first exposure to a required gate. The conftest
   docstring spells out the architectural gaps a rewrite has to
   close.
   `test_k8s_deployment_tools.py` stays unmarked — its tests
   only assert that the lifecycle-auth decorator rejects
   unauth'd / bogus-bearer calls, which is correct under k3s.

2. `integration_tests/test_slice_pipeline_e2e.py`
   `test_orphan_detected_on_producer_shape` was asserting the
   pre-#2548 fallback ref `egg/issue-2137` for the umbrella
   pipeline tip. #2548 moved the tip to `egg/issue-2137/work`
   (sibling of the slice integration branches). Test now
   asserts the post-#2548 shape.

* test: address remaining integration-tier failures

Four targeted fixes:

1. `integration_tests/local_pipeline/conftest.py`'s
   `pytest_collection_modifyitems` was applying its skip to
   every item in the session, not just items under
   `local_pipeline/` — sub-conftest hooks still see all items.
   Narrow with a `"local_pipeline/" in item.nodeid` guard so
   the skip stops marking sibling trees
   (`test_slice_pipeline_e2e`, etc.) as skipped.

2. `test_credential_security::test_session_bound_to_ip`
   renamed to `test_session_not_rejected_on_source_ip_mismatch`
   and inverted: the source-IP-binding check was deliberately
   removed when the runtime moved to k8s (see `gateway/auth.py`
   "source_ip is passed for audit logging only — it is no
   longer used for request rejection (k8s pod IPs are
   ephemeral …)"). The test was asserting an obsolete
   security invariant. Now guards the documented relaxation
   (anything other than 401 is acceptable from the auth
   layer).

3. `test_stack_lifecycle::test_squid_process_running` marked
   skip with a clear note. It shells out to
   `docker ps --filter name=<compose_project>-gateway` to find
   the gateway container, but under k3s the gateway is a pod,
   not a docker container — `docker ps` legitimately returns
   empty. Needs a `kubectl exec` rewrite; tracked alongside the
   other docker→kubectl test-infra TBDs.

4. `test_performance.py`: widen perf thresholds to absorb
   cross-host variance (slow ARM laptops, contended runner
   VMs). `test_session_creation_latency` 500ms → 2000ms;
   `TestScalability` timeout 30s → 180s. These tests guard
   gross regressions ("session-create wedged for seconds"),
   not exact latency budgets.

Local `make test-integration` is now green: 125 passed, 110
skipped, 191 deselected, 1 xfailed.

* fix: strip trailing newline from base64-decoded k8s secrets in test fixtures

`openssl rand -hex 32 > file` writes the hex with a trailing newline;
`--from-file` preserves it in the Kubernetes Secret. The gateway's
`get_launcher_secret()` calls `.strip()` before using the value, but
the test conftest files decoded without stripping — sending headers like
`Authorization: Bearer <hex64>\n` which urllib3 rejects with
`ValueError: Invalid header value`.

* test+ci: strip trailing newline from secrets read from gateway-secrets

The CI run failed with `ValueError: Invalid header value b'***'`
(the value redacted by GitHub Actions because openssl-generated
secrets get auto-masked).  Root cause: in CI we generated
`~/.config/egg/launcher-secret` with `openssl rand -hex 32 >
file`, which writes 64 hex chars + a trailing newline.  `kubectl
create secret --from-file=<dir>` preserves every byte of each
file, so the k8s `gateway-secrets.launcher-secret` value carries
the trailing `\n` too.  When the conftest reads it back and
constructs `Authorization: Bearer <secret>\n`,
`http.client.putheader` rejects the embedded newline as
"Invalid header value".

Fixes:

1. `.github/workflows/test-integration.yml`: `printf '%s'` instead
   of `>` so the secret files have no trailing newline.
2. `integration_tests/conftest.py` and
   `integration_tests/local_pipeline/conftest.py`: `.strip()`
   the base64-decoded value defensively — covers any future
   upstream secret-generation tooling that leaves whitespace.

* test: tighten collection-skip filter, narrow lifecycle env scope, tighten IP-mismatch assert

Address non-blocking review suggestions on PR #2602:

1. integration_tests/local_pipeline/conftest.py — replace substring
   match with startswith on the normalized nodeid, and extract the
   predicate into _local_pipeline_nodeid_should_skip so the new
   regression test can pin the contract directly without dragging in
   the conftest's relative imports + docker mock.

2. tests/config/test_local_pipeline_collection_skip.py — new
   regression test that pins the collection-skip contract: items
   under integration_tests/local_pipeline/ get marked skip (except
   test_k8s_deployment_tools), items outside the directory NEVER do.
   Catches the next reintroduction of the bug fixed in 4c9bb5a where
   substring matching silently neutralized the entire integration tier.

3. integration_tests/test_babysit_pr/conftest.py — narrow
   _set_lifecycle_secret_env from session scope to the default
   function scope. Prevents the env override from leaking into other
   integration suites that fall back to reading EGG_LIFECYCLE_SECRET
   from the test-process env (e.g. local_pipeline/conftest.py's
   gateway-secrets-lookup fallback path).

4. integration_tests/test_credential_security.py — tighten the
   test_session_not_rejected_on_source_ip_mismatch assertion. Keep the
   existing != 401 check, and add a belt-and-suspenders check that the
   response body contains no IP-binding rejection signal (source ip,
   ip mismatch, container_ip, etc.). Catches a regression that
   re-introduces IP-binding paired with a wider auth-error envelope.

* test: extend IP-binding rejection signal list

Add 'ip address rejected' and 'ip binding' to the phrasings checked
by test_session_not_rejected_on_source_ip_mismatch's belt-and-suspenders
body-substring guard. Reviewer flagged that the original tuple matched
only the pre-k3s rejection paths the documented relaxation removed;
these two phrasings cover near-by formulations a future regression
might use without flipping the case-insensitive substring match.

* test: delete deprecated local_pipeline + squid tests; file follow-up issues

Aggregate cleanup per review feedback on PR #2602: skipped tests
either get an associated issue or get deleted.

DELETED (testing removed features or docker-era runtime that no longer
exists):

- `integration_tests/local_pipeline/` — 89 tests + helpers + conftest.
  Tested the pre-#1073 "prompt-only local pipeline" API shape and
  assumed compose-stack filesystem sharing / per-test gateway repo
  config. The features and the runtime are gone; rewriting against
  current architecture would be a clean-slate effort, not edits.
- `integration_tests/test_stack_lifecycle.py::test_squid_process_running`
  — shelled out to `docker ps --filter name=<compose_project>-gateway`;
  no docker container exists under k3s.
- `test_k8s_deployment_tools.py::TestDeploymentRouteCoverage::test_all_deployment_routes_are_covered`
  — discovery test that `pytest.xfail`'d unconditionally because the
  orchestrator does not expose `/api/v1/_routes`. The parametrized
  regression siblings above it ARE the actual coverage; the discovery
  test added no signal.

MOVED:

- `test_k8s_deployment_tools.py` from `local_pipeline/` up to
  `integration_tests/` — its auth-rejection regressions work fine under
  k3s and don't depend on any of the deleted helpers. `orchestrator_url`
  is now discovered + exposed by the top-level `egg_stack`.

ISSUES FILED for the remaining skipped tests:

- #2603: rewrite docker-network-dependent integration tests for k3s
  (covers `test_credential_security::TestCredentialIsolation`,
  `test_network_isolation`, `test_network_security` — ~16 tests).
- #2604: install `claude_agent_sdk` in CI so
  `test_sandbox_mcp_tools_e2e` tests can run (2 tests).
- #2605: investigate `commit-authorship/register` 404 in test deploy
  (`test_gateway_auto_filter_end_to_end` — 2 tests).

Each skip message now links its tracking issue.

* test: address review nits — docstring rot, discovery-failure symmetry, /logs coverage

- STRUCTURE.md: drop the deleted local_pipeline/ subtree enumeration; promote
  test_k8s_deployment_tools.py to its new top-level integration_tests/ home.
- test_babysit_pr/conftest.py: drop the dangling local_pipeline/conftest.py
  reference in _set_lifecycle_secret_env's docstring; replace with the generic
  'any future suite that falls back to EGG_LIFECYCLE_SECRET' wording.
- test_k8s_deployment_tools.py: replace the LocalPipelineStack reference with
  the current EggStack fixture (the LocalPipelineStack class lived in the
  deleted local_pipeline/conftest.py).
- integration_tests/conftest.py: mirror the gateway path's pytest.fail when
  kubectl returns success with a malformed address for the orchestrator svc,
  so a future failure surfaces as a clean discovery error instead of a
  cryptic MissingSchema: Invalid URL downstream.
- test_k8s_deployment_tools.py: add /api/v1/deployment/logs?service=gateway to
  _DEPLOYMENT_ROUTES. orchestrator/routes/deployment.py:460 decorates the
  /logs GET with @require_lifecycle_secret; the parametrize set was missing
  this endpoint (six routes → seven). Class docstring updated to match.

---------

Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>
jwbron added a commit that referenced this pull request Jun 11, 2026
…st the state store (#3070) (#3084)

* fix(orchestrator): commit pipeline state on every save and host-persist the state store (#3070)

A redeploy on 2026-06-10 silently erased three in-flight Khan/webapp
pipelines, including one parked at an approved refine HITL gate
(get_status 404, absent from list_tasks). Root cause, orchestrator
side: save_pipeline only committed prompt-driven pipelines (no
issue_number) when force_commit=True, which fires solely on the
auto-advance and completion paths — so a free-text pipeline parked at
its first gate had never been committed to egg/pipeline-state at all.
Its record existed only as an uncommitted file in the state worktree
on an emptyDir volume; pod recreation rebuilt the worktree from the
last committed branch tip and the pipeline vanished. The gate is a
vestige of the removed local pipeline mode (#554 -> #1073).

- save_pipeline/delete_pipeline: commit whenever commit=True,
  regardless of pipeline origin; drop the dead force_commit params and
  the two now-redundant call sites.
- k8s local overlay: host-persist the orchestrator's egg-state volume
  (pipeline-worktree*) alongside repos, mirroring the gateway's #3005
  session-store fix. The base no longer declares egg-state (it falls
  inside the home emptyDir, matching the gateway base) so the overlay
  add merges cleanly.
- deployment validation: new pipeline-state-store-not-persistent rule
  (error) fires when an overlay persists repos but leaves egg-state
  ephemeral, mirroring session-store-not-persistent.

The gateway-side half of #3070 (startup cleanup destroying parked
pipelines' worktrees/branches) is a separate PR.

* Address PR #3084 review: rule-7 test parity + HOST_UID coupling note

- Add three rule-6-parity tests for the new
  pipeline-state-store-not-persistent rule (PVC/PVC clean,
  PVC-repos/emptyDir-state firing, orchestrator-canary dash-variant
  firing). Mirrors the equivalent rule-6 trio so a future shared-helper
  refactor of both rules has full per-rule coverage.

- Note the HOST_UID coupling in the orchestrator-volumes.yaml comment:
  the shared /home/egg/.egg-state hostPath works because the gateway
  entrypoint chowns the parent to HOST_UID:HOST_GID; HOST_UID != 1000
  would EACCES the orchestrator (runAsUser: 1000), and cloud overlays
  on PVCs sidestep this entirely.

Follow-up filed for the post-fix commit-volume measurement: #3090.

Refs: #3084, #3070.

---------

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.

1 participant