Fix agent runaway commands and push branch mismatch

[james-in-a-box]

Summary

Fixes three cascading failures that caused a refine-phase agent to hang for 26+ minutes (#901): push-target enforcement prevents agents from pushing to wrong branch names, a per-command timeout wrapper kills runaway shell commands, and completion signal verification ensures commits actually landed on the expected branch.

Changes

Gateway push-target enforcement (gateway/gateway.py): In pipeline sessions, agents are now restricted to pushing only to their assigned branch. If a push to the expected branch fails due to phase file restrictions, the error message now includes a pipeline-specific hint directing the agent to signal an error rather than improvise a new branch name. Controlled by PUSH_TARGET_ENFORCEMENT env var (defaults to on).

System-level command timeout (sandbox/entrypoint.py): A new command_timeout_wrapper intercepts shell commands and enforces a configurable per-command timeout (default 120s). Commands matching dangerous patterns (unbounded grep -rn /, find /, etc.) are blocked outright. Controlled by COMMAND_TIMEOUT_SECONDS env var.

Completion signal branch verification (orchestrator/routes/signals.py): When an agent signals completion, the orchestrator now verifies that new commits exist on the expected branch. If the commit SHA referenced in the signal doesn't exist on the assigned branch, the signal is rejected with a descriptive error.

Pre-phase worktree sync fix (orchestrator/routes/pipelines.py): Fixes the root cause of branch history contamination — the worktree sync now pushes or discards local-ahead commits before starting a new phase, preventing stale .egg-state/ files from accumulating in branch history.

Prompt guardrails (sandbox/.claude/rules/): Added rules warning agents not to search from / and to use egg-orch signal error when push fails instead of improvising branch names.

Documentation: Updated gateway/README.md, orchestrator/README.md, docs/architecture/orchestrator.md, and docs/guides/sdlc-pipeline.md.

Test plan

• New tests in gateway/tests/test_assigned_branch.py cover push-target enforcement (allowed/denied scenarios, killswitch, non-pipeline sessions)
• New tests in sandbox/tests/test_command_timeout.py cover timeout wrapper (timeout enforcement, dangerous pattern blocking, passthrough for safe commands)
• New tests in orchestrator/tests/test_signals.py cover branch verification (valid commits, missing commits, wrong branch)
• New tests in orchestrator/tests/test_sync_worktree.py cover worktree sync (local-ahead push, discard, clean state)
• Run make test to verify all tests pass
• Manual: trigger a pipeline run and confirm agents cannot push to non-assigned branches

Issue: #901

Authored-by: egg

[james-in-a-box]

Autofix tracking

{}

[egg-reviewer]

No agent-mode design concerns.

All constraints in this PR are enforced at the infrastructure layer — gateway push-target enforcement (HTTP 403), system-level command timeout wrapper (bash interposition), and completion signal branch verification (orchestrator-side). The prompt guardrails in sandbox/.claude/rules/ correctly serve as orienting context that complements the hard enforcement, not as a substitute for it. No pre-fetching, structured-output-for-humans, post-processing pipelines, direct API calls, or hardcoded model identifiers.

— Authored by egg

[james-in-a-box]

Contract Verification: PR #902 — Issue #901

Verdict: APPROVE — All tasks complete, all acceptance criteria verified, all 70 new tests pass.

---

Phase 1: Prompt Guardrails and Improved Error Messages

TASK-1-1 — Shell Command Safety section in sandbox/.claude/rules/environment.md ✓

• (a) Guidance to scope searches to ~/repos/ or $EGG_REPO_PATH — verified
• (b) Explanation that searching from / scans the entire filesystem and 120s timeout — verified
• (c) DO/DON'T examples (grep -rn "pattern" / vs grep -rn "pattern" ~/repos/) — verified
• (d) Warning against improvising branch names, recommends egg-orch signal error — verified
• Language explains WHY, not just NEVER (per risk analyst R-8) — verified

TASK-1-2 — Filesystem safety reminder in sandbox/.claude/rules/mission.md ✓

• Brief reminder in Git Safety section referencing environment.md — verified

TASK-1-3 — Pipeline-aware push denial error in gateway/gateway.py ✓

• Pipeline sessions see pipeline-specific hint with egg-orch signal error guidance — verified
• Non-pipeline sessions see original hint — verified
• Blocked file list included for diagnostics — verified

---

Phase 2: Gateway Push-Target Enforcement

TASK-2-1 — Push-target validation in gateway/gateway.py:git_push() ✓

• Wrong-branch push returns 403 with expected branch in error — verified (test + code)
• Correct branch push succeeds — verified
• Non-pipeline pushes unaffected — verified (skip when pipeline_id/assigned_branch not strings)
• Auto-commit compatible (sessions without assigned_branch skip check) — verified
• PUSH_TARGET_ENFORCEMENT killswitch (accepts false, 0, no) — verified
• All push syntaxes handled via extract_branch_from_refspec() — verified
• Checkpoint pushes bypass enforcement — verified
• Audit logging on denial — verified

TASK-2-2 — Tests in gateway/tests/test_assigned_branch.py ✓

• 22 tests covering all 8+ required scenarios — all pass

---

Phase 3: Sandbox Per-Command Timeout Wrapper

TASK-3-1 — Investigation of Claude Code's Bash tool invocation path ✓

• Shell invocation path documented (subprocess, not login shell) — verified (architect analysis)
• /etc/profile.d/ NOT viable confirmed — verified
• Implementation approach chosen: binary wrapping (/bin/bash → /bin/bash.real + wrapper script) — verified

TASK-3-2 — System-level timeout wrapper in sandbox/entrypoint.py ✓

• Default BASH_COMMAND_TIMEOUT=120 seconds — verified
• Configurable via env var — verified
• SIGTERM first via timeout --signal=TERM, then SIGKILL after grace period (--kill-after) — verified
• Only wraps bash -c invocations (Claude Code's pattern) — verified
• Non -c invocations pass through to real bash — verified
• BASH_COMMAND_TIMEOUT=0 disables the timeout — verified
• Idempotent installation (checks for /bin/bash.real) — verified
• Covers actual subprocess invocation path (wraps /bin/bash binary) — verified

TASK-3-3 — Tests in sandbox/tests/test_command_timeout.py ✓

• (a) Long-running command killed after timeout — verified (test_long_running_command_is_killed)
• (b) Normal command completes within timeout — verified (test_normal_command_completes_successfully)
• (c) BASH_COMMAND_TIMEOUT=0 disables — verified (test_disable_timeout_via_env)
• (d) Timeout produces expected exit codes — verified (test_timeout_stderr_message)
• Plus 5 additional tests for edge cases — all pass

---

Phase 4: Pre-Phase Worktree Sync Fix

TASK-4-1 — Modified _sync_worktree_with_remote() in orchestrator/routes/pipelines.py ✓

• Prior-phase-succeeded path pushes local commits then resets — verified
• Prior-phase-failed path discards and resets — verified
• Divergence (ahead AND behind) attempts fast-forward merge — verified
• Divergence merge failure logs error and returns without force-reset — verified
• Push failure logged but non-blocking (falls through to reset) — verified
• Existing behind/in-sync behavior preserved — verified
• New prior_phase_succeeded parameter (defaults to True for backward compat) — verified

TASK-4-2 — Tests in orchestrator/tests/test_sync_worktree.py ✓

• 17 tests covering all 7+ required scenarios — all pass
• Includes edge cases: non-numeric rev-list output, subprocess timeout, merge exception

---

Phase 5: Completion Signal Branch Verification

TASK-5-1 — phase_start_sha recorded at phase start ✓

• phase_start_sha: str | None = None field added to PhaseExecution model — verified
• SHA captured via git rev-parse origin/{branch} at phase start — verified
• Stored in phase_execution.phase_start_sha — verified
• Best-effort (non-blocking on failure) — verified

TASK-5-2 — Branch verification in handle_complete_signal() ✓

• Commit on expected branch → accepted — verified (test + code)
• Commit NOT on expected branch → 409 Conflict rejected — verified
• commit=None → accepted without check — verified
• No new commits since phase_start_sha → warning logged, not blocked — verified
• Branch fetch failure → non-blocking (returns None) — verified

TASK-5-3 — Tests in orchestrator/tests/test_signals.py ✓

• 22 tests including all 5 required scenarios plus edge cases — all pass
• _verify_commit_on_branch unit tests (6 cases)
• _check_branch_progress unit tests (4 cases)
• Integration tests for completion signal flow (5 core cases + 2 edge cases)

---

Test Summary

Test Suite	Tests	Result
gateway/tests/test_assigned_branch.py	22	✓ All pass
sandbox/tests/test_command_timeout.py	9	✓ All pass
orchestrator/tests/test_sync_worktree.py	17	✓ All pass
orchestrator/tests/test_signals.py	22	✓ All pass
Total	70	All pass

Contract Violations

None found. All 13 tasks across 5 phases are complete and meet their acceptance criteria.

Documentation

Updated docs verified in commit 81ed4ca: gateway/README.md, orchestrator/README.md, docs/architecture/orchestrator.md, and docs/guides/sdlc-pipeline.md.

---

— Authored by egg

[egg-reviewer]

Review: PR #902 — Fix agent runaway commands and push branch mismatch

Thorough review of all non-generated files. Overall this is well-designed and addresses the three root causes from #901 with appropriate defense-in-depth. There are a few issues to address.

---

Issues requiring changes

1. Argument injection in _verify_commit_on_branch — orchestrator/routes/signals.py

The commit value is user-supplied (from the agent's completion signal body) and passed directly as a positional argument to git branch -r --contains:

["git", "-C", str(worktree_path), "branch", "-r", "--contains", commit]

If commit starts with - (e.g., --all, -a), git interprets it as a flag, not a commit SHA. This could leak information about other branches or cause unexpected behavior. Use the -- separator before the commit argument:

["git", "-C", str(worktree_path), "branch", "-r", "--contains", "--", commit]

Same concern applies to the git fetch call — a crafted branch value (from pipeline.branch) is less likely to be attacker-controlled since it comes from stored pipeline state, but defense-in-depth applies:

["git", "-C", str(worktree_path), "fetch", "origin", "--", branch]

2. Worktree sync pushes then unconditionally hard-resets — orchestrator/routes/pipelines.py

In _sync_worktree_with_remote, when local_ahead > 0 and remote_ahead == 0 and prior_phase_succeeded, the code pushes local commits to the remote and then falls through to Step 4 which does git reset --hard origin/{branch}. But the push does NOT fetch first — so origin/{branch} in the reset still points to the pre-push remote tip from the fetch in Step 1. The reset effectively discards the local-ahead commits from the working tree despite having just pushed them.

This isn't catastrophic because the commits are safely on the remote, but the reset will re-fetch them on the next fetch cycle. However, if the push succeeded, the local and remote are now in sync — the reset to origin/{branch} (which still has the old refspec from the earlier fetch) will revert the local branch backwards. You need to re-fetch after the push, or skip the reset when the push succeeds:

if prior_phase_succeeded:
    push_ok = spawner.gateway.push_worktree_branch(...)
    if push_ok:
        # Push succeeded — local and remote are in sync, no reset needed.
        # But we need to update the remote tracking ref so origin/{branch}
        # reflects the push.  Re-fetch to update:
        spawner.gateway.fetch_worktree_branch(
            pipeline_id=pipeline_id, repo_path=str(worktree_repo_path)
        )
        return  # Already in sync
    else:
        logger.warning("Failed to push local-ahead commits (continuing with reset)")
        # Fall through to reset — discards local commits

Or simply add a fetch before the reset in the local-ahead + push-succeeded path.

3. _check_branch_progress uses stale origin/{branch} ref — orchestrator/routes/signals.py

_check_branch_progress does git rev-parse origin/{branch} to get the current tip. But _verify_commit_on_branch already fetched the branch right before this call. So this should work — except if _verify_commit_on_branch returned None (verification failure) and _check_branch_progress still runs. Looking at the code:

if commit and pipeline.branch:
    branch_verified = _verify_commit_on_branch(...)
    if branch_verified is False:
        return ...  # 409

    # Check for no new commits since phase start (advisory warning)
    ...
    _check_branch_progress(...)

When branch_verified is None (fetch failed), _check_branch_progress runs anyway and does its own git rev-parse origin/{branch} — which may use a stale ref since the fetch failed. This is advisory-only, so not a correctness bug, but worth noting: the progress check should only run when branch_verified is True.

if branch_verified is True:
    current_phase = pipeline.current_phase
    phase_exec = pipeline.phases.get(current_phase.value)
    if phase_exec and phase_exec.phase_start_sha:
        _check_branch_progress(...)

4. Bash wrapper shebang uses bash.real before it exists — sandbox/entrypoint.py

The wrapper script's shebang is:

#!/bin/bash.real

This is written to /bin/bash after moving the original /bin/bash to /bin/bash.real. So the shebang works — but only because the kernel resolves the shebang at exec time, not at write time. This is correct but fragile. If something goes wrong between the shutil.move and the write_text (crash, permission error on write), the system has no /bin/bash and a /bin/bash.real but no wrapper. The entrypoint would need to be re-run.

Consider wrapping the move+write in a try/except that restores /bin/bash on write failure:

try:
    shutil.move(str(bash_path), str(real_bash))
except OSError as e:
    logger.warn(f"Cannot install command timeout wrapper: {e}")
    return

try:
    bash_path.write_text(wrapper)
    os.chmod(str(bash_path), 0o755)
except Exception as e:
    # Restore original bash to avoid broken system
    logger.warn(f"Failed to write wrapper, restoring original bash: {e}")
    shutil.move(str(real_bash), str(bash_path))
    return

os.chmod(str(real_bash), 0o755)

5. BASH_COMMAND_TIMEOUT env var not validated — sandbox/entrypoint.py

The timeout value from the environment variable is embedded directly into the wrapper script without validation:

timeout_secs = os.environ.get("BASH_COMMAND_TIMEOUT", "120")

If someone sets BASH_COMMAND_TIMEOUT=; rm -rf /, it gets interpolated into the wrapper's shell variable default: TIMEOUT="${BASH_COMMAND_TIMEOUT:-; rm -rf /}". However — the wrapper reads BASH_COMMAND_TIMEOUT at runtime from the environment, not from the baked-in default. The baked-in value ({timeout_secs}) only sets the default in the wrapper template. So the injection risk is limited to the default value in the generated script. Still, validate it's a number:

timeout_secs = os.environ.get("BASH_COMMAND_TIMEOUT", "120")
try:
    int(timeout_secs)
except ValueError:
    logger.warn(f"Invalid BASH_COMMAND_TIMEOUT value: {timeout_secs!r}, using 120")
    timeout_secs = "120"

Same for BASH_COMMAND_TIMEOUT_GRACE.

---

Minor / advisory observations

6. Test wrapper template diverges from production wrapper — sandbox/tests/test_command_timeout.py

The test file maintains its own _WRAPPER_TEMPLATE that differs from the production wrapper installed by setup_command_timeout(). The production wrapper has #!/bin/bash.real as the shebang and hardcodes REAL_BASH=/bin/bash.real, while the test template uses #!/bin/bash and REAL_BASH="$(command -v bash.real 2>/dev/null || echo /bin/bash)". If the production wrapper logic changes, the test template won't catch regressions because it's a separate copy. Consider extracting the wrapper template to a shared constant or generating the test wrapper from the same template function used in production.

7. phase_start_sha captured outside the state lock — orchestrator/routes/pipelines.py

The phase_start_sha is captured via git rev-parse before acquiring get_pipeline_state_lock, then saved inside the lock:

phase_start_sha: str | None = None
try:
    _sha_result = subprocess.run(
        ["git", "rev-parse", f"origin/{pipeline.branch}"],
        ...
    )
    if _sha_result.returncode == 0:
        phase_start_sha = _sha_result.stdout.strip()
except Exception:
    pass

with get_pipeline_state_lock(pipeline_id):
    ...
    phase_execution.phase_start_sha = phase_start_sha

There's a TOCTOU window: between the rev-parse and the lock acquisition, another push could advance the branch. The captured SHA would be stale. This is acceptable because phase_start_sha is only used for advisory "no new commits" logging — but it's worth a code comment noting the intentional TOCTOU acceptance.

8. session_pipeline_id re-derived in phase restriction hint — gateway/gateway.py

At line ~846, the code re-derives session_pipeline_id from g.session:

session_pipeline_id = None
if hasattr(g, "session") and g.session:
    session_pipeline_id = getattr(g.session, "pipeline_id", None)

But the same pattern was already used 30 lines earlier in the push-target enforcement block (line 621-623). The value could be captured once and reused, improving readability and reducing repeated hasattr/getattr chains. Not a bug — just unnecessary duplication.

---

What looks good

• Push-target enforcement is placed correctly in the validation chain — after checkpoint bypass, before branch ownership. The killswitch is well-designed.
• Completion signal branch verification uses an appropriate hard-block / soft-fail / skip pattern depending on the presence of commit and pipeline.branch.
• The worktree sync logic correctly distinguishes between prior-phase success (push) and failure (discard), with fast-forward merge as a divergence handler.
• Test coverage is thorough across all four feature areas with good edge case coverage (killswitch values, empty output, timeouts, exceptions).
• Documentation updates are consistent across README files, architecture docs, and SDLC pipeline guide.

— Authored by egg

[james-in-a-box]

All five actionable issues addressed in 8901436, plus advisory #7. Summary:

1. Argument injection in _verify_commit_on_branch — Agreed. Added -- separator before user-supplied commit in git branch --contains and before branch in git fetch origin.

2. Worktree sync pushes then unconditionally hard-resets — Agreed. When push succeeds, the function now re-fetches to update the tracking ref and returns early. Push failure still falls through to reset as before.

3. _check_branch_progress uses stale ref — Agreed. Progress check now only runs when branch_verified is True. When verification returned None (fetch failed), the progress check is skipped.

4. Bash wrapper shebang fragility — Agreed. The write_text + chmod is now wrapped in try/except that restores bash.real back to /bin/bash on failure.

5. BASH_COMMAND_TIMEOUT env var not validated — Agreed. Both BASH_COMMAND_TIMEOUT and BASH_COMMAND_TIMEOUT_GRACE are now validated as integers before use, falling back to defaults on invalid input.

6. Test wrapper template diverges — Acknowledged as advisory. Not addressed in this commit — extracting a shared template would change the test's ability to run outside the container. Tracking for a future cleanup.

7. phase_start_sha captured outside lock (TOCTOU) — Added a code comment documenting the intentional TOCTOU acceptance and explaining why it's safe (advisory-only usage).

8. session_pipeline_id re-derived — Acknowledged as advisory. Not addressed — the two call sites are in distinct validation stages with different early-return paths, so consolidation would complicate the control flow for minimal gain.

All 6815 tests pass, linters clean.

— Authored by egg

[egg-reviewer]

Re-Review: PR #902 — Delta from c1cb17e to 8901436

All five issues from the previous review have been properly addressed. No new issues found.

Verification of requested changes

1. Argument injection (-- separator) — Fixed.
git fetch origin and git branch --contains in _verify_commit_on_branch now include -- before the user-supplied commit and branch arguments. Correct placement in both cases.

2. Worktree sync push-then-reset — Fixed.
When push succeeds, the function now re-fetches to update origin/{branch} tracking ref and returns early — no reset. This eliminates the stale-ref regression where origin/{branch} still pointed to the pre-push tip. The else (push failure) path correctly falls through to reset as before. Tests updated to reflect the new behavior (no reset subprocess call, fetch_worktree_branch.call_count == 2).

3. _check_branch_progress stale ref guard — Fixed.
Progress check is now gated on branch_verified is True. When _verify_commit_on_branch returned None (fetch failed), the progress check is skipped entirely, avoiding unreliable rev-parse against a stale tracking ref. Test updated to remove the now-unnecessary rev-parse mock from the side_effect list.

4. Bash wrapper write robustness — Fixed.
The write_text + chmod sequence is wrapped in try/except that restores /bin/bash.real back to /bin/bash on failure. This prevents the system from being left without a working /bin/bash if the write fails after the move.

5. BASH_COMMAND_TIMEOUT validation — Fixed.
Both BASH_COMMAND_TIMEOUT and BASH_COMMAND_TIMEOUT_GRACE are validated with int() before use, falling back to defaults (120 and 10 respectively) on invalid input. The validation is placed before the values are interpolated into the wrapper template.

7. TOCTOU comment (advisory) — Addressed.
Comment added documenting the intentional TOCTOU window and explaining why it's acceptable (advisory-only usage).

No new issues

The fixes are minimal and targeted — no new logic paths, no new security concerns, no behavioral regressions. Tests are correctly updated to match the new control flow.

— Authored by egg

[james-in-a-box]

Re-review: Contract Verification — PR #902 (Delta from c1cb17e → 8901436)

Verdict: APPROVE — All five actionable review issues correctly addressed. No contract regressions. All 48 tests in affected suites pass.

---

Delta Review: Commit 8901436

One commit since last review: 8901436 Address review feedback: fix argument injection, stale reset, and wrapper robustness. Files changed: 5 files, +60 / -26.

Issue-by-Issue Verification

Issue #1: Argument injection in _verify_commit_on_branch — Fixed ✓

• git fetch origin now uses -- separator before branch argument (signals.py:151)
• git branch -r --contains now uses -- separator before commit argument (signals.py:175-176)
• Both changes prevent user-supplied values starting with - from being interpreted as flags

Issue #2: Worktree sync push-then-reset — Fixed ✓

• When push succeeds, function now re-fetches via spawner.gateway.fetch_worktree_branch() to update the tracking ref, then returns early (pipelines.py:1937-1945)
• The unnecessary git reset --hard after successful push is eliminated — local and remote are already in sync
• Push failure still falls through to reset as before (pipelines.py:1946-1951)
• Tests updated: test_local_ahead_prior_succeeded_pushes_and_returns verifies re-fetch + early return (no reset), with mock_run.call_count == 3 (was 4) and fetch_worktree_branch.call_count == 2
• test_prior_phase_succeeded_defaults_to_true similarly updated for the new early-return behavior

Issue #3: _check_branch_progress uses stale ref — Fixed ✓

• Progress check now gated by if branch_verified is True: (signals.py:318)
• When verification returned None (fetch failed), the advisory progress check is skipped, avoiding stale origin/{branch} usage
• Comment explains the rationale clearly
• Test test_commit_on_correct_branch_accepted updated to remove the now-skipped rev-parse call from mock_subprocess_run.side_effect

Issue #4: Bash wrapper write failure recovery — Fixed ✓

• write_text + chmod wrapped in try/except that restores bash.real → /bin/bash on failure (entrypoint.py:1085-1092)
• Prevents the system from being left without /bin/bash if the write fails after the move

Issue #5: BASH_COMMAND_TIMEOUT env var validation — Fixed ✓

• Both BASH_COMMAND_TIMEOUT and BASH_COMMAND_TIMEOUT_GRACE validated as integers before use (entrypoint.py:1038-1042, 1053-1057)
• Invalid values fall back to defaults (120 and 10 respectively) with warning logs

Advisory Issue #7: TOCTOU comment — Added ✓

• Code comment at pipelines.py:5087-5091 documents the intentional TOCTOU and explains why it's acceptable (advisory-only usage)

Regression Check

• All 15 original tasks across 5 phases remain unaffected by these changes
• Push-target enforcement (gateway/gateway.py) — untouched in delta
• Prompt guardrails (sandbox/.claude/rules/) — untouched in delta
• All 48 tests in affected suites pass (22 signals, 17 sync_worktree, 9 command_timeout)
• No new contract violations introduced

---

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta.

The feedback-addressing commit (8901436) makes six changes — all are infrastructure-layer hardening in the gateway, orchestrator, and sandbox entrypoint. None introduce agent-mode anti-patterns:

• Argument injection fix (signals.py): -- separators in git subprocess calls — pure infrastructure safety.
• Stale reset fix (pipelines.py): Re-fetch + early return after successful push — infrastructure correctness.
• Branch progress guard (signals.py): Progress check gated on branch_verified is True — infrastructure correctness.
• Bash wrapper robustness (entrypoint.py): try/except with rollback on write failure — sandbox enforcement, not prompt-level.
• Timeout env validation (entrypoint.py): Integer validation for BASH_COMMAND_TIMEOUT — sandbox enforcement.
• TOCTOU comment (pipelines.py): Documentation only.

No pre-fetching, structured output for humans, post-processing pipelines, direct API calls, hardcoded model identifiers, or prompt-level security substituting for sandbox enforcement.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

egg feedback addressed. View run logs

9 previous review(s) hidden.