Implement Phase 6: Integration tests and documentation

[james-in-a-box]

Summary

Add SDLC pipeline integration tests and documentation for issue #133. This completes Phase 6 of the implementation plan.

Integration tests created:

• test_happy_path.py - Full pipeline success scenario (refine → plan → implement → pr)
• test_review_rejection.py - Reviewer rejects tasks, implementer addresses feedback
• test_circuit_breaker.py - Escalation triggers correctly when thresholds exceeded
• test_hitl_flow.py - Human decision pauses and resumes pipeline
• test_role_enforcement.py - Gateway blocks unauthorized mutations

Documentation:

• Added docs/adr/in-progress/ADR-SDLC-Pipeline.md documenting the architecture
• Updated docs/index.md with links to the new ADR

All 96 tests pass.

Issue: #133

Test plan:

• Run pytest integration_tests/sdlc/ -v to verify all tests pass
• Review ADR for completeness and accuracy
• Verify docs/index.md links work correctly

Authored-by: egg

[james-in-a-box]

egg is investigating the Lint check failure...

[james-in-a-box]

Code Review: Phase 6 Integration Tests and Documentation

Summary

This PR adds integration tests for the SDLC pipeline and accompanying ADR documentation. While the test coverage is comprehensive, there are several issues that need to be addressed before merge.

---

Critical Issues

1. Test isolation: sys.path manipulation is fragile (test_*.py)

All test files manipulate sys.path directly:

_shared_path = Path(__file__).parent.parent.parent / "shared"
if str(_shared_path) not in sys.path:
    sys.path.insert(0, str(_shared_path))

Problem: This pattern is fragile and can cause import collisions in parallel test runs. It also makes the tests non-portable if directory structure changes.

Fix: Add a conftest.py to integration_tests/sdlc/ that handles path setup once, or configure pyproject.toml/setup.cfg with proper test dependencies.

---

2. Mutable dataclass fields in HitlCheckboxState (test_hitl_flow.py:1824)

The test directly mutates debounce_until on the state object:

state.debounce_until = datetime.now(UTC) - timedelta(seconds=10)

Problem: HitlCheckboxState is a @dataclass and this direct mutation bypasses any validation or business logic. The test should use start_debounce() or create a new state object to match how production code would work.

Recommendation: Consider making HitlCheckboxState frozen (frozen=True) or providing a method to update debounce state. Tests should use production APIs where possible.

---

Medium Priority Issues

3. Missing test for close_circuit_breaker without role parameter (test_circuit_breaker.py:838-843)

updated = close_circuit_breaker(
    contract,
    actor="reviewer",
    reason="Provided guidance",
)

The role parameter is omitted, defaulting to AuditRole.HUMAN. This is inconsistent with test_close_circuit_breaker at line 712-717 which explicitly passes role=AuditRole.HUMAN.

Fix: Either always pass the role explicitly for clarity, or add a comment explaining the default behavior.

---

4. Test assertions may pass with wrong data (test_role_enforcement.py:2477)

assert "not authorized" in result.message.lower() or "permission" in result.message.lower()

Problem: This assertion is too permissive. If the error message format changes to something like "access denied", the test would fail unexpectedly. The test should check for the exact field in the result (result.required_role) rather than parsing error messages.

Recommendation:

assert result.success is False
assert result.required_role == Role.REVIEWER.value

---

5. Inconsistent fixture usage (test_happy_path.py vs test_circuit_breaker.py)

test_happy_path.py uses sample_issue_info with number=133:

number=133,
title="Add structurally enforced checkpoints",

While test_circuit_breaker.py uses number=300, test_review_rejection.py uses number=200, etc.

Problem: Each test file uses different issue numbers, but the fixture names are the same (sample_issue_info). If tests are ever combined or run in ways that share fixtures, this could cause confusion.

Recommendation: Document the issue number convention in the module docstring or use more descriptive fixture names.

---

6. ADR diagram mismatch (ADR-SDLC-Pipeline.md:69-73)

The diagram shows:

│  │   Refine    │───▶│    Plan     │───▶│  Implement  │───▶│    PR    │ │
│  │  (Human)    │    │  (Human)    │    │ (Reviewer)  │    │ (Human)  │ │

But the role ownership in roles.py shows current_phase is owned by REVIEWER, not HUMAN. The diagram labels are confusing - they seem to indicate who can exit the phase, not who operates within it.

Fix: Clarify the diagram labels. Perhaps use "Exit requires: Human" or similar phrasing.

---

7. Missing negative test cases

Several scenarios are not tested:

1. Concurrent modifications: What happens if two mutations happen simultaneously?
2. Invalid field paths: Tests for malformed paths like phases.-1.tasks.0.status
3. Contract version mismatch: What if schemaVersion doesn't match expected?
4. Audit log overflow: Large audit logs aren't tested

---

Minor Issues

8. Import order inconsistency (test_review_rejection.py:1857-1858)

from datetime import datetime, UTC

Should be:

from datetime import UTC, datetime

To match the import order in other test files (e.g., test_hitl_flow.py:1332). This is a minor style consistency issue.

---

9. ADR missing implementation status for E2E testing (ADR-SDLC-Pipeline.md:369)

The "Remaining" section shows:

- [ ] E2E testing with real agent execution
- [ ] Documentation updates

But this PR claims to complete Phase 6. If E2E testing is out of scope for this phase, that should be clarified in the PR description.

---

10. Test docstrings inconsistency

Some test methods have detailed docstrings, others don't. For example:

• test_task_under_threshold has: """Task under threshold does not trigger escalation."""
• But some class docstrings are very brief

This is a minor consistency issue but affects documentation quality.

---

ADR Documentation Review

The ADR is well-structured and comprehensive. A few suggestions:

1. Security properties section (lines 323-330): Consider adding a property about rate limiting protection for the contract API endpoints.

2. Threat model (lines 44-57): The "Explicit Non-Goals" section is valuable. Consider adding that this architecture doesn't protect against time-of-check-time-of-use (TOCTOU) vulnerabilities if contracts are modified between load and save.

3. References section: The links to ../../issues/133-*.md files should be verified to ensure they exist.

---

Verdict

The tests provide good coverage for the SDLC pipeline functionality, but there are code quality issues that should be addressed:

1. Fix the sys.path manipulation pattern - use a shared conftest.py for path setup
2. Address the mutable state mutation in tests - create new state objects instead of mutating
3. Clarify ADR diagram labels - indicate whether labels show phase operator or exit approver

These are not blocking issues, but should be addressed to maintain code quality standards.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[james-in-a-box]

egg autofix completed. View run logs

[james-in-a-box]

Re-review: Commit aecd196

Summary

This commit addresses lint issues (unused imports, import order, formatting). However, the substantive issues from the previous review remain unaddressed.

---

Previous Feedback Status

Issue	Status	Notes
1. sys.path manipulation pattern	❌ NOT FIXED	Still present in all 5 test files
2. Mutable dataclass mutation	❌ NOT FIXED	state.debounce_until = datetime.now(UTC)... still present
3. Missing role parameter on close_circuit_breaker	❌ NOT FIXED	Multiple calls still omit explicit role
4. Overly permissive assertion	❌ NOT FIXED	String parsing assertion still used
5. Inconsistent fixture issue numbers	❌ NOT FIXED	No change
6. ADR diagram mismatch	❌ NOT FIXED	No change
7. Missing negative test cases	❌ NOT FIXED	No new tests added
8. Import order	✅ Fixed	from datetime import UTC, datetime
9. ADR implementation status	❌ NOT FIXED	Still claims E2E is remaining
10. Test docstrings	—	Minor, no action needed

---

Issues Requiring Action

1. sys.path manipulation in every test file (Blocking)

All 5 test files contain:

_shared_path = Path(__file__).parent.parent.parent / "shared"
if str(_shared_path) not in sys.path:
    sys.path.insert(0, str(_shared_path))

Problem: This is fragile and can cause import collisions in parallel test execution.

Required fix: Create integration_tests/sdlc/conftest.py with a single path setup, or configure pyproject.toml with proper package paths.

---

2. Missing role parameter on close_circuit_breaker (Medium)

Lines 728, 829, 1773, 1818 call close_circuit_breaker without explicit role:

updated = close_circuit_breaker(
    contract,
    actor="reviewer",
    reason="Provided guidance",
)  # role defaults to AuditRole.HUMAN

While line 706 correctly passes role=AuditRole.HUMAN.

Required fix: Be consistent—either always pass the role explicitly, or document the default behavior in a comment.

---

3. Overly permissive error message assertion (Medium)

Line 2454:

assert "not authorized" in result.message.lower() or "permission" in result.message.lower()

Problem: Fragile string matching that will break if error messages change.

Required fix: Assert on structured error fields like result.required_role instead of parsing messages.

---

New Commit Analysis

The new commit (aecd196) only contains:

1. Removed unused imports (MagicMock, patch, AuditRole in test_happy_path.py, create_contract)
2. Fixed import order in test_review_rejection.py
3. Formatting changes (function arguments on single lines per ruff)

These are purely cosmetic lint fixes and do not address any of the substantive code quality issues raised in the previous review.

---

Verdict

Requesting changes. The lint fixes are welcome, but the previous review raised substantive issues about code quality and test robustness that have not been addressed:

1. The sys.path manipulation should be centralized in conftest.py
2. Function calls should be consistent about explicit parameters
3. Assertions should test structured data, not parse error strings

Please address issues 1-3 before this PR can be approved.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[jwbron]

@james-in-a-box pull main into this branch and address the above feedback

[james-in-a-box]

Merged main and addressed the code review feedback:

Issue	Status
1. sys.path manipulation in every test file	✅ Fixed - Created integration_tests/sdlc/conftest.py to centralize path setup
2. Missing role parameter on close_circuit_breaker	✅ Fixed - Added explicit role=AuditRole.HUMAN to all calls
3. Overly permissive error message assertion	✅ Fixed - Now uses validate_mutation to check required_role directly

All 96 tests pass.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Re-review: Commit ca5e964

Summary

The requested fixes from the previous re-review have been properly addressed.

---

Previous Feedback Status

Issue	Status	Verification
1. sys.path manipulation in every test file	✅ Fixed	Created integration_tests/sdlc/conftest.py; removed from all 5 test files
2. Missing role parameter on close_circuit_breaker	✅ Fixed	All 5 calls now use explicit role=AuditRole.HUMAN
3. Overly permissive error message assertion	✅ Fixed	Now uses validate_mutation() to check result.required_role directly

---

New Commit Analysis (ca5e964)

The commit correctly implements:

1. Centralized path setup: conftest.py handles sys.path manipulation once for all SDLC tests. This is the standard pytest approach for test-specific imports.

2. Explicit role parameters: All close_circuit_breaker calls now pass role=AuditRole.HUMAN explicitly, making the code self-documenting.

3. Structured error validation: The test at test_role_enforcement.py:224-231 now uses:

   validation = validate_mutation(
       role=Role.IMPLEMENTER,
       field_path="phases.0.tasks.0.status",
       new_value=TaskStatus.COMPLETE.value,
   )
   assert validation.valid is False
   assert validation.required_role == Role.REVIEWER.value

   This is robust against error message format changes.

---

Remaining Advisory Notes

The following items from the original review were documented as non-blocking. They remain unchanged but are acceptable:

1. Mutable dataclass mutation in tests (test_hitl_flow.py:500): The test directly sets state.debounce_until to simulate time passing. This is pragmatic for testing time-dependent behavior without introducing sleep delays.

2. ADR diagram labels: The diagram shows (Human) / (Reviewer) labels which indicate the exit approver. The table at lines 112-116 clarifies this. Documentation could be clearer but is not misleading.

3. Missing negative test cases: Concurrent modifications, invalid field paths, and schema version mismatches are not tested. These would be valuable additions but are not blocking for this PR.

---

Verdict

LGTM. All requested changes have been properly implemented. The tests are well-structured, use consistent patterns, and properly verify the SDLC pipeline behavior.

No blocking issues remain.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[jwbron]

@james-in-a-box pull the document here https://github.com/jwbron/egg/blob/466883513ed0565ca3facd9ef6927de4732c1281/docs/guides/sdlc-pipeline.md into this pr and reconcile any differences or overlaps with the ADR you've put together. Also ensure that the ADR is marked as implemented, which will be the state after this PR is merged.

[james-in-a-box]

Re-review: Commit 74475c8

Summary

This re-review confirms that all previously identified issues remain addressed after the branch rebase. The PR is ready to merge.

---

Verification

The three blocking issues from the original review are confirmed fixed:

Issue	Status	Verification
1. sys.path centralized in conftest.py	✅ Fixed	integration_tests/sdlc/conftest.py handles path setup once
2. Explicit role parameter on close_circuit_breaker	✅ Fixed	All 5 calls use role=AuditRole.HUMAN
3. Structured error validation	✅ Fixed	test_role_enforcement.py:222-231 uses validate_mutation() with required_role check

---

Code Quality Assessment

The integration tests are well-structured:

• Test coverage: Happy path, review rejection, circuit breaker, HITL flow, and role enforcement scenarios
• Consistent patterns: All tests use pytest fixtures and follow the same structure
• Clear assertions: Tests verify specific behaviors rather than implementation details
• Proper isolation: Each test creates a temporary repo directory

The ADR documentation clearly explains the SDLC pipeline architecture with diagrams, threat model, and implementation status.

---

No New Issues

The rebase did not introduce any new code changes beyond syncing with main. The PR files are identical to the previously approved version.

---

LGTM. No blocking issues. Ready to merge.

— Authored by egg

[james-in-a-box]

Pulled the SDLC pipeline guide from main and reconciled it with the ADR:

Change	Details
Added docs/guides/sdlc-pipeline.md	Operational guide with CLI commands, triggering instructions, and implementation details
Updated ADR status	Changed from "In Progress" to "Implemented"
Moved ADR	From docs/adr/in-progress/ to docs/adr/implemented/
Added cross-references	Guide links to ADR for threat model; ADR links to guide for operational details
Updated docs/index.md	Added guide to Guides section, updated task-specific guides
Updated docs/adr/README.md	Added SDLC Pipeline to Implemented section

The guide contains operational content (CLI commands, triggering, document standards) while the ADR focuses on architecture (threat model, security properties, design decisions). Both now cross-reference each other.

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Re-review: Commit 5fb7d21

Summary

This re-review found a critical issue that must be fixed before merge: merge conflict markers were committed to the guide file.

---

Critical Issue

Merge conflict markers in docs/guides/sdlc-pipeline.md (lines 385-389)

<<<<<<< HEAD
*See also: [Analysis Template](../templates/analysis.md), [Plan Template](../templates/plan.md), [GitHub Automation](github-automation.md)*
=======
*See also: [ADR: SDLC Pipeline](../adr/implemented/ADR-SDLC-Pipeline.md), [Analysis Template](../templates/analysis.md), [Plan Template](../templates/plan.md), [GitHub Automation](github-automation.md)*
>>>>>>> 0975011 (Add SDLC pipeline operational guide and mark ADR as implemented)

Fix required: Resolve the conflict by keeping the second version (with the ADR link) and remove the conflict markers.

---

Previous Feedback Status

Issue	Status	Notes
1. sys.path centralized in conftest.py	✅ Fixed	integration_tests/sdlc/conftest.py handles path setup
2. Explicit role parameter on close_circuit_breaker	✅ Fixed	All 5 calls use role=AuditRole.HUMAN
3. Structured error validation	✅ Fixed	test_role_enforcement.py:225-231 uses validate_mutation() with required_role

---

New Commit Analysis

The commit correctly:

1. Added docs/guides/sdlc-pipeline.md operational guide
2. Updated ADR status from "In Progress" to "Implemented"
3. Moved ADR from in-progress/ to implemented/
4. Added cross-references between ADR and guide
5. Updated docs/index.md and docs/adr/README.md

However, a merge conflict was not properly resolved before committing.

---

Minor Advisory Notes (Non-blocking)

Broken references in ADR

docs/adr/implemented/ADR-SDLC-Pipeline.md lines 369-370 reference:

- [Specification Document](../../issues/133-structurally-enforced-checkpoints.md)
- [Implementation Plan](../../issues/133-implementation-plan.md)

The docs/issues/ directory does not exist. Consider removing these links or adding the referenced files.

---

Verdict

Requesting changes. The merge conflict markers must be resolved before merge. This is a straightforward fix:

Replace lines 385-390 with:

*See also: [ADR: SDLC Pipeline](../adr/implemented/ADR-SDLC-Pipeline.md), [Analysis Template](../templates/analysis.md), [Plan Template](../templates/plan.md), [GitHub Automation](github-automation.md)*

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

[jwbron]

@james-in-a-box fix the issues covered here #265 (review)

[james-in-a-box]

Fixed the issues from the re-review:

Issue	Fix
Merge conflict markers in docs/guides/sdlc-pipeline.md	✅ Resolved - kept the version with the ADR link
Broken references in ADR (docs/issues/ doesn't exist)	✅ Replaced with link to the operational guide

— Authored by egg

[james-in-a-box]

egg finished successfully. View run logs

[james-in-a-box]

Re-review: Commit 9ffa33c

Summary

All issues from the previous review have been properly addressed. The PR is ready to merge.

---

Verification

Issue	Status	Verification
1. Merge conflict markers in docs/guides/sdlc-pipeline.md	✅ Fixed	File now ends cleanly with ADR link at line 385
2. Broken references in ADR (docs/issues/ doesn't exist)	✅ Fixed	References replaced with SDLC Pipeline Operational Guide

---

Previous Fixes Confirmed Still in Place

Issue	Status
sys.path centralized in conftest.py	✅ Verified - only in conftest.py, not test files
Explicit role parameter on close_circuit_breaker	✅ Verified - all 5 calls use role=AuditRole.HUMAN
Structured error validation	✅ Verified - test_role_enforcement.py:225-231 uses validate_mutation()

---

Documentation Cross-References

All links verified functional:

• docs/index.md correctly links to both ADR and operational guide
• docs/adr/README.md lists SDLC Pipeline in Implemented section
• ADR links to operational guide for CLI commands
• Operational guide links to ADR for threat model

---

LGTM. All blocking issues from previous reviews have been resolved. No blocking issues remain.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs