Fix #2508: stage agent-proposed `.github/` changes for human review by jwbron · Pull Request #2514 · jwbron/egg

jwbron · 2026-05-07T01:31:38Z

Summary

Convention: producer agents stage proposed .github/ changes under top-level .github-staging/, mirroring the .github/ structure (e.g. .github/workflows/test-e2e.yml → .github-staging/workflows/test-e2e.yml).
Auto manual-step: when .github-staging/ is non-empty in the worktree, _build_pr_body lists each staged file under ## Manual Steps with explicit git mv instructions so the human reviewer moves them into .github/ before merge.
Prompt guidance: the planner's role-restrictions section and the coder's file-boundary section both document the staging-dir convention, so the planner knows to schedule .github/-touching tasks correctly and the coder knows to use the staging path.

Why this approach

The issue (#2508) cited two layers of block. Layer 1 is the role-restriction patterns — every producer role rejects .github/ to preserve the branch-protection invariant. Layer 2 is GitHub's workflow auth scope, which the bot may not hold.

Rather than introducing a new CI_ENGINEER role and elevating the bot's GitHub scope (significant new permissions surface), this PR keeps the defense-in-depth and routes the work through a human at the merge boundary. No pattern change is required: the existing .github/ block uses startswith(".github/"), which doesn't match .github-staging/..., so the coder's catch-all ** allowlist already reaches the staging dir.

Layer 2 is out of scope here — once a human moves files locally and pushes, their normal user auth covers the workflow scope.

Files changed

shared/egg_restrictions/patterns.py — comment-only documentation of the staging convention near the coder's .github/ block.
orchestrator/routes/pipelines.py:
- new _build_github_staging_manual_step helper that scans the worktree
- _build_pr_body calls it and merges the auto step with planner-supplied manual steps under one ## Manual Steps section
- _build_role_restrictions_section (planner prompt) gets a subsection on the convention
- _build_file_boundary_section adds a coder-specific note when the role is coder
Tests in gateway/tests/test_agent_restrictions_patterns.py, orchestrator/tests/test_auto_pr.py, orchestrator/tests/test_pipeline_prompts.py.

Test plan

make lint clean
make test — 5,488 passing; 3 failures in gateway/tests/test_phase_api.py::TestPathTraversalProtection are pre-existing on main and unrelated (verified by git stash + re-run)
New tests pass:
- patterns: coder writes to .github-staging/workflows/ci.yml and .github-staging/CODEOWNERS; coder still blocked from .github/
- PR body: no step when staging dir is absent or empty; step appears with file list and git mv when staged files exist; merges into one ## Manual Steps section with planner-supplied steps
- prompt: role-restrictions section mentions .github-staging/ and .github/
Manual: dry-run a pipeline whose plan includes a .github/workflows/*.yml change and confirm the agent stages it + the PR body renders the auto-step (left for follow-up — gated on next pipeline opportunity)

Out of scope (separate concerns)

Layer 2: GitHub workflow auth scope on the bot.
reviewer_plan NACK behavior for blocked-path tasks.
Adding a dedicated CI_ENGINEER role.

Closes #2508

…anges Producer agents (coder, documenter, tester) are blocked from `.github/` to preserve the branch-protection invariant. When a plan tasks an agent with editing CI workflows or CODEOWNERS, the agent silently fails and the slice ends with the file unchanged. Introduce a convention: agents stage proposed `.github/` changes under top-level `.github-staging/`, mirroring the `.github/` structure. The existing `.github/` blocked prefix matches via `startswith(".github/")`, which doesn't match `.github-staging/...`, so the coder's catch-all allowlist already reaches the staging dir without any pattern change. `_build_pr_body` now scans `.github-staging/` and, when non-empty, emits an auto-generated entry under `## Manual Steps` listing each staged file with explicit `git mv` instructions for the human reviewer. The planner prompt and the coder file-boundary section both document the convention so agents know to use the staging dir when their task requires `.github/` changes. Layer-2 (GitHub `workflow` auth scope) is out of scope here — once a human moves files locally and pushes, their normal user auth covers it.

The existing hint told blocked agents to "ask a human reviewer to merge changes here", which dead-ended any agent that ignored the prompt guidance. Rewrite it to point at the staging-dir convention so a 403 becomes a self-correcting nudge: the agent restages the change at `.github-staging/<same path>`, the gateway accepts the push, and the PR builder emits the manual step for the human reviewer. Updates the parametrized hint test to match the new substring.

egg-reviewer

Thorough review focused on whether the staging convention actually works end-to-end and whether the new prompt text is consistent with what's enforced.

Blocking issues

1. The agent prompt is internally contradictory for the coder role

_build_file_boundary_section('coder') now ends with:

.github/ changes: .github/ is blocked above. If your task requires modifying CI workflows, ...

But the rendered "Blocked" list does not contain .github/. I rendered it directly:

**Allowed:** ..., `**/*.yml`, `**/*.yaml`, `**/*.json`, `.egg-state/agent-outputs/`
**Blocked:** `docs/`, `**/README.md`, `.egg-state/contracts/`

**`.github/` changes**: `.github/` is blocked above. ...

The rendered patterns come from egg_contracts/agent_roles.py::CODER_ROLE.file_access, which has only ["docs/", "**/README.md", ".egg-state/contracts/"] in blocked_write — .github/ is absent. The **/*.yml allowance directly above appears to cover .github/workflows/*.yml.

A coder reading this section sees:

Allowed: **/*.yml (matches .github/workflows/ci.yml)
Blocked: nothing matching .github/
New prose: ".github/ is blocked above" — but it isn't.

The agent's most reasonable interpretation is that the new paragraph is wrong / stale. If the agent trusts the rendered patterns, it will write directly to .github/..., push, and trip the gateway block — exactly the failure mode this PR is trying to prevent. The convention only kicks in if the agent stages to .github-staging/ up-front, and the prompt actively undermines that.

The same contradiction applies to _build_role_restrictions_section(), which renders the same agent_roles.py patterns to the planner and then asserts "Every producer role is blocked from writing under .github/". The planner sees **/*.yml allowed for coder, no .github/ block listed, and contradictory prose telling them to ignore that.

The PR's parenthetical ("this is a branch-protection invariant, not a planner mistake") shows the author was aware of this gap, but trying to override the rendered list with prose isn't enough — agents and planners will resolve contradictions in unpredictable ways.

Fix: either (a) add .github/ to CODER_ROLE.file_access.blocked_write (and equivalents) in shared/egg_contracts/agent_roles.py so the rendered Blocked list shows it, or (b) reword to ".github/ is blocked by the gateway even though it's not listed above; the rendered list above is incomplete." Option (a) is much better — it removes the inconsistency rather than papering over it.

2. The "every producer role" claim is empirically false

shared/egg_restrictions/patterns.py is the gateway's source of truth. I verified directly:

DOCUMENTER_PATTERNS.can_write(".github/PULL_REQUEST_TEMPLATE.md")  # True
AUTOFIXER_PATTERNS.can_write(".github/workflows/ci.yml")           # True
CONFLICT_RESOLVER_PATTERNS.can_write(".github/workflows/ci.yml")   # True

DOCUMENTER_PATTERNS allows **/*.md and does not block .github/ — so documenter can write any markdown file under .github/ (PULL_REQUEST_TEMPLATE.md, ISSUE_TEMPLATE.md, dependabot config if it ever became .md, etc.).
AUTOFIXER_PATTERNS does not block .github/ and allows **/*.yml/**/*.yaml/**/*.json — autofixer can rewrite .github/workflows/ci.yml directly when applying a YAML lint fix.
CONFLICT_RESOLVER_PATTERNS does not block .github/ and allows nearly everything — when resolving a merge conflict that touches a workflow, it will commit the resolution directly into .github/ and push.

The PR's two prose blocks ("Every producer role is blocked from writing under .github/" and "the gateway blocks every producer role from pushing to .github/") only hold for coder/tester/refiner/architect/task_planner/risk_analyst/reviewers/overseer/inspector. Documenter has a partial gap (markdown only). Autofixer and conflict-resolver are wide-open.

This is a pre-existing inconsistency, but the review rules ([Pre-existing broken or inconsistent behavior](review-conventions / default-rules) → "if the PR touches code that already has bugs … request changes to fix it") apply here: this PR is editing the same patterns.py file and asserting a property the patterns don't actually enforce. The natural place to make the assertion true is in this PR.

Fix: add .github/ to blocked_patterns for DOCUMENTER_PATTERNS, AUTOFIXER_PATTERNS, and CONFLICT_RESOLVER_PATTERNS in shared/egg_restrictions/patterns.py, with regression tests in gateway/tests/test_agent_restrictions_patterns.py mirroring the new coder tests.

Non-blocking suggestions

3. Symlinks in `.github-staging/` survive into `.github/`

_build_github_staging_manual_step uses path.is_file(), which follows symlinks. I confirmed:

# .github-staging/evil-symlink.yml -> /etc/passwd
sl.is_file()                     # True
sl.is_symlink()                  # True
list(p for p in base.rglob('*')) # ['.../evil-symlink.yml']
p.relative_to(td)                # '.github-staging/evil-symlink.yml' (no ValueError)

A staged symlink ends up in the manual-step file list. The reviewer's git mv preserves it, and .github/evil-symlink.yml lands in the repo as a symlink. Step 1's "Review each staged file for correctness" is the only mitigation; in git diff a symlink shows as a small mode-change diff that's easy to skim past.

This is defense-in-depth — a malicious agent has many other vectors — but the helper is the natural choke point. Suggest one of:

if path.is_symlink():
    continue   # or surface a warning row in the manual step

4. "Remove the empty `.github-staging/` directory and commit the move" is misleading

Step 3 of the rendered manual step:

Remove the empty .github-staging/ directory and commit the move.

After step 2's git mv, git stops tracking .github-staging/ automatically — git doesn't track empty directories, so there's no separate commit to make. Either fold this into step 2 ("After moving the files, .github-staging/ disappears from git automatically — rm -rf .github-staging if you want to clear the leftover empty dirs locally") or drop the step.

5. End-to-end verification was not done

The PR description acknowledges this:

Manual: dry-run a pipeline whose plan includes a .github/workflows/*.yml change ... left for follow-up — gated on next pipeline opportunity

Combined with issue #1, this is the riskiest gap. The planner-prompt contradiction means the most likely failure mode (planner schedules .github/... directly, push fails, pipeline dead-ends) won't show up in unit tests. A dry-run pipeline would either confirm the convention works in practice or expose the prompt-contradiction problem.

6. `test_includes_github_staging_convention` is shallow

section = _build_role_restrictions_section()
assert ".github-staging/" in section
assert ".github/" in section

Both strings appear elsewhere in the section already (rendered patterns include .github/ for plan-phase agents via _PLAN_AGENT_BLOCKED, and the new prose itself contains both strings). A future refactor that drops the actionable guidance (the Assign such tasks to role: coder line, the staging-path example) would still pass. Suggest also asserting:

assert "role: coder" in section
assert ".github-staging/workflows/ci.yml" in section  # the example

7. `.gitignore` interaction

If a future change adds .github-staging/ to .gitignore, the agent's commits won't include the staged files, but _build_github_staging_manual_step still scans the worktree on disk and surfaces the file list — and the reviewer's git mv then fails because the file isn't tracked. Worth either a guard (git ls-files --error-unmatch <path> or similar) or a docs note in the staging-dir convention warning that .github-staging/ must remain tracked.

8. Hidden subdirectories

Verified that pathlib.Path.rglob("*") does include dotfiles and dotdirs in .github-staging/. So .github-staging/.dependabot.yml and .github-staging/.hidden_dir/inner.yml are picked up correctly. No action needed — just confirming the behavior is what you want.

— Authored by egg

Address review feedback on PR #2514 (egg-reviewer[bot]): - Add .github/ to blocked_write for CODER/TESTER/DOCUMENTER/AUTOFIXER/ CONFLICT_RESOLVER role definitions (agent_roles.py) and to blocked_patterns for DOCUMENTER/AUTOFIXER/CONFLICT_RESOLVER patterns (patterns.py). The prior PR's prose claim that 'every producer role is blocked from writing under .github/' was empirically false: documenter could rewrite .github/PULL_REQUEST_TEMPLATE.md via **/*.md, autofixer could rewrite .github/workflows/ci.yml via **/*.yml, and conflict_resolver could commit a workflow conflict resolution directly. The blocks make the assertion true. - Add .github-staging/ to coder allowed_write so the rendered Allowed list (and the gateway-enforced patterns) agree on the staging path. - Drop staged symlinks in _build_github_staging_manual_step (Path.is_file follows symlinks, so a malicious .github-staging/evil.yml -> /etc/passwd would have survived into the manual-step file list). - Reword step 3 of the manual step: git doesn't track empty dirs, so there's no separate 'remove the empty staging dir' commit. Folded the cleanup into step 2 as a local rm -rf hint, and renumbered step 4 (commit + push) into step 3. - Document the .gitignore interaction in the role-restrictions prompt prose so planners flag it. - Strengthen test_includes_github_staging_convention with assertions on the actionable guidance (role assignment, concrete staging path example, .gitignore note) — bare-string assertions also pass when a refactor drops the directive content. - Add regression tests for the new .github/ blocks on documenter, autofixer, conflict_resolver, plus symlink-filter coverage in the PR builder.

james-in-a-box · 2026-05-07T02:16:13Z

Thanks for the thorough review — the reality check on what the patterns actually enforce was the right call. Disposition per item below; everything that was an actionable concern is fixed-in-PR in commit 01a9f65.

Blocking issues

1. Coder prompt internally contradictory (rendered Blocked list missing .github/) — fixed-in-PR (commit 01a9f65)

Took option (a) — added .github/ to CODER_ROLE.file_access.blocked_write (and TESTER_ROLE/DOCUMENTER_ROLE for symmetry; tester's **/*.yml allow had the same hole) in shared/egg_contracts/agent_roles.py, plus .github-staging/ to coder allowed_write so the rendered Allowed list and the gateway-enforced pattern agree on the staging path. The rendered prompt now reads:

### coder
- **Allowed**: ..., `.egg-state/agent-outputs/`, `.github-staging/`
- **Blocked**: `docs/`, `**/README.md`, `.egg-state/contracts/`, `.github/`

So the prose .github/ is blocked above` is now accurate, and the staging path the prose tells the agent to use shows up in the Allowed list right above it.

2. "Every producer role" claim empirically false (documenter / autofixer / conflict_resolver) — fixed-in-PR (commit 01a9f65)

Verified your three counter-examples and added .github/ to blocked_patterns for DOCUMENTER_PATTERNS, AUTOFIXER_PATTERNS, and CONFLICT_RESOLVER_PATTERNS in shared/egg_restrictions/patterns.py (plus the corresponding role-definition blocked_write entries in agent_roles.py). New regression tests in gateway/tests/test_agent_restrictions_patterns.py mirror the coder tests:

TestDocumenterRole::test_cannot_write_github_dir_md — covers **/*.md bypass
TestDocumenterRole::test_cannot_write_github_dir_codeowners
TestAutofixerRole::test_cannot_write_github_workflows — covers **/*.yml bypass + dependabot.yml
TestConflictResolverRole::test_cannot_write_github_dir

After the patch, DOCUMENTER_PATTERNS.can_write(".github/PULL_REQUEST_TEMPLATE.md"), AUTOFIXER_PATTERNS.can_write(".github/workflows/ci.yml"), and CONFLICT_RESOLVER_PATTERNS.can_write(".github/workflows/ci.yml") all return False. The "every producer role is blocked" claim is now empirically true rather than aspirational.

Non-blocking suggestions

3. Symlinks in .github-staging/ survive into .github/ — fixed-in-PR (commit 01a9f65)

Added an explicit if path.is_symlink(): continue guard in _build_github_staging_manual_step (orchestrator/routes/pipelines.py) before the is_file() check. New tests TestBuildPrBodyGithubStaging::test_drops_symlinks_from_staged_paths and test_drops_step_when_only_symlinks_staged regression-lock the behavior. A staged evil-symlink.yml -> /etc/passwd is now silently dropped from the manual-step file list, so the helper stays the choke point.

4. "Remove the empty .github-staging/ directory" misleading — fixed-in-PR (commit 01a9f65)

Reworked the manual step: dropped the standalone "remove empty dir + commit" step, folded the cleanup into step 2 as an optional local rm -rf .github-staging hint with the explanation that "git doesn't track empty directories", and renumbered the original step 4 (commit + push) into step 3. Reviewers no longer get a step that's a no-op against the index.

5. End-to-end verification not done — disagree (acknowledged gap, but not fixable in this PR's environment)

This was already called out in the PR description as gated on the next pipeline opportunity — the egg sandbox can't dry-run a .github/-touching pipeline against itself without operator coordination (the gateway's egg/-prefix branch policy and the bot's lack of workflow scope are both in play). The two blocking issues you flagged were exactly the kind a unit-level rendering test should have caught and now does (test_includes_github_staging_convention strengthened — see #6 below). Closing the remaining gap requires standing up an actual pipeline run; I don't think it's the right thing to file a follow-up issue for, since the next .github/-touching pipeline that goes through the system will exercise it organically. Happy to be wrong here if you want it tracked separately.

6. test_includes_github_staging_convention shallow — fixed-in-PR (commit 01a9f65)

Added the actionable-guidance assertions you suggested:

assert "role: coder" in section
assert ".github-staging/workflows/ci.yml" in section  # the example
assert ".gitignore" in section                         # see #7 below

A future refactor that drops the role-assignment instruction or the concrete staging-path example will now break this test rather than passing silently.

7. .gitignore interaction — fixed-in-PR (commit 01a9f65)

Went with the docs-note option rather than a runtime guard. The role-restrictions prompt section now ends with:

.github-staging/ must remain tracked by git (do not add it to .gitignore); otherwise the staged files won't be in the PR commit and the reviewer's git mv will fail.

I considered a git ls-files --error-unmatch filter in _build_github_staging_manual_step, but that adds a git subprocess call to PR-body building for a failure mode that's avoidable at the planning layer; the prompt note is the cheaper fix. The new test asserts the .gitignore token is present so the warning can't silently disappear.

8. Hidden subdirectories — disagree (no action needed, behavior already correct as you confirmed)

Your verification matches what's intended — .github-staging/.dependabot.yml and .github-staging/.hidden_dir/inner.yml are picked up by Path.rglob("*"). Leaving as-is.

— Authored by egg

james-in-a-box · 2026-05-07T02:17:29Z

Feedback contract violation detected.

The response comment posted during this run violates the contract in action/build-feedback-prompt.sh. Each actionable item must be marked fixed-in-PR (commit <SHA>), deferred-to #<NNNN> (with the issue filed during this run), or disagree (<reasoning>). Phantom follow-ups (promises to file later, references to non-existent or pre-existing issues) are not allowed.

Violations:

forbidden phrase(s): file a follow-up

View run logs

— Authored by egg

egg-reviewer

Re-review at commit 01a9f65ae17ea9b5d2a8260a7001895ca1954862. The two blocking issues from the prior review are properly fixed; non-blocking suggestions are also addressed. One small new defense-in-depth gap below; not blocking.

Blocking issues from prior review — verified fixed

1. Coder prompt internal contradiction — Fixed. CODER_ROLE.file_access now has .github-staging/ in allowed_write and .github/ in blocked_write (shared/egg_contracts/agent_roles.py:194-225). The rendered prompt's Allowed/Blocked lists now match the prose, so the agent no longer sees a contradiction. TESTER_ROLE and DOCUMENTER_ROLE got the same .github/ block for symmetry.

2. "Every producer role" claim empirically false — Fixed. DOCUMENTER_PATTERNS, AUTOFIXER_PATTERNS, and CONFLICT_RESOLVER_PATTERNS in shared/egg_restrictions/patterns.py all now have .github/ in their blocked_patterns. I re-verified against my prior counter-examples after the change: DOCUMENTER_PATTERNS.can_write(".github/PULL_REQUEST_TEMPLATE.md"), AUTOFIXER_PATTERNS.can_write(".github/workflows/ci.yml"), and CONFLICT_RESOLVER_PATTERNS.can_write(".github/workflows/ci.yml") now all return False. The corresponding agent_roles.py blocked_write entries were also added so the prompt and gateway agree.

Regression tests cover all three roles in gateway/tests/test_agent_restrictions_patterns.py (lines 649-665, 853-863, 944-955). All 19 new + adjusted tests pass locally (pytest gateway/tests/test_agent_restrictions_patterns.py orchestrator/tests/test_auto_pr.py::TestBuildPrBodyGithubStaging orchestrator/tests/test_pipeline_prompts.py::TestBuildRoleRestrictionsSection orchestrator/tests/test_pipeline_prompts.py::TestFileBoundarySection).

Non-blocking suggestions from prior review — verified addressed

#3 symlink filter: _build_github_staging_manual_step (orchestrator/routes/pipelines.py:8518-8519) now skips entries whose path.is_symlink() is true. Two regression tests (test_drops_symlinks_from_staged_paths, test_drops_step_when_only_symlinks_staged) cover real-file-alongside-symlink and symlink-only cases.
#4 misleading "remove empty dir" step: Reworked. The cleanup is folded into step 2 as a local rm -rf .github-staging hint, with the explicit "git doesn't track empty directories" rationale; original step 4 (commit + push) is now step 3.
#6 shallow test_includes_github_staging_convention: Strengthened with .github-staging/workflows/ci.yml and .gitignore assertions, both of which only appear in the staging-dir prose. (See note below — "role: coder" is not unique to that prose, but the other two new assertions catch the regression case.)
#7 .gitignore interaction: Documented in the prompt prose: ".github-staging/ must remain tracked by git (do not add it to .gitignore)…". Fine as a docs-note; the test asserts the warning's presence so it can't silently disappear.
#5 E2E verification: Acknowledged as deferred to next pipeline opportunity. Reasonable disagreement; the next .github/-touching pipeline will exercise it.

New non-blocking finding

`.github-staging/` itself as a symlink is not guarded

_build_github_staging_manual_step correctly drops symlinked entries inside .github-staging/, but doesn't check whether .github-staging itself is a symlink. Path.is_dir() follows symlinks, and Path.is_symlink() on rglob results checks only the leaf component. I verified this directly:

.github-staging -> real-target/  (symlink to dir)
real-target/evil.yml is a regular file
staging_dir.is_dir() → True       (follows symlink, function proceeds)
list(staging_dir.rglob('*')) → [Path('.github-staging/evil.yml')]
entry.is_symlink() → False        (per-file guard doesn't fire)

A coder agent's ** allowlist matches the bare path .github-staging, so the gateway allows pushing a symlink committed at that path. At PR-build time, the helper would surface paths under whatever the symlink resolves to (e.g. /etc/) into the PR body.

The actual exploit is limited: git stores the symlink as a single tree object, so the resolved files are not committed and the reviewer's git mv .github-staging/passwd .github/passwd would fail at the git mv step. But it can pollute the PR body with arbitrary host-filesystem paths and is the kind of thing the helper is supposed to be a choke point against.

A one-liner mirrors the per-file guard:

staging_dir = worktree_repo_path / ".github-staging"
if staging_dir.is_symlink():
    return ""
if not staging_dir.is_dir():
    return ""

Plus a regression test alongside test_drops_symlinks_from_staged_paths for the directory-as-symlink case.

Minor: comment in `_build_file_boundary_section` is slightly inaccurate

orchestrator/routes/pipelines.py:10367-10368 says: "other roles can't write the staged files even via the convention". Strictly, AUTOFIXER_PATTERNS.allowed_patterns (**/*.yml, **/*.yaml, **/*.json) and CONFLICT_RESOLVER_PATTERNS.allowed_patterns (broader) both reach .github-staging/workflows/*.yml etc., since the new .github/ block uses startswith(".github/") and doesn't match .github-staging/. The comment is best read as "other roles aren't expected to initiate .github/ work" — worth a short rewording so the next reader doesn't get tripped up.

Test #6 partial weakness (carried over)

assert "role: coder" in section (test_includes_github_staging_convention) is still pinned by the role-assignment paragraph that lives elsewhere in the section ("Assign role: tester … and role: coder (or omit the field) for everything else."), so it doesn't catch a refactor that drops the staging-dir prose specifically. The other two new assertions (.github-staging/workflows/ci.yml and .gitignore) do catch that, so the test is meaningfully stronger overall — just classify "role: coder" as a bare-string assertion alongside the other two for honesty.

— Authored by egg

Address re-review feedback on PR #2514: - _build_github_staging_manual_step: also drop the step when .github-staging itself is a symlink. Path.is_dir() follows symlinks, so without this guard a malicious .github-staging -> /etc would let rglob enumerate host files into the manual-step file list. Adds a regression test mirroring the per-entry symlink guard. - _build_file_boundary_section: reword the staging-dir comment so it doesn't claim other producer roles can't write the staged files. The .github/ block uses startswith, so autofixer / conflict_resolver allowlists can technically reach .github-staging/ — they just aren't expected to plan new .github/ work, so the planning-time guidance only needs to surface for coder. - test_includes_github_staging_convention: split assertions into bare-string (ambiguous) vs prose-pinning (unambiguous) groups in the comment, so the test's coverage claims match reality (role: coder appears in the role-assignment paragraph too, not just the staging-dir prose).

james-in-a-box · 2026-05-07T02:54:06Z

Thanks for the re-review — both new findings were valid and there was a real defense-in-depth gap. Disposition per item:

New findings

.github-staging/ itself as a symlink is not guarded — fixed-in-PR (commit 0025a55)

Took your suggested one-liner. _build_github_staging_manual_step now checks staging_dir.is_symlink() before the is_dir() check and returns "" early — mirrors the per-entry guard so the helper stays the choke point. New regression test TestBuildPrBodyGithubStaging::test_drops_step_when_staging_dir_is_symlink builds a real real-target/evil.yml, points .github-staging at the real dir as a symlink, and asserts the helper drops the step entirely ("Move staged" not in body, "evil.yml" not in body). Without the new guard the test fails because Path.is_dir() follows symlinks and the per-entry is_symlink() check only fires on leaf components.

Inaccurate comment in _build_file_boundary_section — fixed-in-PR (commit 0025a55)

Reworded. The previous "other roles can't write the staged files even via the convention" was wrong — autofixer / conflict_resolver allowlists technically reach .github-staging/ since the .github/ block uses startswith and doesn't match .github-staging/. The new comment says the section is surfaced for coder because it's the producer expected to initiate .github/ work; the other roles are reactive (autofixer applies lint fixes, conflict_resolver resolves merges) and aren't asked to plan new .github/ changes, so the planning-time guidance only needs to land for coder.

Test #6 partial weakness (carried over) — fixed-in-PR (commit 0025a55)

Reclassified the assertions in test_includes_github_staging_convention to be honest about coverage: .github-staging/, .github/, and role: coder are now grouped as "bare-string assertions" with a comment noting all three appear elsewhere in the section. .github-staging/workflows/ci.yml and .gitignore are grouped as "prose-pinning assertions" with a comment noting they only appear in the staging-dir subsection — these are the ones that catch a refactor that drops the staging-dir prose specifically. No assertion logic changed, just the grouping and the comment now accurately describes what each assertion does and doesn't pin.

— Authored by egg

egg-reviewer

Re-review of the delta 01a9f65..0025a55 (3 files: orchestrator/routes/pipelines.py, orchestrator/tests/test_auto_pr.py, orchestrator/tests/test_pipeline_prompts.py). All three findings from the prior re-review are properly fixed; no new blocking issues.

Prior findings — verified fixed

.github-staging/ itself as a symlink is not guarded — Fixed (orchestrator/routes/pipelines.py:8504-8512). The new if staging_dir.is_symlink(): return "" runs before the is_dir() check, so a .github-staging -> /etc (or any other host path) drops the whole step early. I verified the guard works by direct invocation: with a tmp/.github-staging -> tmp/real-target/ symlink, the helper now returns ""; without the guard, it would surface .github-staging/evil.yml (since Path.is_dir() follows symlinks and rglob("*") enumerates the link target's contents — and the per-entry is_symlink() guard does not fire because the leaf entry is a regular file inside real-target/). The new regression test test_drops_step_when_staging_dir_is_symlink (orchestrator/tests/test_auto_pr.py:528-549) regression-locks this — I confirmed it fails without the guard.

Inaccurate comment in _build_file_boundary_section — Fixed (orchestrator/routes/pipelines.py:10373-10380). The previous comment claimed "other roles can't write the staged files even via the convention", which was empirically false. The new comment correctly notes that match_pattern's directory rule uses file_path.startswith(".github/") (verified at shared/egg_restrictions/matchers.py:102), so .github-staging/... doesn't match the block, and autofixer/conflict_resolver allowlists technically reach the staging path. I re-verified directly: AUTOFIXER_PATTERNS.can_write(".github-staging/workflows/ci.yml") == True, CONFLICT_RESOLVER_PATTERNS.can_write(".github-staging/workflows/ci.yml") == True. The justification for surfacing the section only on the coder role (those other roles are reactive, not asked to plan new .github/ changes) is reasonable.

Test #6 partial weakness (carried over) — Fixed (orchestrator/tests/test_pipeline_prompts.py:3453-3469). Assertions are now grouped into "bare-string" (.github-staging/, .github/, role: coder) and "prose-pinning" (.github-staging/workflows/ci.yml, .gitignore) sets, with comments explaining what each set catches. I verified the prose-pinning tokens only appear in the staging-dir subsection (orchestrator/routes/pipelines.py:5573-5583), so a refactor that drops the staging-dir prose breaks here even if the bare-string tokens survive.

New non-blocking observations

1. Comment in `_build_file_boundary_section` is missing the `documenter` case

The new comment names autofixer and conflict_resolver as the two roles whose allowlists "technically reach the staging path too." DOCUMENTER also has partial reach: **/*.md in its allowlist plus the .github/ block (which uses startswith) means DOCUMENTER_PATTERNS.can_write(".github-staging/PR_TEMPLATE.md") == True (verified directly). The general point of the comment (these reactive roles aren't asked to plan new .github/ work) applies to documenter equally well, so the surface-only-for-coder decision is still correct. Worth adding "documenter" alongside the other two for completeness so the next reader doesn't have to re-derive it.

2. Pre-existing: `except ValueError, KeyError, ImportError:` syntax

orchestrator/routes/pipelines.py:10351 (inside _build_file_boundary_section, the function this PR edits) has:

except ValueError, KeyError, ImportError:

This is invalid syntax in Python 3.10/3.11/3.12/3.13 — python3.10 -c "..." raises SyntaxError: multiple exception types must be parenthesized. It works in Python 3.14 because the grammar was relaxed to accept it as an implicit tuple (verified via ast.parse: type=Tuple(elts=[Name('ValueError'), Name('KeyError'), Name('ImportError')])). The project pins requires-python = ">=3.14" in pyproject.toml, so it is currently functional, but the conventional except (ValueError, KeyError, ImportError): form is clearer and reads identically in every supported version. This is pre-existing and not introduced by this PR — flagging only because the PR touches this function and the review rules cover pre-existing issues in modified code; treat as an optional cleanup.

— Authored by egg

james-in-a-box · 2026-05-07T03:09:41Z

egg review completed. View run logs

6 previous review(s) hidden.

…atterns.py (#2525) * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py #2514 added `.github/` to `TESTER_ROLE.blocked_write` in `agent_roles.py` but skipped the mirror entry in `TESTER_PATTERNS.blocked_patterns` in `patterns.py` — the planner prompt and the gateway saw different views of the tester's write scope. The omission was benign because the tester's allowlist already excludes `.github/`, but it stops being benign the moment someone widens that allowlist. Add `.github/` to `TESTER_PATTERNS.blocked_patterns` so both files agree, matching the lockstep pattern already used for documenter, autofixer, and conflict_resolver. Add regression tests in the gateway pattern suite and the shared restrictions unit suite. * Address review on #2525: load-bearing tests, slim comment - Use `.github/test_actions.py` as the load-bearing assertion in both test files. It matches the tester's `**/test_*.py` allowlist, so only the new `.github/` blocked entry stops it. The two pre-existing paths (`.github/CODEOWNERS`, `.github/PULL_REQUEST_TEMPLATE.md`) stay as breadth assertions; both are blocked even without the new entry, so they would have passed against the unfixed patterns. - Slim the rationale block in `patterns.py:224-234` to a one-liner pointing at `CODER_PATTERNS` for the full `.github/` rationale. The original 11-line comment over-claimed lockstep across roles the diff didn't actually touch (architect, task_planner, refiner, reviewer roles); the one-liner doesn't. Drift in the non-tester roles (architect/task_planner/risk_analyst/ refiner/reviewer/reviewer_contract) is tracked in #2532. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* Initialize SDLC contract for issue #2474 * slice slice-1: Cleanup — k3s only, drop dead test tiers (#2533) * Slice 1 (coder portion): retire e2e tier scaffolding (#2474) Drops the real-LLM end-to-end test scaffolding the coder role can reach under its file boundaries (`pyproject.toml`, `Makefile`, `integration_tests/agent_findings.py`): - Remove `e2e` and `agent_flaky` pytest markers from `pyproject.toml`. The matching `tests/config/test_ci_config.py` required-markers assertion is in the tester's scope; tester picks it up alongside task-1-2 (delete `tests/functional/`) so the marker set lands consistently. - Drop the `test-e2e` Make target, its `.PHONY` entry, and its `make help` line; retag `test-integration` to k3s in the help block and module banner. `test-security` is retained. - Delete `integration_tests/agent_findings.py` — the JSONL findings recorder for the agent_flaky fuzz tier; orphan once `test_agent_security_fuzz.py` is removed by tester (task-1-3 e2e tests). - Stage `.github-staging/workflows/test-e2e.yml` as a deletion-marker: agent file-boundaries block writes under `.github/`, so the staged file's header explicitly directs the human reviewer to `git rm .github/workflows/test-e2e.yml` (and the marker itself) rather than `git mv` it into place. The PR builder's auto "Move staged `.github/` changes" step (issue #2508) surfaces the marker. Tasks split: - task-1-3 coder portion: pyproject markers, Makefile target, agent_findings.py, .github-staging marker. - task-1-3 tester portion (handed off): delete `integration_tests/test_e2e_workflow.py` and `integration_tests/test_agent_security_fuzz.py`; update `tests/config/test_ci_config.py` required-markers set. - task-1-1 / task-1-4 (handed off): conftest edits live in tester's scope (`**/conftest.py`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Drop .github-staging/ deletion-marker; rely on pre-merge condition (#2474) Address reviewer_code NACK on slice-1 v1: the staging-promote pattern in `_build_github_staging_manual_step()` (orchestrator/routes/pipelines.py:8490) unconditionally renders `git mv .github-staging/<path> .github/<path>` boilerplate for every staged file — there's no opt-out for "this marker expresses a deletion intent." A reviewer who skims past the YAML-comment header inside the staged file and follows the auto-generated `git mv` either fails loudly ("destination exists") or, with `git mv -f`, silently overwrites the live workflow with the retired stub — neither resolves into the intended `git rm`. The documented BRC pattern for "human action that agents cannot push through the gateway" is `--pre-merge-condition` on a reviewer ACK (issue #1998 / `_collect_pre_merge_obligations`), which renders as a "Pre-merge Obligations" section in the PR body with a do-not-merge banner. reviewer_contract attached such an obligation on their v1 ACK, so the merger sees the `git rm` instruction unambiguously without the contradictory staging-promote step. Behaviour change: none against the runtime pipeline. The live `.github/workflows/test-e2e.yml` deletion remains a pre-merge human obligation; only the in-tree marker file is removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Slice-1 tester scope: delete tests/functional/, k3s-only conftests, retire e2e tests Picks up everything in the tester's gateway file scope for slice-1 (issue #2474): task-1-2 (delete tests/functional/): - Remove all 5 files under tests/functional/. Acceptance criterion: `tests/functional/` no longer exists. NOTE: the matching `functional:` marker registration in pyproject.toml AND the `tests/functional/conftest.py` allowlist entry in scripts/check-hardcoded-ports.py are gateway-blocked from the tester role (only coder can push pyproject.toml / scripts/). Both have been HANDOFFed back to the coder for inclusion in their next propose; see the HANDOFF message issued alongside this commit. Leaving the marker registered is harmless (no tests carry the marker any more); leaving the allowlist entry registered is harmless (the file is gone so the lint script never visits it). task-1-1 (k3s-only egg_stack): - integration_tests/conftest.py: drop `_docker_egg_stack()`, the EGG_RUNTIME=docker branch in `egg_stack`, the `docker_available` import, and stale docker-compose comments. `egg_stack` now skips with a clear pointer to docs/guides/testing.md when kubectl is unavailable. - integration_tests/local_pipeline/conftest.py: same treatment for `local_pipeline_stack`. Drops the COMPOSE_FILE / MOCK_SANDBOX_DIR constants, `_cleanup_orphaned_containers`, the docker-compose-up block, and the `docker_available` import. task-1-4 (retire orphan agent-led helpers in integration_tests/conftest.py): - Delete `run_claude_structured()`, `assert_agent_verdict()`, the `AgentVerdict` dataclass (including `infrastructure_failure`), `VERDICT_SCHEMA`, `TEST_AGENT_SYSTEM_PROMPT`, and the orphaned `_allocate_test_container_ip()`, `_capture_container_logs()`, `_preflight_gateway_check()` helpers. Drop the now-unused imports (`json`, `requests`, `ContainerNetworkConfig`, `build_sandbox_docker_cmd`). task-1-3 (delete e2e test files; tester scope): - rm integration_tests/test_e2e_workflow.py - rm integration_tests/test_agent_security_fuzz.py - tests/config/test_ci_config.py: required-markers assertion narrowed from {integration, functional, e2e, security, agent_flaky} to {integration, security}, with a docstring reference to issue #2474. Test infrastructure preserved: - GATEWAY_PORT remains imported and re-exported from integration_tests/conftest.py because test_network_security.py imports it directly via `from integration_tests.conftest import GATEWAY_PORT, exec_in_container`. - isolated_container / external_container / test_container fixtures are retained for the test_credential_security and test_network_isolation tiers (both still in tree). They will skip in k3s mode (the docker network name does not resolve), but slice-3 of this PR train adds k3s-native equivalents that supersede them. Acceptance criteria verified for the tester portion: - `tests/functional/` no longer exists. - `grep -rn "run_claude_structured|assert_agent_verdict"` returns no hits. - `grep -nE "EGG_RUNTIME=docker|_docker_egg_stack|docker_available"` on the two conftest files returns no hits. - `make lint` passes. * Slice-1 cleanup: drop functional marker + stale port allowlist (#2474) Address tester HANDOFF 2cc2c216-4c53-45 (non-blocking, raised on coder v2 ACK). Now that `tests/functional/` and `integration_tests/docker-compose.yml` are gone (slice-1 tester commit 3827cb5), this commit cleans up the dead-weight references to those paths that the tester role is gateway-blocked from reaching: - `pyproject.toml`: drop the `functional:` marker registration. The marker was the last live reference to the deleted `tests/functional/` tier; `tests/config/test_ci_config.py` was narrowed by tester to `required = {integration, security}` so the required-markers test still passes (subset check) — but the marker registration itself was orphan after the tier deletion. - `scripts/check-hardcoded-ports.py`: remove two stale `ALLOWLIST_PATHS` entries pointing at files that no longer exist: `integration_tests/docker-compose.yml` and `tests/functional/conftest.py`. The latter matters for task-1-2's acceptance criterion `grep -rn "tests.functional|@pytest.mark.functional"` returns no hits — the regex `tests.functional` matches the literal string `tests/functional/conftest.py` (`.` matches `/`), so the allowlist entry was a real gap, not just polish. `make lint` is clean; `tests/config/` test suite still passes with the trimmed marker set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * rm e2e workflow * Address review: drop more dead code from k3s-only cleanup Follow-ups on PR #2533 review (#2533 (review)): - Delete now-orphan integration_tests/local_pipeline/mock-sandbox/ (Dockerfile + phase-runner.sh) — only consumer was the deleted docker fallback in local_pipeline/conftest.py. - Remove tests.utils.gateway_client.docker_available() and its re-export — zero remaining callers after this PR removed the conftest call sites. - Strip stale -m "not functional" from Makefile (test, test-all) and update docs/guides/testing.md §2 step 8. - Rewrite integration_tests/conftest.py docstring to describe what the legacy fixtures actually do under k3s. Add explicit pytest.skip in isolated_container/external_container/test_container when the stack is k8s-backed (was silently skipping with a generic-sounding "could not start container" message). - Drop unused certs_volume field from EggStack; document why compose_project / external_network are retained. - Expand __all__ in integration_tests/conftest.py to cover the re-exported public surface (EggStack, ContainerInfo, exec_in_container, GATEWAY_PORT) — previously listed GATEWAY_PORT only. - Drop STRUCTURE.md mock-sandbox entry. Skipping the "except FileNotFoundError, subprocess.TimeoutExpired:" nit — ruff format 0.15.12 actively strips parens from except tuples, so the parenthesized form would not survive `make lint-fix`. Part of pipeline issue-2474-v2; the terminal slice carries the program-level narrative. * Address review: remove stale entries from STRUCTURE.md Drop the four file entries from the integration_tests/ tree listing that this PR (slice-1 cleanup) deletes: docker-compose.yml, agent_findings.py, test_agent_security_fuzz.py, test_e2e_workflow.py. Reviewer noted the local_pipeline/ subsection was already updated in a423d31 but the parent listing was missed. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2495: discriminate authorization vs. value errors at /mutate boundary (#2517) * Fix #2495: discriminate authorization vs. value errors at /mutate boundary The `/mutate` route was returning 403 for every `MutationResult.success=False`, which is correct for role-authorization rejections but misleading for value/path errors (bad `field_path`, out-of-range index, out-of-domain enum value). A client receiving 403 for `Invalid value for current_phase: …` would reasonably retry with a different role, which can't help. Adds `error_kind: Literal["authorization", "value"] | None` to `MutationResult` so the route can map cleanly without parsing message strings: 403 for authorization, 400 for value errors. Adds regression tests for all three branches. * Address review: assert error_kind in validator tests + export MutationErrorKind Closes the validator-level test gap flagged in the PR review: - test_apply_invalid_mutation_rejected now asserts error_kind == "authorization" so a regression that drops the discriminator on the role-rejection path fails at the unit-test boundary, not just the route boundary. - test_invalid_enum_value_returns_failed_mutation now asserts error_kind == "value" for the pydantic ValidationError path. - New test_invalid_path_returns_failed_mutation covers the (KeyError, IndexError, AttributeError) path through _set_value and asserts error_kind == "value". Re-exports MutationErrorKind from shared/egg_contracts/__init__.py so callers can type-hint against the discriminator without reaching into the validator submodule. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document .github-staging/ convention in agent-roles reference [doc-updater] (#2516) * docs: document .github-staging/ convention in agent-roles reference * docs: correct tester guidance — HANDOFF instead of .github-staging/ The tester's allowed_patterns in shared/egg_restrictions/patterns.py covers only test files, conftest, pin files, and .egg-state/agent-outputs/ — it does not include .yml/.yaml/.json. AgentFilePattern.can_write requires both a non-blocked path AND a positive allowlist hit, so .github-staging/workflows/ci.yml returns False for the tester even though .github-staging/ is not on the tester's blocked list. A tester following the previous text would attempt to stage CI fixes under .github-staging/ and be rejected. Replace that advice with the correct path: hand off to the coder via HANDOFF, mirroring the existing coder→tester handoff pattern. Surfaced by egg-reviewer on PR #2516. The patterns.py / agent_roles.py divergence the same review noted is tracked separately in #2521. * docs: expand tester Directed Coordination to cover outbound HANDOFF --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py (#2525) * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py #2514 added `.github/` to `TESTER_ROLE.blocked_write` in `agent_roles.py` but skipped the mirror entry in `TESTER_PATTERNS.blocked_patterns` in `patterns.py` — the planner prompt and the gateway saw different views of the tester's write scope. The omission was benign because the tester's allowlist already excludes `.github/`, but it stops being benign the moment someone widens that allowlist. Add `.github/` to `TESTER_PATTERNS.blocked_patterns` so both files agree, matching the lockstep pattern already used for documenter, autofixer, and conflict_resolver. Add regression tests in the gateway pattern suite and the shared restrictions unit suite. * Address review on #2525: load-bearing tests, slim comment - Use `.github/test_actions.py` as the load-bearing assertion in both test files. It matches the tester's `**/test_*.py` allowlist, so only the new `.github/` blocked entry stops it. The two pre-existing paths (`.github/CODEOWNERS`, `.github/PULL_REQUEST_TEMPLATE.md`) stay as breadth assertions; both are blocked even without the new entry, so they would have passed against the unfixed patterns. - Slim the rationale block in `patterns.py:224-234` to a one-liner pointing at `CODER_PATTERNS` for the full `.github/` rationale. The original 11-line comment over-claimed lockstep across roles the diff didn't actually touch (architect, task_planner, refiner, reviewer roles); the one-liner doesn't. Drift in the non-tester roles (architect/task_planner/risk_analyst/ refiner/reviewer/reviewer_contract) is tracked in #2532. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2490: extend validate_assignment to sibling Contract models (#2520) * Fix #2490: extend validate_assignment to sibling Contract models #2484 added `model_config = ConfigDict(validate_assignment=True)` to `Contract` so `setattr` on Contract fields coerces values back to their declared type. The reviewer flagged a remaining asymmetry: sibling models (`Task`, `Slice`, `Decision`, `AgentExecutionModel`, …) still silently accepted untyped assignments like `task.status = "garbage"`, so the validation surface was uneven across the contract object graph. Lift the config to a shared `EggContractBaseModel` (Option B from the issue) and have every model in `shared/egg_contracts/models.py` inherit from it, so the strictness applies uniformly without per-model duplication. Drop the per-model config from `Contract` itself — the shared base now provides it. The audit of sibling-model mutation sites (`shared/egg_contracts/orchestration.py` `set_execution`, `orchestrator/routes/decisions.py` `contract.pr =`, etc.) confirms they assign well-typed values (enum members or constructed model instances), so the new strictness does not break existing call sites. * Address PR #2520 feedback: fix nested-model test, refresh validator comment - Rewrite test_pr_metadata_invalid_deferred_actions_raises to assign a raw dict, which actually exercises pydantic's list-element coercion path on the outer setattr; the previous form raised from the inner DeferredAction(...) constructor regardless of validate_assignment (item 1). - Update the validator.py except ValidationError comment to reference EggContractBaseModel (where the config now lives) and add #2490 to the issue list, since the same catch now covers sibling-model setattrs (Task.status, Slice.status, Decision.type, ...) too (item 2). — Authored by egg --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2501: don't flip stale during an in-flight state-store probe (#2519) * Fix #2501: extend probe freshness while a probe is in flight `StateStoreProbe.snapshot()` flipped the cached `healthy` to `False` purely because the cache age crossed `interval * stale_multiplier`, even when a probe was actively running and about to refresh it. Under slice-spawn load `git worktree add` occasionally ran 30-40s, longer than the 30s default staleness window, so the request-path dual-write in `routes/health.py` recorded `unhealthy` and the BG callback recorded `healthy` 0-3s later when the same probe completed — producing the spurious `recent_transitions` flap pairs reported in the issue. Track probe start time and, while a probe is in flight, treat the cache as fresh until the in-flight probe itself has been running longer than the staleness window. A genuinely wedged probe still surfaces as stale once that bound is exceeded. * Address review feedback on #2501 in-flight grace fix - Document worst-case ~2*stale_window wedge-detection bound and the intentional 'fresh-but-old' semantics during the grace in snapshot()'s docstring (reviewer minor: source-recoverable rationale). - Expand the inline comment in the grace branch to cite the 'fix #1' framing from #2501 so the bound's rationale is recoverable from the source alone (reviewer minor). - Add an integration-level test that drives snapshot() while a real probe_now() is parked mid-probe on a worker thread, populating the in-flight flag and _probe_started_at_monotonic via the production code path. Closes the loop end-to-end so a future refactor that stops setting _probe_started_at_monotonic from probe_now() fails this test where the field-poking variants would silently keep passing (reviewer non-blocking suggestion). * Tighten worst-case wedge detection bound in snapshot() docstring Reviewer noted that the '~2 * stale_window' / '60s with 30s default' framing is loose. The BG loop fires every `interval` seconds, so the in-flight probe starts within `interval` of the last good completion, and the grace extends only until that probe's own age exceeds `stale_window`. The tight bound is therefore `stale_window + interval`, which under the defaults (interval=15s, stale_multiplier=2.0) is ~45s of blindness, not ~60s. The 2 * stale_window framing only saturates when stale_multiplier=1.0. Address-only docstring change; no behavior change. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty (#2518) * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty restart_phase reads its respawn roster from phase_exec.agents — a runtime cache that the route's own clear-then-spawn flow resets to []. If the spawn step fails before re-populating the cache, every subsequent restart_phase 400s on "No agents found in phase {phase} to restart" and start_pipeline 409s on the (now CANCELLED) status, leaving the only escape cancel_task(cleanup=true) — which discards all prior work. Fall back to the same deterministic source the executor itself uses: pipeline.active_roles (CUSTOM-mode / BABYSIT overrides, #1762) first, then get_roles_for_phase(repo, has_contract). Same precedence as _run_concurrent_phase, so the recovered roster matches what the next spawn would have produced anyway. * Match _run_concurrent_phase exactly: skip phase-default fallback when active_roles set When pipeline.active_roles is set but every entry is unknown to this orchestrator's AgentRole (defensive case after a role removal in a newer schema), the prior implementation fell through to get_roles_for_phase and expanded to the full phase-default roster. _run_concurrent_phase keeps its roles list empty in the same case, so the route's response (and the downstream worktree-delete / health-monitor reset) would diverge from what the spawn would actually produce. Convert the second 'if not agent_roles:' into an 'else:' attached to the override branch so the strict-parity behaviour matches: when an override is set we use it verbatim and never fall through. The final 'No agents found' 400 still fires honestly when the override is all-unknown. Adds a regression test that mutates active_roles post-construct (bypassing the field validator) to simulate the load-time-drift edge case. * Document deliberate route-vs-worker divergence in roster-derivation try/except The except Exception wrap around _get_roles_for_phase doesn't exist in _run_concurrent_phase, so a future reader auditing the two callsites for parity might mistake the bare-except for a bug rather than a deliberate route-specific safety floor (return 400 not 500). * Tighten line-range citation in roster-derivation comment to 12813-12840 --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2522: enumerate per-agent worktrees on phase restart (#2526) * Fix #2522: enumerate per-agent worktrees on phase restart restart_phase guessed worktree names as ``{pipeline_id}-{role}``, which misses slice-scoped worktrees (``{pipeline_id}-slice-{N}-{role}``) and leaves them on disk after a restart on a slice-based pipeline. Drive deletion off ``agent_salvage.enumerate_agent_worktrees`` (already the source of truth in ``cleanup_pipeline`` and salvage) and filter to the roles being restarted. The pipeline-level worktree (``agent_role=None``) and worktrees for non-restarted roles are intentionally preserved. * Address review: salvage before restart-phase delete; cleanup-style enumeration Blocking review feedback (#2522 / PR #2526): 1. Silent loss of unpushed agent commits during phase restart restart_phase now calls agent_salvage.auto_salvage_pipeline before the deletion loop (mirroring cleanup_pipeline's #2429 invariant). Restart is precisely the scenario where unpushed commits accumulate - operators hit it because agents got stuck or wedged - so the previous code was the one orchestrator-side worktree-delete path that bypassed salvage. Salvage failures are best-effort; deletion still happens. 2. Broken/corrupted worktrees regressed the original #1723 cleanup enumerate_agent_worktrees gates on a usable .git marker, so wedged-btrfs-mount worktrees were being silently skipped after this PR's switch to enumeration. Added validate_git=False flag (default stays True for salvage callers) so cleanup callers receive broken entries with repo_path falling back to the worktree dir itself. restart_phase now opts in to the cleanup-style listing. Non-blocking feedback addressed in the same commit: - Test fixture _make_pipeline_with_slice_agents builds AgentExecution with slice_id populated, matching what concurrent_executor writes. - New test exercises continue-on-error across three worktrees with the middle one's delete raising; locks down loop semantics. - Narrower exception class (OSError | ImportError | RuntimeError) around enumerate_agent_worktrees. - log_extras suppresses slice_id=None on non-slice pipelines. New tests: - test_restart_phase_continues_after_partial_worktree_deletion_failure - test_restart_phase_salvages_before_deleting_worktrees - test_restart_phase_salvage_failure_is_nonfatal - test_restart_phase_deletes_broken_worktree_without_git_marker - test_validate_git_false_returns_broken_worktrees - test_validate_git_false_preserves_validated_repo_path --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2539: drop duplicate `slice ` prefix in non-terminal slice PR titles (#2540) `create_slice_pr` was rendering non-terminal slice PR titles as `slice slice-1: …` because `slice_id` already starts with `slice-`. Drop the literal prefix so the title is just `{slice_id}: {slice_name}` (e.g. `slice-1: Cleanup — k3s only, drop dead test tiers`), and update the two test assertions and docs reference that pinned the buggy form. * Fix #2531: add `--for STATUS` to producer pre-confirm wait-loop (#2536) When every reviewer ACKed the current version, no further `CONSENSUS_ACK` / `CONSENSUS_NACK` events arrive on the bus. The orchestrator's directed `STATUS` nudge ("Ready to confirm — all confirm preconditions satisfied", `metadata.ready_to_confirm == True`) is the only signal that the global preconditions cleared, but the producer prompt's pre-confirm wait-loop filter omitted `STATUS` — so the producer slept through the nudge and only woke via the health-monitor `OVERSEER_ALERT` backstop minutes later, observed in pipeline `issue-2474-v2` slice-1 (6/8 stall, ~57 min phase elapsed). The reference doc at `agent-wait-patterns.md` already prescribed waiting on `STATUS` for the pending-acks recovery path; the prompt template just hadn't caught up. This change closes that gap and adds a regression test that pins `--for STATUS` plus the on-wake guidance ("go to step 5 CONFIRM if `metadata.ready_to_confirm`, otherwise re-enter the wait") across every producer role × phase. The `_send_brc_confirmation_nudge` docstring is updated to reflect the new pre-confirm filter. * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import (#2542) * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import Two bugs surfaced when issue-2474-v2 spawned slice-2: every container exited four seconds in with no work attempted, leaving slice-2's integration branch empty and the PR-create call to fail with "No commits between ...". Bug A (`gateway/git_client module unavailable`): the deployed orchestrator image ships only `orchestrator/`, `routes/`, `health_checks/`, and the shared `egg_*` packages — `gateway/` is not copied. The `from gateway.git_client import build_rebase_onto_args` call added by #2512 always raises ImportError in production, so every slice integration branch reconciliation silently fails. Inline the canonical argv builder as `_build_rebase_onto_args` in `orchestrator/gateway_client.py`; the gateway server's `/git` endpoint remains the authoritative allowlist boundary, and CI test paths that keep `gateway/` on `sys.path` continue to work unchanged. Bug B (slice-2 consensus reached at elapsed_seconds=0.0): the per-slice tracker registry already keys by `{pipeline_id}/{slice_id}`, but `ConcurrentPhaseExecutor.check_consensus()` had two slice-unaware fallback paths. When slice-2's tracker is fresh and empty (the steady state right after spawn, before any agent has proposed), (1) `reconstruct_tracker_from_messages` was called with the bare pipeline_id and (2) the message-bus fallback scanned `store.get_messages(pipeline_id)` pipeline-wide. Slice-1's eight CONSENSUS_CONFIRMED messages are persisted under the bare pipeline_id and have the same role names as slice-2's roster, so both paths falsely declared consensus on slice-2's first poll iteration. Gate both fallbacks (and the matching path in `handle_consensus_confirmed_signal`) on `slice_id is None`. The in-memory per-slice tracker is the authoritative source; an empty fresh tracker correctly returns is_complete=False and the polling loop keeps going. * Address #2542 review: slice-scope idempotency, fix test syntax, doc tweaks Five issues from egg-reviewer on the #2535 PR: 1. test_check_consensus_slice_isolation.py: replace dead try/except that used Python-2 catch-and-bind syntax (`except A, B:`) with a direct `PipelineConfig(concurrent_execution=True)` constructor call. The original block was unreachable — `concurrent_execution` is a normal Pydantic bool field that cannot raise on assignment — and the misleading syntax would surprise any future reader. 2. routes/signals.py: scope `_existing_confirmed_for_role` to a slice so the idempotency probe doesn't see sibling-slice CONFIRMs as "already confirmed for this role". A new `slice_id` parameter filters by `metadata["slice_id"]`; the per-slice tracker path tags CONSENSUS_CONFIRMED writes with that same metadata key. Without this, slice-2's first coder CONFIRMED would be silently suppressed (no bus message, no #1473 marker) because slice-1's coder CONFIRMED was still in the bus under the bare pipeline_id. Pipeline-scoped (slice_id is None) callers continue to see only pipeline-scoped messages, preserving legacy behaviour exactly. 3. orchestrator/gateway_client.py: soften the "Mirrors" claim in the `_build_rebase_onto_args` docstring. The helper does NOT call validate_git_args (which would defeat the inlining) and emits stripped argv, so document those two intentional differences. 4. orchestrator/stacked_pr_reconciler.py: update the module docstring to point at the inlined `_build_rebase_onto_args` in orchestrator.gateway_client (with a note explaining why the inlining is needed and why the security floor is unchanged). 5. tests/test_consensus_confirmed_idempotent.py: extend the helper `_fake_message` with a `slice_id` parameter and add three regression tests: - slice-2's first CONFIRMED is NOT marked idempotent by a slice-1 CONFIRMED in the bus - within slice-2, the second CONFIRMED IS deduped - pipeline-scoped callers ignore slice-scoped CONFIRMs The wider sweep of slice-unaware peer-consensus lookups in kubernetes_monitor.py, startup_reconciliation.py, routes/pipelines.py status display, and the tier-1 health checks is left for #2409 (the existing tracker covers the same root-cause: slice_id needs to flow through more places). PR body updated to flag this. * Fix #2538: slice PRs carry contract.pr narrative on every slice (#2543) Every slice PR — terminal and non-terminal — now renders the planner-authored program title, description, test plan, and manual steps from contract.pr, so reviewers see program rationale on whichever slice they open first. Previously only the terminal slice carried the narrative; reviewers approaching slice-1 (the bottom of the stack and the canonical merge entry point) saw only task bullets plus a pointer to the terminal slice's PR. Title disambiguation: terminal slice gets the bare program_title; non-terminals get a [<slice-id>] prefix so the GitHub PR list stays scannable when several stacked PRs are open at once. Per-merge obligations remain terminal-only (the merge gate is the last-to-merge PR in the stack) — the existing #2354 invariants and fail-fast assertion are preserved. The terminal slice keeps a "merge gate / umbrella" banner so reviewers can spot the merge gate; non-terminals skip it. The old "see terminal slice's PR for the program-level narrative" pointer is gone — the narrative is right there now. * Fix #2537: attribute slice PRs to orchestrator, not coder (#2541) * Fix #2537: attribute slice PRs to orchestrator, not coder The slice-PR creation path is orchestrator-only — `gh pr create*` is blocked for the implement phase, and the pr phase has no agent spawn. But `_run_implement_phase_slices` was hard-coding `agent_role="coder"` on the synthetic session that opens the slice PR, which caused the gateway to label the PR `agent:coder` and inject `agent_role=coder` into the `` comment. Pass `agent_role="orchestrator"` so slice PRs match the attribution the non-sliced `_auto_create_pr` path already uses. * Fix /status/wait test flake: handshake before publish The three event-bus tests in TestStatusWaitRoute used a 0.1s sleep in the fire thread before publishing — racy on slow CI. The route's preamble (cursor parse, terminal short-circuit, staleness probe, current_sequence() snap) can exceed the grace window, so the publish lands before event_bus.subscribe(None, _on_event) and the event is never delivered. Replace the sleep with a deterministic handshake that polls event_bus._wildcard_handlers and returns the moment the route has subscribed. The message-bus path (test_overseer_alert_wakes_route) uses a different wake mechanism and is left untouched. * docs: add --for STATUS to producer pre-confirm wait-loop example (#2546) Syncs docs/guides/concurrent-execution.md with the fix from #2531: the producer RESPOND TO REVIEWS (step 4) wait-loop now includes --for STATUS so the orchestrator's "Ready to confirm" directed nudge wakes the producer when every reviewer has already ACKed and no further CONSENSUS_ACK/CONSENSUS_NACK will arrive. docs/reference/agent-wait-patterns.md was already updated in the same PR; this doc had a stale copy of the canonical snippet. Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* Initialize SDLC contract for issue #2548 * Refine analysis for issue #2548 Analyzes the missing analysis/plan/BRC visibility on slice PRs. Compares four options (context PR / embed in slice-1 / embed in terminal slice / render in PR body), recommends Option A (dedicated context PR + per-slice implement BRC files), and registers five HITL decisions plus five open feedback questions on contract. * Persist agent statefile writes before refine sync * Persist statefiles after refine phase * Persist HITL resolution after refine phase gate * Risk assessment for issue #2548 plan phase Identifies 14 risks (R1–R14) across compatibility, gateway-policy, schema, security, performance, and operator-experience categories. Captures HITL decisions 1–5 and feedback Q1–Q5 as decision_inputs. Key risks: - R1: Gateway slice-integration regex blocks egg/<id>/context push - R2/R9: Hard-switchover (decision-4) needs operator drain runbook - R5: Public-repo exposure of agent transcripts (Q3 chose include) - R8: New PRMetadata fields must be Optional with safe defaults - R10: Decision-3 covers merge gate but not creation failure semantics Recommends: go-with-conditions, gateway change ships first, schema additions ship with safe defaults, surface 2 new HITL questions (creation-failure semantics, transcript size/scrub). * Plan #2548: context PR + per-slice BRC history Five-slice forest chain (slice-1 → slice-2 → slice-3 → slice-4 → slice-5) following the operator's HITL resolutions: - D1: dedicated context PR - D2: hard-split implement BRC into per-slice files; no aggregate - D3: doc-only auto-open (no merge gate before slicing) - D4: hard switchover, no backfill - D5: context PR base = pipeline.base_branch (not hardcoded main) Slices: contract schema delta -> per-slice BRC writer -> context branch + doc-only PR opener -> slice-1 base wiring + per-slice BRC commit + reconciler fallback -> docs. * Architecture analysis for issue #2548 plan phase Architect output describes the design for landing refine/plan analysis docs, agent transcripts, and refine/plan BRC histories on a dedicated context PR (egg/<id>/context, base=<pipeline.base_branch>) that slice-1 stacks on top of, plus splitting the implement-phase BRC history at write time into per-slice files committed to each slice's integration branch before its PR opens. Reflects HITL decisions 1-5 and feedback Q1-Q5 from the refine phase. Hard switchover for new pipelines only; no backfill. * Persist statefiles after plan phase * Fix #2532: align .github/ block in agent_roles.py for plan and reviewer roles (#2550) * Fix #2532: align .github/ block in agent_roles.py for plan and reviewer roles Adds `.github/` to the `blocked_write` list of every plan-side and reviewer role in `shared/egg_contracts/agent_roles.py` whose `patterns.py` counterpart already blocks it: - ARCHITECT_ROLE, TASK_PLANNER_ROLE, RISK_ANALYST_ROLE - _REVIEWER_BLOCKED_WRITE (covers reviewer_code, reviewer_code_holistic, reviewer_agent_design, reviewer_refine, reviewer_plan, reviewer_security, reviewer_concurrency) - _REVIEWER_CONTRACT_BLOCKED_WRITE The planner prompt reads `agent_roles.py` via `get_file_patterns()`; the gateway reads `patterns.py` via `AgentFilePattern.can_write()`. PR #2525 closed the same drift for the tester (issue #2521); this closes the remaining cases. The disagreement is benign today because every affected role's `allowed_write` is confined to `.egg-state/...` paths that never collide with `.github/`, but bringing the two views into lockstep means the next allowlist widening cannot silently bypass the branch-protection invariant from #2508. Adds `shared/tests/test_github_block_alignment.py` with three parametrized regression tests across the affected roles: agent_roles view blocks `.github/`, patterns view blocks `.github/`, and the two views agree. A "load-bearing" test in #2525's style is not constructible here — the allowlists never intersect `.github/` — so the test instead asserts cross-view consistency. Notes vs. the issue inventory: - REFINER is NOT in scope. The issue lists it, but `REFINER_PATTERNS` in `patterns.py` uses its own custom blocked list (not `_PLAN_AGENT_BLOCKED`), and that list also omits `.github/`. The two views already agree for refiner, so there's no drift to fix — whether refiner *should* block `.github/` is a separate change. - The reviewer count expanded from the issue's 3 (reviewer_code, reviewer_code_holistic, reviewer_contract) to 8: every reviewer role sharing `_REVIEWER_BLOCKED_WRITE` is fixed by editing the shared list once. * Extract _PLAN_AGENT_BLOCKED_WRITE shared constant Mirrors patterns.py's _PLAN_AGENT_BLOCKED structure: ARCHITECT_ROLE, TASK_PLANNER_ROLE, and RISK_ANALYST_ROLE now share a single blocked_write list instead of inlining identical 9-element lists. Eliminates one future drift surface within agent_roles.py itself, as suggested in PR #2550 review. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2549: skip already-merged slices on pipeline restart (#2552) * Fix #2549: skip already-merged slices on pipeline restart When a slice's PR is merged into the work branch, the orchestrator restart loop has no signal that the slice is done — `iter_ready()` yields it on the first tick, `create_slice_integration_branch` tries to push the (now post-merge) parent SHA onto the slice's existing ref, and origin rejects it as non-fast-forward. The slice cascade- fails its descendants in ~5 seconds, blocking the entire stacked-PR workflow until an operator manually deletes the stale slice ref. The fix wires three things: * `GatewayClient.is_slice_branch_merged_into_parent` — a new detection helper. ls-remote both refs, fetch them, and check `merge-base --is-ancestor existing parent`. The inverse direction of the #2512 restart-recovery check. * Bootstrap reconciliation in `_run_implement_phase_slices`. Before the run loop starts, fold in (A) slices already marked `SliceStatus.COMPLETE` on the contract (cheap path; trust the contract) and (B) slices the gateway reports as already-merged (the live #2549 repro path). Both transitions persist `status=COMPLETE` so subsequent restarts hit (A). * Race protection in `_run_one_slice_inner`. Re-runs the merged- detection right before `create_slice_integration_branch` so a slice merged between bootstrap and its wave is also handled. Also closes a latent gap: `Slice.status` had `COMPLETE` as a value since the original schema and the #2470 `restart_agent` parent-slice- complete fallback already read it, but nothing wrote it. The successful-completion path in `_run_one_slice_inner` now persists `SliceStatus.COMPLETE` to the contract under the per-pipeline state lock, finally giving the #2470 reader a real signal. * Address #2552 review notes: defer reconciler start, parallelize bootstrap, prefer parent_branch_at_creation, expand test - Move _start_stacked_pr_reconciler call to after the bootstrap pass so an exception during bootstrap (hard imports, programming errors) cannot leak the daemon thread. - Parallelize layer-(B) is_slice_branch_merged_into_parent calls with a ThreadPoolExecutor (cap 8). Each call uses its own synthetic gateway session, so concurrent calls are safe; this keeps startup latency bounded as forests grow. - Prefer slice.parent_branch_at_creation over deriving from dependencies[0] in the bootstrap parent-branch resolution. Today both should agree, but a future re-plan that mutates dependencies post-creation would otherwise compare against the wrong parent. - Expand test_bootstrap_does_nothing_when_pipeline_repo_unset to actually exercise step (A) under repo=None: add a slice with status=COMPLETE alongside a PENDING slice, then assert that step (A) skips the COMPLETE slice and step (B) is wholesale skipped. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Egg/issue 2474 v2/work (#2556) * Initialize SDLC contract for issue #2474 * slice slice-1: Cleanup — k3s only, drop dead test tiers (#2533) * Slice 1 (coder portion): retire e2e tier scaffolding (#2474) Drops the real-LLM end-to-end test scaffolding the coder role can reach under its file boundaries (`pyproject.toml`, `Makefile`, `integration_tests/agent_findings.py`): - Remove `e2e` and `agent_flaky` pytest markers from `pyproject.toml`. The matching `tests/config/test_ci_config.py` required-markers assertion is in the tester's scope; tester picks it up alongside task-1-2 (delete `tests/functional/`) so the marker set lands consistently. - Drop the `test-e2e` Make target, its `.PHONY` entry, and its `make help` line; retag `test-integration` to k3s in the help block and module banner. `test-security` is retained. - Delete `integration_tests/agent_findings.py` — the JSONL findings recorder for the agent_flaky fuzz tier; orphan once `test_agent_security_fuzz.py` is removed by tester (task-1-3 e2e tests). - Stage `.github-staging/workflows/test-e2e.yml` as a deletion-marker: agent file-boundaries block writes under `.github/`, so the staged file's header explicitly directs the human reviewer to `git rm .github/workflows/test-e2e.yml` (and the marker itself) rather than `git mv` it into place. The PR builder's auto "Move staged `.github/` changes" step (issue #2508) surfaces the marker. Tasks split: - task-1-3 coder portion: pyproject markers, Makefile target, agent_findings.py, .github-staging marker. - task-1-3 tester portion (handed off): delete `integration_tests/test_e2e_workflow.py` and `integration_tests/test_agent_security_fuzz.py`; update `tests/config/test_ci_config.py` required-markers set. - task-1-1 / task-1-4 (handed off): conftest edits live in tester's scope (`**/conftest.py`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Drop .github-staging/ deletion-marker; rely on pre-merge condition (#2474) Address reviewer_code NACK on slice-1 v1: the staging-promote pattern in `_build_github_staging_manual_step()` (orchestrator/routes/pipelines.py:8490) unconditionally renders `git mv .github-staging/<path> .github/<path>` boilerplate for every staged file — there's no opt-out for "this marker expresses a deletion intent." A reviewer who skims past the YAML-comment header inside the staged file and follows the auto-generated `git mv` either fails loudly ("destination exists") or, with `git mv -f`, silently overwrites the live workflow with the retired stub — neither resolves into the intended `git rm`. The documented BRC pattern for "human action that agents cannot push through the gateway" is `--pre-merge-condition` on a reviewer ACK (issue #1998 / `_collect_pre_merge_obligations`), which renders as a "Pre-merge Obligations" section in the PR body with a do-not-merge banner. reviewer_contract attached such an obligation on their v1 ACK, so the merger sees the `git rm` instruction unambiguously without the contradictory staging-promote step. Behaviour change: none against the runtime pipeline. The live `.github/workflows/test-e2e.yml` deletion remains a pre-merge human obligation; only the in-tree marker file is removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Slice-1 tester scope: delete tests/functional/, k3s-only conftests, retire e2e tests Picks up everything in the tester's gateway file scope for slice-1 (issue #2474): task-1-2 (delete tests/functional/): - Remove all 5 files under tests/functional/. Acceptance criterion: `tests/functional/` no longer exists. NOTE: the matching `functional:` marker registration in pyproject.toml AND the `tests/functional/conftest.py` allowlist entry in scripts/check-hardcoded-ports.py are gateway-blocked from the tester role (only coder can push pyproject.toml / scripts/). Both have been HANDOFFed back to the coder for inclusion in their next propose; see the HANDOFF message issued alongside this commit. Leaving the marker registered is harmless (no tests carry the marker any more); leaving the allowlist entry registered is harmless (the file is gone so the lint script never visits it). task-1-1 (k3s-only egg_stack): - integration_tests/conftest.py: drop `_docker_egg_stack()`, the EGG_RUNTIME=docker branch in `egg_stack`, the `docker_available` import, and stale docker-compose comments. `egg_stack` now skips with a clear pointer to docs/guides/testing.md when kubectl is unavailable. - integration_tests/local_pipeline/conftest.py: same treatment for `local_pipeline_stack`. Drops the COMPOSE_FILE / MOCK_SANDBOX_DIR constants, `_cleanup_orphaned_containers`, the docker-compose-up block, and the `docker_available` import. task-1-4 (retire orphan agent-led helpers in integration_tests/conftest.py): - Delete `run_claude_structured()`, `assert_agent_verdict()`, the `AgentVerdict` dataclass (including `infrastructure_failure`), `VERDICT_SCHEMA`, `TEST_AGENT_SYSTEM_PROMPT`, and the orphaned `_allocate_test_container_ip()`, `_capture_container_logs()`, `_preflight_gateway_check()` helpers. Drop the now-unused imports (`json`, `requests`, `ContainerNetworkConfig`, `build_sandbox_docker_cmd`). task-1-3 (delete e2e test files; tester scope): - rm integration_tests/test_e2e_workflow.py - rm integration_tests/test_agent_security_fuzz.py - tests/config/test_ci_config.py: required-markers assertion narrowed from {integration, functional, e2e, security, agent_flaky} to {integration, security}, with a docstring reference to issue #2474. Test infrastructure preserved: - GATEWAY_PORT remains imported and re-exported from integration_tests/conftest.py because test_network_security.py imports it directly via `from integration_tests.conftest import GATEWAY_PORT, exec_in_container`. - isolated_container / external_container / test_container fixtures are retained for the test_credential_security and test_network_isolation tiers (both still in tree). They will skip in k3s mode (the docker network name does not resolve), but slice-3 of this PR train adds k3s-native equivalents that supersede them. Acceptance criteria verified for the tester portion: - `tests/functional/` no longer exists. - `grep -rn "run_claude_structured|assert_agent_verdict"` returns no hits. - `grep -nE "EGG_RUNTIME=docker|_docker_egg_stack|docker_available"` on the two conftest files returns no hits. - `make lint` passes. * Slice-1 cleanup: drop functional marker + stale port allowlist (#2474) Address tester HANDOFF 2cc2c216-4c53-45 (non-blocking, raised on coder v2 ACK). Now that `tests/functional/` and `integration_tests/docker-compose.yml` are gone (slice-1 tester commit 3827cb571), this commit cleans up the dead-weight references to those paths that the tester role is gateway-blocked from reaching: - `pyproject.toml`: drop the `functional:` marker registration. The marker was the last live reference to the deleted `tests/functional/` tier; `tests/config/test_ci_config.py` was narrowed by tester to `required = {integration, security}` so the required-markers test still passes (subset check) — but the marker registration itself was orphan after the tier deletion. - `scripts/check-hardcoded-ports.py`: remove two stale `ALLOWLIST_PATHS` entries pointing at files that no longer exist: `integration_tests/docker-compose.yml` and `tests/functional/conftest.py`. The latter matters for task-1-2's acceptance criterion `grep -rn "tests.functional|@pytest.mark.functional"` returns no hits — the regex `tests.functional` matches the literal string `tests/functional/conftest.py` (`.` matches `/`), so the allowlist entry was a real gap, not just polish. `make lint` is clean; `tests/config/` test suite still passes with the trimmed marker set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * rm e2e workflow * Address review: drop more dead code from k3s-only cleanup Follow-ups on PR #2533 review (https://github.com/jwbron/egg/pull/2533#pullrequestreview-4241318207): - Delete now-orphan integration_tests/local_pipeline/mock-sandbox/ (Dockerfile + phase-runner.sh) — only consumer was the deleted docker fallback in local_pipeline/conftest.py. - Remove tests.utils.gateway_client.docker_available() and its re-export — zero remaining callers after this PR removed the conftest call sites. - Strip stale -m "not functional" from Makefile (test, test-all) and update docs/guides/testing.md §2 step 8. - Rewrite integration_tests/conftest.py docstring to describe what the legacy fixtures actually do under k3s. Add explicit pytest.skip in isolated_container/external_container/test_container when the stack is k8s-backed (was silently skipping with a generic-sounding "could not start container" message). - Drop unused certs_volume field from EggStack; document why compose_project / external_network are retained. - Expand __all__ in integration_tests/conftest.py to cover the re-exported public surface (EggStack, ContainerInfo, exec_in_container, GATEWAY_PORT) — previously listed GATEWAY_PORT only. - Drop STRUCTURE.md mock-sandbox entry. Skipping the "except FileNotFoundError, subprocess.TimeoutExpired:" nit — ruff format 0.15.12 actively strips parens from except tuples, so the parenthesized form would not survive `make lint-fix`. Part of pipeline issue-2474-v2; the terminal slice carries the program-level narrative. * Address review: remove stale entries from STRUCTURE.md Drop the four file entries from the integration_tests/ tree listing that this PR (slice-1 cleanup) deletes: docker-compose.yml, agent_findings.py, test_agent_security_fuzz.py, test_e2e_workflow.py. Reviewer noted the local_pipeline/ subsection was already updated in a423d311 but the parent listing was missed. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2495: discriminate authorization vs. value errors at /mutate boundary (#2517) * Fix #2495: discriminate authorization vs. value errors at /mutate boundary The `/mutate` route was returning 403 for every `MutationResult.success=False`, which is correct for role-authorization rejections but misleading for value/path errors (bad `field_path`, out-of-range index, out-of-domain enum value). A client receiving 403 for `Invalid value for current_phase: …` would reasonably retry with a different role, which can't help. Adds `error_kind: Literal["authorization", "value"] | None` to `MutationResult` so the route can map cleanly without parsing message strings: 403 for authorization, 400 for value errors. Adds regression tests for all three branches. * Address review: assert error_kind in validator tests + export MutationErrorKind Closes the validator-level test gap flagged in the PR review: - test_apply_invalid_mutation_rejected now asserts error_kind == "authorization" so a regression that drops the discriminator on the role-rejection path fails at the unit-test boundary, not just the route boundary. - test_invalid_enum_value_returns_failed_mutation now asserts error_kind == "value" for the pydantic ValidationError path. - New test_invalid_path_returns_failed_mutation covers the (KeyError, IndexError, AttributeError) path through _set_value and asserts error_kind == "value". Re-exports MutationErrorKind from shared/egg_contracts/__init__.py so callers can type-hint against the discriminator without reaching into the validator submodule. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document .github-staging/ convention in agent-roles reference [doc-updater] (#2516) * docs: document .github-staging/ convention in agent-roles reference * docs: correct tester guidance — HANDOFF instead of .github-staging/ The tester's allowed_patterns in shared/egg_restrictions/patterns.py covers only test files, conftest, pin files, and .egg-state/agent-outputs/ — it does not include .yml/.yaml/.json. AgentFilePattern.can_write requires both a non-blocked path AND a positive allowlist hit, so .github-staging/workflows/ci.yml returns False for the tester even though .github-staging/ is not on the tester's blocked list. A tester following the previous text would attempt to stage CI fixes under .github-staging/ and be rejected. Replace that advice with the correct path: hand off to the coder via HANDOFF, mirroring the existing coder→tester handoff pattern. Surfaced by egg-reviewer on PR #2516. The patterns.py / agent_roles.py divergence the same review noted is tracked separately in #2521. * docs: expand tester Directed Coordination to cover outbound HANDOFF --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py (#2525) * Fix #2521: align tester `.github/` block between agent_roles.py and patterns.py #2514 added `.github/` to `TESTER_ROLE.blocked_write` in `agent_roles.py` but skipped the mirror entry in `TESTER_PATTERNS.blocked_patterns` in `patterns.py` — the planner prompt and the gateway saw different views of the tester's write scope. The omission was benign because the tester's allowlist already excludes `.github/`, but it stops being benign the moment someone widens that allowlist. Add `.github/` to `TESTER_PATTERNS.blocked_patterns` so both files agree, matching the lockstep pattern already used for documenter, autofixer, and conflict_resolver. Add regression tests in the gateway pattern suite and the shared restrictions unit suite. * Address review on #2525: load-bearing tests, slim comment - Use `.github/test_actions.py` as the load-bearing assertion in both test files. It matches the tester's `**/test_*.py` allowlist, so only the new `.github/` blocked entry stops it. The two pre-existing paths (`.github/CODEOWNERS`, `.github/PULL_REQUEST_TEMPLATE.md`) stay as breadth assertions; both are blocked even without the new entry, so they would have passed against the unfixed patterns. - Slim the rationale block in `patterns.py:224-234` to a one-liner pointing at `CODER_PATTERNS` for the full `.github/` rationale. The original 11-line comment over-claimed lockstep across roles the diff didn't actually touch (architect, task_planner, refiner, reviewer roles); the one-liner doesn't. Drift in the non-tester roles (architect/task_planner/risk_analyst/ refiner/reviewer/reviewer_contract) is tracked in #2532. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2490: extend validate_assignment to sibling Contract models (#2520) * Fix #2490: extend validate_assignment to sibling Contract models #2484 added `model_config = ConfigDict(validate_assignment=True)` to `Contract` so `setattr` on Contract fields coerces values back to their declared type. The reviewer flagged a remaining asymmetry: sibling models (`Task`, `Slice`, `Decision`, `AgentExecutionModel`, …) still silently accepted untyped assignments like `task.status = "garbage"`, so the validation surface was uneven across the contract object graph. Lift the config to a shared `EggContractBaseModel` (Option B from the issue) and have every model in `shared/egg_contracts/models.py` inherit from it, so the strictness applies uniformly without per-model duplication. Drop the per-model config from `Contract` itself — the shared base now provides it. The audit of sibling-model mutation sites (`shared/egg_contracts/orchestration.py` `set_execution`, `orchestrator/routes/decisions.py` `contract.pr =`, etc.) confirms they assign well-typed values (enum members or constructed model instances), so the new strictness does not break existing call sites. * Address PR #2520 feedback: fix nested-model test, refresh validator comment - Rewrite test_pr_metadata_invalid_deferred_actions_raises to assign a raw dict, which actually exercises pydantic's list-element coercion path on the outer setattr; the previous form raised from the inner DeferredAction(...) constructor regardless of validate_assignment (item 1). - Update the validator.py except ValidationError comment to reference EggContractBaseModel (where the config now lives) and add #2490 to the issue list, since the same catch now covers sibling-model setattrs (Task.status, Slice.status, Decision.type, ...) too (item 2). — Authored by egg --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2501: don't flip stale during an in-flight state-store probe (#2519) * Fix #2501: extend probe freshness while a probe is in flight `StateStoreProbe.snapshot()` flipped the cached `healthy` to `False` purely because the cache age crossed `interval * stale_multiplier`, even when a probe was actively running and about to refresh it. Under slice-spawn load `git worktree add` occasionally ran 30-40s, longer than the 30s default staleness window, so the request-path dual-write in `routes/health.py` recorded `unhealthy` and the BG callback recorded `healthy` 0-3s later when the same probe completed — producing the spurious `recent_transitions` flap pairs reported in the issue. Track probe start time and, while a probe is in flight, treat the cache as fresh until the in-flight probe itself has been running longer than the staleness window. A genuinely wedged probe still surfaces as stale once that bound is exceeded. * Address review feedback on #2501 in-flight grace fix - Document worst-case ~2*stale_window wedge-detection bound and the intentional 'fresh-but-old' semantics during the grace in snapshot()'s docstring (reviewer minor: source-recoverable rationale). - Expand the inline comment in the grace branch to cite the 'fix #1' framing from #2501 so the bound's rationale is recoverable from the source alone (reviewer minor). - Add an integration-level test that drives snapshot() while a real probe_now() is parked mid-probe on a worker thread, populating the in-flight flag and _probe_started_at_monotonic via the production code path. Closes the loop end-to-end so a future refactor that stops setting _probe_started_at_monotonic from probe_now() fails this test where the field-poking variants would silently keep passing (reviewer non-blocking suggestion). * Tighten worst-case wedge detection bound in snapshot() docstring Reviewer noted that the '~2 * stale_window' / '60s with 30s default' framing is loose. The BG loop fires every `interval` seconds, so the in-flight probe starts within `interval` of the last good completion, and the grace extends only until that probe's own age exceeds `stale_window`. The tight bound is therefore `stale_window + interval`, which under the defaults (interval=15s, stale_multiplier=2.0) is ~45s of blindness, not ~60s. The 2 * stale_window framing only saturates when stale_multiplier=1.0. Address-only docstring change; no behavior change. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty (#2518) * Fix #2515: restart_phase falls back to deterministic roster when agents cache empty restart_phase reads its respawn roster from phase_exec.agents — a runtime cache that the route's own clear-then-spawn flow resets to []. If the spawn step fails before re-populating the cache, every subsequent restart_phase 400s on "No agents found in phase {phase} to restart" and start_pipeline 409s on the (now CANCELLED) status, leaving the only escape cancel_task(cleanup=true) — which discards all prior work. Fall back to the same deterministic source the executor itself uses: pipeline.active_roles (CUSTOM-mode / BABYSIT overrides, #1762) first, then get_roles_for_phase(repo, has_contract). Same precedence as _run_concurrent_phase, so the recovered roster matches what the next spawn would have produced anyway. * Match _run_concurrent_phase exactly: skip phase-default fallback when active_roles set When pipeline.active_roles is set but every entry is unknown to this orchestrator's AgentRole (defensive case after a role removal in a newer schema), the prior implementation fell through to get_roles_for_phase and expanded to the full phase-default roster. _run_concurrent_phase keeps its roles list empty in the same case, so the route's response (and the downstream worktree-delete / health-monitor reset) would diverge from what the spawn would actually produce. Convert the second 'if not agent_roles:' into an 'else:' attached to the override branch so the strict-parity behaviour matches: when an override is set we use it verbatim and never fall through. The final 'No agents found' 400 still fires honestly when the override is all-unknown. Adds a regression test that mutates active_roles post-construct (bypassing the field validator) to simulate the load-time-drift edge case. * Document deliberate route-vs-worker divergence in roster-derivation try/except The except Exception wrap around _get_roles_for_phase doesn't exist in _run_concurrent_phase, so a future reader auditing the two callsites for parity might mistake the bare-except for a bug rather than a deliberate route-specific safety floor (return 400 not 500). * Tighten line-range citation in roster-derivation comment to 12813-12840 --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2522: enumerate per-agent worktrees on phase restart (#2526) * Fix #2522: enumerate per-agent worktrees on phase restart restart_phase guessed worktree names as ``{pipeline_id}-{role}``, which misses slice-scoped worktrees (``{pipeline_id}-slice-{N}-{role}``) and leaves them on disk after a restart on a slice-based pipeline. Drive deletion off ``agent_salvage.enumerate_agent_worktrees`` (already the source of truth in ``cleanup_pipeline`` and salvage) and filter to the roles being restarted. The pipeline-level worktree (``agent_role=None``) and worktrees for non-restarted roles are intentionally preserved. * Address review: salvage before restart-phase delete; cleanup-style enumeration Blocking review feedback (#2522 / PR #2526): 1. Silent loss of unpushed agent commits during phase restart restart_phase now calls agent_salvage.auto_salvage_pipeline before the deletion loop (mirroring cleanup_pipeline's #2429 invariant). Restart is precisely the scenario where unpushed commits accumulate - operators hit it because agents got stuck or wedged - so the previous code was the one orchestrator-side worktree-delete path that bypassed salvage. Salvage failures are best-effort; deletion still happens. 2. Broken/corrupted worktrees regressed the original #1723 cleanup enumerate_agent_worktrees gates on a usable .git marker, so wedged-btrfs-mount worktrees were being silently skipped after this PR's switch to enumeration. Added validate_git=False flag (default stays True for salvage callers) so cleanup callers receive broken entries with repo_path falling back to the worktree dir itself. restart_phase now opts in to the cleanup-style listing. Non-blocking feedback addressed in the same commit: - Test fixture _make_pipeline_with_slice_agents builds AgentExecution with slice_id populated, matching what concurrent_executor writes. - New test exercises continue-on-error across three worktrees with the middle one's delete raising; locks down loop semantics. - Narrower exception class (OSError | ImportError | RuntimeError) around enumerate_agent_worktrees. - log_extras suppresses slice_id=None on non-slice pipelines. New tests: - test_restart_phase_continues_after_partial_worktree_deletion_failure - test_restart_phase_salvages_before_deleting_worktrees - test_restart_phase_salvage_failure_is_nonfatal - test_restart_phase_deletes_broken_worktree_without_git_marker - test_validate_git_false_returns_broken_worktrees - test_validate_git_false_preserves_validated_repo_path --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2539: drop duplicate `slice ` prefix in non-terminal slice PR titles (#2540) `create_slice_pr` was rendering non-terminal slice PR titles as `slice slice-1: …` because `slice_id` already starts with `slice-`. Drop the literal prefix so the title is just `{slice_id}: {slice_name}` (e.g. `slice-1: Cleanup — k3s only, drop dead test tiers`), and update the two test assertions and docs reference that pinned the buggy form. * Fix #2531: add `--for STATUS` to producer pre-confirm wait-loop (#2536) When every reviewer ACKed the current version, no further `CONSENSUS_ACK` / `CONSENSUS_NACK` events arrive on the bus. The orchestrator's directed `STATUS` nudge ("Ready to confirm — all confirm preconditions satisfied", `metadata.ready_to_confirm == True`) is the only signal that the global preconditions cleared, but the producer prompt's pre-confirm wait-loop filter omitted `STATUS` — so the producer slept through the nudge and only woke via the health-monitor `OVERSEER_ALERT` backstop minutes later, observed in pipeline `issue-2474-v2` slice-1 (6/8 stall, ~57 min phase elapsed). The reference doc at `agent-wait-patterns.md` already prescribed waiting on `STATUS` for the pending-acks recovery path; the prompt template just hadn't caught up. This change closes that gap and adds a regression test that pins `--for STATUS` plus the on-wake guidance ("go to step 5 CONFIRM if `metadata.ready_to_confirm`, otherwise re-enter the wait") across every producer role × phase. The `_send_brc_confirmation_nudge` docstring is updated to reflect the new pre-confirm filter. * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import (#2542) * Fix #2535: stop slice-N from inheriting slice-(N-1) consensus, drop gateway import Two bugs surfaced when issue-2474-v2 spawned slice-2: every container exited four seconds in with no work attempted, leaving slice-2's integration branch empty and the PR-create call to fail with "No commits between ...". Bug A (`gateway/git_client module unavailable`): the deployed orchestrator image ships only `orchestrator/`, `routes/`, `health_checks/`, and the shared `egg_*` packages — `gateway/` is not copied. The `from gateway.git_client import build_rebase_onto_args` call added by #2512 always raises ImportError in production, so every slice integration branch reconciliation silently fails. Inline the canonical argv builder as `_build_rebase_onto_args` in `orchestrator/gateway_client.py`; the gateway server's `/git` endpoint remains the authoritative allowlist boundary, and CI test paths that keep `gateway/` on `sys.path` continue to work unchanged. Bug B (slice-2 consensus reached at elapsed_seconds=0.0): the per-slice tracker registry already keys by `{pipeline_id}/{slice_id}`, but `ConcurrentPhaseExecutor.check_consensus()` had two slice-unaware fallback paths. When slice-2's tracker is fresh and empty (the steady state right after spawn, before any agent has proposed), (1) `reconstruct_tracker_from_messages` was called with the bare pipeline_id and (2) the message-bus fallback scanned `store.get_messages(pipeline_id)` pipeline-wide. Slice-1's eight CONSENSUS_CONFIRMED messages are persisted under the bare pipeline_id and have the same role names as slice-2's roster, so both paths falsely declared consensus on slice-2's first poll iteration. Gate both fallbacks (and the matching path in `handle_consensus_confirmed_signal`) on `slice_id is None`. The in-memory per-slice tracker is the authoritative source; an empty fresh tracker correctly returns is_complete=False and the polling loop keeps going. * Address #2542 review: slice-scope idempotency, fix test syntax, doc tweaks Five issues from egg-reviewer on the #2535 PR: 1. test_check_consensus_slice_isolation.py: replace dead try/except that used Python-2 catch-and-bind syntax (`except A, B:`) with a direct `PipelineConfig(concurrent_execution=True)` constructor call. The original block was unreachable — `concurrent_execution` is a normal Pydantic bool field that cannot raise on assignment — and the misleading syntax would surprise any future reader. 2. routes/signals.py: scope `_existing_confirmed_for_role` to a slice so the idempotency probe doesn't see sibling-slice CONFIRMs as "already confirmed for this role". A new `slice_id` parameter filters by `metadata["slice_id"]`; the per-slice tracker path tags CONSENSUS_CONFIRMED writes with that same metadata key. Without this, slice-2's first coder CONFIRMED would be silently suppressed (no bus message, no #1473 marker) because slice-1's coder CONFIRMED was still in the bus under the bare pipeline_id. Pipeline-scoped (slice_id is None) callers continue to see only pipeline-scoped messages, preserving legacy behaviour exactly. 3. orchestrator/gateway_client.py: soften the "Mirrors" claim in the `_build_rebase_onto_args` docstring. The helper does NOT call validate_git_args (which would defeat the inlining) and emits stripped argv, so document those two intentional differences. 4. orchestrator/stacked_pr_reconciler.py: update the module docstring to point at the inlined `_build_rebase_onto_args` in orchestrator.gateway_client (with a note explaining why the inlining is needed and why the security floor is unchanged). 5. tests/test_consensus_confirmed_idempotent.py: extend the helper `_fake_message` with a `slice_id` parameter and add three regression tests: - slice-2's first CONFIRMED is NOT marked idempotent by a slice-1 CONFIRMED in the bus - within slice-2, the second CONFIRMED IS deduped - pipeline-scoped callers ignore slice-scoped CONFIRMs The wider sweep of slice-unaware peer-consensus lookups in kubernetes_monitor.py, startup_reconciliation.py, routes/pipelines.py status display, and the tier-1 health checks is left for #2409 (the existing tracker covers the same root-cause: slice_id needs to flow through more places). PR body updated to flag this. * Fix #2538: slice PRs carry contract.pr narrative on every slice (#2543) Every slice PR — terminal and non-terminal — now renders the planner-authored program title, description, test plan, and manual steps from contract.pr, so reviewers see program rationale on whichever slice they open first. Previously only the terminal slice carried the narrative; reviewers approaching slice-1 (the bottom of the stack and the canonical merge entry point) saw only task bullets plus a pointer to the terminal slice's PR. Title disambiguation: terminal slice gets the bare program_title; non-terminals get a [<slice-id>] prefix so the GitHub PR list stays scannable when several stacked PRs are open at once. Per-merge obligations remain terminal-only (the merge gate is the last-to-merge PR in the stack) — the existing #2354 invariants and fail-fast assertion are preserved. The terminal slice keeps a "merge gate / umbrella" banner so reviewers can spot the merge gate; non-terminals skip it. The old "see terminal slice's PR for the program-level narrative" pointer is gone — the narrative is right there now. * Fix #2537: attribute slice PRs to orchestrator, not coder (#2541) * Fix #2537: attribute slice PRs to orchestrator, not coder The slice-PR creation path is orchestrator-only — `gh pr create*` is blocked for the implement phase, and the pr phase has no agent spawn. But `_run_implement_phase_slices` was hard-coding `agent_role="coder"` on the synthetic session that opens the slice PR, which caused the gateway to label the PR `agent:coder` and inject `agent_role=coder` into the `` comment. Pass `agent_role="orchestrator"` so slice PRs match the attribution the non-sliced `_auto_create_pr` path already uses. * Fix /status/wait test flake: handshake before publish The three event-bus tests in TestStatusWaitRoute used a 0.1s sleep in the fire thread before publishing — racy on slow CI. The route's preamble (cursor parse, terminal short-circuit, staleness probe, current_sequence() snap) can exceed the grace window, so the publish lands before event_bus.subscribe(None, _on_event) and the event is never delivered. Replace the sleep with a deterministic handshake that polls event_bus._wildcard_handlers and returns the moment the route has subscribed. The message-bus path (test_overseer_alert_wakes_route) uses a different wake mechanism and is left untouched. * docs: add --for STATUS to producer pre-confirm wait-loop example (#2546) Syncs docs/guides/concurrent-execution.md with the fix from #2531: the producer RESPOND TO REVIEWS (step 4) wait-loop now includes --for STATUS so the orchestrator's "Ready to confirm" directed nudge wakes the producer when every reviewer has already ACKed and no further CONSENSUS_ACK/CONSENSUS_NACK will arrive. docs/reference/agent-wait-patterns.md was already updated in the same PR; this doc had a stale copy of the canonical snippet. Authored-by: egg Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> --------- Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * Fix #2527: validate task role↔file alignment at plan time (#2551) * Fix #2527: validate task role↔file alignment at plan time Adds `validate_task_role_alignment` in `shared/egg_contracts/plan_parser.py` that mirrors the gateway's push-time blocked-pattern check for each task's `role` against its `files_affected`. The plan reviewer's prompt now runs the validator on the parsed plan draft and injects a "Structural Role-Alignment Check" section listing every offending task with the eligible-role hint (or the `.github-staging/` remediation when no producer role can push the file set). The plan-review criteria gain a deterministic blocking item that points at this section, so a mis-assignment surfaces as a NACK before any producer cycle is wasted. Per-task logic lives in `_check_role_files` so the #2530 follow-up (`includes_tests: true` opt-in for coders coupling tests with their own production code) has a clear hook point. Section is omitted when the validator reports no violations; the prompt is unchanged for clean plans. Push-time enforcement remains in place as defense in depth. * Move #2527 validator to orchestrator-side propose-time enforcement PR-1 review flagged a cross-module silent no-op: in concurrent BRC mode (the default for plan phase) the original prompt-time helper ``_build_role_alignment_check_section`` always returned ``""`` because ``_run_concurrent_phase`` builds every reviewer prompt up-front before the planner has produced the plan. The criteria text then told the reviewer that "absence of that section means the automated check found no violations" — the opposite of the truth. Replace it with deterministic enforcement at the right seam: ``_validate_planner_role_alignment`` runs in ``handle_consensus_propose_signal`` for ``agent_role=="task_planner"``, mirroring the existing ``_validate_tester_check_coverage`` pattern. It reads the plan content as committed at the proposed SHA via ``git show <commit>:<plan_path>`` (so a stale local checkout can't mask a real misassignment) and raises ``ValueError`` on violations, which the caller turns into HTTP 400 — the proposal is rejected BEFORE the tracker is mutated and BEFORE any reviewer sees it. Also addresses the non-blocking comments: * Lazy ``posixpath`` / ``match_pattern`` / ``AGENT_PATTERNS`` imports in ``_is_file_blocked_for_role`` are moved to module scope (no circular-import risk; per-call overhead removed). * Tests now exercise the production sequence end-to-end: ``test_rejected_proposal_does_not_mutate_tracker`` builds the exact propose signal a planner emits in concurrent BRC mode, mocks ``git show`` to return a misassigned plan, and asserts the tracker is left untouched. The PR-1 prompt-emission tests are removed (the helper they pinned is gone) and replaced with criteria-text regression guards that lock out the "absence = no violations" wording. * Address PR review feedback (round 2) Blocking: - Revert egg_restrictions.patterns import in plan_parser.py to lazy function-local. The module-scope hoist in PR-1 round 1 re-introduced the egg_restrictions ↔ egg_contracts import cycle that shared/egg_restrictions/matchers.py was deliberately split out to avoid (see its docstring), breaking the gateway production boot path (python3 gateway/gateway.py). Conftest pre-load order hid the cycle in pytest. egg_restrictions.matchers.match_pattern stays at module scope — only AGENT_PATTERNS needs to be lazy. - Add TestImportOrderingRegression that subprocess-runs 'import egg_restrictions.patterns' under PYTHONPATH=shared so the cycle surfaces in a clean interpreter (mirrors gateway boot). Non-blocking: - Reword the role-alignment criterion in _get_plan_review_criteria to say 'before the proposal reaches you' instead of 'before this prompt is ever rendered' — concurrent BRC mode builds reviewer prompts up-front, so prompt-render time isn't the right reference point. - Update stale comment in test_pipeline_prompts.py that pointed at test_signals.py::test_propose_validates_planner_role_alignment (no such file/test) — the validator-runs-here tests live in this same file under TestPlannerRoleAlignmentValidation. - Thread already-loaded pipeline_state and worktree_path from handle_consensus_propose_signal into _validate_planner_role_alignment via keyword args (with backward-compat fallback to in-function loads) so the validator's dependency on the prior _verify_commit_on_branch block is explicit and the state-store + worktree lookups aren't duplicated. * Fix stale gateway boot path comment in import-ordering regression test The PYTHONPATH=shared mirror comment cited scripts/start-gateway.sh which doesn't exist. Replace with the actual production references: gateway/Dockerfile:99 (PYTHONPATH=/app), gateway/entrypoint.sh:286 (exec python3 gateway.py), gateway/Dockerfile:70-75 (shared/ COPY). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * docs: document plan-time role↔file alignment validation (#2558) Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg <egg@localhost> * Fix #2554: render per-role agent-status table on BRC dashboard emits (#2557) * Render per-role agent-status table on BRC dashboard emits The Phase 3 monitor previously rendered a 3-line `Pipeline Status` block plus a stacked 4-line `Enhanced dashboard` consensus paragraph on each `wait-status` emit. With 8+ agents active during a busy implement-phase BRC, the prose form blurs — a stalled reviewer is not visually distinct from a working one, and a NACK row drops to the end of the paragraph. Replace the two stacked blocks with one per-role markdown table when `concurrent.consensus` is present. Columns derive directly from the `peer_consensus.evaluate()` envelope (`agents[role].producer_phase` / `reviewer_phase` / `confirmed`, plus structured `unresolved_nacks`), so schema drift surfaces as an empty cell rather than a wrong cell. Dual-role agents (`tester`) render `<producer_phase> / <reviewer_phase>`. Always render full state on BRC emits — the table is an at-a-glance scan, so deltas-only would defeat its purpose. Non-BRC lines keep the existing compact 3-line form with deltas-only behavior. Mirror the same two-path render in Phase S5 (lightweight pipeline) and emit the table one final time in Phase 5 success summary so the operator has a closing snapshot of which roles confirmed. Fix #2554 * Address review: drop nonexistent Slice column; fix S5/S6 mirrors; generalize producer ordering - Drop Slice column entirely. last_status.pipeline.current_slice_id does not exist on the Pipeline model — slice_id lives on AgentExecution (per-agent), not on the pipeline root, and the minimal envelope does not carry it. Rendering it would have produced 'Slice: —' on every emit and silently misled operators about sliced vs non-sliced state. - Phase S5 (lightweight): replace stale 'concise/deltas-only' line with the same dual-path phrasing as Phase 3 line 411, and drop Slice from the Path B header reference. - Phase S6 (lightweight Complete): add the closing-snapshot bullet so BRC consensus is rendered one final time on success — lightweight pipelines start at implement, so this is the common case. - Generalize producer ordering: pull producers/reviewers from concurrent.consensus.review_graph (sorted alphabetically by ReviewGraph.to_dict) instead of the implement-only hardcoded list, so refine/plan producers (refiner, architect, task_planner, risk_analyst) order correctly without further prose drift. - Consensus fallback: specify the Phase column renders '—' when concurrent.consensus is missing, and explicitly forbid inventing a message-type-to-phase mapping (a CONSENSUS_PROPOSE tells you the producer is in PROPOSED but says nothing about reviewer phases). * Address re-review: dedup dual-role agents, fix example ordering The producers/reviewers split sourced from review_graph emits tester in both lists for the implement graph, so an LLM following the rule literally would render tester twice. Add an explicit dedup directive to the Role column rule. Reorder the example table to match the alphabetical ordering rule (review_graph.producers is sorted). --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: egg <egg@localhost> * Fix #2529: runtime escape hatch for impossible tasks (#2553) * Fix #2529: runtime escape hatch for impossible tasks Adds two MCP tools and a typed `Impasse` primitive so a producer that discovers mid-execution that its task is structurally impossible can emit a structured signal instead of inventing workarounds. The orchestrator detects the impasse post-phase and either auto-delegates to a suggested role (first attempt, `wrong_role` only) or escalates to HITL (second attempt or non-`wrong_role`). - `mcp__sdlc__check_file_restriction` — pure-local read against `shared/egg_restrictions/patterns.py`. Returns `can_write` plus `alternative_role` when exactly one producer covers the path. Lets the agent self-check before burning tokens on exploration. - `mcp__sdlc__report_impasse` — persists a typed `egg_contracts.Impasse` (category, reason, suggested_role, blocked_files, evidence) under `AgentOutput.impasse`. Once called, the agent must exit cleanly without committing. - `orchestrator/impasse_routing.py` — `collect_impasses` + `route_impasses`. Auto-delegate fires only for fresh tasks (`delegation_attempts == 0`) with a single eligible alternative producer role; everything else creates a HITL decision via `apply_mutation` with a delegate / cancel / manual-resolve / other option set. - `_run_concurrent_phase_with_impasse_retry` wraps the existing slice-loop spawn so an `all_delegated` outcome triggers one BRC retry against the mutated contract; any escalation surfaces to the operator without auto-retry. - Producer prompt picks up an "If a task is impossible, use these tools instead of inventing workarounds" section. Test surface: schema round-trip, both handlers (allowed/blocked/ batch/error paths), routing helper (delegate, second-impasse HITL, plan_bug / external_blocker / unresolved-task escalations, self-delegation defense), and the existing tool-registry + CLI-drift gates updated for the two new no-CLI verbs (rationale in handler docstrings per decision-13). * Address PR #2553 review: producer escape hatch + routing hardening Blocking fix: - Move runtime escape-hatch instructions out of the task_planner-only _build_role_restrictions_section into a new _build_impasse_escape_hatch_section, then inject it into the coder prompt (early-return branch) and the tester / documenter prompts (post-phase-restrictions branch). Producers — the only roles that emit impasses — now actually see check_file_restriction / report_impasse guidance instead of inventing workarounds. The planner keeps a brief post-failure-delegation summary so it knows the auto-delegation path exists; planners do not emit impasses. - Add end-to-end TestProducerEscapeHatchInPrompts coverage that parametrises over coder/tester/documenter and asserts both tool names plus the "DO NOT invent workarounds" header appear, and that architect / planner stay free of the actionable producer-only directive. Non-blocking fixes: - Routing: route_impasses gains a force_escalate kw. The slice-loop wrapper sets it on its terminal iteration so a delegation that cannot re-run a BRC cycle gets escalated to HITL rather than silently mutating the contract and exiting on a stale role assignment. - Routing: drop the 120-char truncation of impasse.reason in _record_delegate's audit-log entry. The schema caps reason at 2000 chars and the audit log can hold the full payload — preserve it for post-mortem debugging. - Slice loop: clear the impasse field from each producer's per- pipeline agent-output file between iterations. save_agent_output's mode="w" already overwrites when a producer respawns and reaches its handoff write, but a producer that crashes pre-handoff in iter-N+1 would otherwise let iter-N's impasse persist and re-trigger routing as a spurious "second impasse on same task" HITL. - Handler: reject category="wrong_role" without suggested_role at the mcp__sdlc__report_impasse boundary. Without it the orchestrator router can only escalate, which silently degrades the producer's deliberately set wrong_role signal — point the agent back at check_file_restriction so the fix lands in the same iteration. - Handler: also require task_id for category="wrong_role". The router's role-match fallback is fragile when a slice has multiple tasks per role or role-less tasks; explicit task_id eliminates guesswork on the auto-delegation path. Other categories keep task_id optional. - Pipelines: comment the monolithic-implement fallback (the second _run_concurrent_phase call in the implement handler) explaining that auto-delegation is intentionally scoped to the slice loop since it rewires a task within a slice. Closes review feedback items 1-7 on PR #2553. * Address PR #2553 re-review: docs drift + cleanup test Address two of the three non-blocking suggestions from the approve re-review on commit 696d392. * docs/reference/agent-tools.md: mcp__sdlc__report_impasse row now documents that task_id and suggested_role are mandatory for category=wrong_role (handler raises HandlerError when either is missing). Other categories keep both fields optional since they always escalate to HITL. * orchestrator/routes/pipelines.py: extract the per-pipeline agent-output cleanup closure to a module-level helper named _clear_stale_impasses_for_producers so it can be unit tested directly. Behaviour is identical — the helper drops the impasse field after every successful all-DELEGATE iteration. * orchestrator/tests/test_pipeline_impasse_cleanup.py: new file with five focused tests covering the cleanup happy path, the no-impasse no-op path, the missing-output-file path, multi-producer cleanup in one pass, and per-pipeline scoping. The third suggestion (mixed-decision iter-0 still wastes a DELEGATE role flip) was explicitly flagged "Worth a follow-up issue, not blocking here" by the reviewer — filed as #2563. * Address PR #2553 minor observations: type annotation + hoist imports Two non-blocking observations from the third review (commit 7da68cf): - Annotate `producer_roles` on `_clear_stale_impasses_for_producers` as `list[ContractAgentRole]` so a future caller sees the expected element type without grepping the call site. Quoted under TYPE_CHECKING + `# noqa: UP037` to match the file's existing pattern for `ContainerSpawner` (lines 413, 684, 6316, 6707, 7021). - Hoist `load_agent_output` / `save_agent_output` imports from inside the helper to module level, mirroring `impasse_routing.py:50` which already imports `load_agent_output` directly. The seam fallback is unused at runtime (egg_contracts is the actual installed package, not shared.egg_contracts) and impasse_routing.py confirms a plain module-level import works. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> * [slice-1] Add context PR + per-slice BRC history (closes #2548) (#2555) * Add PRMetadata.context_* fields + planner prompt updates (#2548) slice-1 / task-1-1 + task-1-3 — the foundation slice for the context-PR mechanism. Subsequent slices build the gateway primitive, the orchestrator hook, and the slice-1 base rewiring on top of these fields. Schema 1.1 — extends ``PRMetadata`` with four optional fields: - ``context_title`` / ``context_description`` — planner-emitted framing for the dedicated context PR (e.g. "Strategic plan for #N" vs the slice's "Implement …"). Both fall back to ``title`` / ``description`` when omitted. - ``context_branch`` / ``context_pr_number`` — orchestrator-populated runtime values (the ``egg/<id>/context`` branch name and the GitHub PR number once the context PR has been opened). Planners must NOT emit these. Bumps ``Contract.schemaVersion`` default from ``"1.0"`` to ``"1.1"`` and adds an ``after``-mode migration shim that promotes pre-1.1 contracts to 1.1 on load. The bump is purely additive — pre-1.1 JSON loads cleanly with the new fields defaulting to ``None``. Plan-parser plumbing — ``ParseResult`` grows ``pr_context_title`` / ``pr_context_description`` and a new ``extract_pr_context_metadata_from_yaml`` helper extracts the optional keys without breaking the existing ``extract_pr_metadata_from_yaml`` 5-tuple signature (and the ~10 callers + tests that unpack it). Planner prompt — both planner-prompt sites in ``pipelines.py`` (the plan-phase prompt under ``_build_phase_prompt`` and the task_planner-role prompt under ``_build_agent_prompt``) gain the ``_PR_CONTEXT_GUIDANCE`` paragraph and the ``_PR_CONTEXT_YAML_EXAMPLE_LINES`` commented-out hints inside the ``pr:`` YAML block. Both helpers are defined once next to ``_PR_DESCRIPTION_GUIDANCE`` so the two prompt sites stay in sync when the guidance evolves. Contract populator — ``_populate_contract_from_plan`` now copies ``result.pr_context_title`` / ``pr_context_description`` onto the new PRMetadata it builds, and preserves any orchestrator-populated ``context_branch`` / ``context_pr_number`` across re-populates so a later plan re-parse does not blow away runtime state set by slice-3's hook. Test impact: bumping the default ``schemaVersion`` to ``"1.1"`` causes ``tests/shared/egg_contracts/test_models.py::test_minimal_contract`` to fail on the literal ``"1.0"`` assertion. The fix-up belongs to the tester role (task-1-2) along with the new ``PRMetadata.context_*`` round-trip coverage; coder boundaries forbid pushing test edits. Lint (ruff format + check) and mypy delta are clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Add PRMetadata.context_* test coverage + 1.0→1.1 migration tests (#2548) slice-1 / task-1-2 — adversarial + regression coverage for the four new optional ``PRMetadata.context_*`` fields and the ``schemaVersion`` 1.0→1.1 promotion shim added by the coder in commit 75d8ca09c. Coverage: * ``TestPRMetadataContextFields`` — defaults to None, full round-trip with all four fields populated, omitted-keys round-trip preserves None. * ``TestPRMetadataContextPRNumberValidator`` — pins the ``ge=1`` validator: 0/-1 are rejected at construct AND at setattr (under the shared ``EggContractBaseModel.validate_assignment=True`` from #2490); None and large positive ints accepted. * ``TestPRMetadataSchemaVersionMigration`` — 1.0 payload loads with context defaults, dump→reload chain stays at 1.1, default is 1.1, legacy ``deferred_actions`` survive migration, and an unrecognized version (1.2 / 2.0) is NOT silently downgraded. * ``TestPRMetadataContextEmptyStringSemantics`` — empty strings are accepted at the model layer so the orchestrator hook's ``context_title or title`` fallback works for both None and "". * ``TestPlanParserContextFieldExtraction`` — covers task-1-3's ingestion path: ``extract_pr_context_metadata_from_yaml`` returns None pair for missing/None/absent inputs; collapses whitespace to None; warns on non-string ``context_title``; ``parse_plan`` threads the values onto ``ParseResult.pr_context_*``. Also updates ``test_models.py::test_minimal_contract`` from the literal ``"1.0"`` schemaVersion assertion to ``"1.1"`` — the coder flagged this as a known follow-up in commit 75d8ca09c (coder cannot push test edits under the role boundary). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address review feedback on PR #2555 (#2548) Blocking fix: - _populate_contract_from_plan now also preserves PRMetadata.deferred_actions alongside context_branch / context_pr_number. The conditional-ACK gate at decisions.py:complete_phase writes deferred actions; the populator's start_phase=implement re-entry path was silently wiping them, erasing the merge-blocking Pre-merge Obligations handoff. Add a regression test in orchestrator/tests/test_short_flow_contract_population.py. Non-blocking improvements: - extract_pr_context_metadata_from_yaml now warns symmetrically on non-string context_description (mirrors the context_title branch), preventing silent str() coercion of structured planner values. - Updated schemaVersion / _migrate_schema_version_to_1_1 docstrings to reflect that the bump fires at every load (mode="after"), not lazily on next save, and to acknowledge the migration is silent (no audit entry). - Aligned TestPRMetadataContextEmptyStringSemantics docstring with reality (planner path collapses empty strings to None; only hand-edited or migrated payloads can produce a "" PRMetadata). - New tests for the symmetric context_description warning. * docs: document schema 1.1 and pr.context_* fields (#2548) Slice-1 lands the schema delta + planner-prompt update half of the context-PR mechanism (#2548): `PRMetadata` grows four optional `context_*` fields and `Contract.schemaVersion` defaults to `"1.1"` with an additive `1.0 → 1.1` migration. The actual context-PR mechanism (branch creation, PR opening, slice-1 base wiring) is implemented in slices 3-4 and gets its own end-to-end documentation pass in slice-5. This commit updates the docs that reference contract examples and the yaml-tasks `pr:` block so they reflect the slice-1-landed schema state: - `docs/templates/plan.md`: add optional `context_title` / `context_description` keys to the yaml-tasks `pr:` example as commented-out hints, plus a new prose blockquote explaining when planners may emit them and which sibling fields (`context_branch`, `context_pr_number`) are orchestrator-populated. - `docs/architecture/sdlc-pipeline.md`: bump the example `schemaVersion` from `1.0` to `1.1` and add a "Schema 1.1 (#2548)" blockquote summarising the additive migration. - `docs/guides/sdlc-pipeline.md`: same `schemaVersion` bump in the example JSON plus a short blockquote pointing readers at the migration semantics. The PR-stack diagrams, BRC-history file naming, and slice-1-base discussion in `docs/guides/concurrent-execution.md`, `docs/architecture/orchestrator.md`, `docs/reference/orchestrator-cli.md`, and `docs/guides/babysit-pr.md` remain untouched — those describe behavior that does not yet exist on this branch and are slice-5's responsibility once the mechanism is wired end-to-end. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Allowlist plan_parser.py for file-size hard cap on egg/issue-2548/work (#2548) Slice-1 (foundation) tester NACK: on the egg/issue-2548/work merge target, slice-1's extract_pr_context_metadata_from_yaml + ParseResult.pr_context_* plumbing stacks on top of #2527's validate_task_role_alignment additions, pushing shared/egg_contracts/plan_parser.py to ~1,530 lines and breaching the 1,500-line hard cap that scripts/check-file-sizes.py enforces. The slice-1 branch alone is at 1,388 lines (clean), but the work-branch state that the lint actually runs against is over. Fix per reviewer_contract's forward-looking concern and tester's blocking finding: add the file to scripts/file-size-allowlist.yaml under #2548 so make lint passes during the slice-1 BRC. Decomposition is tracked under the same issue and is the cheaper of the two unblock option…

jwbron added 2 commits May 6, 2026 18:31

egg-reviewer Bot suggested changes May 7, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

egg-reviewer Bot approved these changes May 7, 2026

View reviewed changes

This comment has been minimized.

Sign in to view

jwbron merged commit 886d6da into main May 7, 2026
15 checks passed

This comment has been minimized.

Sign in to view

james-in-a-box Bot mentioned this pull request May 7, 2026

docs: document .github-staging/ convention in agent-roles reference [doc-updater] #2516

Merged

egg-reviewer Bot approved these changes May 7, 2026

View reviewed changes

james-in-a-box Bot mentioned this pull request May 7, 2026

Align tester role .github/ block between agent_roles.py and patterns.py #2521

Closed

jwbron mentioned this pull request May 7, 2026

Fix #2521: align tester .github/ block between agent_roles.py and patterns.py #2525

Merged

2 tasks

Navigation Menu

Fix #2508: stage agent-proposed `.github/` changes for human review - #2514

Fix #2508: stage agent-proposed `.github/` changes for human review#2514
jwbron merged 4 commits into
mainfrom
egg/issue-2508

jwbron commented May 7, 2026

Uh oh!

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

Uh oh!

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jwbron commented May 7, 2026

Summary

Why this approach

Files changed

Test plan

Out of scope (separate concerns)

Uh oh!

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Blocking issues

1. The agent prompt is internally contradictory for the coder role

2. The "every producer role" claim is empirically false

Non-blocking suggestions

3. Symlinks in .github-staging/ survive into .github/

4. "Remove the empty .github-staging/ directory and commit the move" is misleading

5. End-to-end verification was not done

6. test_includes_github_staging_convention is shallow

7. .gitignore interaction

8. Hidden subdirectories

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

Blocking issues

Non-blocking suggestions

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Blocking issues from prior review — verified fixed

Non-blocking suggestions from prior review — verified addressed

New non-blocking finding

.github-staging/ itself as a symlink is not guarded

Minor: comment in _build_file_boundary_section is slightly inaccurate

Test #6 partial weakness (carried over)

Uh oh!

This comment has been minimized.

This comment has been minimized.

james-in-a-box Bot commented May 7, 2026

New findings

Uh oh!

Uh oh!

This comment has been minimized.

egg-reviewer Bot left a comment

Choose a reason for hiding this comment

Prior findings — verified fixed

New non-blocking observations

1. Comment in _build_file_boundary_section is missing the documenter case

2. Pre-existing: except ValueError, KeyError, ImportError: syntax

Uh oh!

james-in-a-box Bot commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

3. Symlinks in `.github-staging/` survive into `.github/`

4. "Remove the empty `.github-staging/` directory and commit the move" is misleading

6. `test_includes_github_staging_convention` is shallow

7. `.gitignore` interaction

`.github-staging/` itself as a symlink is not guarded

Minor: comment in `_build_file_boundary_section` is slightly inaccurate

1. Comment in `_build_file_boundary_section` is missing the `documenter` case

2. Pre-existing: `except ValueError, KeyError, ImportError:` syntax