Egg/issue 2474 v2/work

[jwbron]

No description provided.

[egg-reviewer]

No agent-mode design concerns. This PR is mostly slice-DAG plumbing fixes (#2535, #2515, #2522), restart-phase salvage hardening (#2429, #1723), and a state-store-probe in-flight grace fix (#2501); the rest is a cleanup pass.

Notably, the cleanup removes agent-mode anti-patterns rather than introducing them: integration_tests/test_agent_security_fuzz.py, integration_tests/agent_findings.py, and the run_claude_structured() / assert_agent_verdict() / AgentVerdict.infrastructure_failure helpers are gone. Those were doing exactly what the design guide flags — claude --print --output-format json --json-schema (EGG100), a "Report pass/fail" structured-output ask, and a JSONL findings pipeline parsing the verdicts.

The one prompt change in orchestrator/routes/pipelines.py::_build_brc_preamble (adding --for STATUS and the "Ready to confirm" guidance for #2531) is well-aligned: it tells the producer about a directed orchestrator signal the agent can't easily discover on its own — orientation, not over-specification, and it sits squarely under guideline 4's "procedural context is helpful when it provides information the agent can't easily discover" carve-out.

No new direct Anthropic API calls, no hardcoded model identifiers, no new pre-fetching of large diffs/file contents, no new structured-output-for-humans surfaces. The new .github/ blocked entry on TESTER_PATTERNS is gateway-enforced, not prompt-level.

— Authored by egg

[james-in-a-box]

Contract Verification — PR #2556 (issue-2474-v2)

Summary

PR #2556 is the slice-1 PR of the 5-slice stacked train described in
.egg-state/contracts/issue-2474-v2.json (pipeline issue-2474-v2).
It bundles three commits that are clearly in-scope for the slice-1
contract — the contract initialization, slice-1 implementation
(#2533), and the slice-1 PR-narrative scaffolding — together with
13 unrelated Fix #… commits brought in via merges from main.

Because the orchestrator endpoint is unreachable from this reviewer
(egg-orch health → Orchestrator: UNREACHABLE), I verified the
contract from the on-disk file
(.egg-state/contracts/issue-2474-v2.json) and the merged tree at
44da4331. The contract's top-level acceptance_criteria array is
empty ([]) — there are no ac-N IDs to mark via
egg-contract verify-criterion. All acceptance criteria are
task-level (embedded under each slices[].tasks[].acceptance_criteria),
so this review verifies them by re-running each acceptance grep / file
check directly against the merged tree.

Slice 1 — Cleanup, k3s only, drop dead test tiers — ✅ Verified

All four task-level acceptance criteria pass on the merged tree.

task-1-1 — drop EGG_RUNTIME=docker branch — ✅

Acceptance: grep -n "EGG_RUNTIME=docker\|_docker_egg_stack\|docker_available" integration_tests/conftest.py integration_tests/local_pipeline/conftest.py returns no hits.

Verified — re-running the grep returns zero hits. _docker_egg_stack
is gone; only _k8s_egg_stack remains
(integration_tests/conftest.py:165). Runtime selection branch in
egg_stack() is gone (integration_tests/conftest.py:277); skip is
emitted when kubectl is unavailable
(integration_tests/conftest.py:151 _kubectl_available).

task-1-2 — delete tests/functional/ — ✅

Acceptance: tests/functional/ no longer exists; make test-all
passes; grep -rn "tests.functional|@pytest.mark.functional" returns
no live hits.

Verified — tests/functional/ is gone (5 files deleted via PR diff).
functional marker removed from pyproject.toml. The only remaining
tests.functional string is a docstring in
tests/config/test_ci_config.py:48 documenting the retirement, which
is intentional historical context and not a live reference. The
integration_tests/docker-compose.yml allowlist entry was removed
from scripts/check-hardcoded-ports.py as required.

task-1-3 — delete e2e files & markers — ✅

Acceptance: 4 files gone; make test-e2e is no longer a valid
target; make help does not advertise test-e2e.

Verified — .github/workflows/test-e2e.yml,
integration_tests/test_e2e_workflow.py,
integration_tests/test_agent_security_fuzz.py, and
integration_tests/agent_findings.py are all deleted.
grep -n "test-e2e:" Makefile returns no hits; only test-integration
remains (Makefile:392-393). e2e and agent_flaky markers are
removed from pyproject.toml (only integration and security
remain, line 35-37 of the markers list).

task-1-4 — drop orphan run_claude_structured / assert_agent_verdict / infrastructure_failure — ✅

Acceptance: grep -rn "run_claude_structured|assert_agent_verdict"
returns no hits.

Verified — recursive grep across the merged tree returns zero hits
for run_claude_structured, assert_agent_verdict, and
infrastructure_failure. The remaining functions in
integration_tests/conftest.py are scoped to k8s + container helpers
only.

Slices 2–5 — out of scope for this PR (future stacked PRs)

Per the stacked-train plan (drafts/issue-2474-v2-plan.md),
slices 2–5 are queued to land as separate downstream PRs. The PR
diff confirms none of their artifacts are included:

• Slice 2 (Promote ScriptedProvider): not present.
  shared/egg_harness/testing/ directory does not exist;
  ScriptedProvider remains inline at
  shared/tests/test_egg_harness/test_integration.py:130. The
  test_scripted_provider.py test file is absent.
• Slice 3 (k3s regression tests): not present.
  integration_tests/regression/ directory does not exist; none of
  the 8 test_*.py files specified in tasks 3-1…3-8 are present.
• Slice 4 (wire integration tests into PR CI): not present.
  .github/workflows/test.yml still has only unit + security
  jobs in aggregate.needs (lines 59-79); no integration job and
  no workflow_call of test-integration.yml.
• Slice 5 (documentation): not present. CLAUDE.md has no
  make test-integration Quick Reference bullet, no "Integration
  tests" section, and no updated Repo Layout row.
  docs/guides/testing.md only has a 3-line edit dropping a stale
  -m "not functional" filter from a code example (an in-scope
  follow-on of slice-1's marker removal); the planned three-subsection
  "Integration tests" section is absent.

This is consistent with the stacked-train design — flagging here only
so the reviewer / operator knows the contract is intentionally
incomplete and four follow-on PRs are required to ship the full
issue-2474-v2 plan.

Out-of-scope changes from main merges

The PR diff includes 13 Fix #… commits (e.g. #2495, #2521,
#2515, #2522, #2501, #2490, #2531, #2535, #2538, #2537,
#2539, #2541, #2543, #2546) that touch files outside slice-1's
files_affected lists — most of orchestrator/,
shared/egg_contracts/, shared/egg_restrictions/,
gateway/tests/, tests/shared/egg_contracts/, several
docs/reference/* and docs/architecture/* files. These are
already-merged fixes pulled in via main merges and do not
constitute new work in this PR; they are visible in the diff only
because the PR head is several merges behind the auto-rebased base.
Each one was reviewed and merged independently to main and is
out of scope for slice-1 contract verification.

Contract state observations (informational, post-merge)

• The contract file shipped in the PR is the initial state:
  current_phase: "refine", all 5 slices and 17 tasks
  status: "pending", no commit linkage on any task. The orchestrator
  records task/phase completion in its runtime store, not in the
  on-disk contract checked into the repo, so this is expected and
  not a contract violation. (Future stacked PRs — slice-2 onward —
  will surface their own contract files reflecting whatever
  intermediate state the orchestrator persisted.)
• The PR title (Egg/issue 2474 v2/work) and empty body do not match
  the pr.title / pr.description fields the contract carries
  ("Wire integration tests into PR CI; retire dead test tiers; expand coverage"). This is a process / cosmetic concern, not a contract
  violation. The slice-train design expects the terminal slice's
  PR to inherit the rolled-up pr.* narrative; intermediate slice
  PRs may carry slice-scoped titles/bodies. Worth verifying that the
  final slice PR (slice-5) gets the rolled-up narrative when it lands.

Verdict

PR #2556 is already merged (merge commit
f670c1b46cd1093a5024b12dee06802734fcca14, 2026-05-07T19:28:00Z).
This is a post-merge contract audit: every slice-1 task-level
acceptance criterion passes; slices 2–5 are intentionally out of
scope for this PR per the stacked-train plan; no contract violations
were found in the slice-1 changes. No egg-contract verify-criterion
calls were issued — the top-level acceptance_criteria array is
empty and the orchestrator endpoint was unreachable.

— Authored by egg

[egg-reviewer]

Post-merge review

This PR is a "v2 work" stack containing the slice-1 cleanup commit (f3c16e3b) plus 14 follow-on fixes that landed as their own PRs (#2495, #2520, #2521, #2526, #2533, #2536, #2541, #2543, #2546, etc.). The bulk of the diff has therefore already been individually reviewed; this comment focuses on the cross-cutting picture and on a handful of issues I want flagged as follow-up regardless of merge state.

Slice-1 cleanup is justified

I verified the deletions before signing off:

• tests/functional/ (1,400 LoC) was excluded from every CI invocation and from local make test / make test-all via the -m "not functional" filter that this PR also drops. It was verifiably dead code.
• .github/workflows/test-e2e.yml was a workflow_dispatch + weekly cron tier gated on ANTHROPIC_OAUTH_TOKEN; PRs never ran it. The two test files it consumed (test_e2e_workflow.py, test_agent_security_fuzz.py) were either trivial smoke tests or "ask the agent if it escaped" self-attestations — weak signal.
• The boundary coverage the deleted files claimed (git wrappers, network modes, session lifecycle, container/network escape) is replicated and stronger in: gateway/tests/test_git_validation.py, gateway/tests/test_git_client.py, gateway/tests/test_gateway.py, integration_tests/test_gateway_operations.py, integration_tests/test_credential_security.py, integration_tests/test_network_isolation.py, and integration_tests/test_stack_lifecycle.py. No real coverage was lost.
• The EGG_RUNTIME=docker branch + mock-sandbox/ artifacts are correctly removed; make test-integration (k3s-only) is the surviving path. The Makefile / pyproject / scripts/check-hardcoded-ports.py / tests/config/test_ci_config.py updates are internally consistent — no orphaned references to the removed markers or files.

shared/egg_contracts (#2490) — clean

All 19 sibling models in shared/egg_contracts/models.py inherit from EggContractBaseModel, so validate_assignment=True lands on the full set. No subclass overrides model_config. Tests use plain setattr(...) (with the deliberate # noqa: B010) so they exercise the production validator rather than __dict__ / model_construct. The test_pr_metadata_invalid_deferred_actions_raises case correctly assigns a raw dict rather than a pre-constructed DeferredAction and explains why in the docstring (revalidate_instances="never" would short-circuit nested revalidation otherwise). test_invalid_path_returns_failed_mutation (validator) catches the actual #2495 misclassification — phases.99.tasks.0.commit raised IndexError and was previously returned without error_kind, leaving the route boundary unable to discriminate 400 vs 403. No name/assertion contradictions.

Non-blocking issues worth filing

1. orchestrator/routes/signals.py:1620-1627 — message-bus fallback does not filter by metadata.slice_id.
   The fallback at lines 1612-1668 only fires when slice_id is None (the slice-scoped path returns 404 at line 1610). However, confirmed_roles = {m.from_role for m in messages if m.message_type == "CONSENSUS_CONFIRMED"} aggregates every CONFIRMED on the bus regardless of metadata.slice_id. On a previously-sliced pipeline whose tracker has been lost (orchestrator restart), a non-slice CONFIRMED-handling request would falsely satisfy all_roles.issubset(confirmed_roles) from prior slice CONFIRMs and write a bogus pipeline-level CONFIRMED marker. The same #2535 contamination shape, just one path further down. Add and not (m.metadata or {}).get("slice_id") to the comprehension. Today this is dormant because slice and non-slice modes don't normally coexist on a single pipeline_id, but the comment at line 1574-1578 explicitly cites orchestrator-restart recovery (#2409) as a future case where it could become reachable.

2. orchestrator/routes/signals.py:1642 — _existing_confirmed_for_role invoked without slice_id=.
   Reachable only on the pipeline-scoped fallback (the slice-scoped path 404s above this point), so harmless today. But _existing_confirmed_for_role accepts slice_id precisely to avoid the contamination fixed in #2535, and a future caller that pulls this fallback into the slice path would silently lose that scoping. Defensively pass slice_id=slice_id.

3. orchestrator/routes/pipelines.py:3291 — worktrees_to_delete filter is role-only, no slice scope.
   enumerate_agent_worktrees returns slice-scoped ({pipeline_id}-slice-{N}-{role}) and non-sliced worktrees alike; the comprehension keeps every entry whose agent_role is in restart_role_values. The comment ("Mirrors cleanup_pipeline's worktree deletion") suggests this is intentional — pipeline-wide reset, not slice-scoped — but it interacts uncomfortably with the new auto_salvage_pipeline step at line 3303, which now triggers salvage across every slice's unpushed work whenever any one slice is restarted. If sequential-slice execution is the ground truth (worktrees from prior slices on disk are always stale), this is fine. If parallel slice execution is on the roadmap, this is a foot-gun that will silently nuke a live sibling slice's worktree. Worth either tightening the filter or adding a comment that pins the assumption.

4. orchestrator/routes/pipelines.py:3192-3205 — bare except Exception swallows the derivation traceback.
   The fallback path that derives a default roster catches Exception and logs only error=str(exc). A real failure (import error, schema bug, …) yields a 400 "No agents found" with no diagnostics. Add exc_info=True so the traceback reaches structured logs.

5. orchestrator/gateway_client.py:52,84-99 — _REBASE_REF_RE is a copy of the gateway helper's regex.
   Inlining is reasonable to avoid a gateway/ import from the orchestrator, but the docstring says the server-side validator is the security floor, and the regex itself accepts inputs git itself would reject (foo..bar, refs ending .lock, etc.). Pre-existing in both copies, but the duplication adds drift risk: if the gateway tightens its regex without the orchestrator following, the orchestrator's pre-flight rejects fewer strings than the gateway does and the user-visible failure mode regresses to a server-side error.

6. orchestrator/state_store_probe.py:278-289 — in-flight grace bounds in_flight_age, not the cached value's age.
   When a probe call is in flight and _last_check_monotonic is arbitrarily stale, the function still reports fresh=True provided in_flight_age <= stale_window. The docstring acknowledges this ("During the grace, fresh=True is reported even though age_seconds may exceed stale_window"). Documented trade-off, not a bug — flagging because operators reading /api/v1/health mid-grace see a healthy=true derived from the previous (potentially much older) cached observation, which is non-obvious from the field semantics. A note on the public health-route serializer would help.

Conclusion

<!-- has-suggestions -->

The cleanup is well-scoped and the deletions don't cost real coverage. The cross-cutting concerns above are all non-blocking — most either pre-date this PR or sit outside the slice-1 deletion scope — but they're worth follow-up issues, especially the signals.py fallback contamination (#1 and #2 above) which is the same bug pattern as the #2535 fix this stack already pulled in, just on the path the fix didn't touch.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

2 previous review(s) hidden.