Unify local-mode and issue-mode pipeline behavior

[james-in-a-box]

Summary

Unifies local-mode and issue-mode pipelines so both follow the same contract/checkpoint/push discipline. The only difference between the two modes is now where initial context comes from (GitHub issue vs user prompt).

Key changes:

• Phase-based push restrictions: Replaced blanket local-mode push blocking with file-level restrictions per phase. Refine/plan phases can push .egg-state/ files, implement phase can push code but not contracts/drafts, PR phase can push everything.
• State persistence at phase boundaries: Local pipelines now commit state to git when phases transition, matching issue-mode behavior.
• Unified file paths: Local mode now uses {pipeline_id}- prefixed paths (e.g., .egg-state/drafts/{pipeline_id}-analysis.md) to support concurrent pipelines.
• Contract CLI in both modes: Local-mode agent prompts now include egg-contract CLI instructions.
• Checkpoints enabled for local mode: With push restrictions relaxed, checkpoints are captured on successful pushes.
• Container/agent tracking: PhaseExecution.containers and PhaseExecution.agents are now populated during execution.

Issue: #543

Test plan

• Run make test to verify all unit and integration tests pass
• Test a local pipeline (egg local "Test task") and verify:
  • Push works in each phase with appropriate file restrictions
  • Checkpoint JSON appears on egg/checkpoints/v1 branch
  • State files use {pipeline_id}- prefixed paths
  • Contract CLI commands work (egg-contract show)
• Run two local pipelines concurrently and verify no path conflicts
• Query /api/v1/pipelines/{id} and verify containers array is populated

Authored-by: egg

[james-in-a-box]

Code Review: PR #554 - Unify local-mode and issue-mode pipeline behavior

Summary

This PR replaces the blanket local-mode push blocking with phase-based file restrictions, enabling unified behavior between local and issue mode pipelines. The overall design is sound and follows existing patterns, but there are a few issues that need addressing.

---

Issues Requiring Changes

1. BUG: Unhandled ValueError in gateway.py when session_phase is invalid

File: gateway/gateway.py:680

phase_result = check_phase_file_restrictions(session_phase, changed_files)

The convenience function check_phase_file_restrictions raises ValueError for unknown phase strings (line 855 in phase_filter.py: phase = PipelinePhase(phase)). This call is not wrapped in a try-except, so an invalid session_phase value will cause an unhandled exception and a 500 error instead of graceful handling.

Fix: Either:

1. Wrap the call in try-except and handle ValueError (return 500 with clear error message), OR
2. Modify the convenience function to match the class method behavior (which already handles unknown phases gracefully and allows by default with a log message)

The second option is preferred since it maintains fail-safe behavior without exposing internal implementation details in error messages.

---

2. BUG: Inconsistency between class method and convenience function for unknown phases

File: gateway/phase_filter.py

The class method PhaseFilter.check_phase_file_restrictions() (lines 593-597) handles unknown phases gracefully:

if isinstance(phase, str):
    try:
        phase = PipelinePhase(phase)
    except ValueError:
        return FileRestrictionResult.allow(f"Unknown phase '{phase}', allowing by default")

But the convenience function (lines 854-856) raises ValueError:

if isinstance(phase, str):
    phase = PipelinePhase(phase)  # Raises ValueError
return get_phase_filter().check_phase_file_restrictions(phase, files)

This inconsistency means the same input produces different behavior depending on which entry point is used. The test test_unknown_phase_raises_value_error even expects this behavior, but it contradicts the class method's fail-safe design.

Fix: The convenience function should delegate validation to the class method, not duplicate it with different behavior:

def check_phase_file_restrictions(
    phase: str | PipelinePhase, files: list[str]
) -> FileRestrictionResult:
    return get_phase_filter().check_phase_file_restrictions(phase, files)

Then update the test to expect the allow-by-default behavior.

---

3. Potential security consideration: implement phase allows .egg-state/*.json at root

File: .egg/phase-permissions.json:154-161

The implement phase blocks specific subdirectories:

"blocked_patterns": [
    ".egg-state/contracts/*",
    ".egg-state/drafts/*",
    ".egg-state/pipelines/*",
    ".egg-state/reviews/*"
]

However, files directly in .egg-state/ (not in subdirectories) are NOT blocked. For example:

• .egg-state/config.json - ALLOWED
• .egg-state/malicious.json - ALLOWED

This may be intentional (no such files currently exist at that level), but it's worth explicitly documenting or adding .egg-state/*.json to blocked patterns if the intent is to prevent modification of any .egg-state/ files during implement phase (except checkpoints).

Question: Is this the intended behavior? If so, please document it in the phase-permissions.json description.

---

Minor Issues / Suggestions

4. Variable reuse check uses dir() - works but unconventional

File: gateway/gateway.py:655

if "changed_files" not in dir() or changed_files is None:

This works in Python, but the more conventional approach would be to initialize changed_files = None before the conditional blocks and check if changed_files is None. The dir() approach is unusual and may confuse future maintainers.

---

5. Missing test: path traversal in phase restrictions

The tests don't verify that path traversal attempts (e.g., foo/../.egg-state/contracts/123.json) are correctly normalized and blocked. The path normalization in _normalize_path() appears correct, but adding an explicit test would strengthen confidence.

Suggestion: Add a test case like:

def test_path_traversal_normalized_and_blocked(self):
    """Path traversal attempts are normalized and correctly blocked."""
    result = check_phase_file_restrictions(
        "implement",
        ["foo/../.egg-state/contracts/123.json"],  # Normalizes to .egg-state/contracts/123.json
    )
    assert result.allowed is False

---

6. Integration test uses undefined fixture

File: integration_tests/local_pipeline/test_unified_pipeline_behavior.py:418

def test_pipeline_reports_container_state(self, orchestrator_url: str) -> None:

This test method takes orchestrator_url as a parameter but the class TestContainerAgentTracking doesn't have the local_pipeline_stack fixture that other tests use. It appears to expect a simple string fixture, but this may not exist in the test environment.

---

Positive Observations

1. Security model is sound: Phase-based restrictions with fail-closed behavior on file check errors is the right approach.

2. Path normalization: The _normalize_path() method correctly handles ./ prefixes and path traversal via posixpath.normpath().

3. Checkpoint bypass: Explicitly allowing checkpoint branch pushes (egg/checkpoints/v1) regardless of phase is correctly implemented.

4. Test coverage: Good unit test coverage for the new phase file restrictions, including edge cases like mixed blocked/allowed files.

5. Audit logging: All block scenarios are properly logged before returning errors.

---

Conclusion

The core design is correct, but the ValueError handling inconsistency between the convenience function and class method should be fixed before merge. This is currently a bug that could cause unexpected 500 errors in production if an invalid phase value is ever passed to the gateway.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review! I've addressed the actionable items:

Fixed Issues

1. & 2. ValueError inconsistency - Fixed. The convenience function check_phase_file_restrictions() now delegates validation to the class method instead of duplicating it with different behavior. This ensures consistent fail-safe behavior (allow by default for unknown phases) regardless of which entry point is used.

4. Variable reuse check uses dir() - Fixed. Replaced with standard None comparison by initializing changed_files = None before the conditional blocks.

5. Missing path traversal test - Added three test cases for path traversal scenarios to verify normalization works correctly.

6. Integration test undefined fixture - Fixed. Changed test_pipeline_reports_container_state to use local_pipeline_stack fixture instead of the undefined orchestrator_url parameter.

Response to Item #3: .egg-state/*.json at root

This is intentional behavior. The implement phase blocks specific subdirectories that contain SDLC state files:

• .egg-state/contracts/* - Contracts
• .egg-state/drafts/* - Plan/analysis drafts
• .egg-state/pipelines/* - Pipeline state
• .egg-state/reviews/* - Review state

Files directly in .egg-state/ (not in subdirectories) are NOT blocked because:

1. No such files currently exist at that level - all state is organized in subdirectories
2. Checkpoints (.egg-state/checkpoints/*) are intentionally allowed during implement phase
3. Adding .egg-state/*.json would be a new restriction that could have unintended consequences

If you'd like me to document this explicitly in the phase-permissions.json description, I can add that clarification.

— Authored by egg

[james-in-a-box]

Re-Review: PR #554 - Address Review Feedback

Summary

All issues from my previous review have been properly addressed. The changes are correct and maintain consistency between the convenience function and class method behavior.

---

Issues Verified as Fixed

1. ✅ ValueError Inconsistency Fixed

Previous Issue: The convenience function check_phase_file_restrictions() raised ValueError for unknown phases while the class method handled them gracefully.

Fix Verified: The convenience function now delegates directly to the class method without any pre-validation:

# Before (line 854-855):
if isinstance(phase, str):
    phase = PipelinePhase(phase)  # Raised ValueError

# After (line 854-855):
# Delegate to class method which handles unknown phase gracefully
return get_phase_filter().check_phase_file_restrictions(phase, files)

The corresponding test was updated from test_unknown_phase_raises_value_error to test_unknown_phase_allows_by_default, correctly verifying the fail-safe behavior.

2. ✅ Variable Reuse Check Fixed

Previous Issue: Used unconventional if "changed_files" not in dir() pattern.

Fix Verified: Now uses standard Python pattern:

changed_files = None  # May be populated by role check, reused by phase check
...
if changed_files is None:

3. ✅ Path Traversal Tests Added

Three new test cases verify path normalization:

• test_path_traversal_normalized_and_blocked - Single traversal level
• test_path_traversal_multiple_levels_blocked - Multiple traversal levels
• test_path_traversal_mixed_with_valid_files - Mixed valid and traversal paths

All tests properly verify that posixpath.normpath() normalizes paths before restriction checks.

4. ✅ Integration Test Fixture Fixed

The test test_pipeline_reports_container_state now correctly uses the local_pipeline_stack fixture instead of the undefined orchestrator_url parameter:

def test_pipeline_reports_container_state(self, local_pipeline_stack) -> None:
    orchestrator_url = local_pipeline_stack.orchestrator_url

5. ✅ Test Mock Fixed

The mock session now explicitly sets phase = None to prevent MagicMock auto-creation from triggering the phase check code path.

---

Test Verification

All relevant tests pass:

• test_unknown_phase_allows_by_default ✓
• test_path_traversal_normalized_and_blocked ✓
• test_path_traversal_multiple_levels_blocked ✓
• test_path_traversal_mixed_with_valid_files ✓
• test_push_allowed_when_role_unavailable ✓

---

Conclusion

The review feedback has been thoroughly addressed. The code is ready for merge.

— Authored by egg

[james-in-a-box]

All review feedback has been addressed. The re-review confirmed all fixes are correct and the code is ready for merge.

Summary of fixes applied:

1. ✅ ValueError inconsistency fixed - convenience function now delegates to class method
2. ✅ Variable reuse check fixed - using standard if changed_files is None: pattern
3. ✅ Path traversal tests added - three test cases verify normalization
4. ✅ Integration test fixture fixed - using local_pipeline_stack instead of undefined parameter
5. ✅ Test mock fixed - explicitly sets phase = None

All tests pass locally (74 phase_filter tests, 22 gateway phase/push tests).

— Authored by egg

[james-in-a-box]

Contract Verification Report

Performed comprehensive contract verification for issue #543 (Unify local-mode and issue-mode pipeline behavior).

Summary

Core Implementation: Verified ✅

The implementation correctly addresses the main objectives:

• Phase-based push restrictions replace blanket local-mode blocking
• State persistence at phase boundaries via force_commit flag
• File path prefixing with pipeline IDs
• Contract CLI instructions for both modes
• Container and agent tracking populated

Detailed Verification

Phase 1: Git Push Restrictions ✅

• ac-1: phase_file_restrictions section present in .egg/phase-permissions.json with correct patterns for refine/plan/implement/pr phases
• ac-2: check_phase_file_restrictions() implemented in gateway/phase_filter.py:830-880 with proper pattern matching and path normalization
• ac-3: Push handler in gateway/gateway.py:646-704 enforces phase restrictions with audit logging
• ac-4: Blanket local-mode push block removed (diff shows removal of lines 520-533)
• ac-5: Checkpoint branch bypass implemented: CHECKPOINT_BRANCH = "egg/checkpoints/v1" with is_checkpoint_push check

Phase 2: State Persistence ✅

• ac-6: force_commit flag added to StateStore.save_pipeline() at line 182
• ac-7: Phase transitions use force_commit=is_local in pipelines.py:2019-2030
• ac-8: delete_pipeline() accepts force_commit parameter

Phase 3: File Paths ✅

• ac-9: _get_draft_path() returns {pipeline_id}-{phase}.md format for local mode
• ac-10: _verdict_path_for_type() returns {pipeline_id}-{phase}-{type}.json format
• ac-11: _read_phase_draft() updated to pass pipeline_id parameter
• ac-12: Phase prompts use _get_draft_path() to show correct paths

Phase 4: Contract Usage ✅

• ac-13: Contract CLI instructions (egg-contract show/add-commit) now shown for both modes in _build_phase_prompt()
• ac-14: contract_synced=False set in state_store.create_pipeline(), updated to True after successful creation
• ac-15: Contract path uses {pipeline_id}.json (verified in integration tests)

Phase 5: Checkpoints ✅

• ac-16: Checkpoint capture no longer blocked by local-mode restrictions
• ac-17: Checkpoint patterns included in allowed_patterns for refine/plan phases
• ac-18: ⚠️ No explicit test for checkpoint JSON on branch; covered implicitly by architecture

Phase 6: Container/Agent Tracking ✅

• ac-19: _spawn_and_wait() now creates AgentExecution records
• ac-20: phase_execution.agents.append(agent_execution) populates agent tracking
• ac-21: Agent status updated with completed_at, status, error fields
• ac-22: API exposes containers/agents via existing model serialization

Test File Location Issue ⚠️

The contract specifies Phase 7 tests in specific files:

• orchestrator/tests/test_pipeline_push_restrictions.py
• orchestrator/tests/test_local_pipeline_contracts.py
• orchestrator/tests/test_concurrent_pipelines.py
• orchestrator/tests/test_container_tracking.py
• orchestrator/tests/test_state_persistence.py

However, these tests are consolidated in integration_tests/local_pipeline/test_unified_pipeline_behavior.py.

While the test scenarios ARE covered (push restrictions, concurrent pipelines, container tracking, etc.), the file locations don't match the contract specification. The tests are functionally equivalent but organized differently.

Recommendation: This is a minor deviation. The consolidated test file provides better maintainability. Consider updating the contract to reflect the actual file structure, or accept this as an acceptable implementation choice.

Additional Unit Tests ✅

Comprehensive unit tests added in gateway/tests/test_phase_filter.py:

• TestPhaseFileRestrictions class with 20+ test cases
• Path traversal attack prevention tests
• Pattern matching edge cases
• Unknown phase handling

Verdict

All core acceptance criteria verified. The implementation correctly unifies local-mode and issue-mode pipeline behavior with proper security controls, state persistence, and tracking.

The test file location deviation is a minor issue that doesn't affect functionality. The implementation meets the spirit and intent of all acceptance criteria.

— Authored by egg

[james-in-a-box]

egg contract-verification completed. View run logs

6 previous review(s) hidden.