feat(verification): Phase A + Phase B soft rollout — Marcus authors verifications, gate prefers them, falls back to agent (#636)

[lwgray]

Bundle of Phase A + Phase B for issue #636. Implements
Invariant #2 v2 (CLAUDE.md MULTIAGENCY_PROCLAMATION, PR #639):
verification command authorship moves from the integration agent
to Marcus's setup-time pipeline; the runtime smoke gate prefers
contract-authored commands and falls back to agent-supplied on
absence, empty, incomplete coverage, or runtime failure.

What this PR does, in plain English

Marcus today builds software by spawning AI agents in parallel.
After they all finish, an integration-verifier agent confirms the
system works. Until now the agent chose what verification commands
to run — and because the agent has retry pressure to keep weakening
commands until exit 0, they ended up shipping commands that
technically passed without actually testing the user-facing
behavior. Project test58 (purple-dot-snake-game) shipped unplayable
because the agent's verification for the "keyboard input" outcome
never simulated a keypress.

This PR moves verification authorship to Marcus. At project setup
(Phase A), Marcus generates one shell command per in-scope user
outcome via an LLM call. At completion (Phase B), Marcus's smoke
gate prefers those contract-authored commands over the agent's.

Soft rollout: if Marcus's command is wrong (absent, broken,
incomplete coverage), the gate falls back to the agent's verifications.
Each fallback fires a WARNING log so operators can iterate on the
generator prompt over time. Once the contract path is empirically
reliable, a future PR will promote to a hard cutover.

Why soft rollout, not hard cutover

The hard-cutover plan was "contract present -> trust absolutely."
Pre-merge Kaia review pushed back: without #643's scaffold-stack
enforcement, Phase A's LLM-generated commands will reference
unavailable tools, wrong paths, vacuous checks. A buggy contract
under hard cutover blocks ALL completions; under soft rollout it
loses to the agent-supplied fallback with a WARNING that lets us
iterate.

Honest trade-off: Phase B does NOT eliminate the gameability that
motivated #636. It makes the gameability the FALLBACK instead of
the default. As contract reliability grows (via #643), the fallback
fires less.

What changed

File	Change
src/ai/advanced/prd/verification_command_generator.py (new)	ContractVerification typed dataclass + per-outcome LLM generator + parallelized fan-out via asyncio.gather(return_exceptions=True) for per-outcome isolation
src/integrations/integration_verification.py	create_integration_task + enhance_project_with_integration accept contract_verifications parameter; stamps source_context["contract_verifications"] (preserves empty list per Codex P2)
src/integrations/nlp_tools.py (create_project_from_description)	Fans out the generator on in-scope outcomes before creating the integration task; wraps in try/except so generation failure never blocks project creation; WARNING log when generated count is 0
src/marcus_mcp/tools/task.py::_run_product_smoke_gate	NEW: contract-first try-then-fall-through block BEFORE the existing Slice B escape-hatch. Each fallback case fires WARNING.
tests/unit/ai/test_verification_command_generator.py (new)	21 tests: dataclass round-trip, single-outcome generator, fan-out isolation under partial failure
tests/unit/integrations/test_integration_verification.py	4 new tests: contract_verifications parameter (absent / None / empty list / populated)
tests/unit/marcus_mcp/test_integration_smoke_gate.py	TestPhaseBSoftRollout class with 6 tests covering all fallback modes

Commit chain

Commit	What
302a5809	Phase A: generator module + integration-task parameter + caller wiring + 21 tests
87d7fed1	Codex P2 fix: preserve [] (distinct from None) on source_context
7c44b187	Kaia P2-2 + P3: asyncio.gather(return_exceptions=True) isolation + WARNING-level log on empty result
ba1fde95	Phase B: smoke-gate contract-first soft rollout + 6 new tests

How the contract flows (worked example)

Project test58 style — 6 in-scope outcomes:

1. Phase A runs at project creation:

   asyncio.gather(
       generate_verification_command(outcome_play_snake, ...),
       generate_verification_command(outcome_control_snake, ...),
       ... 6 in parallel ...
   )
   

   N LLM calls, ~5s wall-clock total. Maybe 4 succeed, 1 LLM returns
   null (tech stack can't verify that outcome), 1 raises. Result: 4
   ContractVerification objects.

2. Integration task carries source_context["contract_verifications"]
   with those 4 dicts (Codex P2 preservation: even if the list were
   empty, it round-trips intact).

3. Phase B at agent completion:

   • Gate reads source_context["contract_verifications"] — 4 entries
   • Coverage check: 6 in_scope outcomes, only 4 covered -> incomplete
   • WARNING: "contract has incomplete coverage for task X — missing
     2 in-scope outcome(s)"
   • Fall through to agent-supplied path (existing Slice B behavior)
   • If agent supplied verifications and they pass: accept (fallback)
   • If agent didn't supply: existing rejection fires

In the happy-path case (all 6 LLM calls succeed AND all 6 commands
pass at runtime), the agent's path is never consulted — completion
is accepted via contract.

Glossary

Term	Meaning
Phase A	Marcus's setup-time pipeline. Adds generate_verification_command LLM call per in-scope UserOutcome.
Phase B	Runtime completion gate. Reads contract verifications; falls back to agent-supplied on failure.
UserOutcome	User-visible capability the spec promises (id, action, success_signal, scope). Extracted by outcome_extractor.py.
ContractVerification	New dataclass added in this PR. Pairs signal_id (UserOutcome.id) with the shell command Marcus runs.
Soft rollout	Contract preferred at runtime; agent-supplied is fallback. As opposed to hard cutover (contract absolute, agent dead).
Contract-net protocol	1980s MAS pattern: manager specifies WHAT + HOW-TO-VERIFY; bidder implements. Preserves bidder autonomy on implementation.

How to verify it works

Unit tests

python -m pytest tests/unit/ai/test_verification_command_generator.py tests/unit/integrations/test_integration_verification.py tests/unit/marcus_mcp/test_integration_smoke_gate.py -v

Expected: all pass. Currently 21 + 4 + 33 = 58 tests in scope, all passing.

Production smoke

1. Run a /marcus project end-to-end on this branch
2. After project creation, inspect the kanban for the integration
   task's source_context["contract_verifications"]
3. Expected: the field is populated with one dict per in-scope
   outcome the LLM didn't decline
4. Wait for the integration agent to complete the run
5. Grep the Marcus log for PRODUCT SMOKE GATE:
   • Happy path: "Running N contract-authored verification(s)..."
     followed by "All N contract verification(s) passed for task X"
   • Fallback path: "contract verification FAILED" or
     "contract has incomplete coverage" WARNING, followed by the
     existing Slice B agent-supplied flow
6. The first batch of real projects will reveal how reliable the
   generator is — fallback frequency tells us how much bug: verification-command generator infers tech stack from prose, not the scaffold's actual dependency manifest #643's
   scaffold-stack work is needed.

Test plan

• CI green (unit, internal integration, pre-commit, claude-review)
• All 58 in-scope tests pass
• No regressions in broader unit suite
• mypy clean for all 4 changed source files
• Production smoke: a real /marcus project shows contract verifications
  on the integration task's source_context
• Production smoke: Marcus log shows either "contract passed" or
  "fall back to agent-supplied" with WARNING — never a silent
  regression

Related

• Issue bug: composition+verify pipeline ships unplayable apps when spec-coverage gap-fill creates redundant tasks (orphan source trees pass smoke gate) #636 — umbrella
• PR docs(invariants): Invariant #2 v2 — verification belongs to Marcus, not agents #639 — Invariant Task Assigned 4 times #2 v2 doc landed first
• bug: verification-command generator infers tech stack from prose, not the scaffold's actual dependency manifest #643 — scaffold-stack constraint follow-up (will reduce fallback frequency)
• feat(observability): Cato cost-tracking — add 'verification' group for Phase A verification-command generation #644 — Cato cost-tracking: verification group
• bug: Epictetus post-run interrogation broken by ephemeral lifecycle — refactor from tmux pane capture to per-agent-kind transcript reading (Claude / Codex / Gemini) #645 — Epictetus multi-agent transcript refactor
• Simon decisions: b8599a52 (philosophy), thoughts 77ade35a + a4219ac9 (Kaia reviews)

🤖 Generated with Claude Code

[claude]

Claude finished @lwgray's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather() for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls

Potential Issues & Recommendations

Minor Issues:

1. Broad Exception Handling (line 1706 in nlp_tools.py):

   except Exception as gen_err:  # noqa: BLE001

   Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. LLM Response Validation: While JSON validation is comprehensive, consider adding basic sanity checks on generated commands (e.g., detecting obviously malformed shell syntax).

Integration Considerations:

3. Error Context: The verification generator doesn't use the Marcus Error Framework. Consider whether KanbanIntegrationError or similar would provide better error tracking for Phase B integration.

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 378 LOC of tests for 334 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context

Test Quality Examples:

def test_rejects_malformed_llm_json(self):
    """Garbage from the LLM raises ValueError with outcome id in message."""
    llm = _stub_llm("not json at all")
    with pytest.raises(ValueError, match="outcome_play"):
        await generate_verification_command(
            _outcome(), project_description="x", llm_client=llm
        )

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Error Framework: Uses appropriate error handling patterns (though could leverage Marcus Error Framework more)

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 302a580984

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve empty contract verification lists

Store contract_verifications whenever generation ran, even when it produced an empty list. With the current truthy check, [] is dropped and source_context["contract_verifications"] is omitted, which makes the task indistinguishable from “generation never happened” and prevents downstream coverage/gap logic from detecting that in-scope outcomes had no contract-authored commands.

Useful? React with 👍 / 👎.

[lwgray]

Codex P2 fixed in 87d7fed1

You're right — the truthy check dropped empty lists. Changed to is not None so empty lists round-trip intact. Pattern now mirrors the existing in_scope_outcome_ids stamping just above, which already uses if outcomes is not None.

The semantic distinction matters for Phase B:

Value	Meaning	Phase B behavior
None (field absent)	Generation never happened — caller had no outcomes, no LLM engine, or wrapped in try/except that caught a failure	Fall back to legacy agent-authored path
[] (field present, empty)	Generation ran; LLM declined to author commands for every in-scope outcome (tech stack lacks verification tooling)	Surface as a gap, don't silently fall back
[...] (field present, populated)	Normal case	Run the contract commands

Added 4 unit tests covering all three states + the parameter-not-passed default. All 66 tests in test_integration_verification.py pass.

[claude]

Claude finished @lwgray's task —— View job

---

PR Review Complete ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather() for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls

Potential Issues & Recommendations

Minor Issues:

1. Broad Exception Handling (line 1706 in nlp_tools.py):

   except Exception as gen_err:  # noqa: BLE001

   Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. LLM Response Validation: While JSON validation is comprehensive, consider adding basic sanity checks on generated commands (e.g., detecting obviously malformed shell syntax).

Integration Considerations:

3. Error Context: The verification generator doesn't use the Marcus Error Framework. Consider whether KanbanIntegrationError or similar would provide better error tracking for Phase B integration.

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 378 LOC of tests for 334 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context

Test Quality Examples:

def test_rejects_malformed_llm_json(self):
    """Garbage from the LLM raises ValueError with outcome id in message."""
    llm = _stub_llm("not json at all")
    with pytest.raises(ValueError, match="outcome_play"):
        await generate_verification_command(
            _outcome(), project_description="x", llm_client=llm
        )

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Error Framework: Uses appropriate error handling patterns (though could leverage Marcus Error Framework more)

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

[lwgray]

Kaia P2-2 + P3 fixes landed in 7c44b187

P2-2 — asyncio.gather poison-the-well

Switched generate_verification_commands to
asyncio.gather(*coroutines, return_exceptions=True) and iterate
results. Per-outcome failures now log a WARNING with the outcome id
and drop just that one; the remaining N-1 ship.

Before: 1 bad LLM response out of 6 = lose all 6 verifications (~11% per project at typical failure rates).
After: 1 bad LLM response out of 6 = lose 1 verification; 5 still ship.

Test flipped: test_partial_llm_failure_propagates_via_gather →
test_partial_llm_failure_is_isolated_per_outcome. Asserts the
good outcome survives, the bad one is dropped, and a WARNING log line
names the failing outcome's id.

P3 — Empty-list log → WARNING

logger.info("Generated 0 contract verification(s) for N in-scope outcome(s)") was buried in noise. Now WARNING when count == 0.

All 87 in-scope tests pass; mypy clean.

P2-1 (M3 enforcement) — filed as follow-up

Filed separately as #643 — not blocking this PR. The fix needs scaffold-manifest parsing which is non-trivial integration with the scaffold-generator path. Soft M3 (prompt-only constraint) is strictly better than no M3, so we ship this and harden in a follow-up.

[claude]

Claude finished @lwgray's task —— View job

---

PR Review Complete ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather() for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls

Potential Issues & Recommendations

Minor Issues:

1. Broad Exception Handling (line 1714 in nlp_tools.py):

   except Exception as gen_err:  # noqa: BLE001

   Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. LLM Response Validation: While JSON validation is comprehensive, consider adding basic sanity checks on generated commands (e.g., detecting obviously malformed shell syntax).

Integration Considerations:

3. Error Context: The verification generator doesn't use the Marcus Error Framework. Consider whether KanbanIntegrationError or similar would provide better error tracking for Phase B integration.

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 390 LOC of tests for 363 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context

Test Quality Examples:

def test_rejects_malformed_llm_json(self):
    """Garbage from the LLM raises ValueError with outcome id in message."""
    llm = _stub_llm("not json at all")
    with pytest.raises(ValueError, match="outcome_play"):
        await generate_verification_command(
            _outcome(), project_description="x", llm_client=llm
        )

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Error Framework: Uses appropriate error handling patterns (though could leverage Marcus Error Framework more)

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

[claude]

Claude finished @lwgray's task —— View job

---

PR Review Complete ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather() for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls

Potential Issues & Recommendations

Minor Issues:

1. Broad Exception Handling (line 1714 in nlp_tools.py):

   except Exception as gen_err:  # noqa: BLE001

   Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. LLM Response Validation: While JSON validation is comprehensive, consider adding basic sanity checks on generated commands (e.g., detecting obviously malformed shell syntax).

Integration Considerations:

3. Error Context: The verification generator doesn't use the Marcus Error Framework. Consider whether KanbanIntegrationError or similar would provide better error tracking for Phase B integration.

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 390 LOC of tests for 362 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context

Test Quality Examples:

def test_rejects_malformed_llm_json(self):
    """Garbage from the LLM raises ValueError with outcome id in message."""
    llm = _stub_llm("not json at all")
    with pytest.raises(ValueError, match="outcome_play"):
        await generate_verification_command(
            _outcome(), project_description="x", llm_client=llm
        )

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Error Framework: Uses appropriate error handling patterns (though could leverage Marcus Error Framework more)

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

---

[lwgray]

@codex

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ba1fde9584

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Skip contract verification generation when integration is disabled

generate_verification_commands(...) runs before enhance_project_with_integration(...), but that helper internally calls should_add_integration_task and can early-return for demo/POC/test-style projects. In those cases we still pay for one LLM call per in-scope outcome even though no integration task is created and the generated commands are discarded, which adds avoidable latency/cost and noisy warning logs on project creation. Gate this generation behind the same integration-task eligibility check (or move it into the integration enhancer after eligibility is confirmed).

Useful? React with 👍 / 👎.

[lwgray]

Codex P2 fixed in 23ba51c9

You're right — enhance_project_with_integration calls should_add_integration_task and early-returns for demo / POC / prototype projects. Pre-fix, the verification generator ran BEFORE that check, spending N LLM calls per outcome on projects where the integration task is never created and the output is discarded.

Cost shape of the bug:

• ~$0.005 per LLM call × N outcomes per project = ~$0.03 wasted per demo/POC
• ~5s wasted Phase A latency (parallelized but still wall-clock)
• Noisy WARNING logs about results about to be thrown away

Fix: gate the generator behind IntegrationTaskGenerator.should_add_integration_task(description). When ineligible, log INFO and skip entirely.

Considered moving the generator INTO enhance_project_with_integration after the eligibility check (cleaner architecturally), but that helper is sync; making it async to host an async call would force a wider API change for every other caller. The same-predicate-at-the-gate pattern keeps the change minimal.

131 in-scope tests pass; mypy clean.

[claude]

Claude finished @lwgray's task —— View job

---

Comprehensive PR Review Complete ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Review Checklist

• Analyze core implementation files
• Review test coverage and quality
• Check architectural alignment with Marcus patterns
• Assess security considerations
• Evaluate performance implications
• Provide final recommendations

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context explaining issue bug: composition+verify pipeline ships unplayable apps when spec-coverage gap-fill creates redundant tasks (orphan source trees pass smoke gate) #636 motivation
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather(return_exceptions=True) for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls
• Cost Gate: Proper integration with should_add_integration_task prevents wasted LLM calls on demo/POC projects

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 390 LOC of tests for 362 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context
• ✅ Phase B Integration: Comprehensive soft rollout testing with 6 fallback scenarios

Test Quality Examples:

def test_partial_llm_failure_is_isolated_per_outcome(self, caplog):
    """One bad LLM call drops that outcome but keeps the rest (P2-2)."""
    # Tests that return_exceptions=True prevents poison-the-well behavior

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Soft Rollout Strategy: Excellent approach allowing safe deployment with fallback to agent-authored verifications

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Minor Issues & Recommendations

1. Broad Exception Handling (line 1714 in nlp_tools.py):

except Exception as gen_err:  # noqa: BLE001

Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. Optional Enhancement:

Consider adding basic shell command syntax validation to catch obviously malformed commands before they reach subprocess execution.

3. Marcus Error Framework:

While current error handling is appropriate, consider integrating with Marcus Error Framework for consistency across the codebase.

Architectural Contributions

This PR successfully implements a critical architectural change:

1. Moves Verification Ownership: From agents (gameable under retry pressure) to Marcus (contract-net protocol)
2. Preserves Agent Autonomy: Agents still own implementation details, Marcus owns verification contracts
3. Enables Measurable Quality: Contract reliability can be measured and improved via generator prompt iteration
4. Soft Rollout Pattern: Excellent balance between innovation and risk management

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

The soft rollout approach is especially commendable - it provides a safety net while allowing the system to learn and improve contract generation quality over time.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

[claude]

Claude finished @lwgray's task —— View job

---

PR Review: Phase A + Phase B Verification Commands ✅

I've thoroughly reviewed PR #642 implementing Phase A verification command generation. Here's my detailed analysis:

Review Checklist

• Analyze core implementation files
• Review test coverage and quality
• Check architectural alignment with Marcus patterns
• Assess security considerations
• Evaluate performance implications
• Provide final recommendations

---

Overall Assessment

Excellent work! This PR demonstrates high-quality software engineering with thoughtful design, comprehensive testing, and careful attention to the Marcus architecture. The implementation successfully addresses the core problem described in issue #636 while maintaining clean separation of concerns.

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strong Points:

• Clean Architecture: Well-structured separation between dataclass, single-outcome generation, and fan-out coordination
• Comprehensive Documentation: Excellent numpy-style docstrings throughout, clear module-level context explaining issue bug: composition+verify pipeline ships unplayable apps when spec-coverage gap-fill creates redundant tasks (orphan source trees pass smoke gate) #636 motivation
• Type Safety: Proper type hints, dataclass usage, and from_dict/to_dict round-trip validation
• Error Handling: Thoughtful error handling with descriptive messages including outcome IDs for debugging
• Async Design: Proper use of asyncio.gather(return_exceptions=True) for concurrent LLM calls to minimize Phase A latency

Code Quality Examples:

# Well-designed dataclass with validation
@dataclass  
class ContractVerification:
    signal_id: str
    command: str
    description: str
    readiness_probe: Optional[str] = None

# Robust error handling with context
raise ValueError(
    f"Verification-command generator: LLM returned malformed JSON "
    f"for outcome {outcome.id!r}: {exc}"
) from exc

Security Analysis ✅

No security concerns identified. The implementation follows secure practices:

• Input Validation: Strict validation of all fields with whitespace normalization
• No Code Injection: Shell commands are generated by LLM but stored as strings for subprocess execution
• Fail-Safe Design: Returns None for unverifiable outcomes rather than generating potentially unsafe commands
• Isolation: Commands run in project directory scope with clear boundaries

Performance Analysis ✅

Well-optimized for Phase A latency requirements:

• Concurrent Generation: asyncio.gather() parallelizes LLM calls (latency = max(individual_calls) not sum)
• Efficient Filtering: Smart filtering of in-scope outcomes before expensive LLM calls
• Reasonable Token Budget: 800 tokens per command is appropriate for verification command generation
• Early Termination: Empty outcomes list returns immediately without LLM calls
• Cost Gate: Proper integration with should_add_integration_task prevents wasted LLM calls on demo/POC projects

Test Coverage Analysis ⭐⭐⭐⭐⭐

Exceptional test coverage! 390 LOC of tests for 362 LOC of implementation shows strong commitment to quality.

Coverage Highlights:

• ✅ Dataclass Round-trips: Complete serialization/deserialization testing
• ✅ LLM Response Handling: Success, null commands, malformed JSON, missing fields
• ✅ Edge Cases: Empty strings, whitespace normalization, type validation
• ✅ Fan-out Logic: Parallel execution, partial failures, empty inputs
• ✅ Error Scenarios: Malformed responses with outcome ID context
• ✅ Phase B Integration: Comprehensive soft rollout testing with 6 fallback scenarios

Test Quality Examples:

def test_partial_llm_failure_is_isolated_per_outcome(self, caplog):
    """One bad LLM call drops that outcome but keeps the rest (P2-2)."""
    # Tests that return_exceptions=True prevents poison-the-well behavior

Architecture Alignment ✅

Perfect adherence to Marcus patterns:

• Multi-Agency Compliance: Implements Invariant Task Assigned 4 times #2 v2 correctly - Marcus owns contract verification, agents own implementation
• Phase A Integration: Clean integration into setup-time pipeline without affecting runtime behavior
• Backwards Compatibility: New contract_verifications field is unused until Phase B, maintaining system stability
• Soft Rollout Strategy: Excellent approach allowing safe deployment with fallback to agent-authored verifications

Marcus Architecture Patterns

Excellent Pattern Usage:

1. Contract-Net Protocol: Proper separation between Marcus (contract author) and agents (implementation)
2. Phase Separation: Clean Phase A implementation without affecting Phase B runtime behavior
3. Failure Isolation: Generator failures don't block project creation (graceful degradation)
4. Observable State: Contract verifications stored on source_context for Phase B consumption

Minor Issues & Recommendations

1. Broad Exception Handling (line 1714 in nlp_tools.py):

except Exception as gen_err:  # noqa: BLE001

Recommendation: Consider catching more specific exceptions (e.g., ValueError, json.JSONDecodeError, httpx.TimeoutError) to distinguish between different failure modes.

2. Optional Enhancement:

Consider adding basic shell command syntax validation to catch obviously malformed commands before they reach subprocess execution.

3. Marcus Error Framework:

While current error handling is appropriate, consider integrating with Marcus Error Framework for consistency across the codebase.

Architectural Contributions

This PR successfully implements a critical architectural change:

1. Moves Verification Ownership: From agents (gameable under retry pressure) to Marcus (contract-net protocol)
2. Preserves Agent Autonomy: Agents still own implementation details, Marcus owns verification contracts
3. Enables Measurable Quality: Contract reliability can be measured and improved via generator prompt iteration
4. Soft Rollout Pattern: Excellent balance between innovation and risk management

Final Recommendations

1. Consider Specific Exception Types: Replace broad Exception catch with specific types
2. Optional Enhancement: Add basic shell command syntax validation
3. Future Work: Consider Marcus Error Framework integration for consistency
4. Documentation: The PR description is exemplary - comprehensive yet accessible

Summary

This is high-quality production code that successfully implements a critical architectural change while maintaining system stability. The comprehensive testing, thoughtful error handling, and clean integration demonstrate excellent software engineering practices. The Phase A/Phase B separation strategy is particularly well-executed, allowing safe rollout of this significant change.

The soft rollout approach is especially commendable - it provides a safety net while allowing the system to learn and improve contract generation quality over time.

Recommendation: ✅ APPROVE - This PR meets all quality standards and successfully implements the required functionality.

---

[lwgray]

Closing: this builds Marcus-authored verification commands (Phase A/B), the direction we've moved away from. We pivoted to agent self-verification (the agent owns verification; Marcus authors no verification command) and removed the Marcus-run build floor entirely (it was tech-specific and gridlock-prone). The premise here is superseded. Reopen if the verification-command-generation infra is needed standalone.