Add contract schema and core library for issue #133 Phase 1

[james-in-a-box]

Summary

Implements Phase 1 of the structurally enforced agent checkpoints feature (#133):

• Contract JSON schema at .egg/schemas/contract.schema.json defining the structure for phases, tasks, decisions, and audit logs with role-based ownership annotations
• egg_contracts library providing Pydantic models, role-based mutation validation, and contract persistence
• Gateway contract API endpoints for retrieving and mutating contracts with role enforcement
• 62 unit tests covering models, role permissions, and mutation validation

Key implementation details

Role-based field ownership: Each field in the contract is owned by a specific role:

• implementer: Can modify tasks[].commit, tasks[].notes
• reviewer: Can modify tasks[].status, phases[].status, acceptance_criteria[].verified
• human: Can modify decisions[].resolved and all other fields
• system: Owns structural fields like issue, schemaVersion

Gateway enforcement: Role is determined from workflow context (not agent environment), preventing privilege escalation. All mutations are validated and audited.

Issue: #133

Test plan:

• Run pytest tests/shared/egg_contracts/ -v - 62 tests pass
• Run ruff check on new files - all checks pass
• Verify schema renders correctly in GitHub
• Review role ownership mappings against spec

Authored-by: egg

[james-in-a-box]

Review Summary

This is a well-structured Phase 1 implementation of structurally enforced agent checkpoints. The contract schema design, Pydantic models, and role-based permission system are well thought out. However, there are some issues that should be addressed before merging.

Blocking Issues

1. Missing Session Authentication on Contract API Endpoints

Location: gateway/contract_api.py:129-350

The contract API endpoints (get_contract, mutate_contract, validate_contract_mutation, check_contract_exists) do not use the @require_session_auth decorator. Other gateway endpoints (git/push, gh/pr/create, etc.) consistently require session authentication. This is a security gap.

Impact: Without session authentication:

• Any process with network access to the gateway can read/mutate contracts
• The g.session context needed for production role determination won't be populated
• The fallback to X-Egg-Role header becomes the de facto path, which is less secure

Fix: Apply @require_session_auth decorator to contract endpoints:

from gateway import require_session_auth

@contract_bp.route("/<int:issue_number>", methods=["GET"])
@require_session_auth
def get_contract(issue_number: int) -> tuple[Response, int]:
    ...

2. Session Dataclass Lacks agent_role Field

Location: gateway/contract_api.py:72-75 and gateway/session_manager.py:75-96

The get_role_from_context() function expects g.session.agent_role, but the Session dataclass doesn't have an agent_role field. This means the session-based role lookup will never work in production.

Impact: Role determination will always fall back to environment variable or header, bypassing the intended secure workflow context mechanism.

Fix: Add agent_role field to Session dataclass:

@dataclass
class Session:
    ...
    agent_role: str | None = None  # Role set by workflow context

Also update to_dict_for_persistence() and from_persistence() to handle this field.

Non-Blocking Issues

3. Consider Adding Defensive Path Validation in _set_value / _get_value

Location: shared/egg_contracts/validator.py:143-214

The _get_value and _set_value functions traverse objects dynamically. While they do handle errors, consider adding validation for:

• Maximum path depth (to prevent deeply nested attack paths)
• Path component validation (reject special characters that could confuse traversal)

This is advisory given the gateway handles authorization, but defense in depth is good practice.

4. Minor: Test Coverage for Gateway Contract API

The tests cover the egg_contracts library well (62 tests), but I don't see integration tests for gateway/contract_api.py endpoints. Consider adding tests for:

• Authentication enforcement
• Role extraction from various sources (session, header, env)
• Error responses

Suggestions

Documentation of Role Sources Priority

The get_role_from_context() function checks three sources in order: header → session → environment. Consider adding inline comments clarifying why this order was chosen, especially since header takes precedence (which seems like it might be backwards for production security).

Currently header is checked first "for gateway testing", but this means in production the header could override session context. You may want to reorder:

# Production path (session) should take precedence
if hasattr(g, "session") and g.session:
    session_role = getattr(g.session, "agent_role", None)
    if session_role:
        return Role(session_role.lower())

# Development/testing paths (lower priority)
header_role = request.headers.get("X-Egg-Role")
...

Summary

The core design is solid. The blocking issues are around authentication gaps that would undermine the security model. Once those are addressed, this is ready to merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[jwbron]

@james-in-a-box address the above feedback

[james-in-a-box]

Review Summary

Solid Phase 1 implementation of the contract system. The role-based mutation validation is well-designed with gateway enforcement preventing privilege escalation. The Pydantic models provide strong validation and the test coverage is comprehensive.

Issues to Address

1. GET endpoints incorrectly attempt to parse JSON body

In contract_api.py, both get_contract() (line 144) and check_contract_exists() (line 347) call get_repo_path_from_request() which attempts to parse a JSON body via request.get_json(). GET requests typically don't have bodies, and some HTTP clients/proxies may strip them.

Consider using query parameters for repo_path on GET endpoints instead:

repo_path = request.args.get("repo_path")

2. Missing file write atomicity in loader.py

In save_contract() (lines 1242-1247), the contract is written directly to the target path. If the process crashes mid-write, the contract file could be corrupted. Consider writing to a temp file first and then atomically renaming:

import tempfile
with tempfile.NamedTemporaryFile(mode="w", dir=path.parent, delete=False) as f:
    json.dump(contract.model_dump(mode="json"), f, indent=2, default=str)
    f.write("\n")
    temp_path = f.name
os.rename(temp_path, path)

3. Inconsistent role enum usage

roles.py defines Role as a regular Enum, but models.py defines AuditRole as a StrEnum. In validator.py:99, there's a conversion AuditRole(role.value) that relies on both having matching string values. Consider using StrEnum consistently for both to make the relationship explicit and avoid potential mismatches if the enums diverge.

Observations (non-blocking)

• The validate_task_mutation and validate_phase_mutation helpers don't use the phase_id/task_id parameters—they build a wildcard path regardless. The parameters suggest they should look up actual indices. Either remove the unused parameters or implement index resolution.

• The gateway blueprint registration in gateway.py (lines 149-157) has try/except with two different import paths. This works but the duplication could be consolidated.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

Addressed Review Feedback

I've addressed all the blocking and non-blocking issues from the review:

Blocking Issues (Fixed)

1. Missing Session Authentication on Contract API Endpoints

• Added @require_session_auth decorator to all contract endpoints (get_contract, mutate_contract, validate_contract_mutation, check_contract_exists)
• Created new gateway/auth.py module to house the auth decorator and avoid circular imports between gateway.py and contract_api.py

2. Session Dataclass Lacks agent_role Field

• Added agent_role: str | None = None field to Session dataclass
• Updated to_dict_for_persistence() and from_persistence() to handle the new field

Non-Blocking Issues (Fixed)

3. GET endpoints incorrectly attempt to parse JSON body

• Modified get_repo_path_from_request() to accept a from_query parameter
• GET endpoints (get_contract, check_contract_exists) now use query parameters for repo_path

4. Missing file write atomicity in loader.py

• save_contract() now uses atomic write pattern: write to temp file in same directory, then os.rename() for atomic replacement

5. Inconsistent role enum usage

• Changed Role from Enum to StrEnum for consistency with AuditRole

Suggestions (Implemented)

Documentation of Role Sources Priority

• Reordered role source priority: session → header → environment
• Session now takes precedence to prevent header-based privilege escalation in production
• Added comprehensive inline documentation explaining the priority order

Test Updates

• Updated test fixtures in test_gateway.py and test_gateway_integration.py to patch session_manager.validate_session_for_request instead of gateway.validate_session_for_request
• Updated conftest.py to load the new auth.py and contract_api.py modules

All 80 tests pass (62 egg_contracts tests + 18 gateway integration tests). The 2 pre-existing failures in test_gateway.py are due to missing token_refresher module in the test environment, unrelated to these changes.

— Authored by egg

[jwbron]

@james-in-a-box make the necessary changes to this pr based on this commit, if relevant 9f5321f

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Overall this is a well-structured implementation of Phase 1 for the contract system. The role-based access control design is sound, the code quality is high, and test coverage is thorough.

Findings

1. Security: Role determination fallback chain could be tightened (advisory)

In contract_api.py:618-662, the get_role_from_context() function has a fallback chain:

1. Session metadata (secure)
2. X-Egg-Role header (testing)
3. EGG_AGENT_ROLE env var (development)

The header fallback (priority 2) is concerning for production. Even though it's documented as "lower priority than session", if session auth passes but g.session.agent_role is None, the header would be used. Consider adding a warning log when falling back to header or env var, or making header fallback only work in explicit dev/test mode.

# Suggestion: log when using insecure fallbacks
if header_role:
    logger.warning("Using X-Egg-Role header for role - testing mode only")

2. contract_api.py:786 - Consider validating new_value type early

The /mutate endpoint accepts new_value of any type without schema validation before passing to apply_mutation. While Pydantic will catch type mismatches when setting the value, an early validation step could provide clearer error messages. This is minor - current behavior is acceptable.

3. Good: Atomic file writes in loader.py

The save_contract() implementation correctly uses temp-file-then-rename for atomic writes. This prevents corruption on crashes.

4. Good: Test coverage is comprehensive

62 tests covering models, role permissions, and mutation validation is solid. The test organization (separate files for models, roles, validator) is clean.

5. Good: Refactoring of require_session_auth

The extraction of require_session_auth to auth.py to break circular imports is well-handled. The test patches are correctly updated to target session_manager.validate_session_for_request instead of gateway.validate_session_for_request.

Minor notes

• The __all__ list in __init__.py is not alphabetically sorted, but follows a logical grouping which is also reasonable.
• Consider adding a py.typed marker file to shared/egg_contracts/ if you want downstream consumers to benefit from type hints.

All critical functionality looks correct. The role enforcement logic in both the library and gateway API endpoints is sound.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

Applied Changes from PR #215 Commit

Updated the contract directory path from .egg/contracts/ to .egg-state/contracts/ in shared/egg_contracts/loader.py, aligning with the design decision from PR #215 commit 9f5321f.

Rationale: This distinguishes contract instances (per-branch runtime state in .egg-state/) from the contract schema library (shared definitions in .egg/schemas/). Contract instances are committed only to feature branches, while schema files are committed to main.

All 62 tests pass.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[jwbron]

@james-in-a-box pull in main and resolve merge conflicts

[james-in-a-box]

Rebased onto main

Successfully rebased the branch onto the latest main (commit e77d544) and resolved all merge conflicts.

Conflicts Resolved (19 files)

• Gateway core: gateway.py, session_manager.py, Dockerfile
• Gateway tests: conftest.py, test_gateway.py, test_gateway_integration.py, test_git_validation.py, test_policy.py, test_token_refresher.py, test_worktree_manager.py
• GitHub workflows: on-pull-request.yml, on-pull-request-agent-mode-design.yml, test.yml
• Other: pyproject.toml, github_client.py, build-agent-mode-design-review-prompt.sh, test files

Key Integrations

• Kept session auth refactor: require_session_auth decorator now imported from auth.py module
• Preserved contract API blueprint registration in gateway.py
• Integrated main's improvements: query parameter stripping for gh API paths, workflow permissions, test coverage improvements
• All 62 egg_contracts tests pass
• All 18 gateway integration tests pass

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

egg run failed (exit code: 1). View run logs

[jwbron]

@james-in-a-box fix lint and test issues on this pr

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

egg is investigating the Test check failure...

[james-in-a-box]

Review Summary

This is a well-structured implementation of Phase 1 for structurally enforced agent checkpoints. The architecture is sound, role-based field ownership is correctly enforced, and the 62 tests provide good coverage.

However, there are blocking security issues that need to be addressed before merging.

---

Blocking Issues

1. Path Traversal Vulnerability in contract_api.py

Files: gateway/contract_api.py:121-122, gateway/contract_api.py:231

The repo_path parameter from user input is used directly without validation:

if repo_path:
    return Path(repo_path)

And:

repo_path = Path(data["repo_path"]) if data.get("repo_path") else Path.cwd()

This allows an attacker to read/write contracts anywhere on the filesystem by passing paths like ../../etc or /tmp/malicious. The gateway has validation helpers (validate_repo_path) used in other endpoints that should be applied here.

Fix: Add path validation similar to the git endpoint handlers:

from gateway import validate_repo_path  # or wherever it lives
is_valid, error = validate_repo_path(repo_path)
if not is_valid:
    return make_contract_error(error, status_code=400)

2. Role Fallback Chain Allows Privilege Escalation via Header

File: gateway/contract_api.py:86-93

The X-Egg-Role header fallback is documented as "for gateway testing only" but is still active in production. If a session has no agent_role set (which will be the case until launcher is updated), an attacker can escalate to human role by setting X-Egg-Role: human:

# Testing path: role can be passed in request header for gateway testing
# This is lower priority than session to prevent bypassing production auth
header_role = request.headers.get("X-Egg-Role")
if header_role:
    try:
        return Role(header_role.lower())

Since session agent_role is a new field added in this PR, existing sessions won't have it set, making the header fallback active.

Fix: Either:

• Remove the header fallback entirely, OR
• Gate it behind an environment variable like EGG_ENABLE_TEST_ROLE_HEADER=1

---

Non-Blocking Suggestions

3. Missing Field Ownership for files_affected

File: shared/egg_contracts/roles.py:27-44

The task field files_affected is in the schema but not in FIELD_OWNERSHIP. Per the schema, this seems like it should be owned by the implementer. Currently it defaults to SYSTEM, preventing any agent from updating it.

4. Consider Validating field_path Against Known Patterns

File: gateway/contract_api.py:225-227

Currently any field_path string is accepted. Consider validating that it matches a known pattern before hitting the underlying mutation logic. This would provide clearer error messages and prevent probing.

5. Unused phase_id and task_id Parameters

File: shared/egg_contracts/validator.py:203-225

The convenience functions validate_task_mutation and validate_phase_mutation accept phase_id and task_id but never use them. Either use them for additional validation or remove them to avoid confusion.

---

Overall, this is solid foundational work. The role-based ownership model is clean and well-tested. Addressing the two security issues above is required before merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[jwbron]

@james-in-a-box address issues in this review #217 (review)

[james-in-a-box]

Review feedback addressed

Agreed. All issues from review #3766763002 have been addressed:

Blocking Issues (Fixed)

1. Path Traversal Vulnerability - Added validate_repo_path checks to all contract API endpoints. The function is imported from git_client.py and validates paths are within ALLOWED_REPO_PATHS before use.

2. Role Header Fallback Privilege Escalation - The X-Egg-Role header is now gated behind EGG_ENABLE_TEST_ROLE_HEADER=1 environment variable. In production, sessions without agent_role will no longer accept role from headers.

Non-Blocking Suggestions (Fixed)

3. Missing Field Ownership for files_affected - Added ownership mapping for phases.*.tasks.*.files_affected and phases.*.tasks.*.files_affected.* to IMPLEMENTER role.

4. Unused parameters - Removed unused phase_id and task_id parameters from validate_task_mutation and validate_phase_mutation convenience functions, along with their corresponding test updates.

All 62 egg_contracts tests pass, linting passes.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

egg autofix encountered an issue. View run logs

[james-in-a-box]

Re-Review Summary

Reviewed the changes since commit f9e946f395c5757b6fc5e7f120b8c8e69bfa8459. All blocking issues from my previous review have been addressed.

Previous Issues - Now Resolved

1. Path Traversal Vulnerability (Fixed)

• get_repo_path_from_request() now validates all paths using validate_repo_path()
• Returns tuple (path, error) to properly propagate validation failures
• All endpoints (get_contract, mutate_contract, check_contract_exists) handle path errors correctly

2. Role Header Privilege Escalation (Fixed)

• X-Egg-Role header is now gated behind EGG_ENABLE_TEST_ROLE_HEADER=1 environment variable
• Production environments will not allow header-based role override
• Clear security comment documents the rationale

3. Missing files_affected Field Ownership (Fixed)

• Added phases.*.tasks.*.files_affected and phases.*.tasks.*.files_affected.* to FIELD_OWNERSHIP with Role.IMPLEMENTER

4. Unused Function Parameters (Fixed)

• Removed phase_id and task_id parameters from validate_task_mutation() and validate_phase_mutation()
• Updated tests accordingly

Code Quality

The fixes are clean and well-implemented:

• Path validation is applied consistently across all code paths
• Security comments are clear and explain the reasoning
• No regressions introduced in the fix

No blocking issues remain. Ready to merge once CI passes.

— Authored by egg

[james-in-a-box]

egg is investigating the Test check failure...

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

egg autofix completed. View run logs

[james-in-a-box]

egg is investigating the Test check failure...

[james-in-a-box]

Re-Review Summary

Reviewed the changes since commit a5b2d4124796a2e8deebdc5ef467d19abf3e64ce (my previous review). This commit adds test isolation fixes for the auth module.

Changes Reviewed

gateway/auth.py:38-61, 64-87 - Modified lazy import functions to always check sys.modules first before using cached references. This ensures test patches work correctly across different test execution orders.

gateway/tests/conftest.py:81-82, 194-196 - Registers loaded modules under both naming conventions (e.g., session_manager and gateway.session_manager) and resets auth module's cached references after loading.

gateway/tests/test_gateway.py:53-95, 864-893 - Updated auth_headers and private_mode_auth_headers fixtures to:

1. Clear auth module's cached references before each test
2. Patch the module currently in sys.modules rather than the module reference captured at import time
3. Handle both direct and package-style module references

Assessment

The changes correctly address the test isolation issue. The root cause was that when tests ran in different orders (e.g., tests/security/ before gateway/tests/), the auth module would cache the wrong session_manager reference, causing test fixtures to patch the wrong module instance.

The fix is appropriate:

• Checking sys.modules first ensures the auth module always uses the module that tests have patched
• Checking both naming conventions (session_manager and gateway.session_manager) handles the different import paths used in production vs test contexts
• Clearing cached references before patching prevents stale references

Note on CI Failure

The unit tests are failing due to code coverage dropping from 80% to 79%. All 2690 tests pass - this is a coverage threshold issue, not a test failure. The contract_api.py module has only 21% coverage and auth.py has 66% coverage, which are dragging down the total. This is a pre-existing gap unrelated to the test isolation fix in this commit.

Recommendation: Add integration tests for the contract API endpoints to bring coverage above the 80% threshold.

Verdict

No blocking issues in the new changes. The test isolation fix is correct and well-implemented. Ready to merge once coverage is addressed.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

Re-Review Summary

Reviewed the changes since commit 7628e25 (my previous review). This commit adds comprehensive test coverage for the contract API, loader, and audit modules.

Changes Reviewed

New Test Files (3 files, 1488 lines added):

1. gateway/tests/test_contract_api.py (656 lines)

   • Tests for get_role_from_context() covering all role resolution paths
   • Tests for all contract API endpoints: GET, POST validate, POST mutate, contract existence
   • Proper authentication fixture setup that handles module patching correctly
   • Edge cases: missing body, missing fields, invalid roles, contract not found

2. tests/shared/egg_contracts/test_audit.py (420 lines)

   • Tests for create_audit_entry(), create_update_entry(), create_transition_entry()
   • Tests for format_audit_log() with various entry types and limit parameter
   • Coverage for all AuditRole and AuditAction enum values

3. tests/shared/egg_contracts/test_loader.py (412 lines)

   • Tests for contract CRUD operations: load, save, create, delete, list
   • Tests for load_contract_from_branch() git integration
   • Tests for error handling: ContractNotFoundError, ContractValidationError
   • Tests for atomic write behavior and cleanup on failure

Assessment

Test Quality:

• Tests are well-organized with clear class groupings
• Good coverage of happy paths and error cases
• Proper use of fixtures and mocking
• Tests verify both behavior and error messages

Coverage Impact:

• These tests address the coverage gap noted in my previous review
• The contract_api.py module previously had 21% coverage; these tests significantly improve that

Minor Observations (non-blocking):

1. test_contract_api.py:65-76 - The auth fixture clears auth._session_manager and auth._rate_limiter directly. This works but couples tests to auth module internals. Consider adding a reset_cached_modules() helper to auth.py if this pattern is needed elsewhere.

2. test_loader.py:91 - The test_load_invalid_json_raises test uses {not valid json!!! which is a bit unusual syntax (though valid for testing invalid JSON). A malformed JSON like {"unclosed": might be more representative of real errors.

CI Status

All checks passing:

• Integration Tests: SUCCESS
• All Linters: SUCCESS
• Unit Tests: SUCCESS
• Security Scan: SUCCESS

Verdict

No blocking issues. The test additions are solid and address the coverage gap identified in my previous review. Ready to merge.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

egg autofix completed. View run logs