Egg/issue 2548/work

[jwbron]

No description provided.

[egg-reviewer]

No agent-mode design concerns.

Reviewed the new/changed surfaces against the agent-mode design guidelines:

• orchestrator/impasse_routing.py (new) — pure orchestration over the typed Impasse payload the sandbox handler emits. No LLM calls, no prompt construction.
• sandbox/egg_agent_tools/handlers/restrictions.py (new) — sandbox-side fast-fail helper backed by the same shared/egg_restrictions/patterns.py registry the gateway commit-validator enforces. This is correctly a sandbox/gateway boundary, not a prompt-level instruction.
• shared/egg_contracts/plan_parser.py + docs/templates/plan.md — the YAML appendix is consumed by the orchestrator (Slice/Task contract objects), not by humans, so the structured-output requirement is legitimate. Human-facing prose above it stays free-form.
• orchestrator/routes/pipelines.py — the new _read_phase_draft embed in implement-phase prompts (line 9748) is capped at 32K chars and only fires on review_cycle == 0. It's borderline but bounded, justified by the comment ("avoids file-I/O turns inside the sandbox"), and provides orientation rather than constraining what the agent can fetch. Not a clear anti-pattern.
• No new direct Anthropic API calls in orchestrator/gateway/shared. No new pinned model identifiers. No new post-processing pipelines parsing natural-language agent output.

— Authored by egg

[james-in-a-box]

Contract Verification — PR #2572 (issue #2548)

This review verifies the merged PR against the contract at
.egg-state/contracts/issue-2548.json. Posted as a comment because
the PR has already merged into main — request-changes / approve are
no-ops at this point. Verdict at the bottom.

TL;DR

• Slice-1 implementation (PRMetadata + planner prompt) matches its
  contract acceptance criteria. The model delta, schema-version bump,
  migration shim, tests, planner prompt, and YAML ingestion are all in
  place and behave as specified.
• Contract integrity is broken. The on-disk contract is still in
  current_phase: refine with all 14 tasks across all 5 slices in
  status: pending and zero linked commits. Top-level
  acceptance_criteria: [] — there is nothing for
  egg-contract verify-criterion --criterion ac-N to mark, and
  egg-contract is in any case unreachable
  (Orchestrator unreachable — try again).
• Scope is much larger than slice-1 of #2548. The merged diff
  includes work for issues #2532, #2549, #2474, #2527, #2554, #2529
  alongside slice-1 (and apparently slice-2) of #2548. None of those
  appear on the #2548 contract. Reviewers approaching this PR from
  the contract have no traceable mapping from the contract's tasks to
  the merged commits.

Method

1. Loaded .egg-state/contracts/issue-2548.json directly (orchestrator
   unreachable, so egg-contract show could not be used).
2. Walked each slice / task acceptance_criteria block and checked
   the merged diff (gh pr diff 2572) against the criteria.
3. Spot-checked the slice-1-specific child PR (#2571) to disambiguate
   issue-#2548 work from the rollup of unrelated merges this work
   branch caught up to.

Slice-by-slice findings

Slice-1 — Contract schema delta + planner prompt

task-1-1 (PRMetadata + schemaVersion 1.0 → 1.1) — VERIFIED.

• shared/egg_contracts/models.py adds the four required fields with
  the prescribed defaults:
  • context_title: str | None = None
  • context_description: str | None = None
  • context_branch: str | None = None
  • context_pr_number: int | None = None with ge=1 validator
    (matches the recommendation in the original task description).
• Contract.schemaVersion default bumped from "1.0" to "1.1".
• Model-level migration _migrate_schema_version_to_1_1 runs in
  mode="after", idempotent, only fires on exactly "1.0".
• Acceptance criteria for task-1-1 are met:
  • The four fields are exposed with None defaults.
  • A 1.0 contract round-trips and gets promoted to 1.1.

task-1-2 (PRMetadata tests) — VERIFIED, with one deviation.

• Tests live at tests/shared/egg_contracts/test_pr_metadata.py
  (906 lines), not shared/egg_contracts/tests/test_pr_metadata.py
  as the contract's files_affected lists. The test docstring
  documents this as intentional — the tests/... tree is what
  pytest collects, and the in-package path is not in testpaths.
  This is a sensible pragmatic deviation; flagging it for traceability.
• Coverage matches the acceptance criteria:
  • Round-trip with all four context fields populated.
  • Round-trip with all four context fields omitted (defaults None).
  • Round-trip of schemaVersion=1.0 payload, confirming migration to
    1.1 and None defaults.
  • context_pr_number=0 and negative values raise ValidationError;
    None is accepted; validate_assignment re-runs on setattr.
• Tests live in the diff, would discover under make test /
  make test-all.

task-1-3 (Planner prompt + YAML ingestion) — VERIFIED, with
one minor scope deviation.

• orchestrator/routes/pipelines.py adds:
  • _PR_CONTEXT_GUIDANCE — prose explaining
    pr.context_title / pr.context_description, when to emit them,
    and that pr.context_branch / pr.context_pr_number are
    orchestrator-populated (must NOT be emitted by the planner).
  • _PR_CONTEXT_YAML_EXAMPLE_LINES — commented-out YAML hints
    threaded into the # yaml-tasks example inside _build_phase_prompt.
• shared/egg_contracts/plan_parser.py gains
  extract_pr_context_metadata_from_yaml() and threads
  pr_context_title / pr_context_description through ParseResult
  and parse_plan(). Non-string scalars produce ParseWarning rather
  than coercing silently.
• The acceptance criterion "A planner-emitted YAML containing
  context_title: and context_description: is parsed without error
  and the values land on contract.pr.context_title /
  pr.context_description" is satisfied by the new extractor + the
  optional fields on ParseResult.
• The acceptance criterion "A planner-emitted YAML omitting the new
  keys still parses and both context fields default to None" is
  satisfied by the is None early returns in the extractor.
• Deviation: .github/scripts/checks/plan_yaml_check.py is not
  modified in this PR — but inspection of the file shows it is a
  structure-only validator (presence of phases: / tasks: /
  required ID fields). It does not reject unknown pr.* keys, so
  the new keys pass through it untouched. The criterion
  "plan_yaml_check.py runs cleanly on both inputs" is therefore
  satisfied de facto, but the contract's files_affected listed it,
  and the file was not touched. Non-blocking.
• Deviation: orchestrator/routes/phases.py is also not modified.
  Inspection shows phases.py does not have an independent
  yaml-tasks ingestion path — the canonical path is
  plan_parser.py. The files_affected listing in the contract
  appears to have been speculative. Non-blocking.

Slices 2–5 — out of scope for this PR per the contract's task list

The contract's tasks 2-1 through 5-1 are all status: pending with no
linked commits. However, the merged diff clearly contains work that
overlaps slice-2 (BRC-history per-slice writes —
orchestrator/routes/pipelines.py _write_brc_history,
orchestrator/tests/test_brc_history.py +1073/-57) and surrounding
infrastructure. This is consistent with the child PR #2571 commit
[slice-2] Add context PR + per-slice BRC history (closes #2548) (#2564).

Without a contract-tracked mapping, I cannot reliably attribute lines
to slice-2/3/4/5 acceptance criteria. The contract simply does not
reflect what was merged. This is the contract-integrity issue called
out in the TL;DR — the tasks were never complete-task'd, no commits
were add-commit'd, and the phase was never advanced past refine.

Cross-cutting work that does NOT belong to issue #2548

The PR also bundles, per its commit list:

• f94c7c1c Fix #2532: align .github/ block in agent_roles.py
• 5582aa03 Fix #2549: skip already-merged slices (#2552)
• 3cb7aef5 Egg/issue 2474 v2/work (#2556)
• f87a5ea7 Fix #2527: validate task role↔file alignment (#2551)
• 14cf8879 docs for #2527 (#2558)
• 08fb8f8c Fix #2554: agent-status table on BRC dashboard
• 802dc2c9 Fix #2529: runtime escape hatch for impossible tasks (#2553)

These show up in the diff (e.g. orchestrator/impasse_routing.py +554,
shared/egg_contracts/impasse.py +156, shared/egg_contracts/agent_roles.py,
shared/egg_contracts/plan_parser.py role alignment, etc.) and are
unrelated to issue #2548's contract. Some of them appear to have already
been merged to main via their own PRs (#2552, #2553, #2556, #2558),
so they are showing up here only as catch-up merges into the
long-running egg/issue-2548/work branch. That's how gh pr diff works
against main, not a sign that they were re-merged. Worth noting for
the reviewer audit trail.

Concerns / red flags

1. Contract was not advanced. The on-disk contract is still in
   current_phase: refine, all tasks pending, no commits linked.
   The orchestrator is unreachable so this could not be reconciled
   live. A future agent or operator looking at this contract will
   see "no work done" and that is materially false.
2. Top-level acceptance_criteria: []. The egg-contract verify-criterion
   surface only accepts ac-N IDs against the top-level list, which is
   empty. There is therefore no programmatic way to record the
   verifications I performed in this review against the contract; the
   trail lives only here.
3. Test path differs from contract. Tests are at
   tests/shared/egg_contracts/test_pr_metadata.py (correct for
   pytest discovery) instead of shared/egg_contracts/tests/test_pr_metadata.py
   (as files_affected listed). The author documents this in the test
   docstring; not a blocker.
4. .github/scripts/checks/plan_yaml_check.py and
   orchestrator/routes/phases.py not modified. The contract's
   files_affected listed both. The check is structure-only and does
   not need updating to admit the new keys; phases.py does not have
   an independent ingestion path. Non-blocking but flagged.
5. PR scope is much larger than the contract describes. Slice-2-5
   work and unrelated bug-fix merges are bundled into the same merge
   commit on main. Reviewer hygiene around tracking what landed is
   degraded.

Verdict

Comment — the slice-1 implementation is correct and matches its
acceptance criteria, but the PR cannot be cleanly verified against the
contract because (a) the contract was never advanced past refine,
(b) the orchestrator is unreachable to mark verification status,
(c) the merged diff materially exceeds slice-1's stated scope. The
implementation looks good; the process trail is broken.

— Authored by egg

[egg-reviewer]

Review: PR #2572 (egg/issue-2548/work → main)

Scope: 80 files, +14,941 / −3,263. The bulk is auto-generated .egg-state/ artifacts (skipped per review rules) plus a docker-compose → k3s test-infra cleanup that's already justified in code (#2474). Production changes I focused on: the new Impasse runtime escape-hatch (shared/egg_contracts/impasse.py, orchestrator/impasse_routing.py, sandbox/egg_agent_tools/handlers/restrictions.py), the planner role↔file alignment validator (shared/egg_contracts/plan_parser.py, orchestrator/routes/signals.py), and the schema additions for the forthcoming context-PR (shared/egg_contracts/models.py).

---

Blocking

1. report_impasse handler does not enforce Impasse.reason length → orchestrator silently drops the impasse.
sandbox/egg_agent_tools/handlers/restrictions.py:208-210 only checks non-empty string, but egg_contracts.impasse.Impasse.reason is declared min_length=1, max_length=2000 (shared/egg_contracts/impasse.py:80-88). When an agent submits a multi-page traceback as the reason — exactly the natural thing to do for an external_blocker or unknown category — the handler returns ok: True and persists the over-long payload to the agent-output file. Post-phase, orchestrator/impasse_routing.py:138-147 calls Impasse.from_dict(impasse_raw), Pydantic raises ValidationError, the orchestrator logs "Discarding malformed impasse payload" at warning level, and the impasse is silently dropped. The producer's container has already been told to "Stop all further work for this task and exit cleanly" — so the task stalls with neither delegation nor HITL escalation, and the only signal is a single warning line in the orchestrator log.

This is the exact "operator-facing silent failure" / "cross-module dead-end" pattern the review rules call out: producer emits, downstream filter drops, no signal. Fix at restrictions.py:208-210: enforce len(reason) <= 2000 (and validate task_id shape if there is one) so the failure surfaces synchronously to the agent rather than being absorbed by a server-side warning.

While you're in this area, consider also capping evidence (line 247-249) — evidence: dict[str, Any] accepts arbitrary nesting/size, and Impasse.from_dict doesn't enforce a JSON-serialised limit either; an agent stuffing megabytes of structured data here would bloat HITL decision bodies.

2. Pre-existing Python 2-style except clauses survive in code this PR modifies.
shared/egg_contracts/plan_parser.py:529 (except ValueError, TypeError:) and shared/egg_contracts/agent_roles.py:1013 (except ValueError, KeyError:). These compile on 3.14 only because of PEP 758 (the project's requires-python = ">=3.14"), but:

• Anyone reading the diff without PEP 758 in mind will read these as Python-2 bugs (in pre-3.14 Python they'd be a SyntaxError or, worse, parse as except ValueError as TypeError: and silently bind to the second name).
• Most linters / type-checkers / IDE syntax highlighters ship parser tables that haven't tracked PEP 758 yet — mypy, pyflakes, and Tree-sitter Python all flag these as errors at the time of writing. Anyone running make lint against a not-yet-updated venv will hit syntax errors here.
• This PR rewrites a large fraction of agent_roles.py (+24/-30) — it's the natural place to normalize.

Fix: parenthesize. except (ValueError, TypeError):, except (ValueError, KeyError):. Per the review rules' "pre-existing inconsistency in modified code" clause this is blocking — the PR is in the file already, do it now.

---

Non-blocking (but should be fixed soon)

3. git show in planner role-alignment validator omits -- separator.
orchestrator/routes/signals.py:997:

["git", "-C", str(worktree_path), "show", f"{commit_sha}:{plan_rel}"]

commit_sha comes from the planner's BRC propose payload — (payload.get("commit_sha") or "").strip() at line 957 — with no shape validation. The sibling _verify_commit_on_branch at line 273 uses "--" correctly:

["git", "-C", ..., "branch", "-r", "--contains", "--", commit]

For a SHA starting with -, git show parses it as an option (e.g. --ext-diff, -c key=val, etc.). The threat model is weak — the planner agent already has subprocess access in its sandbox, so this isn't a privilege escalation — but the asymmetry vs _verify_commit_on_branch is a regression in defensive hygiene. Fix: insert "--" before f"{commit_sha}:{plan_rel}", or re.fullmatch(r"[0-9a-f]{7,64}", commit_sha) upstream.

4. Silent git show non-zero / ImportError swallow loses operator signal.
Same function, signals.py:1003-1004:

if result.returncode != 0:
    return  # Plan not present at this commit — nothing to validate.

This conflates "plan absent at commit" (legitimate) with "permission denied / git timeout / corrupted worktree" (bugs the operator wants to know about). Same at signals.py:1020-1021 — bare except ImportError: return for a routes/pipelines import. The function's docstring says "graceful degradation" — fine for the gateway-backstop case, but log result.stderr.strip() at debug or warning level so post-mortem forensics has something to read.

5. restrictions.py accepts caller-supplied identifier / repo_path without validation.
sandbox/egg_agent_tools/handlers/restrictions.py:268-271:

repo_path = Path(req.get("repo_path") or get_repo_path())
identifier = req.get("identifier") or req.get("issue") or req.get("pipeline_id")

Both flow straight into save_agent_output(repo_path, contract_role, output, identifier=identifier) (line 316) which interpolates identifier into a filename: .egg-state/agent-outputs/{identifier}-{role}-output.json. A malicious identifier="../../../../tmp/foo" writes outside the agent-output dir; a malicious repo_path writes outside the repo. The agent already has full filesystem access in its sandbox so this isn't privilege escalation, but it's inconsistent with the very recent _resolve_env_identifier_for_brc_history hardening (sandbox/egg_agent_tools/handlers/brc.py:855-882) that ignores caller-supplied identifier and pulls only from env. Same hardening should apply here — particularly because a path-traversal write could spoof another role's agent-output file (the orchestrator's role-keyed loader trusts the filename → role mapping).

6. _alternative_role returns a producer role even when blocked_role is non-producer.
restrictions.py:47-67 loops for role in ("coder", "tester", "documenter") and skips the matching role. If an architect or overseer calls check_file_restriction, it gets a "helpful" alternative_role suggestion that the orchestrator-side router (impasse_routing._is_eligible_delegation, gated on _DELEGATION_ELIGIBLE_ROLES = {"coder", "tester", "documenter"}) will refuse to honor anyway. Return None early when blocked_role not in {"coder", "tester", "documenter"} so the agent gets honest "no auto-delegation possible" UX.

7. route_impasses swallows save_contract failure but returns in-memory decisions.
orchestrator/impasse_routing.py:393-401:

if mutated:
    try:
        save_contract(contract, repo_path)
    except Exception as exc:  # pragma: no cover - defensive
        logger.error("Failed to persist contract after impasse routing", ...)
return decisions

The caller proceeds with the in-memory decisions list, but on disk the contract is unchanged. If the orchestrator restarts before the next persist boundary, the routing decision (delegation or HITL escalation) is lost — the next iteration scans agent-output files, finds the same impasse, and re-routes. The force_escalate=True terminal path is particularly affected: an escalation that doesn't persist creates no operator-visible HITL decision. Either retry, or surface the failure to the slice-loop's exit code so it doesn't keep walking.

8. Decision IDs derived from len(contract.decisions) collide if anything else ever mutates the list.
impasse_routing.py:248-250 builds decision_id = f"decision-{next_idx + 1}" and field_path = f"decisions.{next_idx}" from the current length. Inside a single route_impasses call this is fine because apply_mutation appends and the loop re-reads length each iteration. But the contract is also mutated by HITL resolution paths and other routers; if any path ever shrinks decisions (e.g. a future cleanup task), this generator collides silently. Use a UUID or secrets.token_hex(4)-suffixed ID.

9. _find_task falls back to "single role match" with (t.role or "coder") == role.
impasse_routing.py:183-187. A task with role=None matches a coder impasse but not a tester/documenter impasse. If the planner ever emits role-less tasks (it currently shouldn't but neither the type nor the schema forbids it), tester/documenter impasses fall through to force_escalate instead of routing correctly. Either normalize task.role at plan-ingestion or document this assumption with an assert at the start of _find_task.

10. New tests in shared/egg_contracts/tests/ may not be picked up by make test.
shared/egg_contracts/tests/test_validate_task_role_alignment.py (286 lines, new) lives next to the source rather than in the tests/ tree configured under [tool.pytest.ini_options].testpaths. The comment at tests/shared/egg_contracts/test_pr_metadata.py:21-25 (also new in this PR) explicitly notes that the in-package test path is not in testpaths. Verify make test picks these up — run .venv/bin/pytest --collect-only shared/egg_contracts/tests/ and confirm. If not, either move the file under tests/ or add the path to testpaths. Same caveat applies to shared/tests/test_github_block_alignment.py and shared/tests/test_impasse_schema.py.

11. integration_tests/test_credential_security.py silently skips under k3s.
integration_tests/conftest.py skips isolated_container / external_container / test_container fixtures (their docstrings acknowledge this) and the conftest itself notes "the main consumer is currently uncovered in CI and needs a k3s-native replacement before it runs again." This is a security-critical property (credential isolation) producing green CI without exercising the property. Not blocking for this PR — it landed via #2474 — but file/link a follow-up issue and convert the skips to xfail(strict=True) so the gap surfaces.

---

Notes (not findings)

• The "context PR" feature is intentionally inert at this slice — docs/architecture/sdlc-pipeline.md:132-138 and docs/templates/plan.md:136-150 explicitly disclose that only the schema fields and planner-prompt advertisement are wired in slice-1; orchestrator branch-creation/PR-opening hooks land in slices 3-4. The _populate_contract_from_plan preservation block at routes/pipelines.py:15771-15786 and the planner-prompt guidance at :9379-9405 are forward-compatible stubs, not silent dead-ends. Honest, not aspirational.
• The Impasse Pydantic model is well-shaped: extra="forbid", min_length=1 / max_length=2000 on reason, default_factory (not mutable defaults) on list/dict fields. Tests cover empty-reason, unknown-category, and extra-field rejection at the model boundary.
• The .github/ block alignment work (agent_roles.py rewrite + test_github_block_alignment.py) cross-checks egg_contracts.agent_roles.get_file_patterns against egg_restrictions.get_agent_pattern for all 11 roles — clean.
• The plan-time _is_file_blocked_for_role mirror of the gateway's push-time FileRestriction.is_file_blocked is the right shape (1:1 normalization, blocked + block-exempt patterns, rejects .. / leading-slash).

---

The two blocking items (silent over-long-reason drop, and the pre-existing PEP 758 except clauses in code this PR modifies) should be straightforward. The defense-in-depth items (3, 4, 5, 6) are also small fixes worth bundling. Item 7 (save_contract failure swallow) and item 11 (credential isolation skip) deserve follow-up issues if not addressed here.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

2 previous review(s) hidden.