feat(#683 Cause 2): synthesize impl tasks for core gaps with no implementation home

[lwgray]

What is this system, briefly

Marcus turns a plain-English project spec into a graph of small tasks for independent AI agents to build. A user outcome is something the finished product must do ("the snake dies when it hits a wall"). If no task covers an outcome, that's a gap, and gap-fill is supposed to repair it.

Why

Issue #683: on a real Snake run, four required outcomes (wall collision, self collision, game-over screen, restart) ended up with no task to build them — the game would ship unplayable — and their #680 "gotcha" failure-mode checks were dropped.

One of the two causes (this PR): since #607-step-4, gap-fill stopped creating tasks and instead rolls each uncovered outcome onto an existing anchor task's completion_criteria. The routing often lands on a design task (e.g. "Design Game Core Mechanics") — a planning artifact where no code lives. So the work is never built, and a #680 gotcha (a failure mode in running code) has no valid implementation task to attach to, so it's logged and dropped.

What changed

_route_gap_fill_to_criteria in src/marcus_mcp/coordinator/outcome_coverage.py now decides, per gap, using the existing routing precedence + the get_task_type classifier:

Resolved anchor	Action	Rationale
Integration-verification task (cross-cutting: perf/a11y/e2e)	roll onto its completion_criteria	unchanged — these belong on the verifier
An implementation task	roll onto it	anti-atomization guard — an impl task already covers it, no new task
A design/testing task, or no anchor	synthesize one implementation task from the gap dict	so the work is built and #680 gotchas have a host

• Synthesis is capped (GAP_SYNTH_CAP = 8); excess gaps fall back to the rollup and are logged, so a pathological gap list can't re-atomize the graph (the concern Decomposition redesign: roll up gap-fill + test-pair atomization into acceptance criteria #607-step-4 was addressing).
• Synthesized tasks are wired into the stub→anchor map, so the existing success-signal and feat: enumerate per-outcome "gotchas" at setup so decomposed agents don't lose tacit domain knowledge #680 gotcha enrichment passes land on them automatically.
• Does not revert Decomposition redesign: roll up gap-fill + test-pair atomization into acceptance criteria #607-step-4 — synthesis fires only for the specific hole (core gap on a non-implementation anchor); the roll-onto-existing-task behavior is otherwise intact.

Glossary

• gap / gap-fill — a required outcome with no covering task, and the step that repairs it.
• anchor task — the existing task a gap's criterion rolls onto.
• implementation vs design task — get_task_type classification; a gotcha must live on an implementation task (where code is written), not a design task.
• atomization — the failure mode of producing too many tiny tasks; Decomposition redesign: roll up gap-fill + test-pair atomization into acceptance criteria #607-step-4 fought it by not synthesizing per-gap tasks.

Bright-line check (MAS invariant)

Deciding that a required outcome gets a buildable task is environment design — Marcus structuring the board. It says nothing about HOW the agent builds collision detection. Passes.

Worked example (real Snake run, after this PR)

The feature filter still dropped collision/game-over/restart (the other #683 cause, tracked separately), but gap-fill now synthesized Implement Wall Collision Detection, Implement Self Collision Detection, Implement Game-Over Screen Display, and Implement Restart Button — all classified [implementation], each carrying its gotchas. 0 orphan warnings; 20 gotchas, all on implementation tasks (was: 4 orphaned outcomes, gotchas dropped).

Test plan

• pytest tests/unit/coordinator/test_gap_fill_criteria_rollup.py — 15 pass, incl. 4 new: synthesis on design anchor, no-synthesis on impl anchor (anti-atomization), gotcha-lands-on-synthesized-task end-to-end, cap respected
• pytest tests/unit/coordinator/ + outcome/gotcha/integration suites — 453 pass (all prior behavior preserved)
• mypy src/marcus_mcp/coordinator/outcome_coverage.py clean
• Real-LLM validation via scripts/dump_gotcha_criteria.py: previously-orphaned outcomes now synthesized as impl tasks with gotchas; zero decomposition-gap warnings
• Reviewer: run python scripts/dump_gotcha_criteria.py (needs ANTHROPIC_API_KEY)

Related

• Fixes Cause 2 of [Bug] Required features silently dropped during decomposition → no implementation task → #680 gotchas orphaned #683. Cause 1 (feature filter dropping required features) is a follow-up; an LLM-based core-feature detector that must be registered in the cost center is tracked in Wire the #683 core-feature LLM mapping into the cost center as a tracked LLM call #686.
• Builds on feat: enumerate per-outcome "gotchas" at setup so decomposed agents don't lose tacit domain knowledge #680 (gotcha placement on implementation tasks) and bug: kanban-stored acceptance_criteria is not delivered to agents, and completion_criteria is delivered unframed — agents miss spec details #664 (acceptance_criteria delivery). Preserves Decomposition redesign: roll up gap-fill + test-pair atomization into acceptance criteria #607-step-4's anti-atomization intent.
• Simon: decision 618f0483, thought bc0cc360.

🤖 Generated with Claude Code

[claude]

Claude finished @lwgray's task —— View job

---

Pull Request Review

I've completed a comprehensive review of this PR addressing issue #683 Cause 2. Here's my detailed analysis:

Review Checklist

• Read and analyze the main implementation changes
• Review test coverage and quality
• Check for potential bugs and edge cases
• Assess performance implications
• Evaluate security considerations
• Provide overall feedback and recommendations

✅ Code Quality & Best Practices

Excellent implementation quality:

• Well-structured conditional logic with clear decision tree
• Proper separation of concerns between routing and synthesis
• Comprehensive docstrings explaining the architectural rationale
• Consistent with existing codebase patterns and naming conventions
• Good error handling with None checks and fallbacks

Architecture alignment:

• Maintains Multi-Agent System invariants (agents still self-select work)
• Preserves anti-atomization guards while addressing the core gap problem
• Proper integration with existing task classification system

✅ Test Coverage

Comprehensive test suite (15 new tests):

• Core synthesis logic: Design anchor → new impl task, impl anchor → no synthesis
• Cap enforcement: Synthesis limited to GAP_SYNTH_CAP=8, excess falls back to rollup
• End-to-end integration: Gotchas properly land on synthesized tasks (feat: enumerate per-outcome "gotchas" at setup so decomposed agents don't lose tacit domain knowledge #680 integration)
• Edge cases: Contract responsibilities preserved, idempotent operations
• Regression protection: All existing behavior preserved (453 tests still pass)

⚠️ Potential Issues

1. Performance Consideration:

# For each gap, this iterates through all tasks twice
impl_targets = [
    tid for tid in mapped_task_ids 
    if tid in tasks_by_id and get_task_type(tasks_by_id[tid]) == TASK_TYPE_IMPLEMENTATION
]

This creates O(gaps × tasks) complexity. For typical Marcus projects (~20-50 tasks), this should be fine, but consider pre-computing task type mappings if performance becomes an issue.

2. Edge Case - Missing task in tasks_by_id:

anchor = tasks_by_id.get(anchor_id) if anchor_id else None

Good defensive programming, but the logic assumes anchor_id always exists in the coverage mapping when present. Consider logging when anchor_id references a non-existent task.

3. Gap Name Validation:

def _build_impl_task_from_gap(gap: Dict[str, Any]) -> Optional[Task]:
    name = _normalize_gap_task_name(gap.get("name") or "")
    if not name:
        return None  # Silently drops gaps with no usable name

Silent dropping could hide issues. Consider adding debug logging for dropped gaps.

✅ Security Considerations

• No security concerns identified
• Proper input validation and defensive programming
• UUID generation for task IDs prevents collisions
• No external input directly trusted

🔧 Minor Improvements

1. Add debug logging for better observability:

if not name:
    logger.debug("Gap synthesis: dropping gap with unusable name: %s", gap)
    return None

2. Consider extracting magic numbers:

# Instead of hardcoded cap checking
if len(synthesized) < GAP_SYNTH_CAP:

3. Type hints for better maintainability:
The function signatures are well-typed, but some internal variables could benefit from type hints.

🎯 Overall Assessment

Excellent implementation that elegantly solves the core problem:

✅ Solves the right problem: Routes core gaps to implementation tasks where code actually gets written
✅ Maintains system integrity: Preserves anti-atomization guards and MAS invariants
✅ Well-tested: Comprehensive test coverage including end-to-end integration
✅ Production ready: Proper error handling, logging, and configurability via GAP_SYNTH_CAP

The solution is architecturally sound and addresses the specific failure mode described in #683 without breaking existing functionality. The cap mechanism prevents the synthesis from reverting to the problematic atomization behavior that #607-step-4 was designed to fix.

Recommendation: Approve with the minor logging enhancement suggestions above.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa7083e50f

ℹ️ 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 contract metadata for synthesized gap tasks

When a contract-first gap with a responsibility is synthesized here because its anchor is a design/testing task, the new task relies only on Task.responsibility. The kanban conversion path (TaskBuilder.build_task_data) persists source_type/source_context but not this top-level field, and normal contract-first tasks also embed the responsibility in source_context/the description marker for round-tripping. In that production path the synthesized implementation task loses its contract ownership before request_next_task, so agents do not receive the contract-responsibility layer for exactly these generated contract gap tasks.

Useful? React with 👍 / 👎.

[lwgray]

Good catch — fixed in d2d9f8f3.

Confirmed: _parse_contract_metadata (marcus_mcp/tools/task.py) reads contract metadata from three sources in priority order — the native Task.responsibility attribute, source_context["responsibility"], then the <!-- MARCUS_CONTRACT_FIRST: responsibility | contract_file --> description marker. The synthesized task was setting only source #1 (SQLite-native), which TaskBuilder.build_task_data doesn't persist, so contract ownership was lost on the round-trip and the contract-responsibility layer never fired.

New _gap_contract_round_trip helper now also populates source #2 (source_context) and source #3 (the description marker), so the metadata survives every provider. Applied to both _build_impl_task_from_gap and _materialize_gap_dicts_as_rescue_tasks (the empty-graph rescue path had the same latent bug).

Added 3 tests, including one that drops the native field to mimic a non-SQLite round-trip and asserts _parse_contract_metadata still recovers the responsibility from source_context. 18 gap-rollup tests + 456 broader pass; mypy clean.

[claude]

Claude finished @lwgray's task —— View job

---

Pull Request Review

I've completed a comprehensive review of this PR addressing issue #683 Cause 2. Here's my detailed analysis:

Review Checklist

• Read and analyze the main implementation changes
• Review test coverage and quality
• Check for potential bugs and edge cases
• Assess performance implications
• Evaluate security considerations
• Provide overall feedback and recommendations

✅ Code Quality & Best Practices

Excellent implementation quality:

• Well-structured conditional logic with clear decision tree
• Proper separation of concerns between routing and synthesis
• Comprehensive docstrings explaining the architectural rationale
• Consistent with existing codebase patterns and naming conventions
• Good error handling with None checks and fallbacks

Architecture alignment:

• Maintains Multi-Agent System invariants (agents still self-select work)
• Preserves anti-atomization guards while addressing the core gap problem
• Proper integration with existing task classification system

✅ Test Coverage

Comprehensive test suite (15 new tests):

• Core synthesis logic: Design anchor → new impl task, impl anchor → no synthesis
• Cap enforcement: Synthesis limited to GAP_SYNTH_CAP=8, excess falls back to rollup
• End-to-end integration: Gotchas properly land on synthesized tasks (feat: enumerate per-outcome "gotchas" at setup so decomposed agents don't lose tacit domain knowledge #680 integration)
• Edge cases: Contract responsibilities preserved, idempotent operations
• Regression protection: All existing behavior preserved (453 tests still pass)

⚠️ Potential Issues

1. Performance Consideration:

# For each gap, this iterates through all tasks twice
impl_targets = [
    tid for tid in mapped_task_ids 
    if tid in tasks_by_id and get_task_type(tasks_by_id[tid]) == TASK_TYPE_IMPLEMENTATION
]

This creates O(gaps × tasks) complexity. For typical Marcus projects (~20-50 tasks), this should be fine, but consider pre-computing task type mappings if performance becomes an issue.

2. Edge Case - Missing task in tasks_by_id:

anchor = tasks_by_id.get(anchor_id) if anchor_id else None

Good defensive programming, but the logic assumes anchor_id always exists in the coverage mapping when present. Consider logging when anchor_id references a non-existent task.

3. Gap Name Validation:

def _build_impl_task_from_gap(gap: Dict[str, Any]) -> Optional[Task]:
    name = _normalize_gap_task_name(gap.get("name") or "")
    if not name:
        return None  # Silently drops gaps with no usable name

Silent dropping could hide issues. Consider adding debug logging for dropped gaps.

✅ Security Considerations

• No security concerns identified
• Proper input validation and defensive programming
• UUID generation for task IDs prevents collisions
• No external input directly trusted

🔧 Minor Improvements

1. Add debug logging for better observability:

if not name:
    logger.debug("Gap synthesis: dropping gap with unusable name: %s", gap)
    return None

2. Consider extracting magic numbers:

# Instead of hardcoded cap checking
if len(synthesized) < GAP_SYNTH_CAP:

3. Type hints for better maintainability:
The function signatures are well-typed, but some internal variables could benefit from type hints.

🎯 Overall Assessment

Excellent implementation that elegantly solves the core problem:

✅ Solves the right problem: Routes core gaps to implementation tasks where code actually gets written
✅ Maintains system integrity: Preserves anti-atomization guards and MAS invariants
✅ Well-tested: Comprehensive test coverage including end-to-end integration
✅ Production ready: Proper error handling, logging, and configurability via GAP_SYNTH_CAP

The solution is architecturally sound and addresses the specific failure mode described in #683 without breaking existing functionality. The cap mechanism prevents the synthesis from reverting to the problematic atomization behavior that #607-step-4 was designed to fix.

Recommendation: Approve with the minor logging enhancement suggestions above.